[PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[André Almeida]

Hi,

AMD hardware can do more on the async flip path than just the primary plane, so
to lift up the current restrictions, this patchset allows drivers to write their
own check for planes for async flips.

I'm not sure if adding something new to drm_plane_funcs is the right way to do,
because if we want to expand the other object types (crtc, connector) we would
need to add their own drm_XXX_funcs, so feedbacks are welcome!

	André

André Almeida (2):
  drm/atomic: Allow drivers to write their own plane check for async
    flips
  drm/amdgpu: Implement check_async_props for planes

 .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 30 +++++++++
 drivers/gpu/drm/drm_atomic_uapi.c             | 62 ++++++++++++++-----
 include/drm/drm_atomic_uapi.h                 | 12 ++++
 include/drm/drm_plane.h                       |  5 ++
 4 files changed, 92 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH 1/2] drm/atomic: Allow drivers to write their own plane check for async flips

[André Almeida]

Some hardware are more flexible on what they can flip asynchronously, so
rework the plane check so drivers can implement their own check, lifting
up some of the restrictions.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 62 ++++++++++++++++++++++---------
 include/drm/drm_atomic_uapi.h     | 12 ++++++
 include/drm/drm_plane.h           |  5 +++
 3 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index aee4a65d4959..6d5b9fec90c7 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -620,7 +620,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 	return 0;
 }
 
-static int
+int
 drm_atomic_plane_get_property(struct drm_plane *plane,
 		const struct drm_plane_state *state,
 		struct drm_property *property, uint64_t *val)
@@ -683,6 +683,7 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_atomic_plane_get_property);
 
 static int drm_atomic_set_writeback_fb_for_connector(
 		struct drm_connector_state *conn_state,
@@ -1026,18 +1027,54 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
 	return ret;
 }
 
-static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
+int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
 					 struct drm_property *prop)
 {
 	if (ret != 0 || old_val != prop_value) {
 		drm_dbg_atomic(prop->dev,
-			       "[PROP:%d:%s] No prop can be changed during async flip\n",
+			       "[PROP:%d:%s] This prop cannot be changed during async flip\n",
 			       prop->base.id, prop->name);
 		return -EINVAL;
 	}
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_atomic_check_prop_changes);
+
+/* plane changes may have exceptions, so we have a special function for them */
+static int drm_atomic_check_plane_changes(struct drm_property *prop,
+					  struct drm_plane *plane,
+					  struct drm_plane_state *plane_state,
+					  struct drm_mode_object *obj,
+					  u64 prop_value, u64 old_val)
+{
+	struct drm_mode_config *config = &plane->dev->mode_config;
+	int ret;
+
+	if (plane->funcs->check_async_props)
+		return plane->funcs->check_async_props(prop, plane, plane_state,
+							     obj, prop_value, old_val);
+
+	/*
+	 * if you are trying to change something other than the FB ID, your
+	 * change will be either rejected or ignored, so we can stop the check
+	 * here
+	 */
+	if (prop != config->prop_fb_id) {
+		ret = drm_atomic_plane_get_property(plane, plane_state,
+						    prop, &old_val);
+		return drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+	}
+
+	if (plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
+		drm_dbg_atomic(prop->dev,
+			       "[OBJECT:%d] Only primary planes can be changed during async flip\n",
+			       obj->id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
 
 int drm_atomic_set_property(struct drm_atomic_state *state,
 			    struct drm_file *file_priv,
@@ -1100,7 +1137,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 	case DRM_MODE_OBJECT_PLANE: {
 		struct drm_plane *plane = obj_to_plane(obj);
 		struct drm_plane_state *plane_state;
-		struct drm_mode_config *config = &plane->dev->mode_config;
 
 		plane_state = drm_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state)) {
@@ -1108,19 +1144,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 			break;
 		}
 
-		if (async_flip && prop != config->prop_fb_id) {
-			ret = drm_atomic_plane_get_property(plane, plane_state,
-							    prop, &old_val);
-			ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
-			break;
-		}
-
-		if (async_flip && plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
-			drm_dbg_atomic(prop->dev,
-				       "[OBJECT:%d] Only primary planes can be changed during async flip\n",
-				       obj->id);
-			ret = -EINVAL;
-			break;
+		if (async_flip) {
+			ret = drm_atomic_check_plane_changes(prop, plane, plane_state,
+							     obj, prop_value, old_val);
+			if (ret)
+				break;
 		}
 
 		ret = drm_atomic_plane_set_property(plane,
diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
index 4c6d39d7bdb2..d65fa8fbbca0 100644
--- a/include/drm/drm_atomic_uapi.h
+++ b/include/drm/drm_atomic_uapi.h
@@ -29,6 +29,8 @@
 #ifndef DRM_ATOMIC_UAPI_H_
 #define DRM_ATOMIC_UAPI_H_
 
+#include <linux/types.h>
+
 struct drm_crtc_state;
 struct drm_display_mode;
 struct drm_property_blob;
@@ -37,6 +39,9 @@ struct drm_crtc;
 struct drm_connector_state;
 struct dma_fence;
 struct drm_framebuffer;
+struct drm_mode_object;
+struct drm_property;
+struct drm_plane;
 
 int __must_check
 drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
@@ -53,4 +58,11 @@ int __must_check
 drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 				  struct drm_crtc *crtc);
 
+int drm_atomic_plane_get_property(struct drm_plane *plane,
+				  const struct drm_plane_state *state,
+				  struct drm_property *property, uint64_t *val);
+
+int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
+				  struct drm_property *prop);
+
 #endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index c6565a6f9324..73dac8d76831 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -540,6 +540,11 @@ struct drm_plane_funcs {
 	 */
 	bool (*format_mod_supported)(struct drm_plane *plane, uint32_t format,
 				     uint64_t modifier);
+
+	int (*check_async_props)(struct drm_property *prop, struct drm_plane *plane,
+				 struct drm_plane_state *plane_state,
+				 struct drm_mode_object *obj,
+				 u64 prop_value, u64 old_val);
 };
 
 /**
-- 
2.43.0

Re: [PATCH 1/2] drm/atomic: Allow drivers to write their own plane check for async flips

[kernel test robot]

Hi André,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-tip/drm-tip linus/master next-20240119]
[cannot apply to drm-intel/for-linux-next-fixes v6.7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andr-Almeida/drm-atomic-Allow-drivers-to-write-their-own-plane-check-for-async-flips/20240116-125406
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240116045159.1015510-2-andrealmeid%40igalia.com
patch subject: [PATCH 1/2] drm/atomic: Allow drivers to write their own plane check for async flips
config: hexagon-randconfig-002-20240119 (https://download.01.org/0day-ci/archive/20240120/202401200526.Xggix2DE-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project d92ce344bf641e6bb025b41b3f1a77dd25e2b3e9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240120/202401200526.Xggix2DE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401200526.Xggix2DE-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/drm_atomic_uapi.c:31:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:32:
   In file included from include/drm/drm_modes.h:33:
   In file included from include/drm/drm_connector.h:32:
   In file included from include/drm/drm_util.h:35:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/gpu/drm/drm_atomic_uapi.c:31:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:32:
   In file included from include/drm/drm_modes.h:33:
   In file included from include/drm/drm_connector.h:32:
   In file included from include/drm/drm_util.h:35:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/gpu/drm/drm_atomic_uapi.c:31:
   In file included from include/drm/drm_atomic.h:31:
   In file included from include/drm/drm_crtc.h:32:
   In file included from include/drm/drm_modes.h:33:
   In file included from include/drm/drm_connector.h:32:
   In file included from include/drm/drm_util.h:35:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/gpu/drm/drm_atomic_uapi.c:1149:30: warning: variable 'old_val' is uninitialized when used here [-Wuninitialized]
    1149 |                                                              obj, prop_value, old_val);
         |                                                                               ^~~~~~~
   drivers/gpu/drm/drm_atomic_uapi.c:1087:13: note: initialize the variable 'old_val' to silence this warning
    1087 |         u64 old_val;
         |                    ^
         |                     = 0
   7 warnings generated.


vim +/old_val +1149 drivers/gpu/drm/drm_atomic_uapi.c

  1078	
  1079	int drm_atomic_set_property(struct drm_atomic_state *state,
  1080				    struct drm_file *file_priv,
  1081				    struct drm_mode_object *obj,
  1082				    struct drm_property *prop,
  1083				    u64 prop_value,
  1084				    bool async_flip)
  1085	{
  1086		struct drm_mode_object *ref;
  1087		u64 old_val;
  1088		int ret;
  1089	
  1090		if (!drm_property_change_valid_get(prop, prop_value, &ref))
  1091			return -EINVAL;
  1092	
  1093		switch (obj->type) {
  1094		case DRM_MODE_OBJECT_CONNECTOR: {
  1095			struct drm_connector *connector = obj_to_connector(obj);
  1096			struct drm_connector_state *connector_state;
  1097	
  1098			connector_state = drm_atomic_get_connector_state(state, connector);
  1099			if (IS_ERR(connector_state)) {
  1100				ret = PTR_ERR(connector_state);
  1101				break;
  1102			}
  1103	
  1104			if (async_flip) {
  1105				ret = drm_atomic_connector_get_property(connector, connector_state,
  1106									prop, &old_val);
  1107				ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
  1108				break;
  1109			}
  1110	
  1111			ret = drm_atomic_connector_set_property(connector,
  1112					connector_state, file_priv,
  1113					prop, prop_value);
  1114			break;
  1115		}
  1116		case DRM_MODE_OBJECT_CRTC: {
  1117			struct drm_crtc *crtc = obj_to_crtc(obj);
  1118			struct drm_crtc_state *crtc_state;
  1119	
  1120			crtc_state = drm_atomic_get_crtc_state(state, crtc);
  1121			if (IS_ERR(crtc_state)) {
  1122				ret = PTR_ERR(crtc_state);
  1123				break;
  1124			}
  1125	
  1126			if (async_flip) {
  1127				ret = drm_atomic_crtc_get_property(crtc, crtc_state,
  1128								   prop, &old_val);
  1129				ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
  1130				break;
  1131			}
  1132	
  1133			ret = drm_atomic_crtc_set_property(crtc,
  1134					crtc_state, prop, prop_value);
  1135			break;
  1136		}
  1137		case DRM_MODE_OBJECT_PLANE: {
  1138			struct drm_plane *plane = obj_to_plane(obj);
  1139			struct drm_plane_state *plane_state;
  1140	
  1141			plane_state = drm_atomic_get_plane_state(state, plane);
  1142			if (IS_ERR(plane_state)) {
  1143				ret = PTR_ERR(plane_state);
  1144				break;
  1145			}
  1146	
  1147			if (async_flip) {
  1148				ret = drm_atomic_check_plane_changes(prop, plane, plane_state,
> 1149								     obj, prop_value, old_val);
  1150				if (ret)
  1151					break;
  1152			}
  1153	
  1154			ret = drm_atomic_plane_set_property(plane,
  1155					plane_state, file_priv,
  1156					prop, prop_value);
  1157			break;
  1158		}
  1159		default:
  1160			drm_dbg_atomic(prop->dev, "[OBJECT:%d] has no properties\n", obj->id);
  1161			ret = -EINVAL;
  1162			break;
  1163		}
  1164	
  1165		drm_property_change_valid_put(prop, ref);
  1166		return ret;
  1167	}
  1168	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/2] drm/amdgpu: Implement check_async_props for planes

[André Almeida]

AMD GPUs can do async flips with overlay planes and other props rather
than just FB ID, so implement a custom check_async_props for AMD planes.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
index 116121e647ca..127cae1f9fb4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
@@ -25,6 +25,7 @@
  */
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_atomic_uapi.h>
 #include <drm/drm_blend.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_plane_helper.h>
@@ -1430,6 +1431,34 @@ static void amdgpu_dm_plane_drm_plane_destroy_state(struct drm_plane *plane,
 	drm_atomic_helper_plane_destroy_state(plane, state);
 }
 
+static int amdgpu_dm_plane_check_async_props(struct drm_property *prop,
+					  struct drm_plane *plane,
+					  struct drm_plane_state *plane_state,
+					  struct drm_mode_object *obj,
+					  u64 prop_value, u64 old_val)
+{
+	struct drm_mode_config *config = &plane->dev->mode_config;
+	int ret;
+
+	if (prop != config->prop_fb_id &&
+	    prop != config->prop_in_fence_fd &&
+	    prop != config->prop_fb_damage_clips) {
+		ret = drm_atomic_plane_get_property(plane, plane_state,
+						    prop, &old_val);
+		return drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+	}
+
+	if (plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY
+	    && plane_state->plane->type != DRM_PLANE_TYPE_OVERLAY) {
+		drm_dbg_atomic(prop->dev,
+			       "[OBJECT:%d] Only primary or overlay planes can be changed during async flip\n",
+			       obj->id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct drm_plane_funcs dm_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
@@ -1438,6 +1467,7 @@ static const struct drm_plane_funcs dm_plane_funcs = {
 	.atomic_duplicate_state = amdgpu_dm_plane_drm_plane_duplicate_state,
 	.atomic_destroy_state = amdgpu_dm_plane_drm_plane_destroy_state,
 	.format_mod_supported = amdgpu_dm_plane_format_mod_supported,
+	.check_async_props = amdgpu_dm_plane_check_async_props,
 };
 
 int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
-- 
2.43.0

Re: [PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

On Tue, 16 Jan 2024 01:51:57 -0300
André Almeida <andrealmeid@igalia.com> wrote:

> Hi,
> 
> AMD hardware can do more on the async flip path than just the primary plane, so
> to lift up the current restrictions, this patchset allows drivers to write their
> own check for planes for async flips.

Hi,

what's the userspace story for this, how could userspace know it could do more?
What kind of userspace would take advantage of this and in what situations?

Or is this not meant for generic userspace?


Thanks,
pq

> I'm not sure if adding something new to drm_plane_funcs is the right way to do,
> because if we want to expand the other object types (crtc, connector) we would
> need to add their own drm_XXX_funcs, so feedbacks are welcome!
> 
> 	André
> 
> André Almeida (2):
>   drm/atomic: Allow drivers to write their own plane check for async
>     flips
>   drm/amdgpu: Implement check_async_props for planes
> 
>  .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 30 +++++++++
>  drivers/gpu/drm/drm_atomic_uapi.c             | 62 ++++++++++++++-----
>  include/drm/drm_atomic_uapi.h                 | 12 ++++
>  include/drm/drm_plane.h                       |  5 ++
>  4 files changed, 92 insertions(+), 17 deletions(-)
> 


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

Re: [PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[André Almeida]

Hi Pekka,

Em 16/01/2024 06:45, Pekka Paalanen escreveu:
> On Tue, 16 Jan 2024 01:51:57 -0300
> André Almeida <andrealmeid@igalia.com> wrote:
> 
>> Hi,
>>
>> AMD hardware can do more on the async flip path than just the primary plane, so
>> to lift up the current restrictions, this patchset allows drivers to write their
>> own check for planes for async flips.
> 
> Hi,
> 
> what's the userspace story for this, how could userspace know it could do more?
> What kind of userspace would take advantage of this and in what situations?
> 
> Or is this not meant for generic userspace?

Sorry, I forgot to document this. So the idea is that userspace will 
query what they can do here with DRM_MODE_ATOMIC_TEST_ONLY calls, 
instead of having capabilities for each prop.

> 
> 
> Thanks,
> pq
> 
>> I'm not sure if adding something new to drm_plane_funcs is the right way to do,
>> because if we want to expand the other object types (crtc, connector) we would
>> need to add their own drm_XXX_funcs, so feedbacks are welcome!
>>
>> 	André
>>
>> André Almeida (2):
>>    drm/atomic: Allow drivers to write their own plane check for async
>>      flips
>>    drm/amdgpu: Implement check_async_props for planes
>>
>>   .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 30 +++++++++
>>   drivers/gpu/drm/drm_atomic_uapi.c             | 62 ++++++++++++++-----
>>   include/drm/drm_atomic_uapi.h                 | 12 ++++
>>   include/drm/drm_plane.h                       |  5 ++
>>   4 files changed, 92 insertions(+), 17 deletions(-)
>>
> 

Re: [PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2149 bytes --]

On Tue, 16 Jan 2024 08:50:59 -0300
André Almeida <andrealmeid@igalia.com> wrote:

> Hi Pekka,
> 
> Em 16/01/2024 06:45, Pekka Paalanen escreveu:
> > On Tue, 16 Jan 2024 01:51:57 -0300
> > André Almeida <andrealmeid@igalia.com> wrote:
> >   
> >> Hi,
> >>
> >> AMD hardware can do more on the async flip path than just the primary plane, so
> >> to lift up the current restrictions, this patchset allows drivers to write their
> >> own check for planes for async flips.  
> > 
> > Hi,
> > 
> > what's the userspace story for this, how could userspace know it could do more?
> > What kind of userspace would take advantage of this and in what situations?
> > 
> > Or is this not meant for generic userspace?  
> 
> Sorry, I forgot to document this. So the idea is that userspace will 
> query what they can do here with DRM_MODE_ATOMIC_TEST_ONLY calls, 
> instead of having capabilities for each prop.

That's the theory, but do you have a practical example?

What other planes and props would one want change in some specific use
case?

Is it just "all or nothing", or would there be room to choose and pick
which props you change and which you don't based on what the driver
supports? If the latter, then relying on TEST_ONLY might be yet another
combinatorial explosion to iterate through.


Thanks,
pq

> >> I'm not sure if adding something new to drm_plane_funcs is the right way to do,
> >> because if we want to expand the other object types (crtc, connector) we would
> >> need to add their own drm_XXX_funcs, so feedbacks are welcome!
> >>
> >> 	André
> >>
> >> André Almeida (2):
> >>    drm/atomic: Allow drivers to write their own plane check for async
> >>      flips
> >>    drm/amdgpu: Implement check_async_props for planes
> >>
> >>   .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 30 +++++++++
> >>   drivers/gpu/drm/drm_atomic_uapi.c             | 62 ++++++++++++++-----
> >>   include/drm/drm_atomic_uapi.h                 | 12 ++++
> >>   include/drm/drm_plane.h                       |  5 ++
> >>   4 files changed, 92 insertions(+), 17 deletions(-)
> >>  
> >   


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

Re: [PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[André Almeida]

+ Joshua

Em 16/01/2024 10:14, Pekka Paalanen escreveu:
> On Tue, 16 Jan 2024 08:50:59 -0300
> André Almeida <andrealmeid@igalia.com> wrote:
> 
>> Hi Pekka,
>>
>> Em 16/01/2024 06:45, Pekka Paalanen escreveu:
>>> On Tue, 16 Jan 2024 01:51:57 -0300
>>> André Almeida <andrealmeid@igalia.com> wrote:
>>>    
>>>> Hi,
>>>>
>>>> AMD hardware can do more on the async flip path than just the primary plane, so
>>>> to lift up the current restrictions, this patchset allows drivers to write their
>>>> own check for planes for async flips.
>>>
>>> Hi,
>>>
>>> what's the userspace story for this, how could userspace know it could do more?
>>> What kind of userspace would take advantage of this and in what situations?
>>>
>>> Or is this not meant for generic userspace?
>>
>> Sorry, I forgot to document this. So the idea is that userspace will
>> query what they can do here with DRM_MODE_ATOMIC_TEST_ONLY calls,
>> instead of having capabilities for each prop.
> 
> That's the theory, but do you have a practical example?
> 
> What other planes and props would one want change in some specific use
> case?
> 
> Is it just "all or nothing", or would there be room to choose and pick
> which props you change and which you don't based on what the driver
> supports? If the latter, then relying on TEST_ONLY might be yet another
> combinatorial explosion to iterate through.
> 

That's a good question, maybe Simon, Xaver or Joshua can share how they 
were planning to use this on Gamescope or Kwin.

> 
> Thanks,
> pq
> 
>>>> I'm not sure if adding something new to drm_plane_funcs is the right way to do,
>>>> because if we want to expand the other object types (crtc, connector) we would
>>>> need to add their own drm_XXX_funcs, so feedbacks are welcome!
>>>>
>>>> 	André
>>>>
>>>> André Almeida (2):
>>>>     drm/atomic: Allow drivers to write their own plane check for async
>>>>       flips
>>>>     drm/amdgpu: Implement check_async_props for planes
>>>>
>>>>    .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 30 +++++++++
>>>>    drivers/gpu/drm/drm_atomic_uapi.c             | 62 ++++++++++++++-----
>>>>    include/drm/drm_atomic_uapi.h                 | 12 ++++
>>>>    include/drm/drm_plane.h                       |  5 ++
>>>>    4 files changed, 92 insertions(+), 17 deletions(-)
>>>>   
>>>    
> 

Re: [PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5293 bytes --]

On Tue, 16 Jan 2024 17:10:18 +0100
Xaver Hugl <xaver.hugl@gmail.com> wrote:

> My plan is to require support for IN_FENCE_FD at least. If the driver
> doesn't
> allow tearing with that, then tearing just doesn't happen.

That's an excellent point. I think this is important enough in its own
right, that it should be called out in the patch series.

Is it important enough to be special-cased, e.g. to be always allowed
with async commits?

Now that I think of it, if userspace needs to wait for the in-fence
itself before kicking KMS async, that would defeat much of the async's
point, right? And cases where in-fence is not necessary are so rare
they might not even exist?

So if driver/hardware cannot do IN_FENCE_FD with async, is there any
use of supporting async to begin with?

> For overlay planes though, it depends on how the compositor prioritizes
> things.
> If the compositor prioritizes overlay planes and would like to do tearing
> if possible,
> then this patch works.

Ok, I can see that.

> If the compositor prioritizes tearing and would like to do overlay planes
> if possible,
> it would have to know that switching to synchronous commits for a single
> frame,
> setting up the overlay planes and then switching back to async commits
> works, and
> that can't be figured out with TEST_ONLY commits.

I had to ponder a bit why. So I guess the synchronous commit in between
is because driver/hardware may not be able to enable/disable extra
planes in async, so you need a synchronous commit to set them up, but
afterwards updates can tear.

The comment about Intel needing one more synchronous commit when
switching from sync to async updates comes to mind as well, would that
be a problem?

> So I think having a CAP or immutable plane property to signal that async
> commits
> with overlay and/or cursor planes is supported would be useful.

Async cursor planes a good point, particularly moving them around. I'm
not too informed about the prior/on-going efforts to allow cursor
movement more often than refresh rate, I recall something about
amending atomic commits? How would these interact?

I suppose the kernel still prevents any new async commit while a
previous commit is not finished, so amending commits would still be
necessary for cursor plane motion? Or would it, if you time "big
commits" to finish quickly and then spam async "cursor commits" in the
mean time?


Thanks,
pq

> Am Di., 16. Jan. 2024 um 14:35 Uhr schrieb André Almeida <
> andrealmeid@igalia.com>:  
> 
> > + Joshua
> >
> > Em 16/01/2024 10:14, Pekka Paalanen escreveu:  
> > > On Tue, 16 Jan 2024 08:50:59 -0300
> > > André Almeida <andrealmeid@igalia.com> wrote:
> > >  
> > >> Hi Pekka,
> > >>
> > >> Em 16/01/2024 06:45, Pekka Paalanen escreveu:  
> > >>> On Tue, 16 Jan 2024 01:51:57 -0300
> > >>> André Almeida <andrealmeid@igalia.com> wrote:
> > >>>  
> > >>>> Hi,
> > >>>>
> > >>>> AMD hardware can do more on the async flip path than just the primary  
> > plane, so  
> > >>>> to lift up the current restrictions, this patchset allows drivers to  
> > write their  
> > >>>> own check for planes for async flips.  
> > >>>
> > >>> Hi,
> > >>>
> > >>> what's the userspace story for this, how could userspace know it could  
> > do more?  
> > >>> What kind of userspace would take advantage of this and in what  
> > situations?  
> > >>>
> > >>> Or is this not meant for generic userspace?  
> > >>
> > >> Sorry, I forgot to document this. So the idea is that userspace will
> > >> query what they can do here with DRM_MODE_ATOMIC_TEST_ONLY calls,
> > >> instead of having capabilities for each prop.  
> > >
> > > That's the theory, but do you have a practical example?
> > >
> > > What other planes and props would one want change in some specific use
> > > case?
> > >
> > > Is it just "all or nothing", or would there be room to choose and pick
> > > which props you change and which you don't based on what the driver
> > > supports? If the latter, then relying on TEST_ONLY might be yet another
> > > combinatorial explosion to iterate through.
> > >  
> >
> > That's a good question, maybe Simon, Xaver or Joshua can share how they
> > were planning to use this on Gamescope or Kwin.
> >  
> > >
> > > Thanks,
> > > pq
> > >  
> > >>>> I'm not sure if adding something new to drm_plane_funcs is the right  
> > way to do,  
> > >>>> because if we want to expand the other object types (crtc, connector)  
> > we would  
> > >>>> need to add their own drm_XXX_funcs, so feedbacks are welcome!
> > >>>>
> > >>>>    André
> > >>>>
> > >>>> André Almeida (2):
> > >>>>     drm/atomic: Allow drivers to write their own plane check for async
> > >>>>       flips
> > >>>>     drm/amdgpu: Implement check_async_props for planes
> > >>>>
> > >>>>    .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 30 +++++++++
> > >>>>    drivers/gpu/drm/drm_atomic_uapi.c             | 62  
> > ++++++++++++++-----  
> > >>>>    include/drm/drm_atomic_uapi.h                 | 12 ++++
> > >>>>    include/drm/drm_plane.h                       |  5 ++
> > >>>>    4 files changed, 92 insertions(+), 17 deletions(-)
> > >>>>  
> > >>>  
> > >  
> >  


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

Re: [PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[Xaver Hugl]

Am Mi., 17. Jan. 2024 um 09:55 Uhr schrieb Pekka Paalanen <ppaalanen@gmail.com>:
> Is it important enough to be special-cased, e.g. to be always allowed
> with async commits?

I thought so, and sent a patch to dri-devel to make it happen, but
there are some
concerns about untested driver paths.
https://lists.freedesktop.org/archives/dri-devel/2024-January/437511.html

> Now that I think of it, if userspace needs to wait for the in-fence
> itself before kicking KMS async, that would defeat much of the async's
> point, right? And cases where in-fence is not necessary are so rare
> they might not even exist?
>
> So if driver/hardware cannot do IN_FENCE_FD with async, is there any
> use of supporting async to begin with?

KWin never commits a buffer where IN_FENCE_FD would actually delay the
pageflip; it's really only used to disable implicit sync, as there's some edge
cases where it can wrongly delay the pageflip. The waiting for buffers to become
readable on the compositor side isn't really significant in terms of latency.

If hardware doesn't support IN_FENCE_FD with async commits, checking if the
fence is already signaled at commit time would thus still make things work, at
least for KWin.

> > If the compositor prioritizes tearing and would like to do overlay planes
> > if possible,
> > it would have to know that switching to synchronous commits for a single
> > frame,
> > setting up the overlay planes and then switching back to async commits
> > works, and
> > that can't be figured out with TEST_ONLY commits.
>
> I had to ponder a bit why. So I guess the synchronous commit in between
> is because driver/hardware may not be able to enable/disable extra
> planes in async, so you need a synchronous commit to set them up, but
> afterwards updates can tear.

The hardware could be a factor, yes, but I've been thinking more about the API.
With this patchset, the compositor is still only allowed to change a
limited set of
plane properties - but it needs to set at least SRC_X, SRC_Y, CRTC_X etc on
the overlay plane(s) to the correct values before it can only change the allowed
properties again.

> The comment about Intel needing one more synchronous commit when
> switching from sync to async updates comes to mind as well, would that
> be a problem?

With only one synchronous update, the compositor could theoretically mask the
issue by committing it right before vblank; with that one
implicitly-sync "async"
commit though, you'd definitely get one frame without async commits. Having a
flag for a sync-but-then-async-again commit could solve that issue.

In practice I don't think anyone will notice one or two frames without
async commits.
It should be a pretty rare occurance, usually when the game or match
starts or an
overlay gets opened, so I doubt it's worth putting effort in to fix that.

> > So I think having a CAP or immutable plane property to signal that async
> > commits
> > with overlay and/or cursor planes is supported would be useful.
>
> Async cursor planes a good point, particularly moving them around. I'm
> not too informed about the prior/on-going efforts to allow cursor
> movement more often than refresh rate, I recall something about
> amending atomic commits? How would these interact?
>
> I suppose the kernel still prevents any new async commit while a
> previous commit is not finished, so amending commits would still be
> necessary for cursor plane motion? Or would it, if you time "big
> commits" to finish quickly and then spam async "cursor commits" in the
> mean time?

With async commits for cursor planes I'm really only talking about
getting to use
the cursor plane while doing async commits on the primary plane.

FWIW I personally consider the amend stuff mostly solved - KWin does that
internally since a few months ago now, with a separate thread to amend and
even reorder commits in a queue, and only actually commit immediately
before vblank.

>
> Thanks,
> pq
>
> > Am Di., 16. Jan. 2024 um 14:35 Uhr schrieb André Almeida <
> > andrealmeid@igalia.com>:
> >
> > > + Joshua
> > >
> > > Em 16/01/2024 10:14, Pekka Paalanen escreveu:
> > > > On Tue, 16 Jan 2024 08:50:59 -0300
> > > > André Almeida <andrealmeid@igalia.com> wrote:
> > > >
> > > >> Hi Pekka,
> > > >>
> > > >> Em 16/01/2024 06:45, Pekka Paalanen escreveu:
> > > >>> On Tue, 16 Jan 2024 01:51:57 -0300
> > > >>> André Almeida <andrealmeid@igalia.com> wrote:
> > > >>>
> > > >>>> Hi,
> > > >>>>
> > > >>>> AMD hardware can do more on the async flip path than just the primary
> > > plane, so
> > > >>>> to lift up the current restrictions, this patchset allows drivers to
> > > write their
> > > >>>> own check for planes for async flips.
> > > >>>
> > > >>> Hi,
> > > >>>
> > > >>> what's the userspace story for this, how could userspace know it could
> > > do more?
> > > >>> What kind of userspace would take advantage of this and in what
> > > situations?
> > > >>>
> > > >>> Or is this not meant for generic userspace?
> > > >>
> > > >> Sorry, I forgot to document this. So the idea is that userspace will
> > > >> query what they can do here with DRM_MODE_ATOMIC_TEST_ONLY calls,
> > > >> instead of having capabilities for each prop.
> > > >
> > > > That's the theory, but do you have a practical example?
> > > >
> > > > What other planes and props would one want change in some specific use
> > > > case?
> > > >
> > > > Is it just "all or nothing", or would there be room to choose and pick
> > > > which props you change and which you don't based on what the driver
> > > > supports? If the latter, then relying on TEST_ONLY might be yet another
> > > > combinatorial explosion to iterate through.
> > > >
> > >
> > > That's a good question, maybe Simon, Xaver or Joshua can share how they
> > > were planning to use this on Gamescope or Kwin.
> > >
> > > >
> > > > Thanks,
> > > > pq
> > > >
> > > >>>> I'm not sure if adding something new to drm_plane_funcs is the right
> > > way to do,
> > > >>>> because if we want to expand the other object types (crtc, connector)
> > > we would
> > > >>>> need to add their own drm_XXX_funcs, so feedbacks are welcome!
> > > >>>>
> > > >>>>    André
> > > >>>>
> > > >>>> André Almeida (2):
> > > >>>>     drm/atomic: Allow drivers to write their own plane check for async
> > > >>>>       flips
> > > >>>>     drm/amdgpu: Implement check_async_props for planes
> > > >>>>
> > > >>>>    .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 30 +++++++++
> > > >>>>    drivers/gpu/drm/drm_atomic_uapi.c             | 62
> > > ++++++++++++++-----
> > > >>>>    include/drm/drm_atomic_uapi.h                 | 12 ++++
> > > >>>>    include/drm/drm_plane.h                       |  5 ++
> > > >>>>    4 files changed, 92 insertions(+), 17 deletions(-)
> > > >>>>
> > > >>>
> > > >
> > >
>

Re: [PATCH 0/2] drm/atomic: Allow drivers to write their own plane check for async

[Michel Dänzer]

On 2024-01-17 13:57, Xaver Hugl wrote:
> Am Mi., 17. Jan. 2024 um 09:55 Uhr schrieb Pekka Paalanen <ppaalanen@gmail.com>:
>> Is it important enough to be special-cased, e.g. to be always allowed
>> with async commits?
> 
> I thought so, and sent a patch to dri-devel to make it happen, but
> there are some
> concerns about untested driver paths.
> https://lists.freedesktop.org/archives/dri-devel/2024-January/437511.html
> 
>> Now that I think of it, if userspace needs to wait for the in-fence
>> itself before kicking KMS async, that would defeat much of the async's
>> point, right? And cases where in-fence is not necessary are so rare
>> they might not even exist?
>>
>> So if driver/hardware cannot do IN_FENCE_FD with async, is there any
>> use of supporting async to begin with?
> 
> KWin never commits a buffer where IN_FENCE_FD would actually delay the
> pageflip; it's really only used to disable implicit sync, as there's some edge
> cases where it can wrongly delay the pageflip. The waiting for buffers to become
> readable on the compositor side isn't really significant in terms of latency.
> 
> If hardware doesn't support IN_FENCE_FD with async commits, checking if the
> fence is already signaled at commit time would thus still make things work, at
> least for KWin.

That's how IN_FENCE_FD (and implicit sync) is handled anyway, in common code: It waits for all fences to signal before calling into the driver to commit the atomic commit.

I can't see why this wouldn't work with async commits, the same as with synchronous ones, with any driver.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer