Re: [PATCH 2/6] drm: Introduce drm_mode_object_{get,put}()

From: Daniel Vetter <daniel@ffwll.ch>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/6] drm: Introduce drm_mode_object_{get,put}()
Date: Tue, 14 Feb 2017 21:12:56 +0100	[thread overview]
Message-ID: <20170214201256.GD22762@dvetter-linux.ger.corp.intel.com> (raw)
In-Reply-To: <CACvgo51hAbTiVOVkmoUSpcB9h43_BUM1CN0Xmpfg0-RpL9GChA@mail.gmail.com>

On Thu, Feb 09, 2017 at 09:55:22PM +0000, Emil Velikov wrote:
> On 9 February 2017 at 20:41, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Thu, Feb 09, 2017 at 06:08:10PM +0100, Daniel Vetter wrote:
> >> On Wed, Feb 08, 2017 at 02:28:14PM -0500, Sean Paul wrote:
> >> > On Wed, Feb 08, 2017 at 07:24:04PM +0100, Thierry Reding wrote:
> >> > > From: Thierry Reding <treding@nvidia.com>
> >> > >
> >> > > For consistency with other reference counting APIs in the kernel, add
> >> > > drm_mode_object_get() and drm_mode_object_put() to reference count DRM
> >> > > mode objects.
> >> > >
> >> > > Compatibility aliases are added to keep existing code working. To help
> >> > > speed up the transition, all the instances of the old functions in the
> >> > > DRM core are already replaced in this commit.
> >> > >
> >> >
> >> > drm code looks good and is
> >> >
> >> > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> >> >
> >> > > A semantic patch is provided that can be used to convert all drivers to
> >> > > the new helpers.
> >> >
> >> > I'm not convinced we need to commit the cocci patch. I think including it in
> >> > your cover letter and then following up with a follow on series to actually make
> >> > the change is sufficient (See: ickle's s/fence/dma_fence/ series).
> >>
> >> Yeah, if you do a large-scale refactor anyway, I think it's best to just
> >> store the cocci in the commit message. I think storing the cocci is ok if
> >> you have thousands of hits among lots of subsystems, and it's clear it's
> >> going to take at least a few release cycles or maybe even years to clean
> >> it all up. drm is luckily not yet that big :-)
> >>
> >> I'll drop this while applying if no one minds ...
> >
> > I thought it was actually quite nice that this was part of the series.
> > That way it doesn't get lost and it is really easy to rerun. Also it can
> > trivially be removed once we've converted everyone to the new functions
> > and removed the old ones.
> >
> Hidden bonus:
> Some of the people who run those on semi-regular basis can update
> some/all the drivers ;-)

I agree that this is a nice bonus, but thus far we just mass-converted
everyone. That seems to be the plan here too, which is why I don't see
much value in recording the cocci patch (outside of the commit message).

But drm is growing, and in the future I guess we'll get more refactorings
that can't be done in one go, and then adding the spatch makes perfect
sense.

Anyway, no strong opinions from my side at all, whatever makes sense, just
wanted to explain my reasoning quickly.
-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