from:"Lukasz Spintzyk"

Re: drm/udl: Implementation of atomic cursor drm_plane

2024-07-02 Thread Lukasz Spintzyk

On 6/24/2024 12:05 PM, Daniel Vetter wrote:

CAUTION: Email originated externally, do not click links or open attachments
unless you recognize the sender and know the content is safe.

On Mon, Jun 24, 2024 at 09:28:29AM +0200, Thomas Zimmermann wrote:

Am 24.06.24 um 09:10 schrieb lukasz.spint...@synaptics.com:

This brings cursor on DisplayLink USB2.0 device on ChromeOS compositor that
requires either crtc'c cursor_set callback
or cursor drm_plane. Patch was tested on ChromeOS and Ubuntu 22.04 with
Gnome/Wayland

NAK on this patchset. UDL has no HW cursor support, so we won't implement
this in the driver. Software blending should be done in userspace, where you
have CPU SIMD available.

I think for drivers which do cpu upload and are vblank driven there's
maybe a case for kernel implemented cursors due to latency reduction if
the blending happens as late as possible. But udl just goes right ahead
and uploads, so this doesn't help I think. damage tracking should, but we
already have that.

Sorry, I don't get what you mean here. This implementation uploads only
damaged primary plane area (gathered in
udl_cursor_plane_helper_atomic_update) and uploads it to device in
following immediately udl_primary_plane_helper_atomic_update. No full
primary plane update is done in this case (which is done only when
plane_state->fb != old_plane_state->fb, please check updated
udl_primary_plane_helper_atomic_update)

Sorry i don't have hard numbers for that but this in overall should
improve cursor latency (especially for move events) in which case whole
primary plane render is saved and cursor update can be done without
waiting for next vsync of the primary plane.

Ihmo this implementation can be useful to test and implement atomic
updates for cursor plane in compositors.

Anyway thanks for your(Daniel and Thomas) responses, if there is
something I can do to push it then let me know.

regards
Łukasz Spintzyk

So concurring here.
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
https://urldefense.proofpoint.com/v2/url?u=http-3A__blog.ffwll.ch=DwIBAg=7dfBJ8cXbWjhc0BhImu8wVIoUFmBzj1s88r8EGyM0UY=0p1mI1lUPD-etxZm5xwnMe2dJnw8yCkW8EVTCGmBDPo=M1yfa7XuJVH9rCMPSuFfUD51RJsC3N6CNXSv9yDKmfmSUijg5Z3ngLzUqKBnbTgJ=Cs7RKpEwGAz4cdSnl3jqFctOKhaOpoHPGw3tSRPW2OI=

Re: drm/udl: Implementation of atomic cursor drm_plane

2024-07-02 Thread Lukasz Spintzyk

On 7/2/2024 2:02 PM, Lukasz Spintzyk wrote:

On 6/24/2024 12:05 PM, Daniel Vetter wrote:
CAUTION: Email originated externally, do not click links or open
attachments unless you recognize the sender and know the content is safe.

On Mon, Jun 24, 2024 at 09:28:29AM +0200, Thomas Zimmermann wrote:

Am 24.06.24 um 09:10 schrieb lukasz.spint...@synaptics.com:
This brings cursor on DisplayLink USB2.0 device on ChromeOS
compositor that requires either crtc'c cursor_set callback
or cursor drm_plane. Patch was tested on ChromeOS and Ubuntu 22.04
with Gnome/Wayland

NAK on this patchset. UDL has no HW cursor support, so we won't
implement
this in the driver. Software blending should be done in userspace,
where you

have CPU SIMD available.

Thank you Thomas,

Please check, my answer below Daniel's response.
I think that compositor still benefits cursor blending in the driver.
At least for ChromeOS in which udl based devices are only one without
cursor plane support. I believe it can simplify compositor code in this
case.

Another topic,
What do you think about "[PATCH 4/4] drm/udl: Shutdown all CRTCs on usb
disconnect", IMO it is unrelated and happened to appear more frequently
with cursor plane.

Shall I re-upload it as separate patchset?

regards
Łukasz Spintzyk

Ihmo this implementation can be useful to test and implement atomic
updates for cursor plane in compositors.

Anyway thanks for your(Daniel and Thomas) responses, if there is
something I can do to push it then let me know.

