Date: Fri, 21 Oct 2016 15:00:51 +0200
From: Daniel Vetter <daniel@ffwll.ch>
To: Brian Starkey <brian.starkey@arm.com>
Cc: Gustavo Padovan <gustavo@padovan.org>, dri-devel@lists.freedesktop.org,
        marcheu@google.com, Daniel Stone <daniels@collabora.com>,
        seanpaul@google.com, Daniel Vetter <daniel.vetter@ffwll.ch>,
        linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com,
        Gustavo Padovan <gustavo.padovan@collabora.co.uk>,
        John Harrison <John.C.Harrison@Intel.com>, m.chehab@samsung.com
Subject: Re: [PATCH v5 4/4] drm/fence: add out-fences support
Message-ID: <20161021130051.GG20761@phenom.ffwll.local>
Mail-Followup-To: Brian Starkey <brian.starkey@arm.com>,
        Gustavo Padovan <gustavo@padovan.org>,
        dri-devel@lists.freedesktop.org, marcheu@google.com,
        Daniel Stone <daniels@collabora.com>, seanpaul@google.com,
        linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com,
        Gustavo Padovan <gustavo.padovan@collabora.co.uk>,
        John Harrison <John.C.Harrison@Intel.com>, m.chehab@samsung.com
References: <1476975005-30441-1-git-send-email-gustavo@padovan.org>
 <1476975005-30441-5-git-send-email-gustavo@padovan.org>
 <20161020174810.GB7165@e106950-lin.cambridge.arm.com>
 <20161020203017.GC2734@joana>
 <20161021105552.GB14913@e106950-lin.cambridge.arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161021105552.GB14913@e106950-lin.cambridge.arm.com>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1955
Lines: 47

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