On Sun, 28 Jan 2024 18:25:13 -0300 André Almeida <andrealm...@igalia.com> 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 <andrealm...@igalia.com>
> ---
> 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.
> +
> + /*
> + * 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.
> + }
> +
> + 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);
> };
>
> /**
pgp3Q5a5tiEMe.pgp
Description: OpenPGP digital signature