regards
Łukasz Spintzyk

[PATCH 1/4] drm/udl: Port CrOS cursor blending on primary plane in usb transfer

2024-06-24 Thread lukasz . spintzyk

From: Douglas Anderson 

Port applicable parts of CrOS udl cursor implementation from 5.4 CrOS kernel 
fork.
 - removed legacy non-atomic udl_cursor_move, udl_cursor_set ioctl's 
implementation
 - modified udl_cursor_download to copy cursor content to the buffer from 
iosys_map
 - removed unnecessary cursor copy in udl_handle_damage
 - simplify code by making struct udl_cursor public

Cursor was tested on ChromeOS and Ubuntu 22.04 with gnome-wayland.

(cherry picked from commit efb4c23afa3e1de185a1a4f8ff5b7ec412aec0fe)
ChromiumOS fork at 
https://chromium.googlesource.com/chromiumos/third_party/kernel)

Signed-off-by: Haixia Shi 
Signed-off-by: Ross Zwisler 
Signed-off-by: Douglas Anderson 
Signed-off-by: Guenter Roeck 
Signed-off-by: Łukasz Spintzyk 
---
 drivers/gpu/drm/udl/Makefile   |  2 +-
 drivers/gpu/drm/udl/udl_cursor.c   | 78 ++
 drivers/gpu/drm/udl/udl_cursor.h   | 47 ++
 drivers/gpu/drm/udl/udl_drv.h  |  6 ++-
 drivers/gpu/drm/udl/udl_modeset.c  |  9 +++-
 drivers/gpu/drm/udl/udl_transfer.c | 36 +-
 6 files changed, 173 insertions(+), 5 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 3f6db179455d..0abd04b0e829 100644
