[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

✓ Fi.CI.BAT: success for series starting with [1/3] drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()
URL   : https://patchwork.freedesktop.org/series/39875/
State : success

== Summary ==

Series 39875v1 series starting with [1/3] drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()
https://patchwork.freedesktop.org/api/1.0/series/39875/revisions/1/mbox/

---- Known issues:

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test kms_chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#103841

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:434s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:383s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:530s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:299s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:509s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:505s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:508s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:494s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:421s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:582s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:591s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:428s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:314s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:533s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:402s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:418s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:469s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:474s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:463s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:516s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:638s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:440s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:526s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:537s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:516s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:485s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:425s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:435s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:409s
Blacklisted hosts:
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:513s
fi-cnl-drrs      total:288  pass:257  dwarn:3   dfail:0   fail:0   skip:28  time:520s

874b86a759851707e26286c22062f6ccc526e46f drm-tip: 2018y-03m-13d-12h-36m-17s UTC integration manifest
bef910279d95 drm: Store the calculated vrefresh in the user mode
25abbb935d42 drm: Make drm_mode_vrefresh() a bit more accurate
edecdbece3ee drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8330/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/3] drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Nuke the useless 'ret' variable from drm_mode_convert_umode()
URL   : https://patchwork.freedesktop.org/series/39875/
State : success

== Summary ==

---- Possible new issues:

Test kms_cursor_crc:
        Subgroup cursor-64x64-suspend:
                skip       -> PASS       (shard-snb)
Test kms_properties:
        Subgroup crtc-properties-legacy:
                skip       -> PASS       (shard-snb)

---- Known issues:

Test kms_flip:
        Subgroup plain-flip-ts-check:
                fail       -> PASS       (shard-hsw) fdo#100368 +3
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-apl) fdo#100047
Test kms_vblank:
        Subgroup pipe-c-accuracy-idle:
                pass       -> FAIL       (shard-hsw) fdo#102583

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#102583 https://bugs.freedesktop.org/show_bug.cgi?id=102583

shard-apl        total:3444 pass:1817 dwarn:1   dfail:0   fail:7   skip:1619 time:13033s
shard-hsw        total:3444 pass:1767 dwarn:1   dfail:0   fail:4   skip:1671 time:11814s
shard-snb        total:3444 pass:1359 dwarn:1   dfail:0   fail:3   skip:2081 time:7209s
Blacklisted hosts:
shard-kbl        total:3371 pass:1899 dwarn:1   dfail:0   fail:9   skip:1461 time:9077s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8330/shards.html
_______________________________________________
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