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

Sharma, Shashank shashank.sharma at intel.com
Fri Nov 17 12:20:11 UTC 2017


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

- Shashank
>> - Shashank
>>>>> +
>>>>>     	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
>>>>>     		struct drm_display_mode cea_mode = edid_cea_modes[vic];
>>>>>     		unsigned int clock1, clock2;
>>>>> @@ -2921,7 +2925,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m
>>>>>     			continue;
>>>>>     
>>>>>     		do {
>>>>> -			if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode))
>>>>> +			if (drm_mode_match(to_match, &cea_mode, match_flags))
>>>>>     				return vic;
>>>>>     		} while (cea_mode_alternate_timings(vic, &cea_mode));
>>>>>     	}
>>>>> @@ -2938,11 +2942,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m
>>>>>      */
>>>>>     u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
>>>>>     {
>>>>> +	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;
>>>> same here, and probably in other CEA match functions.
>>>>> +
>>>>>     	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
>>>>>     		struct drm_display_mode cea_mode = edid_cea_modes[vic];
>>>>>     		unsigned int clock1, clock2;
>>>>> @@ -2956,7 +2964,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
>>>>>     			continue;
>>>>>     
>>>>>     		do {
>>>>> -			if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode))
>>>>> +			if (drm_mode_match(to_match, &cea_mode, match_flags))
>>>>>     				return vic;
>>>>>     		} while (cea_mode_alternate_timings(vic, &cea_mode));
>>>>>     	}
>>>>> @@ -3003,6 +3011,7 @@ hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode)
>>>>>     static u8 drm_match_hdmi_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)
>>>>> @@ -3020,7 +3029,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
>>>>>     		    abs(to_match->clock - clock2) > clock_tolerance)
>>>>>     			continue;
>>>>>     
>>>>> -		if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
>>>>> +		if (drm_mode_match(to_match, hdmi_mode, match_flags))
>>>>>     			return vic;
>>>>>     	}
>>>>>     
>>>>> @@ -3037,6 +3046,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
>>>>>      */
>>>>>     static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
>>>>>     {
>>>>> +	unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS;
>>>>>     	u8 vic;
>>>>>     
>>>>>     	if (!to_match->clock)
>>>>> @@ -3052,7 +3062,7 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
>>>>>     
>>>>>     		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
>>>>>     		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
>>>>> -		    drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
>>>>> +		    drm_mode_match(to_match, hdmi_mode, match_flags))
>>>>>     			return vic;
>>>>>     	}
>>>>>     	return 0;
>>>> - Shashank



More information about the dri-devel mailing list