[PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment

[PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

All the VICs apart from 58 and 59 have the word "Hz" included in the
comment. Include it for 59 and 59 as well.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9506933b41cd..7eec18925b70 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -957,13 +957,13 @@ static const struct drm_display_mode edid_cea_modes[] = {
 		   798, 858, 0, 480, 489, 495, 525, 0,
 		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
 	  .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },
-	/* 58 - 720(1440)x480i@240 */
+	/* 58 - 720(1440)x480i@240Hz */
 	{ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 54000, 720, 739,
 		   801, 858, 0, 480, 488, 494, 525, 0,
 		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
 			DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK),
 	  .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, },
-	/* 59 - 720(1440)x480i@240 */
+	/* 59 - 720(1440)x480i@240Hz */
 	{ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 54000, 720, 739,
 		   801, 858, 0, 480, 488, 494, 525, 0,
 		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[ville.syrjala]

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>
---
 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);
+
+	if (((vic == 8 || vic == 9 ||
+	      vic == 12 || vic == 13) && mode->vtotal < 263) ||
+	    ((vic == 23 || vic == 24 ||
+	      vic == 27 || vic == 28) && mode->vtotal < 314)) {
+		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);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment

[Andrzej Hajda]

On 03.11.2016 13:53, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> All the VICs apart from 58 and 59 have the word "Hz" included in the
> comment. Include it for 59 and 59 as well.
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
--
Regards
Andrzej

> ---
>  drivers/gpu/drm/drm_edid.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9506933b41cd..7eec18925b70 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -957,13 +957,13 @@ static const struct drm_display_mode edid_cea_modes[] = {
>  		   798, 858, 0, 480, 489, 495, 525, 0,
>  		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
>  	  .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },
> -	/* 58 - 720(1440)x480i@240 */
> +	/* 58 - 720(1440)x480i@240Hz */
>  	{ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 54000, 720, 739,
>  		   801, 858, 0, 480, 488, 494, 525, 0,
>  		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
>  			DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK),
>  	  .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, },
> -	/* 59 - 720(1440)x480i@240 */
> +	/* 59 - 720(1440)x480i@240Hz */
>  	{ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 54000, 720, 739,
>  		   801, 858, 0, 480, 488, 494, 525, 0,
>  		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/edid: Add the missing "Hz" to VIC 58,59 comment

[Daniel Vetter]

On Fri, Nov 04, 2016 at 08:29:11AM +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>
> >
> > All the VICs apart from 58 and 59 have the word "Hz" included in the
> > comment. Include it for 59 and 59 as well.
> >
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Applied to drm-misc. Can you pls also review patch 2/2?

Thanks, Daniel

> --
> Regards
> Andrzej
> 
> > ---
> >  drivers/gpu/drm/drm_edid.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 9506933b41cd..7eec18925b70 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -957,13 +957,13 @@ static const struct drm_display_mode edid_cea_modes[] = {
> >  		   798, 858, 0, 480, 489, 495, 525, 0,
> >  		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
> >  	  .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_16_9, },
> > -	/* 58 - 720(1440)x480i@240 */
> > +	/* 58 - 720(1440)x480i@240Hz */
> >  	{ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 54000, 720, 739,
> >  		   801, 858, 0, 480, 488, 494, 525, 0,
> >  		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
> >  			DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK),
> >  	  .vrefresh = 240, .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3, },
> > -	/* 59 - 720(1440)x480i@240 */
> > +	/* 59 - 720(1440)x480i@240Hz */
> >  	{ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 54000, 720, 739,
> >  		   801, 858, 0, 480, 488, 494, 525, 0,
> >  		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrzej Hajda]

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.

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


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




_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

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

[Ville Syrjälä]

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>

I decieded to leave the ideas for further improvements to cook
longer.

Patch pushed to drm-misc-next. Thanks for the review.

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

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