--- a/drivers/gpu/drm/udl/Makefile
+++ b/drivers/gpu/drm/udl/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-udl-y := udl_drv.o udl_modeset.o udl_main.o udl_transfer.o
+udl-y := udl_drv.o udl_modeset.o udl_main.o udl_transfer.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 ..594bb3b6b056
--- /dev/null
+++ b/drivers/gpu/drm/udl/udl_cursor.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * 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"
+
+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);
+}
+
+int udl_cursor_download(struct udl_cursor *cursor,
+   const struct iosys_map *map)
+{
+   uint32_t *src_ptr, *dst_ptr;
+   size_t i;
+
+   src_ptr = map->vaddr;
+   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_move(struct udl_cursor *cursor, int x, int y)
+{
+   cursor->x = x;
+   cursor->y = y;
+   return 0;
+}
diff --git a/drivers/gpu/drm/udl/udl_cursor.h b/drivers/gpu/drm/udl/udl_cursor.h
new file mode 100644
index ..6a848accc106
--- /dev/null
+++ b/drivers/gpu/drm/udl/udl_cursor.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * udl_cursor.h
+ *
+ * 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

drm/udl: Implementation of atomic cursor drm_plane

2024-06-24 Thread lukasz . spintzyk

This brings cursor on DisplayLink USB2.0 device on ChromeOS compositor that 
requires either crtc'c cursor_set callback
or cursor drm_plane. Patch was tested on ChromeOS and Ubuntu 22.04 with 
Gnome/Wayland

[PATCH 4/4] drm/udl: Shutdown all CRTCs on usb disconnect

2024-06-24 Thread lukasz . spintzyk

From: Łukasz Spintzyk 

This is fixing some kernel panics on device unplug, that started to be
more visible after implementing cursor plane support.

Signed-off-by: Łukasz Spintzyk 
---
 drivers/gpu/drm/udl/udl_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 1506094a8009..adaa7703c118 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (C) 2012 Red Hat
+ * Copyright (c) 2024 Synaptics Incorporated. All Rights Reserved.
  */
 
 #include 
@@ -14,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "udl_drv.h"
 
@@ -129,6 +131,7 @@ static void udl_usb_disconnect(struct usb_interface 
*interface)
drm_kms_helper_poll_fini(dev);
udl_drop_usb(dev);
drm_dev_unplug(dev);
+   drm_atomic_helper_shutdown(dev);
 }
 
 /*
-- 
2.34.1

[PATCH 3/4] drm/udl: Allow smaller plane sizes to allow cursor plane

2024-06-24 Thread lukasz . spintzyk

From: Łukasz Spintzyk 

Signed-off-by: Łukasz Spintzyk 
---
 drivers/gpu/drm/udl/udl_modeset.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c 
b/drivers/gpu/drm/udl/udl_modeset.c
index 0bd4e2f02dcf..28b7c269e913 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -659,8 +659,8 @@ int udl_modeset_init(struct drm_device *dev)
if (ret)
return ret;
 
-   dev->mode_config.min_width = 640;
-   dev->mode_config.min_height = 480;
+   dev->mode_config.min_width = UDL_CURSOR_W;
+   dev->mode_config.min_height = UDL_CURSOR_H;
dev->mode_config.max_width = 2048;
dev->mode_config.max_height = 2048;
dev->mode_config.preferred_depth = 16;
-- 
2.34.1

[PATCH 2/4] drm/udl: Add cursor drm_plane support

2024-06-24 Thread lukasz . spintzyk

From: Łukasz Spintzyk 

Atomic support for cursor plane was inspired by evdi drm driver that is 
maintained on github.com/displaylink/evdi.
Also added ARGB plane format as it is used by cursor plane.

Signed-off-by: Łukasz Spintzyk 
---
 drivers/gpu/drm/udl/udl_cursor.c  |  32 +++-
 drivers/gpu/drm/udl/udl_cursor.h  |   8 ++
 drivers/gpu/drm/udl/udl_drv.h |   1 +
 drivers/gpu/drm/udl/udl_modeset.c | 129 +-
 4 files changed, 150 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_cursor.c b/drivers/gpu/drm/udl/udl_cursor.c
index 594bb3b6b056..d60eccb704f4 100644
--- a/drivers/gpu/drm/udl/udl_cursor.c
+++ b/drivers/gpu/drm/udl/udl_cursor.c
@@ -3,6 +3,7 @@
  * udl_cursor.c
  *
  * Copyright (c) 2015 The Chromium OS Authors
+ * Copyright (c) 2024 Synaptics Incorporated. All Rights Reserved.
  *
  * 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
@@ -69,10 +70,39 @@ int udl_cursor_download(struct udl_cursor *cursor,
return 0;
 }
 
-
 int udl_cursor_move(struct udl_cursor *cursor, int x, int y)
 {
cursor->x = x;
cursor->y = y;
return 0;
 }
+
+void udl_cursor_damage_clear(struct udl_cursor *cursor)
+{
+   cursor->damage.x1 = INT_MAX;
+   cursor->damage.y1 = INT_MAX;
+   cursor->damage.x2 = 0;
+   cursor->damage.y2 = 0;
+}
+
+void udl_rect_merge(struct drm_rect *rect, struct drm_rect *rect2)
+{
+   rect->x1 = min(rect->x1, rect2->x1);
+   rect->y1 = min(rect->y1, rect2->y1);
+   rect->x2 = max(rect->x2, rect2->x2);
+   rect->y2 = max(rect->y2, rect2->y2);
+}
+
+void udl_cursor_mark_damage_from_plane(struct udl_cursor *cursor, struct 
drm_plane_state *state)
+{
+   struct drm_rect rect;
+
+   rect.x1 = (state->crtc_x < 0) ? 0 : state->crtc_x;
+   rect.y1 = (state->crtc_y < 0) ? 0 : state->crtc_y;
+   rect.x2 = state->crtc_x + state->crtc_w;
+   rect.y2 = state->crtc_y + state->crtc_h;
+
+   udl_rect_merge(>damage, );
+}
+
+
diff --git a/drivers/gpu/drm/udl/udl_cursor.h b/drivers/gpu/drm/udl/udl_cursor.h
index 6a848accc106..2375323bae55 100644
--- a/drivers/gpu/drm/udl/udl_cursor.h
+++ b/drivers/gpu/drm/udl/udl_cursor.h
@@ -3,6 +3,7 @@
  * udl_cursor.h
  *
  * Copyright (c) 2015 The Chromium OS Authors
+ * Copyright (c) 2024 Synaptics Incorporated. All Rights Reserved.
  *
  * 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
@@ -27,12 +28,15 @@
 #define UDL_CURSOR_W 64
 #define UDL_CURSOR_H 64
 #define UDL_CURSOR_BUF (UDL_CURSOR_W * UDL_CURSOR_H)
+
 struct udl_cursor {
uint32_t buffer[UDL_CURSOR_BUF];
+   struct drm_rect damage; // damage on primary
bool enabled;
int x;
int y;
 };
+
 struct udl_cursor_hline {
uint32_t *buffer;
int width;
@@ -43,5 +47,9 @@ extern void udl_cursor_get_hline(struct udl_cursor *cursor, 
int x, int y,
struct udl_cursor_hline *hline);
 extern int udl_cursor_move(struct udl_cursor *cursor, int x, int y);
 extern int udl_cursor_download(struct udl_cursor *cursor, const struct 
iosys_map *map);
+void udl_cursor_damage_clear(struct udl_cursor *cursor);
+void udl_rect_merge(struct drm_rect *rect, struct drm_rect *rect2);
+void udl_cursor_mark_damage_from_plane(struct udl_cursor *cursor,
+   struct drm_plane_state *state);
 
 #endif
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index ccd813bec1a9..935bcabcd593 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -68,6 +68,7 @@ struct udl_device {
struct device *dmadev;
 
struct drm_plane primary_plane;
+   struct drm_plane cursor_plane;
struct drm_crtc crtc;
struct drm_encoder encoder;
 
diff --git a/drivers/gpu/drm/udl/udl_modeset.c 
b/drivers/gpu/drm/udl/udl_modeset.c
index 21594144fec5..0bd4e2f02dcf 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2009 Roberto De Ioris 
  * Copyright (C) 2009 Jaya Kumar 
  * Copyright (C) 2009 Bernie Thompson 
+ * Copyright (c) 2024 Synaptics Incorporated. All Rights Reserved.
  */
 
 #include 
