Re: [PATCH] drm/tilcdc: Force recalculation of vrefresh in tilcdc_crtc_set_mode()

[Kevin Hao]

On Thu, Oct 12, 2017 at 12:39:25PM +0300, Jyri Sarha wrote:
> We are using the vrefresh to check if we are too close to vertical
> sync to update the two framebuffer DMA registers and risk a
> collision. The vrefresh is coming from user space and normally it is
> not used for anything. For instance xserver leaves vrefresh to zero
> causing a division by zero when setting the mode. We want to make sure
> the value is valid and force its recalculation in
> tilcdc_crtc_set_mode().
> 
> Reported-by: Kevin Hao 
> Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if 
> CRTC is enabled")
> Cc:  # v4.11+
> Signed-off-by: Jyri Sarha 
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 704db24..aa5e87c 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -459,6 +459,16 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>   drm_framebuffer_get(fb);
>  
>   crtc->hwmode = crtc->state->adjusted_mode;
> +
> + /* We are using the vrefresh to check if we are too close to
> +  * vertical sync to update the two framebuffer DMA registers
> +  * and risk a collision. The vrefresh is coming from user
> +  * space and normally it is not used for anything. We want to
> +  * make sure the value is valid and force its recalculation
> +  * here.
> +  */
> + crtc->hwmode.vrefresh = 0;

Why do we need this?

Thanks,
Kevin

> + crtc->hwmode.vrefresh = drm_mode_vrefresh(>hwmode);
>  }
>  
>  static void tilcdc_crtc_enable(struct drm_crtc *crtc)
> -- 
> 1.9.1
> 
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> 


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tilcdc: Force recalculation of vrefresh in tilcdc_crtc_set_mode()

[Jyri Sarha]

There is a new patch to replace this one:
https://lists.freedesktop.org/archives/dri-devel/2017-October/154589.html

Best regards,
Jyri


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

On 10/12/17 12:39, Jyri Sarha wrote:
> We are using the vrefresh to check if we are too close to vertical
> sync to update the two framebuffer DMA registers and risk a
> collision. The vrefresh is coming from user space and normally it is
> not used for anything. For instance xserver leaves vrefresh to zero
> causing a division by zero when setting the mode. We want to make sure
> the value is valid and force its recalculation in
> tilcdc_crtc_set_mode().
> 
> Reported-by: Kevin Hao 
> Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if 
> CRTC is enabled")
> Cc:  # v4.11+
> Signed-off-by: Jyri Sarha 
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 704db24..aa5e87c 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -459,6 +459,16 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>   drm_framebuffer_get(fb);
>  
>   crtc->hwmode = crtc->state->adjusted_mode;
> +
> + /* We are using the vrefresh to check if we are too close to
> +  * vertical sync to update the two framebuffer DMA registers
> +  * and risk a collision. The vrefresh is coming from user
> +  * space and normally it is not used for anything. We want to
> +  * make sure the value is valid and force its recalculation
> +  * here.
> +  */
> + crtc->hwmode.vrefresh = 0;
> + crtc->hwmode.vrefresh = drm_mode_vrefresh(>hwmode);
>  }
>  
>  static void tilcdc_crtc_enable(struct drm_crtc *crtc)
> 


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

Re: [PATCH] drm/tilcdc: Force recalculation of vrefresh 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/12/17 12:53, Kevin Hao wrote:
> On Thu, Oct 12, 2017 at 12:39:25PM +0300, Jyri Sarha wrote:
>> We are using the vrefresh to check if we are too close to vertical
>> sync to update the two framebuffer DMA registers and risk a
>> collision. The vrefresh is coming from user space and normally it is
>> not used for anything. For instance xserver leaves vrefresh to zero
>> causing a division by zero when setting the mode. We want to make sure
>> the value is valid and force its recalculation in
>> tilcdc_crtc_set_mode().
>>
>> Reported-by: Kevin Hao 
>> Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if 
>> CRTC is enabled")
>> Cc:  # v4.11+
>> Signed-off-by: Jyri Sarha 
>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
>> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> index 704db24..aa5e87c 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> @@ -459,6 +459,16 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>>  drm_framebuffer_get(fb);
>>  
>>  crtc->hwmode = crtc->state->adjusted_mode;
>> +
>> +/* We are using the vrefresh to check if we are too close to
>> + * vertical sync to update the two framebuffer DMA registers
>> + * and risk a collision. The vrefresh is coming from user
>> + * space and normally it is not used for anything. We want to
>> + * make sure the value is valid and force its recalculation
>> + * here.
>> + */
>> +crtc->hwmode.vrefresh = 0;
> 
> Why do we need this?
> 

The drm_mode_vrefresh() [1] checks if the vrefresh is > 0 and only
calculates it if it is not. I want to make sure the value is correct by
recalculating it always. This is to prevent buggy user space SW from
causing undefined behaviour.

Best regards,
Jyri

[1] 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;

if (mode->flags & DRM_MODE_FLAG_INTERLACE)
refresh *= 2;
if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
refresh /= 2;
if (mode->vscan > 1)
refresh /= mode->vscan;
}
return refresh;
}



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

[PATCH] drm/tilcdc: Force recalculation of vrefresh in tilcdc_crtc_set_mode()

[Jyri Sarha]

We are using the vrefresh to check if we are too close to vertical
sync to update the two framebuffer DMA registers and risk a
collision. The vrefresh is coming from user space and normally it is
not used for anything. For instance xserver leaves vrefresh to zero
causing a division by zero when setting the mode. We want to make sure
the value is valid and force its recalculation in
tilcdc_crtc_set_mode().

Reported-by: Kevin Hao 
Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if 
CRTC is enabled")
Cc:  # v4.11+
Signed-off-by: Jyri Sarha 
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 704db24..aa5e87c 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -459,6 +459,16 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
drm_framebuffer_get(fb);
 
crtc->hwmode = crtc->state->adjusted_mode;
+
+   /* We are using the vrefresh to check if we are too close to
+* vertical sync to update the two framebuffer DMA registers
+* and risk a collision. The vrefresh is coming from user
+* space and normally it is not used for anything. We want to
+* make sure the value is valid and force its recalculation
+* here.
+*/
+   crtc->hwmode.vrefresh = 0;
+   crtc->hwmode.vrefresh = drm_mode_vrefresh(>hwmode);
 }
 
 static void tilcdc_crtc_enable(struct drm_crtc *crtc)
-- 
1.9.1


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


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