| Message ID | 20240119181235.255060-3-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 Fri, Jan 19, 2024 at 03:12:35PM -0300, André Almeida wrote: > AMD GPUs can do async flips with changes on more properties than just > the FB ID, so implement a custom check_async_props for AMD planes. > > Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS > properties. For userspace to check if a driver support this two > properties, the strategy for now is to use TEST_ONLY commits. > > Signed-off-by: André Almeida <andrealmeid@igalia.com> > --- > v2: Drop overlay plane option for now > > .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++++++++++++++++++ > 1 file changed, 29 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..7afe8c1b62d4 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,33 @@ 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 && IN_FENCE should just be allowed always. > + prop != config->prop_fb_damage_clips) { This seems a bit dubious to me. How is amdgpu using the damage information during async flips? > + 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; > +} > + > 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 +1466,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
On 2024-01-19 13:25, Ville Syrjälä wrote: > On Fri, Jan 19, 2024 at 03:12:35PM -0300, André Almeida wrote: >> AMD GPUs can do async flips with changes on more properties than just >> the FB ID, so implement a custom check_async_props for AMD planes. >> >> Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS >> properties. For userspace to check if a driver support this two >> properties, the strategy for now is to use TEST_ONLY commits. >> >> Signed-off-by: André Almeida <andrealmeid@igalia.com> >> --- >> v2: Drop overlay plane option for now >> >> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++++++++++++++++++ >> 1 file changed, 29 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..7afe8c1b62d4 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,33 @@ 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 && > > IN_FENCE should just be allowed always. > >> + prop != config->prop_fb_damage_clips) { > > This seems a bit dubious to me. How is amdgpu using the damage > information during async flips? Yeah, I'm also not sure this is right. Has anyone tested this with a PSR SU panel? Harry > >> + 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; >> +} >> + >> 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 +1466,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 >
Am Mo., 22. Jan. 2024 um 16:50 Uhr schrieb Harry Wentland <harry.wentland@amd.com>: > > > > On 2024-01-19 13:25, Ville Syrjälä wrote: > > On Fri, Jan 19, 2024 at 03:12:35PM -0300, André Almeida wrote: > >> AMD GPUs can do async flips with changes on more properties than just > >> the FB ID, so implement a custom check_async_props for AMD planes. > >> > >> Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS > >> properties. For userspace to check if a driver support this two > >> properties, the strategy for now is to use TEST_ONLY commits. > >> > >> Signed-off-by: André Almeida <andrealmeid@igalia.com> > >> --- > >> v2: Drop overlay plane option for now > >> > >> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++++++++++++++++++ > >> 1 file changed, 29 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..7afe8c1b62d4 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,33 @@ 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 && > > > > IN_FENCE should just be allowed always. > > > >> + prop != config->prop_fb_damage_clips) { > > > > This seems a bit dubious to me. How is amdgpu using the damage > > information during async flips? > > Yeah, I'm also not sure this is right. Has anyone tested this > with a PSR SU panel? > > Harry I attempted to, but according to /sys/kernel/debug/dri/1/eDP-1/psr_state, PSR never kicks in on my laptop at all. The only reason I wanted this property though is to reduce the number of special cases for async pageflips compositors have to implement; as it's not necessary for any functionality I think it's also fine to leave it out. > >> + 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; > >> +} > >> + > >> 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 +1466,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 > >
On 2024-01-23 13:02, Xaver Hugl wrote: > Am Mo., 22. Jan. 2024 um 16:50 Uhr schrieb Harry Wentland > <harry.wentland@amd.com>: >> >> >> >> On 2024-01-19 13:25, Ville Syrjälä wrote: >>> On Fri, Jan 19, 2024 at 03:12:35PM -0300, André Almeida wrote: >>>> AMD GPUs can do async flips with changes on more properties than just >>>> the FB ID, so implement a custom check_async_props for AMD planes. >>>> >>>> Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS >>>> properties. For userspace to check if a driver support this two >>>> properties, the strategy for now is to use TEST_ONLY commits. >>>> >>>> Signed-off-by: André Almeida <andrealmeid@igalia.com> >>>> --- >>>> v2: Drop overlay plane option for now >>>> >>>> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++++++++++++++++++ >>>> 1 file changed, 29 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..7afe8c1b62d4 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,33 @@ 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 && >>> >>> IN_FENCE should just be allowed always. >>> >>>> + prop != config->prop_fb_damage_clips) { >>> >>> This seems a bit dubious to me. How is amdgpu using the damage >>> information during async flips? >> >> Yeah, I'm also not sure this is right. Has anyone tested this >> with a PSR SU panel? >> >> Harry > > I attempted to, but according to > /sys/kernel/debug/dri/1/eDP-1/psr_state, PSR never kicks in on my > laptop at all. The only reason I wanted this property though is to > reduce the number of special cases for async pageflips compositors > have to implement; as it's not necessary for any functionality I think > it's also fine to leave it out. > Yeah, PSR panels aren't super common. PSR SU (Selective Update) panels even less so. I'd prefer to keep the damage clips out of async for now unless we can actually test it with a PSR SU panel. Harry >>>> + 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; >>>> +} >>>> + >>>> 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 +1466,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 >>>
Hi Ville, Em 19/01/2024 15:25, Ville Syrjälä escreveu: > On Fri, Jan 19, 2024 at 03:12:35PM -0300, André Almeida wrote: >> AMD GPUs can do async flips with changes on more properties than just >> the FB ID, so implement a custom check_async_props for AMD planes. >> >> Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS >> properties. For userspace to check if a driver support this two >> properties, the strategy for now is to use TEST_ONLY commits. >> >> Signed-off-by: André Almeida <andrealmeid@igalia.com> >> --- >> v2: Drop overlay plane option for now >> >> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++++++++++++++++++ >> 1 file changed, 29 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..7afe8c1b62d4 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,33 @@ 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 && > > IN_FENCE should just be allowed always. Do you mean that the common path should allow IN_FENCE_FD for all drivers?
On Wed, Jan 24, 2024 at 11:14:40AM -0300, André Almeida wrote: > Hi Ville, > > Em 19/01/2024 15:25, Ville Syrjälä escreveu: > > On Fri, Jan 19, 2024 at 03:12:35PM -0300, André Almeida wrote: > >> AMD GPUs can do async flips with changes on more properties than just > >> the FB ID, so implement a custom check_async_props for AMD planes. > >> > >> Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS > >> properties. For userspace to check if a driver support this two > >> properties, the strategy for now is to use TEST_ONLY commits. > >> > >> Signed-off-by: André Almeida <andrealmeid@igalia.com> > >> --- > >> v2: Drop overlay plane option for now > >> > >> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++++++++++++++++++ > >> 1 file changed, 29 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..7afe8c1b62d4 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,33 @@ 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 && > > > > IN_FENCE should just be allowed always. > > Do you mean that the common path should allow IN_FENCE_FD for all drivers? Yes.
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..7afe8c1b62d4 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,33 @@ 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) { + drm_dbg_atomic(prop->dev, + "[OBJECT:%d] Only primary 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 +1466,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,
AMD GPUs can do async flips with changes on more properties than just the FB ID, so implement a custom check_async_props for AMD planes. Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS properties. For userspace to check if a driver support this two properties, the strategy for now is to use TEST_ONLY commits. Signed-off-by: André Almeida <andrealmeid@igalia.com> --- v2: Drop overlay plane option for now .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+)