Re: [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 17, 2018 at 02:35:40PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 1/12/2018 11:51 AM, Nautiyal, Ankit K 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 1818a71..3d57c41 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2908,11 +2908,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;
> > +
> >   	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
> >   		struct drm_display_mode cea_mode = edid_cea_modes[vic];
> >   		unsigned int clock1, clock2;
> > @@ -2926,7 +2930,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));
> >   	}
> > @@ -2943,11 +2947,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;
> > +
> How about stereo flags ? CEA modes can be 3d and in that case we might 
> want to add DRM_MODE_MATCH_3D_FLAGS here

There are no separate VICs for stereo vs. no stereo. So ignoring the
stereo flags here seems like the correct thing to do.

> 
> - 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;
> > @@ -2961,7 +2969,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));
> >   	}
> > @@ -3008,6 +3016,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)
> > @@ -3025,7 +3034,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;
> >   	}
> >   
> > @@ -3042,6 +3051,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)
> > @@ -3057,7 +3067,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;

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