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

From: Daniel Vetter <daniel@ffwll.ch>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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>,
	Harry Wentland <harry.wentland@amd.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 15:00:36 +0100	[thread overview]
Message-ID: <20171219140036.GH26573@phenom.ffwll.local> (raw)
In-Reply-To: <5672659.FuCFnFTodp@avalon>

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 ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx