[PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Jyri Sarha]

We need the total frame refresh time to check if we are too close to
vertical sync when updating the two framebuffer DMA registers and risk
a collision. This new method is more accurate that the previous that
based on mode's vrefresh value, which itself is inaccurate or may not
even be initialized.

Reported-by: Kevin Hao <kexin.hao@windriver.com>
Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled")
Cc: <stable@vger.kernel.org> # v4.11+
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
Since first version:

Change frametime* variable names to hvtotal and use 64-bit division
instead of dynamcally scaled 32-bit division as suggested by Tomi
Valkeinen.

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 704db24..a8b22aa 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -24,6 +24,7 @@
 #include <linux/completion.h>
 #include <linux/dma-mapping.h>
 #include <linux/of_graph.h>
+#include <linux/math64.h>
 
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
@@ -48,6 +49,7 @@ struct tilcdc_crtc {
 	unsigned int lcd_fck_rate;
 
 	ktime_t last_vblank;
+	unsigned int hvtotal_us;
 
 	struct drm_framebuffer *curr_fb;
 	struct drm_framebuffer *next_fb;
@@ -292,6 +294,16 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
 				LCDC_V2_CORE_CLK_EN);
 }
 
+uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode)
+{
+	uint ret;
+
+	ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal,
+			     mode->clock);
+
+	return ret;
+}
+
 static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
@@ -459,6 +471,9 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 	drm_framebuffer_get(fb);
 
 	crtc->hwmode = crtc->state->adjusted_mode;
+
+	tilcdc_crtc->hvtotal_us =
+		tilcdc_mode_hvtotal(&crtc->hwmode);
 }
 
 static void tilcdc_crtc_enable(struct drm_crtc *crtc)
@@ -642,7 +657,7 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 		spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
 
 		next_vblank = ktime_add_us(tilcdc_crtc->last_vblank,
-					   1000000 / crtc->hwmode.vrefresh);
+					   tilcdc_crtc->hvtotal_us);
 		tdiff = ktime_to_us(ktime_sub(next_vblank, ktime_get()));
 
 		if (tdiff < TILCDC_VBLANK_SAFETY_THRESHOLD_US)
-- 
1.9.1

[PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Jyri Sarha]

We need the total frame refresh time to check if we are too close to
vertical sync when updating the two framebuffer DMA registers and risk
a collision. This new method is more accurate that the previous that
based on mode's vrefresh value, which itself is inaccurate or may not
even be initialized.

Reported-by: Kevin Hao <kexin.hao@windriver.com>
Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled")
Cc: <stable@vger.kernel.org> # v4.11+
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
Since first version:

Change frametime* variable names to hvtotal and use 64-bit division
instead of dynamcally scaled 32-bit division as suggested by Tomi
Valkeinen.

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 704db24..a8b22aa 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -24,6 +24,7 @@
 #include <linux/completion.h>
 #include <linux/dma-mapping.h>
 #include <linux/of_graph.h>
+#include <linux/math64.h>
 
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
@@ -48,6 +49,7 @@ struct tilcdc_crtc {
 	unsigned int lcd_fck_rate;
 
 	ktime_t last_vblank;
+	unsigned int hvtotal_us;
 
 	struct drm_framebuffer *curr_fb;
 	struct drm_framebuffer *next_fb;
@@ -292,6 +294,16 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
 				LCDC_V2_CORE_CLK_EN);
 }
 
+uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode)
+{
+	uint ret;
+
+	ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal,
+			     mode->clock);
+
+	return ret;
+}
+
 static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
@@ -459,6 +471,9 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
 	drm_framebuffer_get(fb);
 
 	crtc->hwmode = crtc->state->adjusted_mode;
+
+	tilcdc_crtc->hvtotal_us =
+		tilcdc_mode_hvtotal(&crtc->hwmode);
 }
 
 static void tilcdc_crtc_enable(struct drm_crtc *crtc)
@@ -642,7 +657,7 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 		spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
 
 		next_vblank = ktime_add_us(tilcdc_crtc->last_vblank,
-					   1000000 / crtc->hwmode.vrefresh);
+					   tilcdc_crtc->hvtotal_us);
 		tdiff = ktime_to_us(ktime_sub(next_vblank, ktime_get()));
 
 		if (tdiff < TILCDC_VBLANK_SAFETY_THRESHOLD_US)
