Regards
Shashank
On 10/17/2016 8:30 PM, Ville Syrjälä wrote:
> On Mon, Oct 17, 2016 at 08:21:21PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 10/17/2016 7:36 PM, Ville Syrjälä wrote:
>>> On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 10/17/2016 6:01 PM, Ville Syrjälä wrote:
>>>>> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
>>>>>> Current DRM layer functions don't parse aspect ratio information
>>>>>> while converting a user mode->kernel mode or vice versa. This
>>>>>> causes modeset to pick mode with wrong aspect ratio, eventually
>>>>>> causing failures in HDMI compliance test cases, due to wrong VIC.
>>>>>>
>>>>>> This patch adds aspect ratio information in DRM's mode conversion
>>>>>> and mode comparision functions, to make sure kernel picks mode
>>>>>> with right aspect ratio (as per the VIC).
>>>>>>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>>>>> Signed-off-by: Lin, Jia <lin.a.jia at intel.com>
>>>>>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma at intel.com>
>>>>>> Reviewed-by: Jim Bride <jim.bride at linux.intel.com>
>>>>>> Reviewed-by: Jose Abreu <Jose.Abreu at synopsys.com>
>>>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>>>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>>>>>>
>>>>>> V2: Addressed review comments from Sean:
>>>>>> - Fix spellings/typo
>>>>>> - No need to handle aspect ratio none
>>>>>> - Add a break, for default case too
>>>>>> V3: Rebase
>>>>>> V4: Added r-b from Jose
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 31 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>>>>>> index 53f07ac..fde927a 100644
>>>>>> --- a/drivers/gpu/drm/drm_modes.c
>>>>>> +++ b/drivers/gpu/drm/drm_modes.c
>>>>>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct
>>>>>> drm_display_mode *mode1,
>>>>>> mode1->vsync_end == mode2->vsync_end &&
>>>>>> mode1->vtotal == mode2->vtotal &&
>>>>>> mode1->vscan == mode2->vscan &&
>>>>>> + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio
>>>>>> &&
>>>>>> (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>>>>>> (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>>>>>> return true;
>>>>>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct
>>>>>> drm_mode_modeinfo *out,
>>>>>> out->vrefresh = in->vrefresh;
>>>>>> out->flags = in->flags;
>>>>>> out->type = in->type;
>>>>>> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>>>>>> +
>>>>>> + switch (in->picture_aspect_ratio) {
>>>>>> + case HDMI_PICTURE_ASPECT_4_3:
>>>>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
>>>>>> + break;
>>>>>> + case HDMI_PICTURE_ASPECT_16_9:
>>>>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
>>>>>> + break;
>>>>>> + case HDMI_PICTURE_ASPECT_RESERVED:
>>>>>> + default:
>>>>>> + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>>>>> out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>>>>> }
>>>>>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct
>>>>>> drm_display_mode *out,
>>>>>> strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>>>>> out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>>>>>
>>>>>> + /* Clearing picture aspect ratio bits from out flags */
>>>>>> + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>>>>>> +
>>>>>> + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
>>>>>> + case DRM_MODE_FLAG_PIC_AR_4_3:
>>>>>> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
>>>>>> + break;
>>>>>> + case DRM_MODE_FLAG_PIC_AR_16_9:
>>>>>> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>>>>>> + break;
>>>>>> + default:
>>>>>> + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>>> + break;
>>>>>> + }
>>>>> Hmm. So now we have the mode aspect ratio infromation duplicated
>>>>> in two different places. Not sure that won't lead to some confusion.
>>>> In drm layer, this is the only place. Actually till now, DRM layer dint
>>>> have the support for aspect ratio at all. This was causing
>>>> HDMI compliance tests like 7-27 fail, which expect a particular unique
>>>> VIC in AVI infoframe on modeset.
>>>>
>>>> I have given some details about this in cover-letter.
>>>>> Although we do want the user to be able to override via the property I
>>>>> suppose, so we'd have to change that (+ the inforframe code) to
>>>>> look at the mode flags instead if we would eliminate
>>>>> 'picture_aspect_ratio'.
>>>> I had a small discussion on this with Daniel, and we discussed that it
>>>> doesnt make sense to override just the aspect ratio if the monitor
>>>> doesn't even support that aspect ratio.
>>>> And currently the was how this property is implemented is, we just
>>>> override the aspect ratio without any validity check.
>>>>
>>>> Now as we have all the supported aspect ratio added properly in the mode
>>>> info itself, we need not to have this property at all, So Daniel
>>>> suggested me to remove that property too.
>>>>> And that brings me to the other point. At least i915 will simply
>>>>> override the mode's aspect ration with the property. So this looks like
>>>>> a big no-op for now on i915.
>>>> Yes, This is a bug in I915. When I published the first version of this
>>>> series, I had one patch, which was overriding the value only when the
>>>> property is set.
>>>> This should be the right case. And then Daniel suggested to remove the
>>>> property all together (and it makes sense as we have proper aspect
>>>> ratios in mode information
>>>> itself) So I kept that patch separate, to be merged separately.
>>> Yeah, removing the property entirely seems like an OKish solution since
>>> userspace can just stuff that information into the mode flags instead.
>>> The only slight concern is that someone's setup might depend on the
>>> property, and now we're removing it.
>> Thanks, will send the patch for it soon, adding you in the mail chain.
>>>>> It's looking like we might need a new
>>>>> propetty value to differentiate between "auto" and "none" for real.
>>>> Now we have the exact aspect ratio given by monitor EDID, so IMHO it
>>>> would be better if we can just have real aspect.
>>> Well, "real" isn't quite the right term. It's still a user specified
>>> thing, which means the user can ask for somehting the display didn't
>>> even advertise. Which is fine as such, however since the AVI infoframe
>>> lacks the capability to transmit these new aspect ratios we have a bit
>>> of problem on our hands if the user asks for something we can't even
>>> tell the display to do. I guess we would just need to return an error
>>> to the user in that case.
>> The current implementation which we have in andoroid, is hardware
>> composer shows the whole mode in the list like:
>> - 1920x1080 at 60 16:9 /* From CEA block of EDID */
>> - 1920x1080 at 60 0:0 /* From detailed descriptor block of EDID */
>> - 1280x720 at 60 4:3 /* From CEA block of EDID */
>> -etc
>> These mode is the connector->probed_modes which we populated while
>> parsing the EDID.
>> Now, when user selects the mode, its a complete set of resolution +
>> refresh rate + aspect ratio, so user can only pick what
>> we are showing to it. In this case, there is no option which we cant
>> support.
>>
>> Do you think that there would be some other model of use case, where we
>> can mix something among these?
> You can't assume the user just picks the mode off the list. They can
> stuff whatever random garbage into the mode if they wish.
>
I agree, there can be a use case model.
Regards
Shashank
