[PATCH 2/2] drm/udl: implement cursor.

2015-03-05 Thread Haixia Shi 
On Thu, Mar 5, 2015 at 3:54 AM, David Herrmann  wrote:
> Hi
>
> Why pretend the hw supports cursors if it clearly does not? I don't
> see why we cannot leave this to user-space (which can apply
> optimizations for software-only devices, which the kernel cannot). And
> a commit-message would also be nice.

IMHO implementing the drm cursor functions allows user-space to be
platform agnostic.

Will address the other comments and send updated patch in this thread, thanks.

>
> Thanks
> David

[PATCH 2/2] drm/udl: implement cursor.

2015-03-05 Thread David Herrmann 
Hi

On Thu, Mar 5, 2015 at 3:26 AM, Haixia Shi  wrote:
> Signed-off-by: Haixia Shi 
> Reviewed-by: Stéphane Marchesin 
> Tested-by: Haixia Shi 
> ---
>  drivers/gpu/drm/udl/Makefile   |   2 +-
>  drivers/gpu/drm/udl/udl_cursor.c   | 156 
> +
>  drivers/gpu/drm/udl/udl_cursor.h   |  46 +++
>  drivers/gpu/drm/udl/udl_drv.h  |   4 +
>  drivers/gpu/drm/udl/udl_fb.c   |  31 
>  drivers/gpu/drm/udl/udl_gem.c  |   3 +
>  drivers/gpu/drm/udl/udl_main.c |  10 +++
>  drivers/gpu/drm/udl/udl_modeset.c  |  45 +++
>  drivers/gpu/drm/udl/udl_transfer.c |  36 -
>  9 files changed, 317 insertions(+), 16 deletions(-)
>  create mode 100644 drivers/gpu/drm/udl/udl_cursor.c
>  create mode 100644 drivers/gpu/drm/udl/udl_cursor.h

Why pretend the hw supports cursors if it clearly does not? I don't
see why we cannot leave this to user-space (which can apply
optimizations for software-only devices, which the kernel cannot). And
a commit-message would also be nice.

Thanks
David

> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
> index 195bcac..67ccab4 100644
> --- a/drivers/gpu/drm/udl/Makefile
> +++ b/drivers/gpu/drm/udl/Makefile
> @@ -1,6 +1,6 @@
>
>  ccflags-y := -Iinclude/drm
>
> -udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_encoder.o udl_main.o 
> udl_fb.o udl_transfer.o udl_gem.o udl_dmabuf.o
> +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_encoder.o udl_main.o 
> udl_fb.o udl_transfer.o udl_gem.o udl_dmabuf.o udl_cursor.o
>
>  obj-$(CONFIG_DRM_UDL) := udl.o
> diff --git a/drivers/gpu/drm/udl/udl_cursor.c 
> b/drivers/gpu/drm/udl/udl_cursor.c
> new file mode 100644
> index 000..3b38ef5
> --- /dev/null
> +++ b/drivers/gpu/drm/udl/udl_cursor.c
> @@ -0,0 +1,156 @@
> +/*
> + * udl_cursor.c
> + *
> + * Copyright (c) 2015 The Chromium OS Authors
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see .
> + */
> +
> +#include 
> +#include 
> +
> +#include "udl_cursor.h"
> +#include "udl_drv.h"
> +
> +#define UDL_CURSOR_W 64
> +#define UDL_CURSOR_H 64
> +#define UDL_CURSOR_BUF (UDL_CURSOR_W * UDL_CURSOR_H)
> +
> +/*
> + * UDL drm cursor private structure.
> + */
> +struct udl_cursor {
> +   uint32_t buffer[UDL_CURSOR_BUF];
> +   bool enabled;
> +   int x;
> +   int y;
> +};
> +
> +int udl_cursor_alloc(struct udl_cursor **cursor)
> +{
> +   struct udl_cursor *new_cursor = kzalloc(sizeof(struct udl_cursor),
> +   GFP_KERNEL);
> +   if (!new_cursor)
> +   return -ENOMEM;
> +   *cursor = new_cursor;
> +   return 0;
> +}
> +
> +void udl_cursor_free(struct udl_cursor *cursor)
> +{
> +   BUG_ON(!cursor);
> +   kfree(cursor);
> +}
> +
> +void udl_cursor_copy(struct udl_cursor *dst, struct udl_cursor *src)
> +{
> +   memcpy(dst, src, sizeof(struct udl_cursor));
> +}
> +
> +bool udl_cursor_enabled(struct udl_cursor *cursor)
> +{
> +   return cursor->enabled;
> +}
> +
> +void udl_cursor_get_hline(struct udl_cursor *cursor, int x, int y,
> +   struct udl_cursor_hline *hline)
> +{
> +   if (!cursor || !cursor->enabled ||
> +   x >= cursor->x + UDL_CURSOR_W ||
> +   y < cursor->y || y >= cursor->y + UDL_CURSOR_H) {
> +   hline->buffer = NULL;
> +   return;
> +   }
> +
> +   hline->buffer = >buffer[UDL_CURSOR_W * (y - cursor->y)];
> +   hline->width = UDL_CURSOR_W;
> +   hline->offset = x - cursor->x;
> +}
> +
> +/*
> + * Return pre-computed cursor blend value defined as:
> + * R: 5 bits (bit 0:4)
> + * G: 6 bits (bit 5:10)
> + * B: 5 bits (bit 11:15)
> + * A: 7 bits (bit 16:22)
> + */
> +static uint32_t cursor_blend_val32(uint32_t pix)
> +{
> +   /* range of alpha_scaled is 0..64 */
> +   uint32_t alpha_scaled = ((pix >> 24) * 65) >> 8;
> +   return ((pix >> 3) & 0x1f) |
> +   ((pix >> 5) & 0x7e0) |
> +   ((pix >> 8) & 0xf800) |
> +   (alpha_scaled << 16);
> +}
> +
> +static int udl_cursor_download(struct udl_cursor *cursor,
> +   struct drm_gem_object *obj)
> +{
> +   struct udl_gem_object *udl_gem_obj = to_udl_bo(obj);
> +   uint32_t *src_ptr, *dst_ptr;
> +   size_t i;
> +
> +   int ret = udl_gem_vmap(udl_gem_obj);
> +   if (ret != 0) {
> +

[PATCH 2/2] drm/udl: implement cursor.

2015-03-05 Thread Chris Wilson 
On Wed, Mar 04, 2015 at 06:26:24PM -0800, Haixia Shi wrote:
> Signed-off-by: Haixia Shi 
> Reviewed-by: Stéphane Marchesin 
> Tested-by: Haixia Shi 
> ---

> +static int udl_cursor_download(struct udl_cursor *cursor,
> + struct drm_gem_object *obj)
> +{
> + struct udl_gem_object *udl_gem_obj = to_udl_bo(obj);
> + uint32_t *src_ptr, *dst_ptr;
> + size_t i;
> +
> + int ret = udl_gem_vmap(udl_gem_obj);
> + if (ret != 0) {
> + DRM_ERROR("failed to vmap cursor\n");
> + return ret;
> + }
> +
> + src_ptr = udl_gem_obj->vmapping;
> + dst_ptr = cursor->buffer;
> + for (i = 0; i < UDL_CURSOR_BUF; ++i)
> + dst_ptr[i] = cursor_blend_val32(le32_to_cpu(src_ptr[i]));

You never verify that udl_gem_obj->size >= UDL_CURSOR_BUF*sizeof(uint32_t)

> @@ -220,14 +216,21 @@ int udl_handle_damage(struct udl_framebuffer *fb, int 
> x, int y,
>   return 0;
>   cmd = urb->transfer_buffer;
>  
> + mutex_lock(>struct_mutex);
> + if (udl_cursor_alloc(_copy) == 0)
> + udl_cursor_copy(cursor_copy, udl->cursor);
> + mutex_unlock(>struct_mutex);

Keeping a reference to the udl->cursor->buffer and making it COW looks
quite attrative now. Note that struct_mutex is completely the wrong lock
to be using here! It is not the lock you hold when you are modifing the
cursor. *

cursor = udl_cursor_get(udl->cursor);

return NULL on error and make the subsequent code deal with it (rather
than discarding the whole frame update). But if you take the COW reference
approach it should not fail (the failure will only occur when modifying
the cursor, where the error can be propagated back to the user
immediately).


> +static int udl_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file,
> + uint32_t handle, uint32_t width, uint32_t height)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct udl_device *udl = dev->dev_private;
> + int ret;
> +
> + mutex_lock(>struct_mutex);
> + ret = udl_cursor_set(crtc, file, handle, width, height, udl->cursor);
> + mutex_unlock(>struct_mutex);

* Oh, I see. udl_crtc_cursor_set() is already locked, so you don't need
struct_mutex as well.

> + if (ret) {
> + DRM_ERROR("Failed to set UDL cursor\n");
> + return ret;
> + }
> +
> + return udl_crtc_page_flip(crtc, NULL, NULL, 0);

You have to be kidding me! You redraw the entire frame because the
cursor changes is is moved! We might as well leave the cursor to
userspace, it will be much more performant.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/2] drm/udl: implement cursor.

2015-03-04 Thread Haixia Shi 
Signed-off-by: Haixia Shi 
Reviewed-by: Stéphane Marchesin 
Tested-by: Haixia Shi 
---
 drivers/gpu/drm/udl/Makefile   |   2 +-
 drivers/gpu/drm/udl/udl_cursor.c   | 156 +
 drivers/gpu/drm/udl/udl_cursor.h   |  46 +++
 drivers/gpu/drm/udl/udl_drv.h  |   4 +
 drivers/gpu/drm/udl/udl_fb.c   |  31 
 drivers/gpu/drm/udl/udl_gem.c  |   3 +
 drivers/gpu/drm/udl/udl_main.c |  10 +++
 drivers/gpu/drm/udl/udl_modeset.c  |  45 +++
 drivers/gpu/drm/udl/udl_transfer.c |  36 -
 9 files changed, 317 insertions(+), 16 deletions(-)
 create mode 100644 drivers/gpu/drm/udl/udl_cursor.c
 create mode 100644 drivers/gpu/drm/udl/udl_cursor.h

diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
index 195bcac..67ccab4 100644
--- a/drivers/gpu/drm/udl/Makefile
+++ b/drivers/gpu/drm/udl/Makefile
@@ -1,6 +1,6 @@

 ccflags-y := -Iinclude/drm

-udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_encoder.o udl_main.o 
udl_fb.o udl_transfer.o udl_gem.o udl_dmabuf.o
+udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_encoder.o udl_main.o 
udl_fb.o udl_transfer.o udl_gem.o udl_dmabuf.o udl_cursor.o

 obj-$(CONFIG_DRM_UDL) := udl.o
diff --git a/drivers/gpu/drm/udl/udl_cursor.c b/drivers/gpu/drm/udl/udl_cursor.c
new file mode 100644
index 000..3b38ef5
--- /dev/null
+++ b/drivers/gpu/drm/udl/udl_cursor.c
@@ -0,0 +1,156 @@
+/*
+ * udl_cursor.c
+ *
+ * Copyright (c) 2015 The Chromium OS Authors
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see .
+ */
+
+#include 
+#include 
+
+#include "udl_cursor.h"
+#include "udl_drv.h"
+
+#define UDL_CURSOR_W 64
+#define UDL_CURSOR_H 64
+#define UDL_CURSOR_BUF (UDL_CURSOR_W * UDL_CURSOR_H)
+
+/*
+ * UDL drm cursor private structure.
+ */
+struct udl_cursor {
+   uint32_t buffer[UDL_CURSOR_BUF];
+   bool enabled;
+   int x;
+   int y;
+};
+
+int udl_cursor_alloc(struct udl_cursor **cursor)
+{
+   struct udl_cursor *new_cursor = kzalloc(sizeof(struct udl_cursor),
+   GFP_KERNEL);
+   if (!new_cursor)
+   return -ENOMEM;
+   *cursor = new_cursor;
+   return 0;
+}
+
+void udl_cursor_free(struct udl_cursor *cursor)
+{
+   BUG_ON(!cursor);
+   kfree(cursor);
+}
+
+void udl_cursor_copy(struct udl_cursor *dst, struct udl_cursor *src)
+{
+   memcpy(dst, src, sizeof(struct udl_cursor));
+}
+
+bool udl_cursor_enabled(struct udl_cursor *cursor)
+{
+   return cursor->enabled;
+}
+
+void udl_cursor_get_hline(struct udl_cursor *cursor, int x, int y,
+   struct udl_cursor_hline *hline)
+{
+   if (!cursor || !cursor->enabled ||
+   x >= cursor->x + UDL_CURSOR_W ||
+   y < cursor->y || y >= cursor->y + UDL_CURSOR_H) {
+   hline->buffer = NULL;
+   return;
+   }
+
+   hline->buffer = >buffer[UDL_CURSOR_W * (y - cursor->y)];
+   hline->width = UDL_CURSOR_W;
+   hline->offset = x - cursor->x;
+}
+
+/*
+ * Return pre-computed cursor blend value defined as:
+ * R: 5 bits (bit 0:4)
+ * G: 6 bits (bit 5:10)
+ * B: 5 bits (bit 11:15)
+ * A: 7 bits (bit 16:22)
+ */
+static uint32_t cursor_blend_val32(uint32_t pix)
+{
+   /* range of alpha_scaled is 0..64 */
+   uint32_t alpha_scaled = ((pix >> 24) * 65) >> 8;
+   return ((pix >> 3) & 0x1f) |
+   ((pix >> 5) & 0x7e0) |
+   ((pix >> 8) & 0xf800) |
+   (alpha_scaled << 16);
+}
+
+static int udl_cursor_download(struct udl_cursor *cursor,
+   struct drm_gem_object *obj)
+{
+   struct udl_gem_object *udl_gem_obj = to_udl_bo(obj);
+   uint32_t *src_ptr, *dst_ptr;
+   size_t i;
+
+   int ret = udl_gem_vmap(udl_gem_obj);
+   if (ret != 0) {
+   DRM_ERROR("failed to vmap cursor\n");
+   return ret;
+   }
+
+   src_ptr = udl_gem_obj->vmapping;
+   dst_ptr = cursor->buffer;
+   for (i = 0; i < UDL_CURSOR_BUF; ++i)
+   dst_ptr[i] = cursor_blend_val32(le32_to_cpu(src_ptr[i]));
+   return 0;
+}
+
+int udl_cursor_set(struct drm_crtc *crtc, struct drm_file *file,
+   uint32_t handle, uint32_t width, uint32_t height,
+   struct udl_cursor *cursor)
+{
+   if (handle) {
+   struct drm_gem_object *obj;
+   int err;
+   /* Currently we only