@@ -202,6 +203,23 @@ static long udl_log_cpp(unsigned int cpp)
return __ffs(cpp);
 }
 
+static void udl_trim_rect_to_framebuffer(
+   const struct drm_framebuffer *fb,
+   struct drm_rect *clip)
+{
+   if (clip->x1 > fb->width)
+   clip->x1 = fb->width;
+
+   if (clip->y1 > fb->height)
+   clip->y1 = fb->height;
+
+   if (clip->x2 > fb->width)
+   clip->x2 = fb->width;
+
+   if (clip->y2 > fb->height)
+   clip->y2 = fb->height;
+}
+
 static int udl_handle_damage(struct drm_framebuffer *fb,

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

2018-04-10 Thread Lukasz Spintzyk




On 05/04/2018 01:49, Deepak Rawat wrote:

From: Lukasz Spintzyk <lukasz.spint...@displaylink.com>

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk <lukasz.spint...@displaylink.com>
Signed-off-by: Deepak Rawat <dra...@vmware.com>
---
  drivers/gpu/drm/drm_atomic.c| 42 +
  drivers/gpu/drm/drm_atomic_helper.c |  4 
  drivers/gpu/drm/drm_mode_config.c   |  5 +
  drivers/gpu/drm/drm_plane.c | 12 +++
  include/drm/drm_mode_config.h   | 15 +
  include/drm/drm_plane.h | 16 ++
  include/uapi/drm/drm_mode.h | 15 +
  7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
  }
  
  /**

+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
   * drm_atomic_get_plane_state - get plane state
   * @state: global atomic state object
   * @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);
+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->color_encoding;
} else if (property == plane->color_range_property) {
*val = state->color_range;
+   } else if (property == config->prop_damage_clips) {
+   *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index c356545..55b44e3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,
  
  	state->fence = NULL;

state->commit = NULL;
+   state->damage_clips = NULL;
+   state->num_clips = 0;
  }
  EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
  
@@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
  
  	if (state->commit)

drm_crtc_commit_put(state->commit);
+
+   drm_property_blob_put(state->damage_clips);
  }
  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
  
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config

Re: [RFC 0/3] drm: page-flip with damage

2018-04-10 Thread Lukasz Spintzyk


Hi,

Many thanks that you have picked it up.
Unfortunately I didn't had time to work on it for a while.

I am ok with what you have done, ihmo the review is going in good direction.
I will try not to miss your update of it.
From my side I can say that damage rects with frame-buffer coordinates 
are perfectly fine for DisplayLink case.


Btw I have noticed that there is also a patch from Rob Clark that is 
supplying helper for legacy dirtyfb callback.
Maybe we should unify the naming of the rects. Here we have 
"damage_clips", Clark's patch has 'dirty_rects' notion.
On other hand existing legacy dirtyfb callback in drm_framebuffer_funcs 
is also using 'clip_rects' :).



Thanks

Łukasz Spintzyk


On 05/04/2018 01:49, Deepak Rawat wrote:

Hi All,

This is extension to Lukasz Spintzyk earlier draft of damage interface 
for drm.
Bascially a new plane property is added called "DAMAGE_CLIPS" which is 
simply

an array of drm_rect (exported to userspace as drm_mode_rect). The clips
represents damage in framebuffer coordinate of attached fb to the plane.

Helper iterator is added to traverse the damage rectangles and get the 
damage

clips in framebuffer, plane or crtc coordinates as need by driver
implementation. Finally a helper to reset damage in case need full 
update is
required. Drivers interested in page-flip with damage should call this 
from

atomic_check hook.

With the RFC for atomic implementation of dirtyfb ioctl I was thinking
should we need to consider dirty_fb flags, especially
DRM_MODE_FB_DIRTY_ANNOTATE_COPY to be passed with atomic
DAMAGE_CLIPS property blob? I didn't considered that untill now. If no 
driver

uses that in my opinion for simplicity this can be ignored?

About overlaping of damage rectangles is also not finalized. This really
depends on driver specific implementation and can be left open-ended?

My knowledge is limited to vmwgfx so would like to hear about other 
driver use

cases and this can be modified in keeping other drivers need.

Going forward driver implementation for vmwgfx and user-space 
implementation

of kmscube/weston will be next step to test the changes.

Thanks,
Deepak

Deepak Rawat (2):
drm: Add helper iterator functions to iterate over plane damage.
drm: Add helper to validate damage during modeset_check

Lukasz Spintzyk (1):
drm: Add DAMAGE_CLIPS property to plane

drivers/gpu/drm/drm_atomic.c | 42 +
drivers/gpu/drm/drm_atomic_helper.c | 173 


drivers/gpu/drm/drm_mode_config.c | 5 ++
drivers/gpu/drm/drm_plane.c | 12 +++
include/drm/drm_atomic_helper.h | 41 +
include/drm/drm_mode_config.h | 15 
include/drm/drm_plane.h | 16 
include/uapi/drm/drm_mode.h | 15 
8 files changed, 319 insertions(+)

--
2.7.4

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


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

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

2018-01-04 Thread Lukasz Spintzyk




On 28/12/2017 10:03, Thomas Hellstrom wrote:

Hi, Lukasz!

(Sorry for top-posting).

We have Deepak from our team working on the same subject. I think he's 
in over the holidays so I'll add him to the CC list.

Great!


Adding damage to the plane state is, IMO the correct way to do it. 
However, from your proposal it's not clear whether damage is given in 
the plane-, crtc- or framebuffer coordinates. The last conclusion from 
our email thread discussion was that they should be given in 
framebuffer coordinates with helpers to compute plane coordinates or 
crtc coordinates. The reason being that it's easier for user-space 
apps to send damage that way, and again, we have the full information 
that we can clip and scale if necessary. Most drivers probably want 
the damage in clipped plane- or crtc coordinates. Helpers could for 
example be in the form of region iterators.
Personally i don't know the difference between plane rects and 
framebuffer rects. I don't know what would be better. I was thinking 
about coordinates of framebuffer that is attached to drm_plane_state.


Full (multi-rect) damage regions are OK with us, although we should 
keep in mind that we won't be able to compute region unions in the 
kernel (yet?). Implying: Should we forbid overlapping rects at the 
interface level or should we just recommend rects not to be 
overlapping? The former would be pretty hard to enforce efficiently.
I would be for recommendation. We can add some helper functions to 
combine rects and set some limits on number of rects to prevent abuse of 
that interface.


Another thing we should think about is how to use this interface for 
the legacy "dirtyfb" call. Probably we need to clear the damage 
property on each state-update, or set a flag that "this is a dirtyfb 
state update".


IMO we should also have as an end goal of this work to have 
gnome-shell on drm sending damage regions on page-flip, which means 
either porting gnome-shell to atomic or set up a new legacy 
page-flip-with-atomic ioctl.
Can't we reuse dirtyfb ioctl for this purpose? It would be called before 
page_flip ioctl?


/Thomas


On 12/21/2017 12:10 PM, Lukasz Spintzyk wrote:

Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
Signed-off-by: Lukasz Spintzyk <lukasz.spint...@displaylink.com>
---
  drivers/gpu/drm/drm_atomic.c  | 10 ++
  drivers/gpu/drm/drm_mode_config.c |  6 ++
  drivers/gpu/drm/drm_plane.c   |  1 +
  include/drm/drm_mode_config.h |  5 +
  include/drm/drm_plane.h   |  3 +++
  5 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b76d49218cf1..cd3b4ed7b04c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct 
drm_plane *plane,

  state->rotation = val;
  } else if (property == plane->zpos_property) {
  state->zpos = val;
+    } else if (property == config->dirty_rects_property) {
+    bool replaced = false;
+    int ret = drm_atomic_replace_property_blob_from_id(dev,
+    >dirty_blob,
+    val,
+    -1,
+    );
+    return ret;
  } else if (plane->funcs->atomic_set_property) {
  return plane->funcs->atomic_set_property(plane, state,
  property, val);
@@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane 
*plane,

  *val = state->rotation;
  } else if (property == plane->zpos_property) {
  *val = state->zpos;
+    } else if (property == config->dirty_rects_property) {
+    *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0;
  } else if (plane->funcs->atomic_get_property) {
  return plane->funcs->atomic_get_property(plane, state, 
property, val);

  } else {
diff --git a/drivers/gpu/drm/drm_mode_config.c 
b/drivers/gpu/drm/drm_mode_config.c

index bc5c46306b3d..d5f1021c6ece 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -293,6 +293,12 @@ static int 
drm_mode_create_standard_properties(struct drm_device *dev)

  return -ENOMEM;
  dev->mode_config.prop_crtc_id = prop;
  +    prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+    "DIRTY_RECTS", 0);
+    if (!prop)
+    return -ENOMEM;
+    dev->mode_config.dirty_rects_property = prop;
+
  prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
  "ACTIVE");
  if (!prop)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 37a93cdffb4a..add110f025e5 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device 
*dev, struct drm_plane *plane,
  drm_object_attach_property(>base, 
config->prop_src_

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

2017-12-22 Thread Lukasz Spintzyk


Thanks Ville and Daniel for for your response.

I will try to come back with something later.

thanks
Lukasz
On 21/12/2017 14:10, Daniel Vetter wrote:

On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> > Signed-off-by: Lukasz Spintzyk <lukasz.spint...@displaylink.com>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 10 ++
> > drivers/gpu/drm/drm_mode_config.c | 6 ++
> > drivers/gpu/drm/drm_plane.c | 1 +
> > include/drm/drm_mode_config.h | 5 +
> > include/drm/drm_plane.h | 3 +++
> > 5 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c 
b/drivers/gpu/drm/drm_atomic.c

> > index b76d49218cf1..cd3b4ed7b04c 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -759,6 +759,14 @@ static int 
drm_atomic_plane_set_property(struct drm_plane *plane,

> > state->rotation = val;
> > } else if (property == plane->zpos_property) {
> > state->zpos = val;
> > + } else if (property == config->dirty_rects_property) {
> > + bool replaced = false;
> > + int ret = drm_atomic_replace_property_blob_from_id(dev,
> > + >dirty_blob,
> > + val,
> > + -1,
> > + );
> > + return ret;
> > } else if (plane->funcs->atomic_set_property) {
> > return plane->funcs->atomic_set_property(plane, state,
> > property, val);
> > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane 
*plane,

> > *val = state->rotation;
> > } else if (property == plane->zpos_property) {
> > *val = state->zpos;
> > + } else if (property == config->dirty_rects_property) {
> > + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0;
> > } else if (plane->funcs->atomic_get_property) {
> > return plane->funcs->atomic_get_property(plane, state, property, val);
> > } else {
> > diff --git a/drivers/gpu/drm/drm_mode_config.c 
b/drivers/gpu/drm/drm_mode_config.c

> > index bc5c46306b3d..d5f1021c6ece 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -293,6 +293,12 @@ static int 
drm_mode_create_standard_properties(struct drm_device *dev)

> > return -ENOMEM;
> > dev->mode_config.prop_crtc_id = prop;
> >
> > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > + "DIRTY_RECTS", 0);
> > + if (!prop)
> > + return -ENOMEM;
> > + dev->mode_config.dirty_rects_property = prop;
> > +
> > prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
> > "ACTIVE");
> > if (!prop)
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 37a93cdffb4a..add110f025e5 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device 
*dev, struct drm_plane *plane,

> > drm_object_attach_property(>base, config->prop_src_y, 0);
> > drm_object_attach_property(>base, config->prop_src_w, 0);
> > drm_object_attach_property(>base, config->prop_src_h, 0);
> > + drm_object_attach_property(>base, 
config->dirty_rects_property, 0);

> > }
> >
> > if (config->allow_fb_modifiers)
> > diff --git a/include/drm/drm_mode_config.h 
b/include/drm/drm_mode_config.h

> > index e5f3b43014e1..65f64eb04c0c 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -599,6 +599,11 @@ struct drm_mode_config {
> > * _crtc.
> > */
> > struct drm_property *prop_crtc_id;
> > + /**
> > + * @dirty_rects_property: Optional plane property to mark damaged
> > + * regions on the plane framebuffer.
>
> What exactly would the blob contain?
>
> The comment seems to be implying these are in fb coordiantes as
> opposed to plane crtc coordinates? Not sure which would be more
> convenient. At least if they're fb coordinates then you probably
> want some helpers to translate/rotate/scale those rects to the
> crtc coordinates. Actual use depends on the driver/hw I suppose.

Yeah I think we also should add a decoded state to the drm_plane_state,
which has the full structure and all the details.

Ok.
Initially for model I was thinking to take struct drm_drawable_info with 
simple c style array of struct drm_clip_rect *rects in it.

Do you think that something more complex will be needed?



And when we discussed this iirc we've identified a clear need for at least
some drivers to deal in crtc dirty re

[PATCH 0/1] Damage rectangles interface for DRM

2017-12-21 Thread Lukasz Spintzyk

This is a draft of damage interface for drm. Alluding to the topic 
"RFC: page-flip with damage?" on dri-devel
https://lists.freedesktop.org/archives/dri-devel/2017-September/153235.html
The following patch is a proof of concept, how we can deliver dirty rectangles 
to the drm drivers.
Patch was tested on Chromium OS to deliver damage regions to atomic version of 
evdi driver. Hence this supports atomic drivers for now.
We should agree on the approach how this can be implemented.
In the patch, I intend to use drm blob properties to pass c-style array of 
drm_clip_rects.
Is DRM properties framework suitable for this solution? The only doubt I have 
is that it requires to create and destroy property blob every 
atomic_commit/page flip as this is the only way to update blob with damage 
regions. This is associated with kmalloc and free for each flip. Do you think 
if this approach relates to some performance risk?

The proposed solution is attaching DIRTY_RECTS property to the 
drm_plane. The property means to hold list of rectangles in plane coordinates.
This is opposite to initial plan in thread from September 2017. I wanted to 
present our rationale for this.
Single rect vs rect list:
Some compositors like Chromium can have quite precise damage 
information that would be lost if we would collapse them. 
Damage rectangles on crtc vs per plane
The property is attached to drm plane initially. We had a talk with 
guys from Chromium compositor. The plan is to use overlay planes more 
extensively. Also, Chromium compositor operates on dirty rects per plane so it 
would be easier to use them as is.
In my opinion this is more natural to deliver information about damage per 
plane. Damage information is used primarily for bandwidth savings. For driver 
that deliver plane data it is more useful to know what parts of the plane have 
changed so it can send only these memory areas.
I don't know if this reasoning is valid. Probably it depends on hardware and 
driver.
I can speak about use cases I have faced. One of them is additional cursor 
plane. With damage regions on crtc I don't know if it is related to primary 
plane, or it is connected to cursor movement. Also, user space applications 
must generate additional damage regions when such cursor plane is moving over 
primary plane. For such cases passing dirty rectangles for planes seems to be 
more straightforward.

This is draft with minimal functionality. I would like to hear what is your 
opinion on it.  
I am happy to hear what are the requirements of other driver vendors.


Thanks
Lukasz Spintzyk

Lukasz Spintzyk (1):
  drm: Add dirty_rects atomic blob property for drm_plane

 drivers/gpu/drm/drm_atomic.c  | 10 ++
 drivers/gpu/drm/drm_mode_config.c |  6 ++
 drivers/gpu/drm/drm_plane.c   |  1 +
 include/drm/drm_mode_config.h |  5 +
 include/drm/drm_plane.h   |  3 +++
 5 files changed, 25 insertions(+)

-- 
2.15.1

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

[PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

2017-12-21 Thread Lukasz Spintzyk

Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
Signed-off-by: Lukasz Spintzyk <lukasz.spint...@displaylink.com>
---
 drivers/gpu/drm/drm_atomic.c  | 10 ++
 drivers/gpu/drm/drm_mode_config.c |  6 ++
 drivers/gpu/drm/drm_plane.c   |  1 +
 include/drm/drm_mode_config.h |  5 +
 include/drm/drm_plane.h   |  3 +++
 5 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b76d49218cf1..cd3b4ed7b04c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->rotation = val;
} else if (property == plane->zpos_property) {
state->zpos = val;
+   } else if (property == config->dirty_rects_property) {
+   bool replaced = false;
+   int ret = drm_atomic_replace_property_blob_from_id(dev,
+   >dirty_blob,
+   val,
+   -1,
+   );
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->rotation;
} else if (property == plane->zpos_property) {
*val = state->zpos;
+   } else if (property == config->dirty_rects_property) {
+   *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
} else {
diff --git a/drivers/gpu/drm/drm_mode_config.c 
b/drivers/gpu/drm/drm_mode_config.c
index bc5c46306b3d..d5f1021c6ece 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct 
drm_device *dev)
return -ENOMEM;
dev->mode_config.prop_crtc_id = prop;
 
+   prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+   "DIRTY_RECTS", 0);
+   if (!prop)
+   return -ENOMEM;
+   dev->mode_config.dirty_rects_property = prop;
+
prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
"ACTIVE");
if (!prop)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 37a93cdffb4a..add110f025e5 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct 
drm_plane *plane,
drm_object_attach_property(>base, config->prop_src_y, 0);
drm_object_attach_property(>base, config->prop_src_w, 0);
drm_object_attach_property(>base, config->prop_src_h, 0);
+   drm_object_attach_property(>base, 
config->dirty_rects_property, 0);
}
 
if (config->allow_fb_modifiers)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e5f3b43014e1..65f64eb04c0c 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -599,6 +599,11 @@ struct drm_mode_config {
 * _crtc.
 */
