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

[Andrzej Hajda]

On 03.11.2016 13:53, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at intel.com>
> Cc: Andrzej Hajda <a.hajda at samsung.com>
> Cc: Adam Jackson <ajax at redhat.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at 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 at 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.

> +
> +	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 at 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 :)


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


Regards

Andrzej