Re: [PATCH v3 1/3] drm/atomic: Allow drivers to write their own plane check for async flips
From: André Almeida
Date: Thu Feb 01 2024 - 13:44:59 EST
Hi Pekka,
Em 29/01/2024 05:49, Pekka Paalanen escreveu:
On Sun, 28 Jan 2024 18:25:13 -0300
André Almeida <andrealmeid@xxxxxxxxxx> wrote:
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@xxxxxxxxxx>
---
v3: no changes
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,
Hi,
should the word "async" be somewhere in the function name?
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);
Is it really ok to allow drivers to opt-in to also *reject* the FB_ID
and IN_FENCE_FD props on async commits?
Either intentionally or by accident.
Right, perhaps I should write this function in a way that you can only
lift restrictions, and not add new ones.
+
+ /*
+ * 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);
When I first read this code, it seemed to say: "If the prop is not
FB_ID, then do the usual prop change checking that happens on all
changes, not only async.". Reading this patch a few more times over, I
finally realized drm_atomic_check_prop_changes() is about async
specifically.
I see that the lack of the async word is giving some confusion, so I'll
add it to the functions.
Thanks,
André
+ }
+
+ 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,
Should there be "async" somewhere in the name of
drm_atomic_check_plane_changes()?
Thanks,
pq
+ 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);
};
/**