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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Sharma, Shashank" <shashank.sharma@intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>,
	"Lin, Jia" <lin.a.jia@intel.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Nautiyal Ankit <ankit.k.nautiyal@intel.com>
Subject: Re: [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling
Date: Thu, 16 Nov 2017 18:23:57 +0200	[thread overview]
Message-ID: <20171116162357.GX10981@intel.com> (raw)
In-Reply-To: <67ddbed5-421d-d81f-9e0f-7ad4055f3c4e@intel.com>

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@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@intel.com>
> > Cc: "Lin, Jia" <lin.a.jia@intel.com>
> > Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> > Cc: Jim Bride <jim.bride@linux.intel.com>
> > Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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.

> 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.

> > +
> >   	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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx