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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
	Alex Deucher <alexander.deucher@amd.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Ben Skeggs <bskeggs@redhat.com>
Subject: Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects
Date: Tue, 19 Dec 2017 16:08:04 +0200	[thread overview]
Message-ID: <1919473.CnoUZJE4zg@avalon> (raw)
In-Reply-To: <20171219140036.GH26573@phenom.ffwll.local>

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 &drm_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
> > > >> + * &drm_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
> > > >> &drm_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