Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 21, 2016 at 11:55:52AM +0100, Brian Starkey wrote:
> On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:
> > 2016-10-20 Brian Starkey <brian.starkey@arm.com>:
> > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > > index 657a33a..b898604 100644
> > > > --- a/include/drm/drm_crtc.h
> > > > +++ b/include/drm/drm_crtc.h
> > > > @@ -165,6 +165,8 @@ struct drm_crtc_state {
> > > > 	struct drm_property_blob *ctm;
> > > > 	struct drm_property_blob *gamma_lut;
> > > >
> > > > +	u64 __user *out_fence_ptr;
> > > > +
> > > 
> > > I'm somewhat not convinced about stashing a __user pointer in the
> > > CRTC atomic state. I know it gets cleared before the driver sees it,
> > > but if anything I think that hints that this isn't the right place to
> > > store it.
> > > 
> > > I've been trying to think of other ways to get from a property to
> > > something drm_mode_atomic_ioctl can use - maybe we can store some
> > > stuff in drm_atomic_state which only lives for the length of the ioctl
> > > call and put this pointer in there.
> > 
> > The drm_atomic_state is still visible by the drivers. Between there and
> > crtc_state, I would keep in the crtc_state for now.
> > 
> 
> Mm, yeah I suppose they could get to it if they went looking for it
> in ->atomic_set_property or something, but I was thinking of freeing
> it before the drm_atomic_commit.
> 
> Anyway, this way is certainly simpler, and setting it to NULL should
> be enough to limit the damage a driver can do :-)

+1 on moving this out of drm_crtc_state. drm_atomic_state already has
per-crtc structs, so easy to extend them with this. And yes drivers can
still see it, but mostly they're not supposed to touch drm_atomic_state
internals - the book-keeping is all done by the core.

The per-object state structs otoh are meant to be massively mangled by
drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch