[PATCH 0/7] drm/udl: Prepare damage handler for simple-pipe KMS

[PATCH 0/7] drm/udl: Prepare damage handler for simple-pipe KMS

[Thomas Zimmermann]

Udl's damage-handling code requires clean up before switching the driver
to simple-pipe helpers. Patches 1, 2 and 7 remove unused state variables
and statistics. Patches 3 to 5 reorganizes the damage handler to be more
readable. Patch 6 moves dma-buf begin/end calls into the damage handler,
so it will run during page-flip andmodeset operations.

The patchset has been tested by running fbdev console emulation, X11, and
Weston with a DisplayLink adapter.

Thomas Zimmermann (7):
  drm/udl: Remove unused statistics counters
  drm/udl: Don't track number of identical and sent pixels per line
  drm/udl: Vmap framebuffer after all tests succeeded in damage handling
  drm/udl: Move clip-rectangle code out of udl_handle_damage()
  drm/udl: Move log-cpp code out of udl_damage_handler()
  drm/udl: Begin/end access to imported buffers in damage-handler
  drm/udl: Remove field lost_pixels from struct udl_device

 drivers/gpu/drm/udl/udl_drv.h      |   8 +-
 drivers/gpu/drm/udl/udl_fb.c       | 129 +++++++++++++++--------------
 drivers/gpu/drm/udl/udl_main.c     |   3 -
 drivers/gpu/drm/udl/udl_transfer.c |  12 ++-
 4 files changed, 72 insertions(+), 80 deletions(-)

--
2.23.0

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

[PATCH 1/7] drm/udl: Remove unused statistics counters

[Thomas Zimmermann]

None of the udl driver's statistics counters is used anywhere. Remove
them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h |  4 ----
 drivers/gpu/drm/udl/udl_fb.c  | 14 +-------------
 2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 66cbe04f832a..42426407c318 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -66,10 +66,6 @@ struct udl_device {
 
 	char mode_buf[1024];
 	uint32_t mode_buf_len;
-	atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
-	atomic_t bytes_identical; /* saved effort with backbuffer comparison */
-	atomic_t bytes_sent; /* to usb, after compression including overhead */
-	atomic_t cpu_kcycles_used; /* transpired during pixel processing */
 };
 
 #define to_udl(x) container_of(x, struct udl_device, drm)
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index c1996ac73a1f..bc033779f6e4 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -68,7 +68,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	struct udl_device *udl = to_udl(dev);
 	int i, ret;
 	char *cmd;
-	cycles_t start_cycles, end_cycles;
 	int bytes_sent = 0;
 	int bytes_identical = 0;
 	struct urb *urb;
@@ -105,8 +104,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 		goto err_drm_gem_shmem_vunmap;
 	}
 
-	start_cycles = get_cycles();
-
 	urb = udl_get_urb(dev);
 	if (!urb)
 		goto out;
@@ -120,7 +117,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 				     &cmd, byte_offset, dev_byte_offset,
 				     width << log_bpp,
 				     &bytes_identical, &bytes_sent))
-			goto error;
+			goto out;
 	}
 
 	if (cmd > (char *) urb->transfer_buffer) {
@@ -134,15 +131,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	} else
 		udl_urb_completion(urb);
 
-error:
-	atomic_add(bytes_sent, &udl->bytes_sent);
-	atomic_add(bytes_identical, &udl->bytes_identical);
-	atomic_add((width * height) << log_bpp, &udl->bytes_rendered);
-	end_cycles = get_cycles();
-	atomic_add(((unsigned int) ((end_cycles - start_cycles)
-		    >> 10)), /* Kcycles */
-		   &udl->cpu_kcycles_used);
-
 out:
 	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
 
-- 
2.23.0

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

[PATCH 2/7] drm/udl: Don't track number of identical and sent pixels per line

[Thomas Zimmermann]

A call to udl_render_hline() returns the number of identical and
sent pixels. None of these values is used. Remove the parameters.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h      | 3 +--
 drivers/gpu/drm/udl/udl_fb.c       | 6 +-----
 drivers/gpu/drm/udl/udl_transfer.c | 4 +---
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 42426407c318..d732c9e47812 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -93,8 +93,7 @@ udl_fb_user_fb_create(struct drm_device *dev,
 
 int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 		     const char *front, char **urb_buf_ptr,
-		     u32 byte_offset, u32 device_byte_offset, u32 byte_width,
-		     int *ident_ptr, int *sent_ptr);
+		     u32 byte_offset, u32 device_byte_offset, u32 byte_width);
 
 struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev,
 						    size_t size);
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index bc033779f6e4..ed6d9476b25b 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -68,8 +68,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	struct udl_device *udl = to_udl(dev);
 	int i, ret;
 	char *cmd;
