| Message ID | 20240128212515.630345-2-andrealmeid@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic: Allow drivers to write their own plane check for async | expand |
On Sun, 28 Jan 2024 18:25:13 -0300 André Almeida <andrealmeid@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 <andrealmeid@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); > }; > > /**
Hi Pekka, Em 29/01/2024 05:49, Pekka Paalanen escreveu: > On Sun, 28 Jan 2024 18:25:13 -0300 > André Almeida <andrealmeid@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 <andrealmeid@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. > 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); >> }; >> >> /** >
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); }; /**
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> --- 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(-)