diff mbox

drm/edid : calculate vsync and hsync from range limits block according to the EDID 1.4

Message ID 1462287925-2668-2-git-send-email-vitaly.prosyak@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vitaly Prosyak May 3, 2016, 3:05 p.m. UTC
Do calculation of vsync and hsync from range limits
EDID block according to the spec. EDID 1.4.

Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
---
 drivers/gpu/drm/drm_edid.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Daniel Vetter May 3, 2016, 8:26 p.m. UTC | #1
On Tue, May 03, 2016 at 11:05:25AM -0400, Vitaly Prosyak wrote:
> Do calculation of vsync and hsync from range limits
> EDID block according to the spec. EDID 1.4.
> 
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>

Seems unrelated to the previous patch, please submit in a separate series.

> ---
>  drivers/gpu/drm/drm_edid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7e49962..601152b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1977,11 +1977,11 @@ mode_in_hsync_range(const struct drm_display_mode *mode,
>  	int hsync, hmin, hmax;
>  
>  	hmin = t[7];
> -	if (edid->revision >= 4)
> -	    hmin += ((t[4] & 0x04) ? 255 : 0);
> +	if (edid->revision >= 4 && ((t[4] & 0x0c) == 0x0c))
> +	    hmin += 255 ;
>  	hmax = t[8];
> -	if (edid->revision >= 4)
> -	    hmax += ((t[4] & 0x08) ? 255 : 0);
> +	if (edid->revision >= 4 && (t[4] & 0x08))
> +	    hmax += 255;

Please don't reshuffle code without functional changes. Same above, for
such small bugfixes it's best to only change what you need changed.

It might also be useful to introduce symbolic #defines for all these magic
constants.
-Daniel


>  	hsync = drm_mode_hsync(mode);
>  
>  	return (hsync <= hmax && hsync >= hmin);
> @@ -1994,11 +1994,11 @@ mode_in_vsync_range(const struct drm_display_mode *mode,
>  	int vsync, vmin, vmax;
>  
>  	vmin = t[5];
> -	if (edid->revision >= 4)
> -	    vmin += ((t[4] & 0x01) ? 255 : 0);
> +	if (edid->revision >= 4 && ((t[4] & 0x03) == 0x03))
> +	    vmin += 255;
>  	vmax = t[6];
> -	if (edid->revision >= 4)
> -	    vmax += ((t[4] & 0x02) ? 255 : 0);
> +	if (edid->revision >= 4 && (t[4] & 0x02))
> +	    vmax += 255;
>  	vsync = drm_mode_vrefresh(mode);
>  
>  	return (vsync <= vmax && vsync >= vmin);
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter May 3, 2016, 8:26 p.m. UTC | #2
On Tue, May 03, 2016 at 11:05:25AM -0400, Vitaly Prosyak wrote:
> Do calculation of vsync and hsync from range limits
> EDID block according to the spec. EDID 1.4.
> 
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>

Forgot one: For anything related to edid please cc Adam Jackson. Applies
to both patches.
-Daniel

> ---
>  drivers/gpu/drm/drm_edid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7e49962..601152b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1977,11 +1977,11 @@ mode_in_hsync_range(const struct drm_display_mode *mode,
>  	int hsync, hmin, hmax;
>  
>  	hmin = t[7];
> -	if (edid->revision >= 4)
> -	    hmin += ((t[4] & 0x04) ? 255 : 0);
> +	if (edid->revision >= 4 && ((t[4] & 0x0c) == 0x0c))
> +	    hmin += 255 ;
>  	hmax = t[8];
> -	if (edid->revision >= 4)
> -	    hmax += ((t[4] & 0x08) ? 255 : 0);
> +	if (edid->revision >= 4 && (t[4] & 0x08))
> +	    hmax += 255;
>  	hsync = drm_mode_hsync(mode);
>  
>  	return (hsync <= hmax && hsync >= hmin);
> @@ -1994,11 +1994,11 @@ mode_in_vsync_range(const struct drm_display_mode *mode,
>  	int vsync, vmin, vmax;
>  
>  	vmin = t[5];
> -	if (edid->revision >= 4)
> -	    vmin += ((t[4] & 0x01) ? 255 : 0);
> +	if (edid->revision >= 4 && ((t[4] & 0x03) == 0x03))
> +	    vmin += 255;
>  	vmax = t[6];
> -	if (edid->revision >= 4)
> -	    vmax += ((t[4] & 0x02) ? 255 : 0);
> +	if (edid->revision >= 4 && (t[4] & 0x02))
> +	    vmax += 255;
>  	vsync = drm_mode_vrefresh(mode);
>  
>  	return (vsync <= vmax && vsync >= vmin);
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Vitaly Prosyak May 4, 2016, 1:42 p.m. UTC | #3
Reduce the image size.

Yes, this is unrelated to the previous patch, but for the  coding  it is important to have same style for range limit EIDD block.
I mean the following: for calculation  if mode is in range one style ,but for ranges retrieval another style.
I am proposing to follow the style which looks to me closer to the spec.

I have attached the screenshot demonstrates those magic numbers.
Vitaly

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, May 03, 2016 4:26 PM
To: Prosyak, Vitaly
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/edid : calculate vsync and hsync from range limits block according to the EDID 1.4

On Tue, May 03, 2016 at 11:05:25AM -0400, Vitaly Prosyak wrote:
> Do calculation of vsync and hsync from range limits EDID block 
> according to the spec. EDID 1.4.
> 
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>

Seems unrelated to the previous patch, please submit in a separate series.

> ---
>  drivers/gpu/drm/drm_edid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
> index 7e49962..601152b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1977,11 +1977,11 @@ mode_in_hsync_range(const struct drm_display_mode *mode,
>  	int hsync, hmin, hmax;
>  
>  	hmin = t[7];
> -	if (edid->revision >= 4)
> -	    hmin += ((t[4] & 0x04) ? 255 : 0);
> +	if (edid->revision >= 4 && ((t[4] & 0x0c) == 0x0c))
> +	    hmin += 255 ;
>  	hmax = t[8];
> -	if (edid->revision >= 4)
> -	    hmax += ((t[4] & 0x08) ? 255 : 0);
> +	if (edid->revision >= 4 && (t[4] & 0x08))
> +	    hmax += 255;

Please don't reshuffle code without functional changes. Same above, for such small bugfixes it's best to only change what you need changed.

It might also be useful to introduce symbolic #defines for all these magic constants.
-Daniel


>  	hsync = drm_mode_hsync(mode);
>  
>  	return (hsync <= hmax && hsync >= hmin); @@ -1994,11 +1994,11 @@ 
> mode_in_vsync_range(const struct drm_display_mode *mode,
>  	int vsync, vmin, vmax;
>  
>  	vmin = t[5];
> -	if (edid->revision >= 4)
> -	    vmin += ((t[4] & 0x01) ? 255 : 0);
> +	if (edid->revision >= 4 && ((t[4] & 0x03) == 0x03))
> +	    vmin += 255;
>  	vmax = t[6];
> -	if (edid->revision >= 4)
> -	    vmax += ((t[4] & 0x02) ? 255 : 0);
> +	if (edid->revision >= 4 && (t[4] & 0x02))
> +	    vmax += 255;
>  	vsync = drm_mode_vrefresh(mode);
>  
>  	return (vsync <= vmax && vsync >= vmin);
> --
> 1.9.1
> 
> _______________________________________________
> 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
Jani Nikula May 4, 2016, 2:31 p.m. UTC | #4
On Tue, 03 May 2016, Vitaly Prosyak <vitaly.prosyak@amd.com> wrote:
> Do calculation of vsync and hsync from range limits
> EDID block according to the spec. EDID 1.4.
>
> Signed-off-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7e49962..601152b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1977,11 +1977,11 @@ mode_in_hsync_range(const struct drm_display_mode *mode,
>  	int hsync, hmin, hmax;
>  
>  	hmin = t[7];
> -	if (edid->revision >= 4)
> -	    hmin += ((t[4] & 0x04) ? 255 : 0);
> +	if (edid->revision >= 4 && ((t[4] & 0x0c) == 0x0c))
> +	    hmin += 255 ;
>  	hmax = t[8];
> -	if (edid->revision >= 4)
> -	    hmax += ((t[4] & 0x08) ? 255 : 0);
> +	if (edid->revision >= 4 && (t[4] & 0x08))
> +	    hmax += 255;
>  	hsync = drm_mode_hsync(mode);
>  
>  	return (hsync <= hmax && hsync >= hmin);
> @@ -1994,11 +1994,11 @@ mode_in_vsync_range(const struct drm_display_mode *mode,
>  	int vsync, vmin, vmax;
>  
>  	vmin = t[5];
> -	if (edid->revision >= 4)
> -	    vmin += ((t[4] & 0x01) ? 255 : 0);
> +	if (edid->revision >= 4 && ((t[4] & 0x03) == 0x03))
> +	    vmin += 255;
>  	vmax = t[6];
> -	if (edid->revision >= 4)
> -	    vmax += ((t[4] & 0x02) ? 255 : 0);
> +	if (edid->revision >= 4 && (t[4] & 0x02))
> +	    vmax += 255;

Please fix the indentation to use tabs on the lines you change while
you're at it.

BR,
Jani.


>  	vsync = drm_mode_vrefresh(mode);
>  
>  	return (vsync <= vmax && vsync >= vmin);
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7e49962..601152b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1977,11 +1977,11 @@  mode_in_hsync_range(const struct drm_display_mode *mode,
 	int hsync, hmin, hmax;
 
 	hmin = t[7];
-	if (edid->revision >= 4)
-	    hmin += ((t[4] & 0x04) ? 255 : 0);
+	if (edid->revision >= 4 && ((t[4] & 0x0c) == 0x0c))
+	    hmin += 255 ;
 	hmax = t[8];
-	if (edid->revision >= 4)
-	    hmax += ((t[4] & 0x08) ? 255 : 0);
+	if (edid->revision >= 4 && (t[4] & 0x08))
+	    hmax += 255;
 	hsync = drm_mode_hsync(mode);
 
 	return (hsync <= hmax && hsync >= hmin);
@@ -1994,11 +1994,11 @@  mode_in_vsync_range(const struct drm_display_mode *mode,
 	int vsync, vmin, vmax;
 
 	vmin = t[5];
-	if (edid->revision >= 4)
-	    vmin += ((t[4] & 0x01) ? 255 : 0);
+	if (edid->revision >= 4 && ((t[4] & 0x03) == 0x03))
+	    vmin += 255;
 	vmax = t[6];
-	if (edid->revision >= 4)
-	    vmax += ((t[4] & 0x02) ? 255 : 0);
+	if (edid->revision >= 4 && (t[4] & 0x02))
+	    vmax += 255;
 	vsync = drm_mode_vrefresh(mode);
 
 	return (vsync <= vmax && vsync >= vmin);