| Message ID | 20220929184307.258331-1-contact@emersion.fr (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for atomic async page-flips | expand |
On 9/29/22 14:43, Simon Ser wrote: > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic > commits, aka. "immediate flip" (which might result in tearing). > The feature was only available via the legacy uAPI, however for > gaming use-cases it may be desirable to enable it via the atomic > uAPI too. > > - Patchwork: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F107683%2F&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=J3DbO0agtOXfEt0XNA%2FKvmSLmJrFPW048T214BSl4dA%3D&reserved=0 > - User-space patch: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FPlagman%2Fgamescope%2Fpull%2F595&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6vJftf%2Bl5h4vR3ANZKWw9AhghZpPhAcHdXR5EI8Xwrs%3D&reserved=0 > - IGT patch: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F107681%2F&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=hOJssKhjj39wIHkrd%2FEI8csLvpZDoENbakqqkN%2F22O8%3D&reserved=0 > > Main changes in v2: add docs, fail atomic commit if async flip isn't > possible. > > Changes in v3: add a note in the documentation about Intel hardware, > add R-b tags. > > Tested on an AMD Picasso iGPU (Simon) and an AMD Vangogh GPU (André). > > Simon Ser (6): > drm: document DRM_MODE_PAGE_FLIP_ASYNC > amd/display: only accept async flips for fast updates > drm: introduce drm_mode_config.atomic_async_page_flip_not_supported > drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits > drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP > amd/display: indicate support for atomic async page-flips on DC > Finally had a chance to go through this. Series looks good and is Reviewed-by: Harry Wentland <harry.wentland@amd.com> Harry > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++++ > .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 10 +++++++ > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 1 + > drivers/gpu/drm/drm_atomic_uapi.c | 28 +++++++++++++++++-- > drivers/gpu/drm/drm_ioctl.c | 5 ++++ > drivers/gpu/drm/i915/display/intel_display.c | 1 + > drivers/gpu/drm/nouveau/nouveau_display.c | 1 + > drivers/gpu/drm/vc4/vc4_kms.c | 1 + > include/drm/drm_mode_config.h | 11 ++++++++ > include/uapi/drm/drm.h | 10 ++++++- > include/uapi/drm/drm_mode.h | 16 +++++++++++ > 11 files changed, 88 insertions(+), 4 deletions(-) >
On Thu, Sep 29, 2022 at 06:43:15PM +0000, Simon Ser wrote: > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic > commits, aka. "immediate flip" (which might result in tearing). > The feature was only available via the legacy uAPI, however for > gaming use-cases it may be desirable to enable it via the atomic > uAPI too. > > - Patchwork: https://patchwork.freedesktop.org/series/107683/ > - User-space patch: https://github.com/Plagman/gamescope/pull/595 > - IGT patch: https://patchwork.freedesktop.org/series/107681/ So no tests that actually verify that the kernel properly rejects stuff stuff like modesets, gamma LUT updates, plane movement, etc.? > > Main changes in v2: add docs, fail atomic commit if async flip isn't > possible. > > Changes in v3: add a note in the documentation about Intel hardware, > add R-b tags. > > Tested on an AMD Picasso iGPU (Simon) and an AMD Vangogh GPU (André). > > Simon Ser (6): > drm: document DRM_MODE_PAGE_FLIP_ASYNC > amd/display: only accept async flips for fast updates > drm: introduce drm_mode_config.atomic_async_page_flip_not_supported > drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits > drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP > amd/display: indicate support for atomic async page-flips on DC > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++++ > .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 10 +++++++ > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 1 + > drivers/gpu/drm/drm_atomic_uapi.c | 28 +++++++++++++++++-- > drivers/gpu/drm/drm_ioctl.c | 5 ++++ > drivers/gpu/drm/i915/display/intel_display.c | 1 + > drivers/gpu/drm/nouveau/nouveau_display.c | 1 + > drivers/gpu/drm/vc4/vc4_kms.c | 1 + > include/drm/drm_mode_config.h | 11 ++++++++ > include/uapi/drm/drm.h | 10 ++++++- > include/uapi/drm/drm_mode.h | 16 +++++++++++ > 11 files changed, 88 insertions(+), 4 deletions(-) > > -- > 2.37.3 >
On Fri, Sep 30, 2022 at 04:52:56PM +0300, Ville Syrjälä wrote: > On Thu, Sep 29, 2022 at 06:43:15PM +0000, Simon Ser wrote: > > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic > > commits, aka. "immediate flip" (which might result in tearing). > > The feature was only available via the legacy uAPI, however for > > gaming use-cases it may be desirable to enable it via the atomic > > uAPI too. > > > > - Patchwork: https://patchwork.freedesktop.org/series/107683/ > > - User-space patch: https://github.com/Plagman/gamescope/pull/595 > > - IGT patch: https://patchwork.freedesktop.org/series/107681/ > > So no tests that actually verify that the kernel properly rejects > stuff stuff like modesets, gamma LUT updates, plane movement, > etc.? Pondering this a bit more, it just occurred to me the current driver level checks might easily lead to confusing behaviour. Eg. is the ioctl going to succeed if you ask for an async change of some random property while the crtc disabled, but fails if you ask for the same async property change when the crtc is active? So another reason why rejecting most properties already at the uapi level might be a good idea.
On Fri, Sep 30, 2022 at 05:19:07PM +0300, Ville Syrjälä wrote: > On Fri, Sep 30, 2022 at 04:52:56PM +0300, Ville Syrjälä wrote: > > On Thu, Sep 29, 2022 at 06:43:15PM +0000, Simon Ser wrote: > > > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic > > > commits, aka. "immediate flip" (which might result in tearing). > > > The feature was only available via the legacy uAPI, however for > > > gaming use-cases it may be desirable to enable it via the atomic > > > uAPI too. > > > > > > - Patchwork: https://patchwork.freedesktop.org/series/107683/ > > > - User-space patch: https://github.com/Plagman/gamescope/pull/595 > > > - IGT patch: https://patchwork.freedesktop.org/series/107681/ > > > > So no tests that actually verify that the kernel properly rejects > > stuff stuff like modesets, gamma LUT updates, plane movement, > > etc.? > > Pondering this a bit more, it just occurred to me the current driver > level checks might easily lead to confusing behaviour. Eg. is > the ioctl going to succeed if you ask for an async change of some > random property while the crtc disabled, but fails if you ask for > the same async property change when the crtc is active? > > So another reason why rejecting most properties already at > the uapi level might be a good idea. And just to be clear this is pretty much what I suggest: diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 79730fa1dd8e..471a2c703847 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, goto out; } + if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC && + prop != dev->mode_config.prop_fb_id) { + drm_mode_object_put(obj); + ret = -EINVAL; + goto out; + } + if (copy_from_user(&prop_value, prop_values_ptr + copied_props, sizeof(prop_value))) { That would actively discourage people from even attempting the "just dump all the state into the ioctl" approach with async flips since even the props whose value isn't even changing would be rejected.
On Fri, 30 Sep 2022 18:09:55 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > That would actively discourage people from even attempting the > "just dump all the state into the ioctl" approach with async flips > since even the props whose value isn't even changing would be rejected. About that. To me it looks like just a classic case of broken communication. The kernel developers assume that of course userspace will minimize the state set to only those properties that change, because...? Userspace developers think that of course the kernel will optimize the state set into minimal changes, because the kernel actually has the authoritative knowledge of what the current state is, and the driver actually knows which properties are costly and need to be optimized and which ones don't matter. It has never even occurred to me that the kernel would not compare next state to current state. No-one ever documented any expectations, did they? Thanks, pq
On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > On Fri, 30 Sep 2022 18:09:55 +0300 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > That would actively discourage people from even attempting the > > "just dump all the state into the ioctl" approach with async flips > > since even the props whose value isn't even changing would be rejected. > > About that. > > To me it looks like just a classic case of broken communication. > > The kernel developers assume that of course userspace will minimize the > state set to only those properties that change, because...? > > Userspace developers think that of course the kernel will optimize the > state set into minimal changes, because the kernel actually has the > authoritative knowledge of what the current state is, and the driver > actually knows which properties are costly and need to be optimized and > which ones don't matter. It has never even occurred to me that the > kernel would not compare next state to current state. > > No-one ever documented any expectations, did they? Do you really want that for async flips? Async flips can't be done atomically with anything else, so why are you even asking the kernel to do that?
On Fri, Sep 30, 2022 at 06:45:09PM +0300, Ville Syrjälä wrote: > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > > On Fri, 30 Sep 2022 18:09:55 +0300 > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > That would actively discourage people from even attempting the > > > "just dump all the state into the ioctl" approach with async flips > > > since even the props whose value isn't even changing would be rejected. > > > > About that. > > > > To me it looks like just a classic case of broken communication. > > > > The kernel developers assume that of course userspace will minimize the > > state set to only those properties that change, because...? > > > > Userspace developers think that of course the kernel will optimize the > > state set into minimal changes, because the kernel actually has the > > authoritative knowledge of what the current state is, and the driver > > actually knows which properties are costly and need to be optimized and > > which ones don't matter. It has never even occurred to me that the > > kernel would not compare next state to current state. > > > > No-one ever documented any expectations, did they? > > Do you really want that for async flips? Async flips can't be > done atomically with anything else, so why are you even asking > the kernel to do that? Also what if you want plane 1 to do an async flip, and plane 2 to do a sync flip? With the single async flag per commit you will end up with one of the following results: plane 1 plane 2 outcome can async can async async flip on both planes (totally not what you wanted) can async can't async -EINVAL (what you actually wanted but got unfairly rejected) can't async can async -EINVAL can't async can't async -EINVAL Those last two are actually reasonable outcomes since the plane where you did want to async flip didn't support it. But the first two are just nonsense results.
On Fri, 30 Sep 2022 18:45:09 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > > On Fri, 30 Sep 2022 18:09:55 +0300 > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > That would actively discourage people from even attempting the > > > "just dump all the state into the ioctl" approach with async flips > > > since even the props whose value isn't even changing would be rejected. > > > > About that. > > > > To me it looks like just a classic case of broken communication. > > > > The kernel developers assume that of course userspace will minimize the > > state set to only those properties that change, because...? > > > > Userspace developers think that of course the kernel will optimize the > > state set into minimal changes, because the kernel actually has the > > authoritative knowledge of what the current state is, and the driver > > actually knows which properties are costly and need to be optimized and > > which ones don't matter. It has never even occurred to me that the > > kernel would not compare next state to current state. > > > > No-one ever documented any expectations, did they? > > Do you really want that for async flips? Async flips can't be > done atomically with anything else, so why are you even asking > the kernel to do that? I'm not talking about async flips only. I'm talking about atomic commits in general. I don't think it's a good idea to make async atomic commits behave fundamentally different from sync atomic commits wrt. what non-changing state you are allowed to list in your state set or not. Isn't there common DRM code to convert an atomic commit state set into state objects? It probably starts by copying the current state, and then playing through the commit state set, setting all listed properties to their new values? Why wouldn't that loop maintain the knowledge of what actually changed? When you copy the current data, reset all changed-flags to false. When you apply each property from the commit set, compare the value and set the changed-flag only if the value changes. This manufacturing of the new tentative state set happens at atomic commit ioctl time before the ioctl returns, right? So the current state is well-defined: any previous atomic sync or async commit can be assumed to have succeeded even if it hasn't applied in hardware yet if the commit ioctl for it succeeded, right? Thanks, pq
On Mon, Oct 03, 2022 at 11:48:49AM +0300, Pekka Paalanen wrote: > On Fri, 30 Sep 2022 18:45:09 +0300 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > > > On Fri, 30 Sep 2022 18:09:55 +0300 > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > > > That would actively discourage people from even attempting the > > > > "just dump all the state into the ioctl" approach with async flips > > > > since even the props whose value isn't even changing would be rejected. > > > > > > About that. > > > > > > To me it looks like just a classic case of broken communication. > > > > > > The kernel developers assume that of course userspace will minimize the > > > state set to only those properties that change, because...? > > > > > > Userspace developers think that of course the kernel will optimize the > > > state set into minimal changes, because the kernel actually has the > > > authoritative knowledge of what the current state is, and the driver > > > actually knows which properties are costly and need to be optimized and > > > which ones don't matter. It has never even occurred to me that the > > > kernel would not compare next state to current state. > > > > > > No-one ever documented any expectations, did they? > > > > Do you really want that for async flips? Async flips can't be > > done atomically with anything else, so why are you even asking > > the kernel to do that? > > I'm not talking about async flips only. > > I'm talking about atomic commits in general. I don't think it's a good > idea to make async atomic commits behave fundamentally different from > sync atomic commits wrt. what non-changing state you are allowed to > list in your state set or not. > > Isn't there common DRM code to convert an atomic commit state set into > state objects? It probably starts by copying the current state, and > then playing through the commit state set, setting all listed > properties to their new values? Why wouldn't that loop maintain the > knowledge of what actually changed? Any such book keeping is entirely ad-hoc atm. It's also not super obvious how much of it is actually useful. You have to do a real commit on the crtc anyway if the crtc (or on any of its associated objects) is listed in the commit, so there's not necessarily much to be gained by tracking chages in all properties. And that behaviour again enters very muddy waters when combined with the async flip flag for the entire commit. The current approach being proposed seems to suggest that we can instead short circuit async commits when nothing has changed. That is not at all how sync atomic commits work. > When you copy the current data, reset all changed-flags to false. When > you apply each property from the commit set, compare the value and set > the changed-flag only if the value changes. > > This manufacturing of the new tentative state set happens at atomic > commit ioctl time before the ioctl returns, right? So the current state > is well-defined: any previous atomic sync or async commit can be assumed to > have succeeded even if it hasn't applied in hardware yet if the commit > ioctl for it succeeded, right? Yes. We could certainly try to fully track all changes in properties, but no has measured if there's any real benefit of doing so.
> > > So no tests that actually verify that the kernel properly rejects > > > stuff stuff like modesets, gamma LUT updates, plane movement, > > > etc.? > > > > Pondering this a bit more, it just occurred to me the current driver > > level checks might easily lead to confusing behaviour. Eg. is > > the ioctl going to succeed if you ask for an async change of some > > random property while the crtc disabled, but fails if you ask for > > the same async property change when the crtc is active? > > > > So another reason why rejecting most properties already at > > the uapi level might be a good idea. > > And just to be clear this is pretty much what I suggest: > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index 79730fa1dd8e..471a2c703847 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > goto out; > } > > + if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC && > + prop != dev->mode_config.prop_fb_id) { > + drm_mode_object_put(obj); > + ret = -EINVAL; > + goto out; > + } > + > if (copy_from_user(&prop_value, > prop_values_ptr + copied_props, > sizeof(prop_value))) { > > > That would actively discourage people from even attempting the > "just dump all the state into the ioctl" approach with async flips > since even the props whose value isn't even changing would be rejected. How does this sound? diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 945761968428..ffd16bdc7b83 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state, struct drm_file *file_priv, struct drm_mode_object *obj, struct drm_property *prop, - uint64_t prop_value) + uint64_t prop_value, + bool async_flip) { struct drm_mode_object *ref; int ret; + uint64_t old_val; if (!drm_property_change_valid_get(prop, prop_value, &ref)) return -EINVAL; + if (async_flip && prop != prop->dev->mode_config.prop_fb_id) { + ret = drm_atomic_get_property(obj, prop, &old_val); + if (ret != 0 || old_val != prop_value) { + drm_dbg_atomic(prop->dev, + "[PROP:%d:%s] cannot be changed during async flip\n", + prop->base.id, prop->name); + return -EINVAL; + } + } + switch (obj->type) { case DRM_MODE_OBJECT_CONNECTOR: { struct drm_connector *connector = obj_to_connector(obj);
On 10/13/22 13:02, Simon Ser wrote: >>>> So no tests that actually verify that the kernel properly rejects >>>> stuff stuff like modesets, gamma LUT updates, plane movement, >>>> etc.? >>> >>> Pondering this a bit more, it just occurred to me the current driver >>> level checks might easily lead to confusing behaviour. Eg. is >>> the ioctl going to succeed if you ask for an async change of some >>> random property while the crtc disabled, but fails if you ask for >>> the same async property change when the crtc is active? >>> >>> So another reason why rejecting most properties already at >>> the uapi level might be a good idea. >> >> And just to be clear this is pretty much what I suggest: >> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >> index 79730fa1dd8e..471a2c703847 100644 >> --- a/drivers/gpu/drm/drm_atomic_uapi.c >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >> @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, >> goto out; >> } >> >> + if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC && >> + prop != dev->mode_config.prop_fb_id) { >> + drm_mode_object_put(obj); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> if (copy_from_user(&prop_value, >> prop_values_ptr + copied_props, >> sizeof(prop_value))) { >> >> >> That would actively discourage people from even attempting the >> "just dump all the state into the ioctl" approach with async flips >> since even the props whose value isn't even changing would be rejected. > > How does this sound? > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index 945761968428..ffd16bdc7b83 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state, > struct drm_file *file_priv, > struct drm_mode_object *obj, > struct drm_property *prop, > - uint64_t prop_value) > + uint64_t prop_value, > + bool async_flip) > { > struct drm_mode_object *ref; > int ret; > + uint64_t old_val; > > if (!drm_property_change_valid_get(prop, prop_value, &ref)) > return -EINVAL; > > + if (async_flip && prop != prop->dev->mode_config.prop_fb_id) { > + ret = drm_atomic_get_property(obj, prop, &old_val); > + if (ret != 0 || old_val != prop_value) { > + drm_dbg_atomic(prop->dev, > + "[PROP:%d:%s] cannot be changed during async flip\n", > + prop->base.id, prop->name); I would write this as "[PROP:%d:%s] No prop can be changed during async flips" to make it clear that it's not just this prop that can't, but any. > + return -EINVAL; > + } > + } > + > switch (obj->type) { > case DRM_MODE_OBJECT_CONNECTOR: { > struct drm_connector *connector = obj_to_connector(obj);
On 10/13/22 13:02, Simon Ser wrote: >>>> So no tests that actually verify that the kernel properly rejects >>>> stuff stuff like modesets, gamma LUT updates, plane movement, >>>> etc.? >>> >>> Pondering this a bit more, it just occurred to me the current driver >>> level checks might easily lead to confusing behaviour. Eg. is >>> the ioctl going to succeed if you ask for an async change of some >>> random property while the crtc disabled, but fails if you ask for >>> the same async property change when the crtc is active? >>> >>> So another reason why rejecting most properties already at >>> the uapi level might be a good idea. >> >> And just to be clear this is pretty much what I suggest: >> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >> index 79730fa1dd8e..471a2c703847 100644 >> --- a/drivers/gpu/drm/drm_atomic_uapi.c >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >> @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, >> goto out; >> } >> >> + if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC && >> + prop != dev->mode_config.prop_fb_id) { >> + drm_mode_object_put(obj); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> if (copy_from_user(&prop_value, >> prop_values_ptr + copied_props, >> sizeof(prop_value))) { >> >> >> That would actively discourage people from even attempting the >> "just dump all the state into the ioctl" approach with async flips >> since even the props whose value isn't even changing would be rejected. > > How does this sound? > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index 945761968428..ffd16bdc7b83 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state, > struct drm_file *file_priv, > struct drm_mode_object *obj, > struct drm_property *prop, > - uint64_t prop_value) > + uint64_t prop_value, > + bool async_flip) > { > struct drm_mode_object *ref; > int ret; > + uint64_t old_val; > > if (!drm_property_change_valid_get(prop, prop_value, &ref)) > return -EINVAL; > > + if (async_flip && prop != prop->dev->mode_config.prop_fb_id) { > + ret = drm_atomic_get_property(obj, prop, &old_val); > + if (ret != 0 || old_val != prop_value) { > + drm_dbg_atomic(prop->dev, > + "[PROP:%d:%s] cannot be changed during async flip\n", > + prop->base.id, prop->name); > + return -EINVAL; > + } > + } > + > switch (obj->type) { > case DRM_MODE_OBJECT_CONNECTOR: { > struct drm_connector *connector = obj_to_connector(obj); drm_atomic_get_property() needs the object lock to be used, so we need to check the property inside the switch-case like this: -- >8 -- From f3ee5a1163bfe5a88109d7084208940fe5566967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Almeida?= <andrealmeid@igalia.com> Date: Thu, 27 Oct 2022 17:23:09 -0300 Subject: [PATCH] drm: Check for prop changes in atomic async flip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No prop changes are allowed during an async flip via atomic DRM API, so make sure to reject them. Signed-off-by: André Almeida <andrealmeid@igalia.com> --- drivers/gpu/drm/drm_atomic_uapi.c | 47 +++++++++++++++++++++++++++-- drivers/gpu/drm/drm_crtc_internal.h | 2 +- drivers/gpu/drm/drm_mode_object.c | 2 +- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index ee24ed7e2edb..f63f23305621 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -964,13 +964,28 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, return ret; } +static bool 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->base.id, prop->name); + return true; + } + + return false; +} + int drm_atomic_set_property(struct drm_atomic_state *state, struct drm_file *file_priv, struct drm_mode_object *obj, struct drm_property *prop, - uint64_t prop_value) + uint64_t prop_value, + bool async_flip) { struct drm_mode_object *ref; + uint64_t old_val; int ret; if (!drm_property_change_valid_get(prop, prop_value, &ref)) @@ -987,6 +1002,15 @@ int drm_atomic_set_property(struct drm_atomic_state *state, break; } + if (async_flip) { + ret = drm_atomic_connector_get_property(connector, connector_state, + prop, &old_val); + if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) { + ret = -EINVAL; + break; + } + } + ret = drm_atomic_connector_set_property(connector, connector_state, file_priv, prop, prop_value); @@ -1002,6 +1026,14 @@ int drm_atomic_set_property(struct drm_atomic_state *state, break; } + if (async_flip) { + ret = drm_atomic_crtc_get_property(crtc, crtc_state, prop, &old_val); + if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) { + ret = -EINVAL; + break; + } + } + ret = drm_atomic_crtc_set_property(crtc, crtc_state, prop, prop_value); break; @@ -1016,6 +1048,14 @@ int drm_atomic_set_property(struct drm_atomic_state *state, break; } + if (async_flip && prop != prop->dev->mode_config.prop_fb_id) { + ret = drm_atomic_plane_get_property(plane, plane_state, prop, &old_val); + if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) { + ret = -EINVAL; + break; + } + } + ret = drm_atomic_plane_set_property(plane, plane_state, file_priv, prop, prop_value); @@ -1304,6 +1344,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_out_fence_state *fence_state; int ret = 0; unsigned int i, j, num_fences; + bool async = false; /* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1340,6 +1381,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported with atomic\n"); return -EINVAL; } + + async = true; } /* can't test and expect an event at the same time. */ @@ -1420,7 +1463,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, } ret = drm_atomic_set_property(state, file_priv, - obj, prop, prop_value); + obj, prop, prop_value, async); if (ret) { drm_mode_object_put(obj); goto out; diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 56041b604881..42ff11706fd4 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -250,7 +250,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state, struct drm_file *file_priv, struct drm_mode_object *obj, struct drm_property *prop, - uint64_t prop_value); + uint64_t prop_value, bool async_flip); int drm_atomic_get_property(struct drm_mode_object *obj, struct drm_property *property, uint64_t *val); diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index ba1608effc0f..64f519254895 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -536,7 +536,7 @@ static int set_property_atomic(struct drm_mode_object *obj, obj_to_connector(obj), prop_value); } else { - ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value); + ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value, false); if (ret) goto out; ret = drm_atomic_commit(state);
Ville, any news on this?
Hm, thinking about this again, there's still something which is a bit off with the new approach. Let's say the caller sets MODE_ID to another blob ID, but with the same blob payload. DRM core is smart enough to figure out that the mode didn't change and skip the modeset. However, the check implemented here isn't smart enough. tl;dr there are still discrepancies between the regular code-path and the async page-flip one. Does that matter?