-- 
1.9.1

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

Re: [PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Tomi Valkeinen]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 13/10/17 12:35, Jyri Sarha wrote:
> We need the total frame refresh time to check if we are too close to
> vertical sync when updating the two framebuffer DMA registers and risk
> a collision. This new method is more accurate that the previous that
> based on mode's vrefresh value, which itself is inaccurate or may not
> even be initialized.
> 
> Reported-by: Kevin Hao <kexin.hao@windriver.com>
> Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled")
> Cc: <stable@vger.kernel.org> # v4.11+
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
> Since first version:
> 
> Change frametime* variable names to hvtotal and use 64-bit division
> instead of dynamcally scaled 32-bit division as suggested by Tomi
> Valkeinen.
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 704db24..a8b22aa 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -24,6 +24,7 @@
>  #include <linux/completion.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/of_graph.h>
> +#include <linux/math64.h>
>  
>  #include "tilcdc_drv.h"
>  #include "tilcdc_regs.h"
> @@ -48,6 +49,7 @@ struct tilcdc_crtc {
>  	unsigned int lcd_fck_rate;
>  
>  	ktime_t last_vblank;
> +	unsigned int hvtotal_us;
>  
>  	struct drm_framebuffer *curr_fb;
>  	struct drm_framebuffer *next_fb;
> @@ -292,6 +294,16 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
>  				LCDC_V2_CORE_CLK_EN);
>  }
>  
> +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode)
> +{
> +	uint ret;
> +
> +	ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal,
> +			     mode->clock);
> +
> +	return ret;
> +}

I don't think "uint" is recommended. Just use u32. And drop the ret
variable, just one-line return statement should be enough.

Otherwise:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

Re: [PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Tomi Valkeinen]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 13/10/17 12:35, Jyri Sarha wrote:
> We need the total frame refresh time to check if we are too close to
> vertical sync when updating the two framebuffer DMA registers and risk
> a collision. This new method is more accurate that the previous that
> based on mode's vrefresh value, which itself is inaccurate or may not
> even be initialized.
> 
> Reported-by: Kevin Hao <kexin.hao@windriver.com>
> Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled")
> Cc: <stable@vger.kernel.org> # v4.11+
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
> Since first version:
> 
> Change frametime* variable names to hvtotal and use 64-bit division
> instead of dynamcally scaled 32-bit division as suggested by Tomi
> Valkeinen.
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 704db24..a8b22aa 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -24,6 +24,7 @@
>  #include <linux/completion.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/of_graph.h>
> +#include <linux/math64.h>
>  
>  #include "tilcdc_drv.h"
>  #include "tilcdc_regs.h"
> @@ -48,6 +49,7 @@ struct tilcdc_crtc {
>  	unsigned int lcd_fck_rate;
>  
>  	ktime_t last_vblank;
> +	unsigned int hvtotal_us;
>  
>  	struct drm_framebuffer *curr_fb;
>  	struct drm_framebuffer *next_fb;
> @@ -292,6 +294,16 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
>  				LCDC_V2_CORE_CLK_EN);
>  }
>  
> +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode)
> +{
> +	uint ret;
> +
> +	ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal,
> +			     mode->clock);
> +
> +	return ret;
> +}

I don't think "uint" is recommended. Just use u32. And drop the ret
variable, just one-line return statement should be enough.

Otherwise:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

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

Re: [PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Jyri Sarha]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 10/13/17 14:04, Tomi Valkeinen wrote:
>> +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode)
>> +{
>> +	uint ret;
>> +
>> +	ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal,
>> +			     mode->clock);
>> +
>> +	return ret;
>> +}
> I don't think "uint" is recommended. Just use u32. And drop the ret
> variable, just one-line return statement should be enough.
> 

The ret variable can of course be dropped, but how u32 is better than
uint? If the driver would ever be used in 64bit architecture (highly
unlikely), then it would automatically use the higher precision, and on
32-bit architecture there is no difference.

Also the data member in the private struct is uint and so are the other
similar data members like lcd_fck_rate.

So why change?

Best regards,
Jyri

> Otherwise:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
>  Tomi