-	int bytes_sent = 0;
-	int bytes_identical = 0;
 	struct urb *urb;
 	int aligned_x;
 	int log_bpp;
@@ -115,8 +113,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 		const int dev_byte_offset = (fb->width * i + x) << log_bpp;
 		if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
 				     &cmd, byte_offset, dev_byte_offset,
-				     width << log_bpp,
-				     &bytes_identical, &bytes_sent))
+				     width << log_bpp))
 			goto out;
 	}
 
@@ -127,7 +124,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 			*cmd++ = 0xAF;
 		len = cmd - (char *) urb->transfer_buffer;
 		ret = udl_submit_urb(dev, urb, len);
-		bytes_sent += len;
 	} else
 		udl_urb_completion(urb);
 
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index 1973a4c1e358..686358d1f669 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -212,8 +212,7 @@ static void udl_compress_hline16(
 int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 		     const char *front, char **urb_buf_ptr,
 		     u32 byte_offset, u32 device_byte_offset,
-		     u32 byte_width,
-		     int *ident_ptr, int *sent_ptr)
+		     u32 byte_width)
 {
 	const u8 *line_start, *line_end, *next_pixel;
 	u32 base16 = 0 + (device_byte_offset >> log_bpp) * 2;
@@ -237,7 +236,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 			int len = cmd - (u8 *) urb->transfer_buffer;
 			if (udl_submit_urb(dev, urb, len))
 				return 1; /* lost pixels is set */
-			*sent_ptr += len;
 			urb = udl_get_urb(dev);
 			if (!urb)
 				return 1; /* lost_pixels is set */
-- 
2.23.0

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

[PATCH 3/7] drm/udl: Vmap framebuffer after all tests succeeded in damage handling

[Thomas Zimmermann]

We now do the fast tests before the potentially expensive vmap operation.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_fb.c       | 19 +++++++------------
 drivers/gpu/drm/udl/udl_transfer.c |  1 -
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index ed6d9476b25b..dd7ba7f63214 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -85,12 +85,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	}
 	spin_unlock(&udl->active_fb_16_lock);
 
-	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
-	if (IS_ERR(vaddr)) {
-		DRM_ERROR("failed to vmap fb\n");
-		return 0;
-	}
-
 	aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long));
 	width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long));
 	x = aligned_x;
@@ -98,8 +92,13 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	if ((width <= 0) ||
 	    (x + width > fb->width) ||
 	    (y + height > fb->height)) {
-		ret = -EINVAL;
-		goto err_drm_gem_shmem_vunmap;
+		return -EINVAL;
+	}
+
+	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
+	if (IS_ERR(vaddr)) {
+		DRM_ERROR("failed to vmap fb\n");
+		return 0;
 	}
 
 	urb = udl_get_urb(dev);
@@ -131,10 +130,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
 
 	return 0;
-
-err_drm_gem_shmem_vunmap:
-	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
-	return ret;
 }
 
 static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index 686358d1f669..5fae48723286 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -249,4 +249,3 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 
 	return 0;
 }
-
-- 
2.23.0

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

Re: [PATCH 3/7] drm/udl: Vmap framebuffer after all tests succeeded in damage handling

[Sam Ravnborg]

Hi Thomas.

I did a casual browse of the patches.
Looks like some nice cleanup.

On Wed, Dec 04, 2019 at 02:24:26PM +0100, Thomas Zimmermann wrote:
> We now do the fast tests before the potentially expensive vmap operation.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_fb.c       | 19 +++++++------------
>  drivers/gpu/drm/udl/udl_transfer.c |  1 -
>  2 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index ed6d9476b25b..dd7ba7f63214 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -85,12 +85,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>  	}
>  	spin_unlock(&udl->active_fb_16_lock);
>  
> -	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
> -	if (IS_ERR(vaddr)) {
> -		DRM_ERROR("failed to vmap fb\n");
> -		return 0;
> -	}
> -
>  	aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long));
>  	width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long));
>  	x = aligned_x;
> @@ -98,8 +92,13 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>  	if ((width <= 0) ||
>  	    (x + width > fb->width) ||
>  	    (y + height > fb->height)) {
> -		ret = -EINVAL;
> -		goto err_drm_gem_shmem_vunmap;
> +		return -EINVAL;
> +	}
> +
> +	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
> +	if (IS_ERR(vaddr)) {
> +		DRM_ERROR("failed to vmap fb\n");
> +		return 0;
>  	}
>  
>  	urb = udl_get_urb(dev);
> @@ -131,10 +130,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>  	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
>  
>  	return 0;
> -
> -err_drm_gem_shmem_vunmap:
> -	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
> -	return ret;
This label is reintroduced two patches later.
Is this on purpose it is gone and reintroduced?

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

