[Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

[Sharma, Shashank]

Regards

Shashank


On 11/17/2017 6:19 PM, Ville Syrjälä wrote:
> On Fri, Nov 17, 2017 at 05:50:11PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 11/17/2017 5:05 PM, Ville Syrjälä wrote:
>>> On Fri, Nov 17, 2017 at 08:49:49AM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 11/16/2017 9:53 PM, Ville Syrjälä wrote:
>>>>> On Thu, Nov 16, 2017 at 08:21:44PM +0530, Sharma, Shashank wrote:
>>>>>> Regards
>>>>>>
>>>>>> Shashank
>>>>>>
>>>>>>
>>>>>> On 11/13/2017 10:34 PM, Ville Syrjala wrote:
>>>>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>>>
>>>>>>> commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
>>>>>>> cause us to not send out any VICs in the AVI infoframes. That commit
>>>>>>> was since reverted, but if and when we add aspect ratio handing back
>>>>>>> we need to be more careful.
>>>>>>>
>>>>>>> Let's handle this by considering the aspect ratio as a requirement
>>>>>>> for cea mode matching only if the passed in mode actually has a
>>>>>>> non-zero aspect ratio field. This will keep userspace that doesn't
>>>>>>> provide an aspect ratio working as before by matching it to the
>>>>>>> first otherwise equal cea mode. And once userspace starts to
>>>>>>> provide the aspect ratio it will be considerd a hard requirement
>>>>>>> for the match.
>>>>>>>
>>>>>>> Also change the hdmi mode matching to use drm_mode_match() for
>>>>>>> consistency, but we don't match on aspect ratio there since the
>>>>>>> spec doesn't list a specific aspect ratio for those modes.
>>>>>>>
>>>>>>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>>>>>>> Cc: "Lin, Jia" <lin.a.jia at intel.com>
>>>>>>> Cc: Akashdeep Sharma <akashdeep.sharma at intel.com>
>>>>>>> Cc: Jim Bride <jim.bride at linux.intel.com>
>>>>>>> Cc: Jose Abreu <Jose.Abreu at synopsys.com>
>>>>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>>>>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>>>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/drm_edid.c | 18 ++++++++++++++----
>>>>>>>      1 file changed, 14 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>>>>> index 7220b8f9a7e8..00aa98f3e55d 100644
>>>>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>>>>> @@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct drm_display_mode *mode)
>>>>>>>      static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match,
>>>>>>>      					     unsigned int clock_tolerance)
>>>>>>>      {
>>>>>>> +	unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS;
>>>>>>>      	u8 vic;
>>>>>>>      
>>>>>>>      	if (!to_match->clock)
>>>>>>>      		return 0;
>>>>>>>      
>>>>>>> +	if (to_match->picture_aspect_ratio)
>>>>>>> +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
>>>>>> This doesn't look right. This means we are expecting a CEA mode without
>>>>>> a pic aspect ratio field,
>>>>>> which is invalid.
>>>>> No, it's perfectly valid. It's what we currently get from userspace.
>>>> Yep, but that's due to missing Aspect ratio handling in the DRM layer.
>>>> If that's fixed, as per the list of CEA modes,
>>>> each CEA VIC contains an aspect ratio, which is a part of its unique
>>>> identity.
>>>>
>>>> I guess once we have the aspect ratio handling in DRM layer, it
>>>> would/should look like this:
>>>> - EDID gives you all supported modes, including CEA modes with Aspect ratio
>>>> - Userspcae gets the mode information, with aspect ratio (for CEA modes)
>>>> If ( Userspace picks one of the CEA modes)
>>>>        - sends a modeset
>>>>        - we find a matching CEA VIC, found one from modedb
>>>>        - we load this VIC = nonzero information in AVI IF VIC field,
>>>> else
>>>>        - sends a modeset
>>>>        - we could not find a matching CEA VIC, as aspect ratio is 0
>>>>        - we make VIC field in AVI IF as 0a
>>> No. That would break current userspace.
>> I guess I forgot to make it clear, that userspace will set the cap, only
>> then we will provide aspect ratio information.
>> So this should not break userspace, isn't it ?
>>>> This is important, as HDMI compliance test 7-27 inspects if the VIC
>>>> field in the AVI IF is accurate.
>>> Complicance is secondary to not breaking things that work. Also I find
>>> it hard to see what purpose there is in having a complicance test that
>>> sets a CEA modes w/o aspect ratio and then expects the infoframe to have
>>> VIC 0.
>> Again, typically this is how these analyzers force modeset:
>> - They send EDID with only one mode, which is the test mode, with aspect
>> ratio.
>> - They expect that VIC to be present in AVI IF
>>>> - Shashank
>>>>>> Ankit is going to publish the aspect ratio patch
>>>>>> series again, with proper DRM cap and flags check. Would it be
>>>>>> ok if we have a look that handling first ?
>>>>> This patch will be needed by that work. Otherwise we're going to stop
>>>>> sending a VIC for CEA modes with current userspace.
>>>> I guess we should force userspaces to start bothering about aspect ratio
>>>> field, right now we
>>>> are doing this for Wayland based compositors, may be we should extend it
>>>> to X based too.
>>> Yes, I've been saying that someone should look into extending the randr
>>> protocol with the necessary bits. But that still doesn't allow us to
>>> change the current behaviour as old userspace would anyway linger around
>>> for a long time.
>> I think cap will cove this part
> The cap is irrelevant to this discussion. It will not be set by old
> userspace, hence it won't change anything for old userspace. Modes
> coming from old userspace will still have aspect_ratio=0.
Agree, and in that case, why bother about aspect ratio flags here in 
this series ? Probably let this be taken care in aspect ratio series.
- Shashank