subject:"\[PATCH 4\/5\] drm\/atomic\: document how to handle driver private objects"

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-19 Thread Laurent Pinchart

Hi Daniel,

On Tuesday, 19 December 2017 16:00:36 EET Daniel Vetter wrote:
> On Tue, Dec 19, 2017 at 03:33:57PM +0200, Laurent Pinchart wrote:
> > On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote:
> > > On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote:
> > > > On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> > > >> + * Drivers are recommend to wrap these for each type of driver
> > > >> private
> > > >> state
> > > >> + * object they have, filtering on _private_obj.funcs using
> > > >> for_each_if(), at
> > > >> + * least if they want to iterate over all objects of a given type.
> > > >> + *
> > > >> + * An earlier way to handle driver private state was by subclassing
> > > >> struct
> > > >> + * _atomic_state. But since that encourages non-standard ways to
> > > >> implement
> > > >> + * the check/commit split atomic requires (by using e.g. "check and
> > > >> rollback or
> > > >> + * commit instead" of "duplicate state, check, then either commit or
> > > >> release
> > > >> + * duplicated state) it is deprecated in favour of using
> > > >> _private_state.
> > > > 
> > > > This I still don't agree with. I think it still makes sense to
> > > > subclass
> > > > the global state object when you have true global state data. How
> > > > about
> > > > starting by making it a recommendation instead, moving state data
> > > > related
> > > > to driver- specific objects to the new framework, and keeping global
> > > > data
> > > > in the drm_atomic_state subclass ?
> > > 
> > > Converting all the existing drivers over is somewhere on my todo. I'm
> > > also
> > > not really clear what you mean with global data compared to
> > > driver-specific objects ...
> > 
> > I'll take an example related to the rcar-du driver. The hardware groups
> > CRTCs by two and share resources (such as planes) between CRTCs in a
> > group. This is something I currently implement in a convoluted way, and
> > using private objects to handle groups (I already have a group object in
> > my driver) will likely help to model the group state.
> > 
> > On the other hand, if the hardware didn't have groups but shared planes
> > between all CRTCs, the shared resources would be global, and it would make
> > sense to store them in the global state.
> 
> Yeah the private stuff should probably get a hole lot better for singleton
> objects. I still think one global thing overall (and with a state handling
> pattern that's different from everything else) is not a good idea.
> 
> Now it would be fairly easy to generate all the silly boilerplate with a
> macro, but that tends to wreak havoc with cscope and friends, so I'm not
> sure it's a great idea really. I'll probably have better ideas once the
> i915 conversion exists ...

So how about splitting this in two steps then, first deprecating subclassing 
drm_atomic_state to store private object state, and only in a second step also 
deprecating subclassing the structure for global state ?

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-19 Thread Daniel Vetter

On Tue, Dec 19, 2017 at 03:33:57PM +0200, Laurent Pinchart wrote:
> On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote:
> > On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote:
> > > On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> > >> + * Drivers are recommend to wrap these for each type of driver private
> > >> state
> > >> + * object they have, filtering on _private_obj.funcs using
> > >> for_each_if(), at
> > >> + * least if they want to iterate over all objects of a given type.
> > >> + *
> > >> + * An earlier way to handle driver private state was by subclassing
> > >> struct
> > >> + * _atomic_state. But since that encourages non-standard ways to
> > >> implement
> > >> + * the check/commit split atomic requires (by using e.g. "check and
> > >> rollback or
> > >> + * commit instead" of "duplicate state, check, then either commit or
> > >> release
> > >> + * duplicated state) it is deprecated in favour of using
> > >> _private_state.
> > > 
> > > This I still don't agree with. I think it still makes sense to subclass
> > > the global state object when you have true global state data. How about
> > > starting by making it a recommendation instead, moving state data related
> > > to driver- specific objects to the new framework, and keeping global data
> > > in the drm_atomic_state subclass ?
> > 
> > Converting all the existing drivers over is somewhere on my todo. I'm also
> > not really clear what you mean with global data compared to
> > driver-specific objects ...
> 
> I'll take an example related to the rcar-du driver. The hardware groups CRTCs 
> by two and share resources (such as planes) between CRTCs in a group. This is 
> something I currently implement in a convoluted way, and using private 
> objects 
> to handle groups (I already have a group object in my driver) will likely 
> help 
> to model the group state.
> 
> On the other hand, if the hardware didn't have groups but shared planes 
> between all CRTCs, the shared resources would be global, and it would make 
> sense to store them in the global state.

Yeah the private stuff should probably get a hole lot better for singleton
objects. I still think one global thing overall (and with a state handling
pattern that's different from everything else) is not a good idea.

Now it would be fairly easy to generate all the silly boilerplate with a
macro, but that tends to wreak havoc with cscope and friends, so I'm not
sure it's a great idea really. I'll probably have better ideas once the
i915 conversion exists ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-19 Thread Laurent Pinchart

Hi Daniel,

On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote:
> On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote:
> > On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> >> DK put some nice docs into the commit introducing driver private
> >> state, but in the git history alone it'll be lost.
> > 
> > You might want to spell DK's name fully.
> > 
> >> Also, since Ville remove the void* usage it's a good opportunity to
> >> give the driver private stuff some tlc on the doc front.
> >> 
> >> Finally try to explain why the "let's just subclass drm_atomic_state"
> >> approach wasn't the greatest, and annotate all those functions as
> >> deprecated in favour of more standardized driver private states. Also
> >> note where we could/should extend driver private states going forward
> >> (atm neither locking nor synchronization is handled in core/helpers,
> >> which isn't really all that great).
> >> 
> >> Cc: Harry Wentland 
> >> Cc: Dhinakaran Pandiyan 
> >> Cc: Maarten Lankhorst 
> >> Cc: Ville Syrjälä 
> >> Cc: Laurent Pinchart 
> >> Cc: Rob Clark 
> >> Cc: Alex Deucher 
> >> Cc: Ben Skeggs 
> >> Signed-off-by: Daniel Vetter 
> >> ---
> >> 
> >>  Documentation/gpu/drm-kms.rst |  6 ++
> >>  drivers/gpu/drm/drm_atomic.c  | 45 ++---
> >>  include/drm/drm_atomic.h  | 28 +++
> >>  include/drm/drm_mode_config.h |  9 +
> >>  4 files changed, 85 insertions(+), 3 deletions(-)

[snip]

> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 37445d50816a..15e1a35c74a8 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c

[snip]

> >> +/**
> >> + * DOC: handling driver private state
> >> + *
> >> + * Very often the DRM objects exposed to userspace in the atomic
> >> modeset api
> > 
> > s/api/API/
> > 
> >> + * (_connector, _crtc and _plane) do not map neatly to the
> >> + * underlying hardware. Especially for any kind of shared resources
> >> (e.g. shared
> >> + * clocks, scaler units, bandwidth and fifo limits shared among a group
> >> of
> >> + * planes or CRTCs, and so on) it makes sense to model these as
> >> independent
> >> + * objects. Drivers then need to similar state tracking and commit
> >> ordering for
> >> + * such private (since not exposed to userpace) objects as the atomic
> >> core and
> >> + * helpers already provide for connectors, planes and CRTCs.
> >> 
> >> + * To make this easier on drivers the atomic core provides some support
> >> to track
> >> + * driver private state objects using struct _private_obj, with the
> >> + * associated state struct _private_state.
> >> + *
> >> + * Similar to userspace-exposed objects, state structures can be
> >> acquired by
> >> + * calling drm_atomic_get_private_obj_state(). Since this function does
> >> not take
> >> + * care of locking, drivers should wrap it for each type of private
> >> state object
> >> + * they have with the required call to drm_modeset_lock() for the
> >> corresponding
> >> + * _modeset_lock.
> > 
> > This paragraph could benefit from an explanation of what the corresponding
> > drm_modeset_lock is. The rest of the document is pretty clear.
> 
> Hm yeah ... This is also one of those things I'd like to improve in the
> private state stuff: If we add a filed for the lock (a pointer, not the
> lock itself) we could simplify this stuff a lot.

We don't have to fix everything in one go of course, but a small explanation 
of what drivers are supposed to do would be helpful.

> >> + * All private state structures contained in a _atomic_state update
> >> can be
> >> + * iterated using for_each_oldnew_private_obj_in_state(),
> >> + * for_each_old_private_obj_in_state() and
> >> for_each_old_private_obj_in_state().
> > 
> > I think one of those two was meant to be
> > for_each_new_private_obj_in_state().
> 
> Fixed.
> 
> >> + * Drivers are recommend to wrap these for each type of driver private
> >> state
> >> + * object they have, filtering on _private_obj.funcs using
> >> for_each_if(), at
> >> + * least if they want to iterate over all objects of a given type.
> >> + *
> >> + * An earlier way to handle driver private state was by subclassing
> >> struct
> >> + * _atomic_state. But since that encourages non-standard ways to
> >> implement
> >> + * the check/commit split atomic requires (by using e.g. "check and
> >> rollback or
> >> + * commit instead" of "duplicate state, check, then either commit or
> >> release
> >> + * duplicated state) it is deprecated in favour of using
> >> _private_state.
> > 
> > This I still don't agree with. I think it still makes sense to subclass
> > the global state object when you have true

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-19 Thread Daniel Vetter

On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> > DK put some nice docs into the commit introducing driver private
> > state, but in the git history alone it'll be lost.
> 
> You might want to spell DK's name fully.
> 
> > Also, since Ville remove the void* usage it's a good opportunity to
> > give the driver private stuff some tlc on the doc front.
> > 
> > Finally try to explain why the "let's just subclass drm_atomic_state"
> > approach wasn't the greatest, and annotate all those functions as
> > deprecated in favour of more standardized driver private states. Also
> > note where we could/should extend driver private states going forward
> > (atm neither locking nor synchronization is handled in core/helpers,
> > which isn't really all that great).
> > 
> > Cc: Harry Wentland 
> > Cc: Dhinakaran Pandiyan 
> > Cc: Maarten Lankhorst 
> > Cc: Ville Syrjälä 
> > Cc: Laurent Pinchart 
> > Cc: Rob Clark 
> > Cc: Alex Deucher 
> > Cc: Ben Skeggs 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  Documentation/gpu/drm-kms.rst |  6 ++
> >  drivers/gpu/drm/drm_atomic.c  | 45 +---
> >  include/drm/drm_atomic.h  | 28 +++
> >  include/drm/drm_mode_config.h |  9 +
> >  4 files changed, 85 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index 307284125d7a..420025bd6a9b 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -271,6 +271,12 @@ Taken all together there's two consequences for the
> > atomic design: Read on in this chapter, and also in
> > :ref:`drm_atomic_helper` for more detailed coverage of specific topics.
> > 
> > +Handling Driver Private State
> > +-
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> > +   :doc: handling driver private state
> > +
> >  Atomic Mode Setting Function Reference
> >  --
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 37445d50816a..15e1a35c74a8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
> >   * @state: atomic state
> >   *
> >   * Free all the memory allocated by drm_atomic_state_init.
> > - * This is useful for drivers that subclass the atomic state.
> > + * This should only be used by drivers which are still subclassing
> > + * _atomic_state and haven't switched to _private_state yet.
> >   */
> >  void drm_atomic_state_default_release(struct drm_atomic_state *state)
> >  {
> > @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
> >   * @state: atomic state
> >   *
> >   * Default implementation for filling in a new atomic state.
> > - * This is useful for drivers that subclass the atomic state.
> > + * This should only be used by drivers which are still subclassing
> > + * _atomic_state and haven't switched to _private_state yet.
> >   */
> >  int
> >  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state
> > *state)
> > @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
> >   * @state: atomic state
> >   *
> >   * Default implementation for clearing atomic state.
> > - * This is useful for drivers that subclass the atomic state.
> > + * This should only be used by drivers which are still subclassing
> > + * _atomic_state and haven't switched to _private_state yet.
> >   */
> >  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> >  {
> > @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct
> > drm_printer *p, plane->funcs->atomic_print_state(p, state);
> >  }
> > 
> > +/**
> > + * DOC: handling driver private state
> > + *
> > + * Very often the DRM objects exposed to userspace in the atomic modeset
> > api
> 
> s/api/API/
> 
> > + * (_connector, _crtc and _plane) do not map neatly to the
> > + * underlying hardware. Especially for any kind of shared resources (e.g.
> > shared
> > + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> > + * planes or CRTCs, and so on) it makes sense to model these as independent
> > + * objects. Drivers then need to similar state tracking and commit ordering
> > for
> > + * such private (since not exposed to userpace) objects as the atomic core
> > and
> > + * helpers already provide for connectors, planes and CRTCs.
> > 
> > + * To make this easier on drivers the atomic core provides some support to
> > track
> > + * driver private state objects using struct _private_obj,

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-18 Thread Laurent Pinchart

Hi Daniel,

Thank you for the patch.

On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> DK put some nice docs into the commit introducing driver private
> state, but in the git history alone it'll be lost.

You might want to spell DK's name fully.

> Also, since Ville remove the void* usage it's a good opportunity to
> give the driver private stuff some tlc on the doc front.
> 
> Finally try to explain why the "let's just subclass drm_atomic_state"
> approach wasn't the greatest, and annotate all those functions as
> deprecated in favour of more standardized driver private states. Also
> note where we could/should extend driver private states going forward
> (atm neither locking nor synchronization is handled in core/helpers,
> which isn't really all that great).
> 
> Cc: Harry Wentland 
> Cc: Dhinakaran Pandiyan 
> Cc: Maarten Lankhorst 
> Cc: Ville Syrjälä 
> Cc: Laurent Pinchart 
> Cc: Rob Clark 
> Cc: Alex Deucher 
> Cc: Ben Skeggs 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/gpu/drm-kms.rst |  6 ++
>  drivers/gpu/drm/drm_atomic.c  | 45 +---
>  include/drm/drm_atomic.h  | 28 +++
>  include/drm/drm_mode_config.h |  9 +
>  4 files changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 307284125d7a..420025bd6a9b 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -271,6 +271,12 @@ Taken all together there's two consequences for the
> atomic design: Read on in this chapter, and also in
> :ref:`drm_atomic_helper` for more detailed coverage of specific topics.
> 
> +Handling Driver Private State
> +-
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> +   :doc: handling driver private state
> +
>  Atomic Mode Setting Function Reference
>  --
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..15e1a35c74a8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
>   * @state: atomic state
>   *
>   * Free all the memory allocated by drm_atomic_state_init.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  void drm_atomic_state_default_release(struct drm_atomic_state *state)
>  {
> @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
>   * @state: atomic state
>   *
>   * Default implementation for filling in a new atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  int
>  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state
> *state)
> @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
>   * @state: atomic state
>   *
>   * Default implementation for clearing atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  {
> @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct
> drm_printer *p, plane->funcs->atomic_print_state(p, state);
>  }
> 
> +/**
> + * DOC: handling driver private state
> + *
> + * Very often the DRM objects exposed to userspace in the atomic modeset
> api

s/api/API/

> + * (_connector, _crtc and _plane) do not map neatly to the
> + * underlying hardware. Especially for any kind of shared resources (e.g.
> shared
> + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> + * planes or CRTCs, and so on) it makes sense to model these as independent
> + * objects. Drivers then need to similar state tracking and commit ordering
> for
> + * such private (since not exposed to userpace) objects as the atomic core
> and
> + * helpers already provide for connectors, planes and CRTCs.
> 
> + * To make this easier on drivers the atomic core provides some support to
> track
> + * driver private state objects using struct _private_obj, with the
> + * associated state struct _private_state.
> + *
> + * Similar to userspace-exposed objects, state structures can be acquired
> by
> + * calling drm_atomic_get_private_obj_state(). Since this function does not
> take
> + * care of locking, drivers should wrap it for each type of

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-18 Thread Laurent Pinchart

Hi Alex,

On Friday, 15 December 2017 03:57:48 EET Alex Deucher wrote:
> On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter wrote:
> > DK put some nice docs into the commit introducing driver private
> > state, but in the git history alone it'll be lost.
> > 
> > Also, since Ville remove the void* usage it's a good opportunity to
> > give the driver private stuff some tlc on the doc front.
> > 
> > Finally try to explain why the "let's just subclass drm_atomic_state"
> > approach wasn't the greatest, and annotate all those functions as
> > deprecated in favour of more standardized driver private states. Also
> > note where we could/should extend driver private states going forward
> > (atm neither locking nor synchronization is handled in core/helpers,
> > which isn't really all that great).
> > 
> > Cc: Harry Wentland 
> > Cc: Dhinakaran Pandiyan 
> > Cc: Maarten Lankhorst 
> > Cc: Ville Syrjälä 
> > Cc: Laurent Pinchart 
> > Cc: Rob Clark 
> > Cc: Alex Deucher 
> > Cc: Ben Skeggs 
> > Signed-off-by: Daniel Vetter 
> > ---
> > 
> >  Documentation/gpu/drm-kms.rst |  6 ++
> >  drivers/gpu/drm/drm_atomic.c  | 45 +++---
> >  include/drm/drm_atomic.h  | 28 +++
> >  include/drm/drm_mode_config.h |  9 +
> >  4 files changed, 85 insertions(+), 3 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 37445d50816a..15e1a35c74a8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c

[snip]

> > +/**
> > + * DOC: handling driver private state
> > + *
> > + * Very often the DRM objects exposed to userspace in the atomic modeset
> > api + * (_connector, _crtc and _plane) do not map neatly to
> > the + * underlying hardware. Especially for any kind of shared resources
> > (e.g. shared + * clocks, scaler units, bandwidth and fifo limits shared
> > among a group of + * planes or CRTCs, and so on) it makes sense to model
> > these as independent + * objects. Drivers then need to similar state
> > tracking and commit ordering for + * such private (since not exposed to
> > userpace) objects as the atomic core and + * helpers already provide for
> > connectors, planes and CRTCs.
> 
> This last sentence doesn't quite parse.  I think it should be as follows:
> 
> Drivers then need to do similar state tracking and commit ordering for
> such private (since not exposed to userpace) objects as the atomic core that
> helpers already provide for DRM objects (connectors, planes and CRTCs).

I think Daniel meant

"Drivers then need similar state tracking and commit ordering for such private 
(since not exposed to userpace) objects as the atomic core and helpers already 
provide for connectors, planes and CRTCs."

[snip]

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-14 Thread Pandiyan, Dhinakaran


On Thu, 2017-12-14 at 21:30 +0100, Daniel Vetter wrote:
> DK put some nice docs into the commit introducing driver private
> state, but in the git history alone it'll be lost.
> 
> Also, since Ville remove the void* usage it's a good opportunity to
> give the driver private stuff some tlc on the doc front.
> 
> Finally try to explain why the "let's just subclass drm_atomic_state"
> approach wasn't the greatest, and annotate all those functions as
> deprecated in favour of more standardized driver private states. Also
> note where we could/should extend driver private states going forward
> (atm neither locking nor synchronization is handled in core/helpers,
> which isn't really all that great).
> 

I have pointed out a couple of typos below, other than lgtm
Reviewed-by: Dhinakaran Pandiyan 

> Cc: Harry Wentland 
> Cc: Dhinakaran Pandiyan 
> Cc: Maarten Lankhorst 
> Cc: Ville Syrjälä 
> Cc: Laurent Pinchart 
> Cc: Rob Clark 
> Cc: Alex Deucher 
> Cc: Ben Skeggs 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/gpu/drm-kms.rst |  6 ++
>  drivers/gpu/drm/drm_atomic.c  | 45 
> ---
>  include/drm/drm_atomic.h  | 28 +++
>  include/drm/drm_mode_config.h |  9 +
>  4 files changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 307284125d7a..420025bd6a9b 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -271,6 +271,12 @@ Taken all together there's two consequences for the 
> atomic design:
>  Read on in this chapter, and also in :ref:`drm_atomic_helper` for more 
> detailed
>  coverage of specific topics.
>  
> +Handling Driver Private State
> +-
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> +   :doc: handling driver private state
> +
>  Atomic Mode Setting Function Reference
>  --
>  
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..15e1a35c74a8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
>   * @state: atomic state
>   *
>   * Free all the memory allocated by drm_atomic_state_init.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  void drm_atomic_state_default_release(struct drm_atomic_state *state)
>  {
> @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
>   * @state: atomic state
>   *
>   * Default implementation for filling in a new atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  int
>  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
> @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
>   * @state: atomic state
>   *
>   * Default implementation for clearing atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  {
> @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct 
> drm_printer *p,
>   plane->funcs->atomic_print_state(p, state);
>  }
>  
> +/**
> + * DOC: handling driver private state
> + *
> + * Very often the DRM objects exposed to userspace in the atomic modeset api
> + * (_connector, _crtc and _plane) do not map neatly to the
> + * underlying hardware. Especially for any kind of shared resources (e.g. 
> shared
> + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> + * planes or CRTCs, and so on) it makes sense to model these as independent
> + * objects. Drivers then need to similar state tracking and commit ordering 
> for
> + * such private (since not exposed to userpace) objects as the atomic core 
> and
> + * helpers already provide for connectors, planes and CRTCs.
> + *
> + * To make this easier on drivers the atomic core provides some support to 
> track
> + * driver private state objects using struct _private_obj, with the
> + * associated state struct _private_state.
> + *
> + * Similar to userspace-exposed objects, state structures can be acquired by
  ^private

> + * calling

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-14 Thread Alex Deucher

On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter  wrote:
> DK put some nice docs into the commit introducing driver private
> state, but in the git history alone it'll be lost.
>
> Also, since Ville remove the void* usage it's a good opportunity to
> give the driver private stuff some tlc on the doc front.
>
> Finally try to explain why the "let's just subclass drm_atomic_state"
> approach wasn't the greatest, and annotate all those functions as
> deprecated in favour of more standardized driver private states. Also
> note where we could/should extend driver private states going forward
> (atm neither locking nor synchronization is handled in core/helpers,
> which isn't really all that great).
>
> Cc: Harry Wentland 
> Cc: Dhinakaran Pandiyan 
> Cc: Maarten Lankhorst 
> Cc: Ville Syrjälä 
> Cc: Laurent Pinchart 
> Cc: Rob Clark 
> Cc: Alex Deucher 
> Cc: Ben Skeggs 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/gpu/drm-kms.rst |  6 ++
>  drivers/gpu/drm/drm_atomic.c  | 45 
> ---
>  include/drm/drm_atomic.h  | 28 +++
>  include/drm/drm_mode_config.h |  9 +
>  4 files changed, 85 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 307284125d7a..420025bd6a9b 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -271,6 +271,12 @@ Taken all together there's two consequences for the 
> atomic design:
>  Read on in this chapter, and also in :ref:`drm_atomic_helper` for more 
> detailed
>  coverage of specific topics.
>
> +Handling Driver Private State
> +-
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> +   :doc: handling driver private state
> +
>  Atomic Mode Setting Function Reference
>  --
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..15e1a35c74a8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
>   * @state: atomic state
>   *
>   * Free all the memory allocated by drm_atomic_state_init.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  void drm_atomic_state_default_release(struct drm_atomic_state *state)
>  {
> @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
>   * @state: atomic state
>   *
>   * Default implementation for filling in a new atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  int
>  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
> @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
>   * @state: atomic state
>   *
>   * Default implementation for clearing atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * _atomic_state and haven't switched to _private_state yet.
>   */
>  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  {
> @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct 
> drm_printer *p,
> plane->funcs->atomic_print_state(p, state);
>  }
>
> +/**
> + * DOC: handling driver private state
> + *
> + * Very often the DRM objects exposed to userspace in the atomic modeset api
> + * (_connector, _crtc and _plane) do not map neatly to the
> + * underlying hardware. Especially for any kind of shared resources (e.g. 
> shared
> + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> + * planes or CRTCs, and so on) it makes sense to model these as independent
> + * objects. Drivers then need to similar state tracking and commit ordering 
> for
> + * such private (since not exposed to userpace) objects as the atomic core 
> and
> + * helpers already provide for connectors, planes and CRTCs.

This last sentence doesn't quite parse.  I think it should be as follows:

Drivers then need to do similar state tracking and commit ordering for
such private (since not exposed to userpace) objects as the atomic core that
helpers already provide for DRM objects (connectors, planes and CRTCs).

Feel free to adjust as you see fit.  With that fixed up:
Reviewed-by: Alex Deucher 

> + *
> + * To make this easier on drivers the atomic core provides some support to 
>

[PATCH 4/5] drm/atomic: document how to handle driver private objects

2017-12-14 Thread Daniel Vetter

DK put some nice docs into the commit introducing driver private
state, but in the git history alone it'll be lost.

Also, since Ville remove the void* usage it's a good opportunity to
give the driver private stuff some tlc on the doc front.

Finally try to explain why the "let's just subclass drm_atomic_state"
approach wasn't the greatest, and annotate all those functions as
deprecated in favour of more standardized driver private states. Also
note where we could/should extend driver private states going forward
(atm neither locking nor synchronization is handled in core/helpers,
which isn't really all that great).

Cc: Harry Wentland 
Cc: Dhinakaran Pandiyan 
Cc: Maarten Lankhorst 
Cc: Ville Syrjälä 
Cc: Laurent Pinchart 
Cc: Rob Clark 
Cc: Alex Deucher 
Cc: Ben Skeggs 
Signed-off-by: Daniel Vetter 
---
 Documentation/gpu/drm-kms.rst |  6 ++
 drivers/gpu/drm/drm_atomic.c  | 45 ---
 include/drm/drm_atomic.h  | 28 +++
 include/drm/drm_mode_config.h |  9 +
 4 files changed, 85 insertions(+), 3 deletions(-)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 307284125d7a..420025bd6a9b 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -271,6 +271,12 @@ Taken all together there's two consequences for the atomic 
design:
 Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed
 coverage of specific topics.
 
+Handling Driver Private State
+-
+
+.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
+   :doc: handling driver private state
+
 Atomic Mode Setting Function Reference
 --
 
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 37445d50816a..15e1a35c74a8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
  * @state: atomic state
  *
  * Free all the memory allocated by drm_atomic_state_init.
- * This is useful for drivers that subclass the atomic state.
+ * This should only be used by drivers which are still subclassing
+ * _atomic_state and haven't switched to _private_state yet.
  */
 void drm_atomic_state_default_release(struct drm_atomic_state *state)
 {
@@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
  * @state: atomic state
  *
  * Default implementation for filling in a new atomic state.
- * This is useful for drivers that subclass the atomic state.
+ * This should only be used by drivers which are still subclassing
+ * _atomic_state and haven't switched to _private_state yet.
  */
 int
 drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
@@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
  * @state: atomic state
  *
  * Default implementation for clearing atomic state.
- * This is useful for drivers that subclass the atomic state.
+ * This should only be used by drivers which are still subclassing
+ * _atomic_state and haven't switched to _private_state yet.
  */
 void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 {
@@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct 
drm_printer *p,
plane->funcs->atomic_print_state(p, state);
 }
 
+/**
+ * DOC: handling driver private state
+ *
+ * Very often the DRM objects exposed to userspace in the atomic modeset api
+ * (_connector, _crtc and _plane) do not map neatly to the
+ * underlying hardware. Especially for any kind of shared resources (e.g. 
shared
+ * clocks, scaler units, bandwidth and fifo limits shared among a group of
+ * planes or CRTCs, and so on) it makes sense to model these as independent
+ * objects. Drivers then need to similar state tracking and commit ordering for
+ * such private (since not exposed to userpace) objects as the atomic core and
+ * helpers already provide for connectors, planes and CRTCs.
+ *
+ * To make this easier on drivers the atomic core provides some support to 
track
+ * driver private state objects using struct _private_obj, with the
+ * associated state struct _private_state.
+ *
+ * Similar to userspace-exposed objects, state structures can be acquired by
+ * calling drm_atomic_get_private_obj_state(). Since this function does not 
take
+ * care of locking, drivers should wrap it for each type of private state 
object
+ * they have with the required call to drm_modeset_lock() for the corresponding
+ * _modeset_lock.
+ *
+ * All private state structures contained in a _atomic_state update can be
+ * iterated using for_each_oldnew_private_obj_in_state(),
+ * for_each_old_private_obj_in_state() and for_each_old_private_obj_in_state().
+ *

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

Re: [Intel-gfx] [PATCH 4/5] drm/atomic: document how to handle driver private objects

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[PATCH 4/5] drm/atomic: document how to handle driver private objects

9 matches

Site Navigation

Mail list logo

Footer information