Re: [PATCH 3/7] drm/udl: Vmap framebuffer after all tests succeeded in damage handling

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2716 bytes --]

Hi Sam

Am 04.12.19 um 15:25 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> I did a casual browse of the patches.
> Looks like some nice cleanup.
> 
> On Wed, Dec 04, 2019 at 02:24:26PM +0100, Thomas Zimmermann wrote:
>> We now do the fast tests before the potentially expensive vmap operation.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/udl/udl_fb.c       | 19 +++++++------------
>>  drivers/gpu/drm/udl/udl_transfer.c |  1 -
>>  2 files changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>> index ed6d9476b25b..dd7ba7f63214 100644
>> --- a/drivers/gpu/drm/udl/udl_fb.c
>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>> @@ -85,12 +85,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>>  	}
>>  	spin_unlock(&udl->active_fb_16_lock);
>>  
>> -	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
>> -	if (IS_ERR(vaddr)) {
>> -		DRM_ERROR("failed to vmap fb\n");
>> -		return 0;
>> -	}
>> -
>>  	aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long));
>>  	width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long));
>>  	x = aligned_x;
>> @@ -98,8 +92,13 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>>  	if ((width <= 0) ||
>>  	    (x + width > fb->width) ||
>>  	    (y + height > fb->height)) {
>> -		ret = -EINVAL;
>> -		goto err_drm_gem_shmem_vunmap;
>> +		return -EINVAL;
>> +	}
>> +
>> +	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
>> +	if (IS_ERR(vaddr)) {
>> +		DRM_ERROR("failed to vmap fb\n");
>> +		return 0;
>>  	}
>>  
>>  	urb = udl_get_urb(dev);
>> @@ -131,10 +130,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>>  	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
>>  
>>  	return 0;
>> -
>> -err_drm_gem_shmem_vunmap:
>> -	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
>> -	return ret;
> This label is reintroduced two patches later.
> Is this on purpose it is gone and reintroduced?

This label is for rollback during error handling. The label in [6/7] is
called out_drm_gem_shmem_vunmap and it's part of the regular flow.

Admittedly, it's a bit unfortunate. OTOH the alternative is to add
dma-buf calls before moving around the vmap call. That seems like the
worse option.

Best regards
Thomas

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 4/7] drm/udl: Move clip-rectangle code out of udl_handle_damage()

[Thomas Zimmermann]

Computing the clip rectable in a separate helper function makes the
damage-handler code more readable.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_fb.c | 48 ++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index dd7ba7f63214..cc2a09a995b8 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -20,9 +20,6 @@
 
 #include "udl_drv.h"
 
-#define DL_ALIGN_UP(x, a) ALIGN(x, a)
-#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
-
 /** Read the red component (0..255) of a 32 bpp colour. */
 #define DLO_RGB_GETRED(col) (uint8_t)((col) & 0xFF)
 
@@ -61,6 +58,28 @@ static uint16_t rgb16(uint32_t col)
 }
 #endif
 
+static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
+				   int width, int height)
+{
+	int x1, x2;
+
+	if (WARN_ON_ONCE(x < 0) ||
+	    WARN_ON_ONCE(y < 0) ||
+	    WARN_ON_ONCE(width < 0) ||
+	    WARN_ON_ONCE(height < 0))
+		return -EINVAL;
+
+	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
+	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
+
+	clip->x1 = x1;
+	clip->y1 = y;
+	clip->x2 = x2;
+	clip->y2 = y + height;
+
+	return 0;
+}
+
 int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 		      int width, int height)
 {
@@ -69,7 +88,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	int i, ret;
 	char *cmd;
 	struct urb *urb;
-	int aligned_x;
+	struct drm_rect clip;
 	int log_bpp;
 	void *vaddr;
 
@@ -85,15 +104,11 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	}
 	spin_unlock(&udl->active_fb_16_lock);
 
-	aligned_x = DL_ALIGN_DOWN(x, sizeof(unsigned long));
-	width = DL_ALIGN_UP(width + (x-aligned_x), sizeof(unsigned long));
-	x = aligned_x;
-
-	if ((width <= 0) ||
-	    (x + width > fb->width) ||
-	    (y + height > fb->height)) {
+	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
+	if (ret)
+		return ret;
+	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
 		return -EINVAL;
-	}
 
 	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
 	if (IS_ERR(vaddr)) {
@@ -106,13 +121,14 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 		goto out;
 	cmd = urb->transfer_buffer;
 
-	for (i = y; i < y + height ; i++) {
+	for (i = clip.y1; i < clip.y2; i++) {
 		const int line_offset = fb->pitches[0] * i;
-		const int byte_offset = line_offset + (x << log_bpp);
-		const int dev_byte_offset = (fb->width * i + x) << log_bpp;
+		const int byte_offset = line_offset + (clip.x1 << log_bpp);
+		const int dev_byte_offset = (fb->width * i + clip.x1) << log_bpp;
+		const int byte_width = (clip.x2 - clip.x1) << log_bpp;
 		if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
 				     &cmd, byte_offset, dev_byte_offset,
-				     width << log_bpp))
+				     byte_width))
 			goto out;
 	}
 
-- 
2.23.0

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

[PATCH 5/7] drm/udl: Move log-cpp code out of udl_damage_handler()

[Thomas Zimmermann]

Computing the cpp value's logarithm in a separate helper function makes
the damage-handler code more readable.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_fb.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index cc2a09a995b8..482786eeea6c 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -58,6 +58,13 @@ static uint16_t rgb16(uint32_t col)
 }
 #endif
 
+static long udl_log_cpp(unsigned int cpp)
+{
+	if (WARN_ON(!is_power_of_2(cpp)))
+		return -EINVAL;
+	return __ffs(cpp);
+}
+
 static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
 				   int width, int height)
 {
@@ -92,11 +99,6 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	int log_bpp;
 	void *vaddr;
 
-	if (WARN_ON(!is_power_of_2(fb->format->cpp[0])))
-		return -EINVAL;
-
-	log_bpp = __ffs(fb->format->cpp[0]);
-
 	spin_lock(&udl->active_fb_16_lock);
 	if (udl->active_fb_16 != fb) {
 		spin_unlock(&udl->active_fb_16_lock);
@@ -104,6 +106,11 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	}
 	spin_unlock(&udl->active_fb_16_lock);
 
+	ret = udl_log_cpp(fb->format->cpp[0]);
+	if (ret < 0)
+		return ret;
+	log_bpp = ret;
+
 	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
 	if (ret)
 		return ret;
-- 
2.23.0

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

[PATCH 6/7] drm/udl: Begin/end access to imported buffers in damage-handler

[Thomas Zimmermann]

The damage-handler code now invokes dma_buf_{begin,end}_access()
for imported buffers. These calls were missing from the page-flip
and modesetting code paths.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_fb.c | 38 ++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 482786eeea6c..7d184ff96a1f 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -92,6 +92,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 {
 	struct drm_device *dev = fb->dev;
 	struct udl_device *udl = to_udl(dev);
+	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
 	int i, ret;
 	char *cmd;
 	struct urb *urb;
@@ -117,15 +118,22 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
 		return -EINVAL;
 
+	if (import_attach) {
+		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
+					       DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
+	}
+
 	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
 	if (IS_ERR(vaddr)) {
 		DRM_ERROR("failed to vmap fb\n");
-		return 0;
+		goto out_dma_buf_end_cpu_access;
 	}
 
 	urb = udl_get_urb(dev);
 	if (!urb)
-		goto out;
+		goto out_drm_gem_shmem_vunmap;
 	cmd = urb->transfer_buffer;
 
 	for (i = clip.y1; i < clip.y2; i++) {
@@ -136,7 +144,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 		if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
 				     &cmd, byte_offset, dev_byte_offset,
 				     byte_width))
-			goto out;
+			goto out_drm_gem_shmem_vunmap;
 	}
 
 	if (cmd > (char *) urb->transfer_buffer) {
@@ -149,10 +157,16 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 	} else
 		udl_urb_completion(urb);
 
-out:
+	ret = 0;
+
+out_drm_gem_shmem_vunmap:
 	drm_gem_shmem_vunmap(fb->obj[0], vaddr);
