diff mbox series

drm/doc: Document legacy_cursor_update better

Message ID 20201020143936.1089259-1-daniel.vetter@ffwll.ch
State New, archived
Headers show
Series drm/doc: Document legacy_cursor_update better | expand

Commit Message

Daniel Vetter Oct. 20, 2020, 2:39 p.m. UTC
It's the horror and shouldn't be used. Realized we're not clear on
this in a discussion with Rob about what msm is doing to better
support async commits.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_atomic.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Oct. 20, 2020, 2:48 p.m. UTC | #1
Also cc Sean, who reviewed the msm patch to double down on the
not-so-awesome async solution (I think at least, I'm still not
entirely sure what's all going on there):

commit 2d99ced787e3d0f251fa370d2aae83cf2085a8d9
Author: Rob Clark <robdclark@chromium.org>
Date:   Thu Aug 29 09:45:16 2019 -0700

   drm/msm: async commit support

On Tue, Oct 20, 2020 at 4:39 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> It's the horror and shouldn't be used. Realized we're not clear on
> this in a discussion with Rob about what msm is doing to better
> support async commits.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  include/drm/drm_atomic.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d07c851d255b..413fd0ca56a8 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -308,7 +308,6 @@ struct __drm_private_objs_state {
>   * struct drm_atomic_state - the global state object for atomic updates
>   * @ref: count of all references to this state (will not be freed until zero)
>   * @dev: parent DRM device
> - * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
>   * @async_update: hint for asynchronous plane update
>   * @planes: pointer to array of structures with per-plane data
>   * @crtcs: pointer to array of CRTC pointers
> @@ -336,6 +335,17 @@ struct drm_atomic_state {
>          * drm_atomic_crtc_needs_modeset().
>          */
>         bool allow_modeset : 1;
> +       /**
> +        * @legacy_cursor_update:
> +        *
> +        * Hint to enforce legacy cursor IOCTL semantics.
> +        *
> +        * WARNING: This is thoroughly broken and pretty much impossible to
> +        * implement correctly. Drivers must ignore this and should instead
> +        * implement &drm_plane_helper_funcs.atomic_async_check and
> +        * &drm_plane_helper_funcs.atomic_async_commit hooks. New users of this
> +        * flag are not allowed.
> +        */
>         bool legacy_cursor_update : 1;
>         bool async_update : 1;
>         /**
> --
> 2.28.0
>
Thomas Zimmermann Oct. 20, 2020, 2:55 p.m. UTC | #2
Hi

On 20.10.20 16:39, Daniel Vetter wrote:
> It's the horror and shouldn't be used. Realized we're not clear on
> this in a discussion with Rob about what msm is doing to better
> support async commits.
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  include/drm/drm_atomic.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d07c851d255b..413fd0ca56a8 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -308,7 +308,6 @@ struct __drm_private_objs_state {
>   * struct drm_atomic_state - the global state object for atomic updates
>   * @ref: count of all references to this state (will not be freed until zero)
>   * @dev: parent DRM device
> - * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
>   * @async_update: hint for asynchronous plane update
>   * @planes: pointer to array of structures with per-plane data
>   * @crtcs: pointer to array of CRTC pointers
> @@ -336,6 +335,17 @@ struct drm_atomic_state {
>  	 * drm_atomic_crtc_needs_modeset().
>  	 */
>  	bool allow_modeset : 1;
> +	/**
> +	 * @legacy_cursor_update:
> +	 *
> +	 * Hint to enforce legacy cursor IOCTL semantics.
> +	 *
> +	 * WARNING: This is thoroughly broken and pretty much impossible to
> +	 * implement correctly. Drivers must ignore this and should instead
> +	 * implement &drm_plane_helper_funcs.atomic_async_check and
> +	 * &drm_plane_helper_funcs.atomic_async_commit hooks. New users of this
> +	 * flag are not allowed.
> +	 */
>  	bool legacy_cursor_update : 1;

The text could also say why the field exists in the first place. If it's
supposed to go away, you could add an item to the TODO list.

Best regards
Thomas

>  	bool async_update : 1;
>  	/**
>
Daniel Vetter Oct. 20, 2020, 4:54 p.m. UTC | #3
On Tue, Oct 20, 2020 at 4:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> On 20.10.20 16:39, Daniel Vetter wrote:
> > It's the horror and shouldn't be used. Realized we're not clear on
> > this in a discussion with Rob about what msm is doing to better
> > support async commits.
> >
> > Cc: Rob Clark <robdclark@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  include/drm/drm_atomic.h | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index d07c851d255b..413fd0ca56a8 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -308,7 +308,6 @@ struct __drm_private_objs_state {
> >   * struct drm_atomic_state - the global state object for atomic updates
> >   * @ref: count of all references to this state (will not be freed until zero)
> >   * @dev: parent DRM device
> > - * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
> >   * @async_update: hint for asynchronous plane update
> >   * @planes: pointer to array of structures with per-plane data
> >   * @crtcs: pointer to array of CRTC pointers
> > @@ -336,6 +335,17 @@ struct drm_atomic_state {
> >        * drm_atomic_crtc_needs_modeset().
> >        */
> >       bool allow_modeset : 1;
> > +     /**
> > +      * @legacy_cursor_update:
> > +      *
> > +      * Hint to enforce legacy cursor IOCTL semantics.
> > +      *
> > +      * WARNING: This is thoroughly broken and pretty much impossible to
> > +      * implement correctly. Drivers must ignore this and should instead
> > +      * implement &drm_plane_helper_funcs.atomic_async_check and
> > +      * &drm_plane_helper_funcs.atomic_async_commit hooks. New users of this
> > +      * flag are not allowed.
> > +      */
> >       bool legacy_cursor_update : 1;
>
> The text could also say why the field exists in the first place. If it's
> supposed to go away, you could add an item to the TODO list.

"Hint to enforce legacy cursor IOCTL semantics." is the purpose of
this thing. It was used way back as pretty shoddy approach to handling
cursor updates, but is slowly dying.

Good point on adding a todo item for this, I'll respin with that added.
-Daniel
diff mbox series

Patch

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d07c851d255b..413fd0ca56a8 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -308,7 +308,6 @@  struct __drm_private_objs_state {
  * struct drm_atomic_state - the global state object for atomic updates
  * @ref: count of all references to this state (will not be freed until zero)
  * @dev: parent DRM device
- * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
  * @async_update: hint for asynchronous plane update
  * @planes: pointer to array of structures with per-plane data
  * @crtcs: pointer to array of CRTC pointers
@@ -336,6 +335,17 @@  struct drm_atomic_state {
 	 * drm_atomic_crtc_needs_modeset().
 	 */
 	bool allow_modeset : 1;
+	/**
+	 * @legacy_cursor_update:
+	 *
+	 * Hint to enforce legacy cursor IOCTL semantics.
+	 *
+	 * WARNING: This is thoroughly broken and pretty much impossible to
+	 * implement correctly. Drivers must ignore this and should instead
+	 * implement &drm_plane_helper_funcs.atomic_async_check and
+	 * &drm_plane_helper_funcs.atomic_async_commit hooks. New users of this
+	 * flag are not allowed.
+	 */
 	bool legacy_cursor_update : 1;
 	bool async_update : 1;
 	/**