Re: [PATCH 2/2] drm/edid: Consider alternate cea timings to be the same VIC

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

On Tue, Nov 08, 2016 at 12:43:55PM +0100, Andrzej Hajda wrote:
> On 03.11.2016 13:53, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > CEA-861 specifies that the vertical front porch may vary by one or two
> > lines for specific VICs. Up to now we've only considered a mode to match
> > the VIC if it matched the shortest possible vertical front porch length
> > (as that is the variant we store in cea_modes[]). Let's allow our VIC
> > matching to work with the other timings variants as well so that that
> > we'll send out the correct VIC if the variant actually used isn't the
> > one with the shortest vertical front porch.
> >
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Adam Jackson <ajax@redhat.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> I have checked against specification and it looks OK.
> I have few nitpicks below regarding implementation, but this could be
> matter of taste, so:
> 
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> 
> > ---
> >  drivers/gpu/drm/drm_edid.c | 66 +++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 54 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 7eec18925b70..728990fee4ef 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2613,6 +2613,41 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode)
> >  	return clock;
> >  }
> >  
> > +static bool
> > +cea_mode_alternate_timings(u8 vic, struct drm_display_mode *mode)
> > +{
> > +	/*
> > +	 * For certain VICs the spec allows the vertical
> > +	 * front porch to vary by one or two lines.
> > +	 *
> > +	 * cea_modes[] stores the variant with the shortest
> > +	 * vertical front porch. We can adjust the mode to
> > +	 * get the other variants by simply increasing the
> > +	 * vertical front porch length.
> > +	 */
> > +	BUILD_BUG_ON(edid_cea_modes[8].vtotal != 262 ||
> > +		     edid_cea_modes[9].vtotal != 262 ||
> > +		     edid_cea_modes[12].vtotal != 262 ||
> > +		     edid_cea_modes[13].vtotal != 262 ||
> > +		     edid_cea_modes[23].vtotal != 312 ||
> > +		     edid_cea_modes[24].vtotal != 312 ||
> > +		     edid_cea_modes[27].vtotal != 312 ||
> > +		     edid_cea_modes[28].vtotal != 312);
> 
> I am not sure if this compile check is really necessary,
> I would rather put comment before edid_cea_modes array
> which mode should be put into array in multi-mode case.

Comments can't guarantee this wouldn't break. Doesn't mean
we can't have a comment in addition though.

> 
> > +
> > +	if (((vic == 8 || vic == 9 ||
> > +	      vic == 12 || vic == 13) && mode->vtotal < 263) ||
> > +	    ((vic == 23 || vic == 24 ||
> > +	      vic == 27 || vic == 28) && mode->vtotal < 314)) {
> 
> I wonder if it wouldn't be better to mark somehow these modes
> in the array as alternating ones. This way all info about cea modes
> will be in one place. For example by (ab)using private_flags:
>     /* 8 - 720(1440)x240@60Hz */
>     { DRM_MODE("720x240", DRM_MODE_TYPE_DRIVER, 13500, 720, 739,
>            801, 858, 0, 240, 244, 247, 262, 0,
>            DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
>             DRM_MODE_FLAG_DBLCLK),
>       .vrefresh = 60, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
>       .private_flags = CEA_MODE_FLAG_VFP2},
> 
> This is of course just an idea, I am not sure if not overkill :)

Sounds potentially sensible. Should do the same for the alternate clock
thing as well then I suppose.

I was pondering whether I should just convert the cea mode array into
some kind of other data structure that could store multiple variants for
each mode. But then I figured it's maybe a bit too much work for just
these few excepttions.

Another thought that occurred to me recently is that the cea mode list
is getting rather long, so I was wondering if could speed up the lookups
somehow. Currently we just do a linear search through the whole thing.
A simple bsearch() might not work out since the modes aren't necessarily
sorted in any useful order, so we might need some kind of a hash or
something, But then I figured that we shouldn't do these lookups too
often so it might not be worth the effort to optimize it.

> 
> 
> > +		mode->vsync_start++;
> > +		mode->vsync_end++;
> > +		mode->vtotal++;
> > +
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match,
> >  					     unsigned int clock_tolerance)
> >  {
> > @@ -2622,19 +2657,21 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m
> >  		return 0;
> >  
> >  	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
> > -		const struct drm_display_mode *cea_mode = &edid_cea_modes[vic];
> > +		struct drm_display_mode cea_mode = edid_cea_modes[vic];
> >  		unsigned int clock1, clock2;
> >  
> >  		/* Check both 60Hz and 59.94Hz */
> > -		clock1 = cea_mode->clock;
> > -		clock2 = cea_mode_alternate_clock(cea_mode);
> > +		clock1 = cea_mode.clock;
> > +		clock2 = cea_mode_alternate_clock(&cea_mode);
> >  
> >  		if (abs(to_match->clock - clock1) > clock_tolerance &&
> >  		    abs(to_match->clock - clock2) > clock_tolerance)
> >  			continue;
> >  
> > -		if (drm_mode_equal_no_clocks(to_match, cea_mode))
> > -			return vic;
> > +		do {
> > +			if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode))
> > +				return vic;
> > +		} while (cea_mode_alternate_timings(vic, &cea_mode));
> >  	}
> >  
> >  	return 0;
> > @@ -2655,18 +2692,23 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
> >  		return 0;
> >  
> >  	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
> > -		const struct drm_display_mode *cea_mode = &edid_cea_modes[vic];
> > +		struct drm_display_mode cea_mode = edid_cea_modes[vic];
> >  		unsigned int clock1, clock2;
> >  
> >  		/* Check both 60Hz and 59.94Hz */
> > -		clock1 = cea_mode->clock;
> > -		clock2 = cea_mode_alternate_clock(cea_mode);
> > +		clock1 = cea_mode.clock;
> > +		clock2 = cea_mode_alternate_clock(&cea_mode);
> >  
> > -		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
> > -		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
> > -		    drm_mode_equal_no_clocks_no_stereo(to_match, cea_mode))
> > -			return vic;
> > +		if (KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock1) &&
> > +		    KHZ2PICOS(to_match->clock) != KHZ2PICOS(clock2))
> > +			continue;
> > +
> > +		do {
> > +			if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode))
> > +				return vic;
> > +		} while (cea_mode_alternate_timings(vic, &cea_mode));
> >  	}
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_match_cea_mode);
> 
> And finally there is no modification of add_alternate_cea_modes, but I
> guess it can be added later.

I doubt we want to add all the variants of these modes to the list.
Having just one should be enough I think.

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