Re: [PATCH 3/3] drm: Add DRM_MODESET_LOCK_BEGIN/END helpers

From: Daniel Vetter <daniel@ffwll.ch>
To: Sean Paul <sean@poorly.run>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	dri-devel@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
	Sean Paul <seanpaul@chromium.org>
Subject: Re: [PATCH 3/3] drm: Add DRM_MODESET_LOCK_BEGIN/END helpers
Date: Wed, 28 Nov 2018 23:42:41 +0100	[thread overview]
Message-ID: <20181128224241.GC21184@phenom.ffwll.local> (raw)
In-Reply-To: <20181128143111.GG154175@art_vandelay>

On Wed, Nov 28, 2018 at 09:31:11AM -0500, Sean Paul wrote:
> On Wed, Nov 28, 2018 at 10:01:07AM +0100, Daniel Vetter wrote:
> > On Tue, Nov 27, 2018 at 05:46:40PM -0500, Sean Paul wrote:
> > > diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> > > index a685d1bb21f26..6213a11445633 100644
> > > --- a/include/drm/drm_modeset_lock.h
> > > +++ b/include/drm/drm_modeset_lock.h
> > > @@ -130,4 +130,58 @@ void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
> > >  int drm_modeset_lock_all_ctx(struct drm_device *dev,
> > >  			     struct drm_modeset_acquire_ctx *ctx);
> > >  
> > > +/**
> > > + * DRM_MODESET_LOCK_ALL_BEGIN - Helper to acquire modeset locks
> > > + * @dev: drm device
> > > + * @ret: local ret/err/etc variable to track error status
> > > + * @ctx: local modeset acquire context, will be dereferenced
> > > + * @flags: DRM_MODESET_ACQUIRE_* flags to pass to acquire_init()
> > 
> > Full function name for the nice hyperlink. Needs a continuation line,
> > which just needs to be indentend.
> > 
> > And a bikeshed: I'd put ret last in both macros, I think that's where
> > usually the cursors/output variables are.
> 
> For _BEGIN is effectively a void, since it can't return with anything but
> ret==0. I agonized a little over doing this for _END, but figured since it was
> setting the value of ret, it might be misleading to put it at the end since
> folks might not realize that if they ignore it ret can still change (if that
> makes sense). In other words, I don't want people to think that:
> 
> ret = DRM_MODESET_LOCAL_ALL_END(ret, ctx);
> 
> behaves differently than 
> 
> DRM_MODESET_LOCAL_ALL_END(ret, ctx);
> 
> By not allowing the assignment, it might poke people to think more about ret on
> END.
> 
> I'm happy to have my mind changed on this, but figured context would be useful.
> 
> All other bikesheds LGTM.

I think I wasn't clear enough: I meant to put ret last in the parameter
list of each. Especially the (ret, ctx) ordering looks very strange to me.

Definitely agreed that these macros shouldn't have some kind of contrived
return value.
-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