[Intel-gfx] [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 Intel-gfx
mailing list