Re: [PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Jyri Sarha]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 10/13/17 14:04, Tomi Valkeinen wrote:
>> +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode)
>> +{
>> +	uint ret;
>> +
>> +	ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal,
>> +			     mode->clock);
>> +
>> +	return ret;
>> +}
> I don't think "uint" is recommended. Just use u32. And drop the ret
> variable, just one-line return statement should be enough.
> 

The ret variable can of course be dropped, but how u32 is better than
uint? If the driver would ever be used in 64bit architecture (highly
unlikely), then it would automatically use the higher precision, and on
32-bit architecture there is no difference.

Also the data member in the private struct is uint and so are the other
similar data members like lcd_fck_rate.

So why change?

Best regards,
Jyri

> Otherwise:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
>  Tomi


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

Re: [PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 228 bytes --]

This message contains a digitally signed email which can be read by opening the attachment.
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki



[-- Attachment #2: Type: message/rfc822, Size: 4662 bytes --]

[-- Attachment #2.1.1.1: Type: text/plain, Size: 1162 bytes --]

On 13/10/17 15:00, Jyri Sarha wrote:
> On 10/13/17 14:04, Tomi Valkeinen wrote:
>>> +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode)
>>> +{
>>> +	uint ret;
>>> +
>>> +	ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal,
>>> +			     mode->clock);
>>> +
>>> +	return ret;
>>> +}
>> I don't think "uint" is recommended. Just use u32. And drop the ret
>> variable, just one-line return statement should be enough.
>>
> 
> The ret variable can of course be dropped, but how u32 is better than
> uint? If the driver would ever be used in 64bit architecture (highly
> unlikely), then it would automatically use the higher precision, and on
> 32-bit architecture there is no difference.

I think int is normally 32 bit on 64 bit platforms too. But that depends
on compiler flags.

Where does "uint" come from anyway? I don't think it's part of a C standard.

> Also the data member in the private struct is uint and so are the other
> similar data members like lcd_fck_rate.

Ok. Well, it's good to stick to one way of doing things, so maybe uint
is fine. For any new code, I would not recommend using it.

 Tomi


[-- Attachment #2.1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 228 bytes --]

This message contains a digitally signed email which can be read by opening the attachment.
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki



[-- Attachment #2: Type: message/rfc822, Size: 4662 bytes --]

[-- Attachment #2.1.1.1: Type: text/plain, Size: 1162 bytes --]

On 13/10/17 15:00, Jyri Sarha wrote:
> On 10/13/17 14:04, Tomi Valkeinen wrote:
>>> +uint tilcdc_mode_hvtotal(const struct drm_display_mode *mode)
>>> +{
>>> +	uint ret;
>>> +
>>> +	ret = (uint) div_u64(1000llu * mode->htotal * mode->vtotal,
>>> +			     mode->clock);
>>> +
>>> +	return ret;
>>> +}
>> I don't think "uint" is recommended. Just use u32. And drop the ret
>> variable, just one-line return statement should be enough.
>>
> 
> The ret variable can of course be dropped, but how u32 is better than
> uint? If the driver would ever be used in 64bit architecture (highly
> unlikely), then it would automatically use the higher precision, and on
> 32-bit architecture there is no difference.

I think int is normally 32 bit on 64 bit platforms too. But that depends
on compiler flags.

Where does "uint" come from anyway? I don't think it's part of a C standard.

> Also the data member in the private struct is uint and so are the other
> similar data members like lcd_fck_rate.

Ok. Well, it's good to stick to one way of doing things, so maybe uint
is fine. For any new code, I would not recommend using it.

 Tomi


[-- Attachment #2.1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Jyri Sarha]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 10/13/17 15:32, Tomi Valkeinen wrote:
> Where does "uint" come from anyway? I don't think it's part of a C standard.

AFAIK, it is just a Linux commodity typedef for unsigned int, and
because of that a defacto typedef in many other environments too.

Best regards,
Jyri

Re: [PATCH v2] drm/tilcdc: Precalculate total frametime in tilcdc_crtc_set_mode()

[Jyri Sarha]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 10/13/17 15:32, Tomi Valkeinen wrote:
> Where does "uint" come from anyway? I don't think it's part of a C standard.

AFAIK, it is just a Linux commodity typedef for unsigned int, and
because of that a defacto typedef in many other environments too.

Best regards,
Jyri

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