diff mbox

[v2,14/22] drm/exynos: fimd: fix dma burst size setting for small plane size

Message ID 1448891617-18830-15-git-send-email-m.szyprowski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marek Szyprowski Nov. 30, 2015, 1:53 p.m. UTC
This patch fixes trashed display of buffers cropped to very small width.
Even if DMA is unstable and causes tearing when changing the burst size,
it is still better than displaying a garbage.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Comments

Daniel Stone Nov. 30, 2015, 3:40 p.m. UTC | #1
Hi,

On 30 November 2015 at 13:53, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> This patch fixes trashed display of buffers cropped to very small width.
> Even if DMA is unstable and causes tearing when changing the burst size,
> it is still better than displaying a garbage.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Inki Dae Dec. 10, 2015, 11:35 a.m. UTC | #2
Hi Marek,

2015? 11? 30? 22:53? Marek Szyprowski ?(?) ? ?:
> This patch fixes trashed display of buffers cropped to very small width.
> Even if DMA is unstable and causes tearing when changing the burst size,
> it is still better than displaying a garbage.

It seems that this patch is different from above description. I think below patch is just cleanup,
which passes each member necessary instead of passing a drm_framebuffer object.

Thanks,
Inki Dae

> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 70cd2681e343..2e2247126581 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -487,7 +487,7 @@ static void fimd_commit(struct exynos_drm_crtc *crtc)
>  
>  
>  static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
> -				struct drm_framebuffer *fb)
> +				uint32_t pixel_format, int width)
>  {
>  	unsigned long val;
>  
> @@ -498,11 +498,11 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
>  	 * So the request format is ARGB8888 then change it to XRGB8888.
>  	 */
>  	if (ctx->driver_data->has_limited_fmt && !win) {
> -		if (fb->pixel_format == DRM_FORMAT_ARGB8888)
> -			fb->pixel_format = DRM_FORMAT_XRGB8888;
> +		if (pixel_format == DRM_FORMAT_ARGB8888)
> +			pixel_format = DRM_FORMAT_XRGB8888;
>  	}
>  
> -	switch (fb->pixel_format) {
> +	switch (pixel_format) {
>  	case DRM_FORMAT_C8:
>  		val |= WINCON0_BPPMODE_8BPP_PALETTE;
>  		val |= WINCONx_BURSTLEN_8WORD;
> @@ -538,17 +538,15 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
>  		break;
>  	}
>  
> -	DRM_DEBUG_KMS("bpp = %d\n", fb->bits_per_pixel);
> -
>  	/*
> -	 * In case of exynos, setting dma-burst to 16Word causes permanent
> -	 * tearing for very small buffers, e.g. cursor buffer. Burst Mode
> -	 * switching which is based on plane size is not recommended as
> -	 * plane size varies alot towards the end of the screen and rapid
> -	 * movement causes unstable DMA which results into iommu crash/tear.
> +	 * Setting dma-burst to 16Word causes permanent tearing for very small
> +	 * buffers, e.g. cursor buffer. Burst Mode switching which based on
> +	 * plane size is not recommended as plane size varies alot towards the
> +	 * end of the screen and rapid movement causes unstable DMA, but it is
> +	 * still better to change dma-burst than displaying garbage.
>  	 */
>  
> -	if (fb->width < MIN_FB_WIDTH_FOR_16WORD_BURST) {
> +	if (width < MIN_FB_WIDTH_FOR_16WORD_BURST) {
>  		val &= ~WINCONx_BURSTLEN_MASK;
>  		val |= WINCONx_BURSTLEN_4WORD;
>  	}
> @@ -723,7 +721,7 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
>  		DRM_DEBUG_KMS("osd size = 0x%x\n", (unsigned int)val);
>  	}
>  
> -	fimd_win_set_pixfmt(ctx, win, fb);
> +	fimd_win_set_pixfmt(ctx, win, fb->pixel_format, state->src.w);
>  
>  	/* hardware window 0 doesn't support color key. */
>  	if (win != 0)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Dec. 10, 2015, 12:59 p.m. UTC | #3
Hello,

On 2015-12-10 12:35, Inki Dae wrote:
> Hi Marek,
>
> 2015? 11? 30? 22:53? Marek Szyprowski ?(?) ? ?:
>> This patch fixes trashed display of buffers cropped to very small width.
>> Even if DMA is unstable and causes tearing when changing the burst size,
>> it is still better than displaying a garbage.
> It seems that this patch is different from above description. I think below patch is just cleanup,
> which passes each member necessary instead of passing a drm_framebuffer object.

Please note the last chunk of this patch. After applying it width is
taken from state->src.w instead of fb->width. If you want, I can split
this patch into 2 parts - one cleanup without functional change, and
second, replacement of fb->width with state->src.w.


>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_fimd.c | 24 +++++++++++-------------
>>   1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index 70cd2681e343..2e2247126581 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -487,7 +487,7 @@ static void fimd_commit(struct exynos_drm_crtc *crtc)
>>   
>>   
>>   static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
>> -				struct drm_framebuffer *fb)
>> +				uint32_t pixel_format, int width)
>>   {
>>   	unsigned long val;
>>   
>> @@ -498,11 +498,11 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
>>   	 * So the request format is ARGB8888 then change it to XRGB8888.
>>   	 */
>>   	if (ctx->driver_data->has_limited_fmt && !win) {
>> -		if (fb->pixel_format == DRM_FORMAT_ARGB8888)
>> -			fb->pixel_format = DRM_FORMAT_XRGB8888;
>> +		if (pixel_format == DRM_FORMAT_ARGB8888)
>> +			pixel_format = DRM_FORMAT_XRGB8888;
>>   	}
>>   
>> -	switch (fb->pixel_format) {
>> +	switch (pixel_format) {
>>   	case DRM_FORMAT_C8:
>>   		val |= WINCON0_BPPMODE_8BPP_PALETTE;
>>   		val |= WINCONx_BURSTLEN_8WORD;
>> @@ -538,17 +538,15 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
>>   		break;
>>   	}
>>   
>> -	DRM_DEBUG_KMS("bpp = %d\n", fb->bits_per_pixel);
>> -
>>   	/*
>> -	 * In case of exynos, setting dma-burst to 16Word causes permanent
>> -	 * tearing for very small buffers, e.g. cursor buffer. Burst Mode
>> -	 * switching which is based on plane size is not recommended as
>> -	 * plane size varies alot towards the end of the screen and rapid
>> -	 * movement causes unstable DMA which results into iommu crash/tear.
>> +	 * Setting dma-burst to 16Word causes permanent tearing for very small
>> +	 * buffers, e.g. cursor buffer. Burst Mode switching which based on
>> +	 * plane size is not recommended as plane size varies alot towards the
>> +	 * end of the screen and rapid movement causes unstable DMA, but it is
>> +	 * still better to change dma-burst than displaying garbage.
>>   	 */
>>   
>> -	if (fb->width < MIN_FB_WIDTH_FOR_16WORD_BURST) {
>> +	if (width < MIN_FB_WIDTH_FOR_16WORD_BURST) {
>>   		val &= ~WINCONx_BURSTLEN_MASK;
>>   		val |= WINCONx_BURSTLEN_4WORD;
>>   	}
>> @@ -723,7 +721,7 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
>>   		DRM_DEBUG_KMS("osd size = 0x%x\n", (unsigned int)val);
>>   	}
>>   
>> -	fimd_win_set_pixfmt(ctx, win, fb);
>> +	fimd_win_set_pixfmt(ctx, win, fb->pixel_format, state->src.w);
>>   
>>   	/* hardware window 0 doesn't support color key. */
>>   	if (win != 0)
>>

Best regards
Daniel Stone Dec. 10, 2015, 3:36 p.m. UTC | #4
Hi Inki,

On 10 December 2015 at 12:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> On 2015-12-10 12:35, Inki Dae wrote:
>> 2015? 11? 30? 22:53? Marek Szyprowski ?(?) ? ?:
>>> This patch fixes trashed display of buffers cropped to very small width.
>>> Even if DMA is unstable and causes tearing when changing the burst size,
>>> it is still better than displaying a garbage.
>>
>> It seems that this patch is different from above description. I think
>> below patch is just cleanup,
>> which passes each member necessary instead of passing a drm_framebuffer
>> object.
>
>
> Please note the last chunk of this patch. After applying it width is
> taken from state->src.w instead of fb->width. If you want, I can split
> this patch into 2 parts - one cleanup without functional change, and
> second, replacement of fb->width with state->src.w.

As Marek says:
http://lists.freedesktop.org/archives/dri-devel/2015-November/094321.html

It fixes the case where state->src.w (what we're actually scanning
out) < MIN_FB_WIDTH_FOR_16WORD_BURST < fb->width (total size of
allocated buffer). Quite subtle, but also correct.

Cheers,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Inki Dae Dec. 11, 2015, 9:04 a.m. UTC | #5
2015? 12? 10? 21:59? Marek Szyprowski ?(?) ? ?:
> Hello,
> 
> On 2015-12-10 12:35, Inki Dae wrote:
>> Hi Marek,
>>
>> 2015? 11? 30? 22:53? Marek Szyprowski ?(?) ? ?:
>>> This patch fixes trashed display of buffers cropped to very small width.
>>> Even if DMA is unstable and causes tearing when changing the burst size,
>>> it is still better than displaying a garbage.
>> It seems that this patch is different from above description. I think below patch is just cleanup,
>> which passes each member necessary instead of passing a drm_framebuffer object.
> 
> Please note the last chunk of this patch. After applying it width is
> taken from state->src.w instead of fb->width. If you want, I can split
> this patch into 2 parts - one cleanup without functional change, and
> second, replacement of fb->width with state->src.w.

Will just merge it.

Thanks,
Inki Dae

> 
> 
>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos_drm_fimd.c | 24 +++++++++++-------------
>>>   1 file changed, 11 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> index 70cd2681e343..2e2247126581 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> @@ -487,7 +487,7 @@ static void fimd_commit(struct exynos_drm_crtc *crtc)
>>>       static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
>>> -                struct drm_framebuffer *fb)
>>> +                uint32_t pixel_format, int width)
>>>   {
>>>       unsigned long val;
>>>   @@ -498,11 +498,11 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
>>>        * So the request format is ARGB8888 then change it to XRGB8888.
>>>        */
>>>       if (ctx->driver_data->has_limited_fmt && !win) {
>>> -        if (fb->pixel_format == DRM_FORMAT_ARGB8888)
>>> -            fb->pixel_format = DRM_FORMAT_XRGB8888;
>>> +        if (pixel_format == DRM_FORMAT_ARGB8888)
>>> +            pixel_format = DRM_FORMAT_XRGB8888;
>>>       }
>>>   -    switch (fb->pixel_format) {
>>> +    switch (pixel_format) {
>>>       case DRM_FORMAT_C8:
>>>           val |= WINCON0_BPPMODE_8BPP_PALETTE;
>>>           val |= WINCONx_BURSTLEN_8WORD;
>>> @@ -538,17 +538,15 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
>>>           break;
>>>       }
>>>   -    DRM_DEBUG_KMS("bpp = %d\n", fb->bits_per_pixel);
>>> -
>>>       /*
>>> -     * In case of exynos, setting dma-burst to 16Word causes permanent
>>> -     * tearing for very small buffers, e.g. cursor buffer. Burst Mode
>>> -     * switching which is based on plane size is not recommended as
>>> -     * plane size varies alot towards the end of the screen and rapid
>>> -     * movement causes unstable DMA which results into iommu crash/tear.
>>> +     * Setting dma-burst to 16Word causes permanent tearing for very small
>>> +     * buffers, e.g. cursor buffer. Burst Mode switching which based on
>>> +     * plane size is not recommended as plane size varies alot towards the
>>> +     * end of the screen and rapid movement causes unstable DMA, but it is
>>> +     * still better to change dma-burst than displaying garbage.
>>>        */
>>>   -    if (fb->width < MIN_FB_WIDTH_FOR_16WORD_BURST) {
>>> +    if (width < MIN_FB_WIDTH_FOR_16WORD_BURST) {
>>>           val &= ~WINCONx_BURSTLEN_MASK;
>>>           val |= WINCONx_BURSTLEN_4WORD;
>>>       }
>>> @@ -723,7 +721,7 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
>>>           DRM_DEBUG_KMS("osd size = 0x%x\n", (unsigned int)val);
>>>       }
>>>   -    fimd_win_set_pixfmt(ctx, win, fb);
>>> +    fimd_win_set_pixfmt(ctx, win, fb->pixel_format, state->src.w);
>>>         /* hardware window 0 doesn't support color key. */
>>>       if (win != 0)
>>>
> 
> Best regards
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 70cd2681e343..2e2247126581 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -487,7 +487,7 @@  static void fimd_commit(struct exynos_drm_crtc *crtc)
 
 
 static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
-				struct drm_framebuffer *fb)
+				uint32_t pixel_format, int width)
 {
 	unsigned long val;
 
@@ -498,11 +498,11 @@  static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
 	 * So the request format is ARGB8888 then change it to XRGB8888.
 	 */
 	if (ctx->driver_data->has_limited_fmt && !win) {
-		if (fb->pixel_format == DRM_FORMAT_ARGB8888)
-			fb->pixel_format = DRM_FORMAT_XRGB8888;
+		if (pixel_format == DRM_FORMAT_ARGB8888)
+			pixel_format = DRM_FORMAT_XRGB8888;
 	}
 
-	switch (fb->pixel_format) {
+	switch (pixel_format) {
 	case DRM_FORMAT_C8:
 		val |= WINCON0_BPPMODE_8BPP_PALETTE;
 		val |= WINCONx_BURSTLEN_8WORD;
@@ -538,17 +538,15 @@  static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
 		break;
 	}
 
-	DRM_DEBUG_KMS("bpp = %d\n", fb->bits_per_pixel);
-
 	/*
-	 * In case of exynos, setting dma-burst to 16Word causes permanent
-	 * tearing for very small buffers, e.g. cursor buffer. Burst Mode
-	 * switching which is based on plane size is not recommended as
-	 * plane size varies alot towards the end of the screen and rapid
-	 * movement causes unstable DMA which results into iommu crash/tear.
+	 * Setting dma-burst to 16Word causes permanent tearing for very small
+	 * buffers, e.g. cursor buffer. Burst Mode switching which based on
+	 * plane size is not recommended as plane size varies alot towards the
+	 * end of the screen and rapid movement causes unstable DMA, but it is
+	 * still better to change dma-burst than displaying garbage.
 	 */
 
-	if (fb->width < MIN_FB_WIDTH_FOR_16WORD_BURST) {
+	if (width < MIN_FB_WIDTH_FOR_16WORD_BURST) {
 		val &= ~WINCONx_BURSTLEN_MASK;
 		val |= WINCONx_BURSTLEN_4WORD;
 	}
@@ -723,7 +721,7 @@  static void fimd_update_plane(struct exynos_drm_crtc *crtc,
 		DRM_DEBUG_KMS("osd size = 0x%x\n", (unsigned int)val);
 	}
 
-	fimd_win_set_pixfmt(ctx, win, fb);
+	fimd_win_set_pixfmt(ctx, win, fb->pixel_format, state->src.w);
 
 	/* hardware window 0 doesn't support color key. */
 	if (win != 0)