[PATCH 1/3] drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()

[PATCH 1/3] drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()

[Ville Syrjala]

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

No need to store the return value in a variable since we don't have to
do any unwinding.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 5a8033fda4e3..4157250140b0 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1596,12 +1596,8 @@ int drm_mode_convert_umode(struct drm_device *dev,
 			   struct drm_display_mode *out,
 			   const struct drm_mode_modeinfo *in)
 {
-	int ret = -EINVAL;
-
-	if (in->clock > INT_MAX || in->vrefresh > INT_MAX) {
-		ret = -ERANGE;
-		goto out;
-	}
+	if (in->clock > INT_MAX || in->vrefresh > INT_MAX)
+		return -ERANGE;
 
 	out->clock = in->clock;
 	out->hdisplay = in->hdisplay;
@@ -1622,14 +1618,11 @@ int drm_mode_convert_umode(struct drm_device *dev,
 
 	out->status = drm_mode_validate_driver(dev, out);
 	if (out->status != MODE_OK)
-		goto out;
+		return -EINVAL;
 
 	drm_mode_set_crtcinfo(out, CRTC_INTERLACE_HALVE_V);
 
-	ret = 0;
-
-out:
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.16.1

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

[PATCH 2/3] drm: Make drm_mode_vrefresh() a bit more accurate

[Ville Syrjala]

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

Do the refresh rate calculation with a single division. This gives
us slightly more accurate results, especially for interlaced since
we don't just double the final truncated result.

We do lose one bit compared to the old way, so with an interlaced
mode the new code can only handle ~2GHz instead of the ~4GHz the
old code handeled.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 4157250140b0..f6b7c0e36a1a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -773,24 +773,23 @@ EXPORT_SYMBOL(drm_mode_hsync);
 int drm_mode_vrefresh(const struct drm_display_mode *mode)
 {
 	int refresh = 0;
-	unsigned int calc_val;
 
 	if (mode->vrefresh > 0)
 		refresh = mode->vrefresh;
 	else if (mode->htotal > 0 && mode->vtotal > 0) {
-		int vtotal;
-		vtotal = mode->vtotal;
-		/* work out vrefresh the value will be x1000 */
-		calc_val = (mode->clock * 1000);
-		calc_val /= mode->htotal;
-		refresh = (calc_val + vtotal / 2) / vtotal;
+		unsigned int num, den;
+
+		num = mode->clock * 1000;
+		den = mode->htotal * mode->vtotal;
 
 		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-			refresh *= 2;
+			num *= 2;
 		if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
-			refresh /= 2;
+			den *= 2;
 		if (mode->vscan > 1)
-			refresh /= mode->vscan;
+			den *= mode->vscan;
+
+		refresh = DIV_ROUND_CLOSEST(num, den);
 	}
 	return refresh;
 }
-- 
2.16.1

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

[PATCH 3/3] drm: Store the calculated vrefresh in the user mode

[Ville Syrjala]

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

Ignore the vrefresh in the mode the user passed in and instead
calculate the value based on the actual timings. This way we can
actually trust mode->vrefresh to some degree.

Or should we compare the user's idea of vrefresh with the one
we get from the timings and return an error if they differ? We
can't really be sure what the user is asking in that case.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f6b7c0e36a1a..021526ec6fa0 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1609,7 +1609,11 @@ int drm_mode_convert_umode(struct drm_device *dev,
 	out->vsync_end = in->vsync_end;
 	out->vtotal = in->vtotal;
 	out->vscan = in->vscan;
-	out->vrefresh = in->vrefresh;
+	 /*
+	  * Ignore what the user is saying here and instead
+	  * calculate vrefresh based on the actual timings.
+	  */
+	out->vrefresh = 0;
 	out->flags = in->flags;
 	out->type = in->type;
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
@@ -1619,6 +1623,8 @@ int drm_mode_convert_umode(struct drm_device *dev,
 	if (out->status != MODE_OK)
 		return -EINVAL;
 
+	out->vrefresh = drm_mode_vrefresh(out);
+
 	drm_mode_set_crtcinfo(out, CRTC_INTERLACE_HALVE_V);
 
 	return 0;
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()

[Daniel Vetter]

On Tue, Mar 13, 2018 at 05:07:57PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> No need to store the return value in a variable since we don't have to
> do any unwinding.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_modes.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 5a8033fda4e3..4157250140b0 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1596,12 +1596,8 @@ int drm_mode_convert_umode(struct drm_device *dev,
>  			   struct drm_display_mode *out,
>  			   const struct drm_mode_modeinfo *in)
>  {
> -	int ret = -EINVAL;
> -
> -	if (in->clock > INT_MAX || in->vrefresh > INT_MAX) {
> -		ret = -ERANGE;
> -		goto out;
> -	}
> +	if (in->clock > INT_MAX || in->vrefresh > INT_MAX)
> +		return -ERANGE;
>  
>  	out->clock = in->clock;
>  	out->hdisplay = in->hdisplay;
> @@ -1622,14 +1618,11 @@ int drm_mode_convert_umode(struct drm_device *dev,
>  
>  	out->status = drm_mode_validate_driver(dev, out);
>  	if (out->status != MODE_OK)
> -		goto out;
> +		return -EINVAL;
>  
>  	drm_mode_set_crtcinfo(out, CRTC_INTERLACE_HALVE_V);
>  
> -	ret = 0;
> -
> -out:
> -	return ret;
> +	return 0;
>  }
>  
>  /**
> -- 
> 2.16.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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm: Store the calculated vrefresh in the user mode

[Maarten Lankhorst]

Op 13-03-18 om 16:07 schreef Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Ignore the vrefresh in the mode the user passed in and instead
> calculate the value based on the actual timings. This way we can
> actually trust mode->vrefresh to some degree.
>
> Or should we compare the user's idea of vrefresh with the one
> we get from the timings and return an error if they differ? We
> can't really be sure what the user is asking in that case.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index f6b7c0e36a1a..021526ec6fa0 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1609,7 +1609,11 @@ int drm_mode_convert_umode(struct drm_device *dev,
>  	out->vsync_end = in->vsync_end;
>  	out->vtotal = in->vtotal;
>  	out->vscan = in->vscan;
> -	out->vrefresh = in->vrefresh;
> +	 /*
> +	  * Ignore what the user is saying here and instead
> +	  * calculate vrefresh based on the actual timings.
> +	  */
> +	out->vrefresh = 0;
>  	out->flags = in->flags;
>  	out->type = in->type;
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> @@ -1619,6 +1623,8 @@ int drm_mode_convert_umode(struct drm_device *dev,
>  	if (out->status != MODE_OK)
>  		return -EINVAL;
>  
> +	out->vrefresh = drm_mode_vrefresh(out);
> +
>  	drm_mode_set_crtcinfo(out, CRTC_INTERLACE_HALVE_V);
>  
>  	return 0;

Could we also get this with dim fixes tags dc911f5bd8aa, so we can backport the alt mode handling patch?

And update the documentation about vrefresh, that you can retrieve it with drm_mode_vrefresh?

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm: Make drm_mode_vrefresh() a bit more accurate

[Daniel Vetter]

On Tue, Mar 13, 2018 at 05:07:58PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Do the refresh rate calculation with a single division. This gives
> us slightly more accurate results, especially for interlaced since
> we don't just double the final truncated result.
> 
> We do lose one bit compared to the old way, so with an interlaced
> mode the new code can only handle ~2GHz instead of the ~4GHz the
> old code handeled.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I kinda want special integers here that Oops on overflow, I thought
they're coming. That aside:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_modes.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 4157250140b0..f6b7c0e36a1a 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -773,24 +773,23 @@ EXPORT_SYMBOL(drm_mode_hsync);
>  int drm_mode_vrefresh(const struct drm_display_mode *mode)
>  {
>  	int refresh = 0;
> -	unsigned int calc_val;
>  
>  	if (mode->vrefresh > 0)
>  		refresh = mode->vrefresh;
>  	else if (mode->htotal > 0 && mode->vtotal > 0) {
> -		int vtotal;
> -		vtotal = mode->vtotal;
> -		/* work out vrefresh the value will be x1000 */
> -		calc_val = (mode->clock * 1000);
> -		calc_val /= mode->htotal;
> -		refresh = (calc_val + vtotal / 2) / vtotal;
> +		unsigned int num, den;
> +
> +		num = mode->clock * 1000;
> +		den = mode->htotal * mode->vtotal;
>  
>  		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> -			refresh *= 2;
> +			num *= 2;
>  		if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> -			refresh /= 2;
> +			den *= 2;
>  		if (mode->vscan > 1)
> -			refresh /= mode->vscan;
> +			den *= mode->vscan;
> +
> +		refresh = DIV_ROUND_CLOSEST(num, den);
>  	}
>  	return refresh;
>  }
> -- 
> 2.16.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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm: Make drm_mode_vrefresh() a bit more accurate

[Ville Syrjälä]

On Wed, Mar 14, 2018 at 02:56:28PM +0100, Daniel Vetter wrote:
> On Tue, Mar 13, 2018 at 05:07:58PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Do the refresh rate calculation with a single division. This gives
> > us slightly more accurate results, especially for interlaced since
> > we don't just double the final truncated result.
> > 
> > We do lose one bit compared to the old way, so with an interlaced
> > mode the new code can only handle ~2GHz instead of the ~4GHz the
> > old code handeled.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I kinda want special integers here that Oops on overflow, I thought
> they're coming.

Would be nice indeed.

> That aside:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_modes.c | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 4157250140b0..f6b7c0e36a1a 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -773,24 +773,23 @@ EXPORT_SYMBOL(drm_mode_hsync);
> >  int drm_mode_vrefresh(const struct drm_display_mode *mode)
> >  {
> >  	int refresh = 0;
> > -	unsigned int calc_val;
> >  
> >  	if (mode->vrefresh > 0)
> >  		refresh = mode->vrefresh;
> >  	else if (mode->htotal > 0 && mode->vtotal > 0) {
> > -		int vtotal;
> > -		vtotal = mode->vtotal;
> > -		/* work out vrefresh the value will be x1000 */
> > -		calc_val = (mode->clock * 1000);
> > -		calc_val /= mode->htotal;
> > -		refresh = (calc_val + vtotal / 2) / vtotal;
> > +		unsigned int num, den;
> > +
> > +		num = mode->clock * 1000;
> > +		den = mode->htotal * mode->vtotal;
> >  
> >  		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > -			refresh *= 2;
> > +			num *= 2;
> >  		if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > -			refresh /= 2;
> > +			den *= 2;
> >  		if (mode->vscan > 1)
> > -			refresh /= mode->vscan;
> > +			den *= mode->vscan;
> > +
> > +		refresh = DIV_ROUND_CLOSEST(num, den);
> >  	}
> >  	return refresh;
> >  }
> > -- 
> > 2.16.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

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

Re: [PATCH 3/3] drm: Store the calculated vrefresh in the user mode

[Ville Syrjälä]

On Tue, Mar 13, 2018 at 08:04:03PM +0100, Maarten Lankhorst wrote:
> Op 13-03-18 om 16:07 schreef Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Ignore the vrefresh in the mode the user passed in and instead
> > calculate the value based on the actual timings. This way we can
> > actually trust mode->vrefresh to some degree.
> >
> > Or should we compare the user's idea of vrefresh with the one
> > we get from the timings and return an error if they differ? We
> > can't really be sure what the user is asking in that case.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_modes.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index f6b7c0e36a1a..021526ec6fa0 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1609,7 +1609,11 @@ int drm_mode_convert_umode(struct drm_device *dev,
> >  	out->vsync_end = in->vsync_end;
> >  	out->vtotal = in->vtotal;
> >  	out->vscan = in->vscan;
> > -	out->vrefresh = in->vrefresh;
> > +	 /*
> > +	  * Ignore what the user is saying here and instead
> > +	  * calculate vrefresh based on the actual timings.
> > +	  */
> > +	out->vrefresh = 0;
> >  	out->flags = in->flags;
> >  	out->type = in->type;
> >  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> > @@ -1619,6 +1623,8 @@ int drm_mode_convert_umode(struct drm_device *dev,
> >  	if (out->status != MODE_OK)
> >  		return -EINVAL;
> >  
> > +	out->vrefresh = drm_mode_vrefresh(out);
> > +
> >  	drm_mode_set_crtcinfo(out, CRTC_INTERLACE_HALVE_V);
> >  
> >  	return 0;
> 
> Could we also get this with dim fixes tags dc911f5bd8aa, so we can backport the alt mode handling patch?

Do we want/need to backport it actually?

> 
> And update the documentation about vrefresh, that you can retrieve it with drm_mode_vrefresh?

The whole "return cached value if present, otherwise calculate but don't
update the cached value" apporach always seemed off to me. Might be a
good idea to change it somehow. Maybe just always calculate it, or do
the cached value updates in sensible places so that you only have to
calculate once. But I haven't actually checked how much work that
would entail.

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

Re: [PATCH 2/3] drm: Make drm_mode_vrefresh() a bit more accurate

[Ville Syrjälä]

On Wed, Mar 14, 2018 at 02:56:28PM +0100, Daniel Vetter wrote:
> On Tue, Mar 13, 2018 at 05:07:58PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Do the refresh rate calculation with a single division. This gives
> > us slightly more accurate results, especially for interlaced since
> > we don't just double the final truncated result.
> > 
> > We do lose one bit compared to the old way, so with an interlaced
> > mode the new code can only handle ~2GHz instead of the ~4GHz the
> > old code handeled.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I kinda want special integers here that Oops on overflow, I thought
> they're coming. That aside:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks. Patches 1-2 pushed to drm-misc-next.

> > ---
> >  drivers/gpu/drm/drm_modes.c | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 4157250140b0..f6b7c0e36a1a 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -773,24 +773,23 @@ EXPORT_SYMBOL(drm_mode_hsync);
> >  int drm_mode_vrefresh(const struct drm_display_mode *mode)
> >  {
> >  	int refresh = 0;
> > -	unsigned int calc_val;
> >  
> >  	if (mode->vrefresh > 0)
> >  		refresh = mode->vrefresh;
> >  	else if (mode->htotal > 0 && mode->vtotal > 0) {
> > -		int vtotal;
> > -		vtotal = mode->vtotal;
> > -		/* work out vrefresh the value will be x1000 */
> > -		calc_val = (mode->clock * 1000);
> > -		calc_val /= mode->htotal;
> > -		refresh = (calc_val + vtotal / 2) / vtotal;
> > +		unsigned int num, den;
> > +
> > +		num = mode->clock * 1000;
> > +		den = mode->htotal * mode->vtotal;
> >  
> >  		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > -			refresh *= 2;
> > +			num *= 2;
> >  		if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > -			refresh /= 2;
> > +			den *= 2;
> >  		if (mode->vscan > 1)
> > -			refresh /= mode->vscan;
> > +			den *= mode->vscan;
> > +
> > +		refresh = DIV_ROUND_CLOSEST(num, den);
> >  	}
> >  	return refresh;
> >  }
> > -- 
> > 2.16.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

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