+out_dma_buf_end_cpu_access:
+	if (import_attach)
+		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
+					     DMA_FROM_DEVICE);
 
-	return 0;
+	return ret;
 }
 
 static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
@@ -162,7 +176,6 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
 				      unsigned num_clips)
 {
 	struct udl_device *udl = fb->dev->dev_private;
-	struct dma_buf_attachment *import_attach;
 	int i;
 	int ret = 0;
 
@@ -175,15 +188,6 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
 	}
 	spin_unlock(&udl->active_fb_16_lock);
 
-	import_attach = fb->obj[0]->import_attach;
-
-	if (import_attach) {
-		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
-					       DMA_FROM_DEVICE);
-		if (ret)
-			goto unlock;
-	}
-
 	for (i = 0; i < num_clips; i++) {
 		ret = udl_handle_damage(fb, clips[i].x1, clips[i].y1,
 					clips[i].x2 - clips[i].x1,
@@ -192,10 +196,6 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
 			break;
 	}
 
-	if (import_attach)
-		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
-					     DMA_FROM_DEVICE);
-
  unlock:
 	drm_modeset_unlock_all(fb->dev);
 
-- 
2.23.0

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

Re: [PATCH 6/7] drm/udl: Begin/end access to imported buffers in damage-handler

[Emil Velikov]

On Wed, 4 Dec 2019 at 13:24, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> The damage-handler code now invokes dma_buf_{begin,end}_access()
> for imported buffers. These calls were missing from the page-flip
> and modesetting code paths.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_fb.c | 38 ++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index 482786eeea6c..7d184ff96a1f 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -92,6 +92,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>  {
>         struct drm_device *dev = fb->dev;
>         struct udl_device *udl = to_udl(dev);
> +       struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
>         int i, ret;
>         char *cmd;
>         struct urb *urb;
> @@ -117,15 +118,22 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>         else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
>                 return -EINVAL;
>
> +       if (import_attach) {
> +               ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> +                                              DMA_FROM_DEVICE);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         vaddr = drm_gem_shmem_vmap(fb->obj[0]);
>         if (IS_ERR(vaddr)) {
>                 DRM_ERROR("failed to vmap fb\n");
> -               return 0;
> +               goto out_dma_buf_end_cpu_access;
>         }
>
>         urb = udl_get_urb(dev);
>         if (!urb)
> -               goto out;
> +               goto out_drm_gem_shmem_vunmap;
>         cmd = urb->transfer_buffer;
>
>         for (i = clip.y1; i < clip.y2; i++) {
> @@ -136,7 +144,7 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>                 if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
>                                      &cmd, byte_offset, dev_byte_offset,
>                                      byte_width))
> -                       goto out;
> +                       goto out_drm_gem_shmem_vunmap;
>         }
>
>         if (cmd > (char *) urb->transfer_buffer) {
> @@ -149,10 +157,16 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
>         } else
>                 udl_urb_completion(urb);
>
> -out:
> +       ret = 0;
> +
> +out_drm_gem_shmem_vunmap:
>         drm_gem_shmem_vunmap(fb->obj[0], vaddr);
> +out_dma_buf_end_cpu_access:
> +       if (import_attach)
> +               ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> +                                            DMA_FROM_DEVICE);
>
> -       return 0;
> +       return ret;
Since you're touching the end_cpu_access call, we might as well
address the following bug. Namely:
Even though we get a failure from udl_render_hline() or otherwise, the
function return "OK" when end_cpu_access() is successful.

If you really want to, one can keep it separate (+ CC stable),
although it's fine to squash it here with a small note in the commit
message.

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

[PATCH 7/7] drm/udl: Remove field lost_pixels from struct udl_device

[Thomas Zimmermann]