struct drm_property *prop_crtc_id;
+   /**
+* @dirty_rects_property: Optional plane property to mark damaged
+* regions on the plane framebuffer.
+*/
+   struct drm_property *dirty_rects_property;
/**
 * @prop_active: Default atomic CRTC property to control the active
 * state, which is the simplified implementation for DPMS in atomic
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 8185e3468a23..7d45b164ccce 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -131,6 +131,9 @@ struct drm_plane_state {
 */
struct drm_crtc_commit *commit;
 
+   /* Optional blob property with damaged regions. */
+   struct drm_property_blob *dirty_blob;
+
struct drm_atomic_state *state;
 };
 
-- 
2.15.1

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

Re: drm/udl: Implementation of atomic cursor drm_plane

Re: drm/udl: Implementation of atomic cursor drm_plane

[PATCH 1/4] drm/udl: Port CrOS cursor blending on primary plane in usb transfer

drm/udl: Implementation of atomic cursor drm_plane

[PATCH 4/4] drm/udl: Shutdown all CRTCs on usb disconnect

[PATCH 3/4] drm/udl: Allow smaller plane sizes to allow cursor plane

[PATCH 2/4] drm/udl: Add cursor drm_plane support

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

Re: [RFC 0/3] drm: page-flip with damage

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[PATCH 0/1] Damage rectangles interface for DRM

[PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

13 matches

Site Navigation

Mail list logo

Footer information