The field lost_pixels in struct udl_device was supposed to signal an
error during USB transfers of the framebuffer data. The driver would
have to schedule a re-transfer at a later point. This code was never
implemented. Remove lost_pixels and return regular error codes instead.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h      | 1 -
 drivers/gpu/drm/udl/udl_fb.c       | 7 ++++---
 drivers/gpu/drm/udl/udl_main.c     | 3 ---
 drivers/gpu/drm/udl/udl_transfer.c | 7 ++++---
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index d732c9e47812..4de00bddef39 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -62,7 +62,6 @@ struct udl_device {
 	int sku_pixel_limit;
 
 	struct urb_list urbs;
-	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
 
 	char mode_buf[1024];
 	uint32_t mode_buf_len;
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 7d184ff96a1f..3c77e8e5e417 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -141,9 +141,10 @@ int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 		const int byte_offset = line_offset + (clip.x1 << log_bpp);
 		const int dev_byte_offset = (fb->width * i + clip.x1) << log_bpp;
 		const int byte_width = (clip.x2 - clip.x1) << log_bpp;
-		if (udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
-				     &cmd, byte_offset, dev_byte_offset,
-				     byte_width))
+		ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
+				       &cmd, byte_offset, dev_byte_offset,
+				       byte_width);
+		if (ret)
 			goto out_drm_gem_shmem_vunmap;
 	}
 
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a23218fc7d8e..ff3e98666e8c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -140,7 +140,6 @@ void udl_urb_completion(struct urb *urb)
 		    urb->status == -ESHUTDOWN)) {
 			DRM_ERROR("%s - nonzero write bulk status received: %d\n",
 				__func__, urb->status);
-			atomic_set(&udl->lost_pixels, 1);
 		}
 	}
 
@@ -271,7 +270,6 @@ struct urb *udl_get_urb(struct drm_device *dev)
 	/* Wait for an in-flight buffer to complete and get re-queued */
 	ret = down_timeout(&udl->urbs.limit_sem, GET_URB_TIMEOUT);
 	if (ret) {
-		atomic_set(&udl->lost_pixels, 1);
 		DRM_INFO("wait for urb interrupted: %x available: %d\n",
 		       ret, udl->urbs.available);
 		goto error;
@@ -304,7 +302,6 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 	ret = usb_submit_urb(urb, GFP_ATOMIC);
 	if (ret) {
 		udl_urb_completion(urb); /* because no one else will */
-		atomic_set(&udl->lost_pixels, 1);
 		DRM_ERROR("usb_submit_urb error %x\n", ret);
 	}
 	return ret;
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index 5fae48723286..971927669d6b 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -234,11 +234,12 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
 
 		if (cmd >= cmd_end) {
 			int len = cmd - (u8 *) urb->transfer_buffer;
-			if (udl_submit_urb(dev, urb, len))
-				return 1; /* lost pixels is set */
+			int ret = udl_submit_urb(dev, urb, len);
+			if (ret)
+				return ret;
 			urb = udl_get_urb(dev);
 			if (!urb)
-				return 1; /* lost_pixels is set */
+				return -EAGAIN;
 			*urb_ptr = urb;
 			cmd = urb->transfer_buffer;
 			cmd_end = &cmd[urb->transfer_buffer_length];
-- 
2.23.0

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

Re: [PATCH 0/7] drm/udl: Prepare damage handler for simple-pipe KMS

[Gerd Hoffmann]

On Wed, Dec 04, 2019 at 02:24:23PM +0100, Thomas Zimmermann wrote:
> Udl's damage-handling code requires clean up before switching the driver
> to simple-pipe helpers. Patches 1, 2 and 7 remove unused state variables
> and statistics. Patches 3 to 5 reorganizes the damage handler to be more
> readable. Patch 6 moves dma-buf begin/end calls into the damage handler,
> so it will run during page-flip andmodeset operations.

Nice cleanups.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

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

Re: [PATCH 0/7] drm/udl: Prepare damage handler for simple-pipe KMS

[Emil Velikov]

On Wed, 4 Dec 2019 at 13:24, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Udl's damage-handling code requires clean up before switching the driver
> to simple-pipe helpers. Patches 1, 2 and 7 remove unused state variables
> and statistics. Patches 3 to 5 reorganizes the damage handler to be more
> readable. Patch 6 moves dma-buf begin/end calls into the damage handler,
> so it will run during page-flip andmodeset operations.
>
> The patchset has been tested by running fbdev console emulation, X11, and
> Weston with a DisplayLink adapter.
>
> Thomas Zimmermann (7):
>   drm/udl: Remove unused statistics counters
>   drm/udl: Don't track number of identical and sent pixels per line
>   drm/udl: Vmap framebuffer after all tests succeeded in damage handling
>   drm/udl: Move clip-rectangle code out of udl_handle_damage()
>   drm/udl: Move log-cpp code out of udl_damage_handler()
>   drm/udl: Begin/end access to imported buffers in damage-handler
>   drm/udl: Remove field lost_pixels from struct udl_device
>
There's a bugfix request in 6/7.

Regardless if you squash it in or choose to keep it separate, the series is:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

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