[Intel-gfx] [PATCH] gpu/drm/i915: globally replace dev_priv with i915

[Lucas De Marchi]

On Fri, Jun 14, 2019 at 03:47:49PM +0300, Jani Nikula wrote:
>On Thu, 13 Jun 2019, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
>> On Thu, Jun 13, 2019 at 09:48:16AM -0700, Lucas De Marchi wrote:
>>> On Thu, Jun 13, 2019 at 09:29:48AM -0700, Lucas De Marchi wrote:
>>> > On Thu, Jun 13, 2019 at 04:12:37PM +0300, Jani Nikula wrote:
>>> > > On Wed, 12 Jun 2019, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
>>> > > > We are slowly converting dev_priv to i915 everywhere, spread into
>>> > > > smaller series. While this is good to avoid unrelated breakages to other
>>> > > > inflight patches, it's bad because inflight patches on nearby paths keep
>>> > > > breaking. Paired with other code moves and refactores this is becoming a
>>> > > > nightmare.
>>> > > >
>>> > > > Now that I915_{READ,WRITE} are gone and implicit access to dev_priv no
>>> > > > longer exists we can simply sed all files and be done with the
>>> > > > conversion. This was generated with the following commands with no
>>> > > > additional fixups:
>>> > > >
>>> > > > 	git ls-files --full-name -z -- drivers/gpu/drm/i915/ | \
>>> > > > 		xargs -0 sed -i 's/\bdev_priv\b/i915/g'
>>> > > >
>>> > > > Any pending series can apply the same search and replace when rebasing.
>>> > >
>>> > > I'm pretty strongly against renaming the implicit dev_priv local
>>> > > variable before we've gotten rid of it. Renaming s/dev_priv/i915/ upon
>>> > > converting a function to not use the implicit dev_priv helps us by
>>> > > catching any leftover references.
>>> >
>>> > I don't think this is strong enough reason to block the conversion. The
>>> > conversion is taking forever and is gonna take year+ if it continues at
>>> > this pace. It affects multiple developers rebasing their work and
>>> > introduce bugs on pending series.
>>> >
>>> > Removing any file-scope reference (I didn't review yet if we still have
>>> > any) and reviewing the macros is sufficient. And if we later we find out
>>> > we missed one, we just go and fix it. I see zero advantage on slow and
>>> > forever. You have a mixed code base and new code following what's around
>>> > will just propagate more the mixed code base :(
>>>
>>> btw, let me be clear the proposal is not "this should be done now". I do
>>> think we should be smart and think on the best opportunity to do it.
>>> Probably like after the big code moves gt/, gem/, display/, etc end.
>>
>> yeap, doing per folder is probably a good thing, specially because gt and gem
>> are far ahead this conversion.
>
>In general I think getting rid of the implicit dev_priv local is much
>more important than a global s/dev_priv/i915/ rename. In the end, it's
>just a local variable; the problematic part is the dependency on the
>implicit name.
>
>IMO you don't even have to do the rename at the time of getting rid of
>the local. It's just a nice bonus to be able to freely choose the
>variable name.

I agree, the renames are causing major headaches for rebasing work
because it changes in some places, not all and give conflicts mixed with
other "real conflicts". End result is that the conflict is much more
difficult to solve.

>For the renames, sure, please do them in gt and gem subdirs first,
>because that's actually feasible:
>
>$ git grep -w dev_priv -- gt | wc -l
>94
>
>$ git grep -w dev_priv -- gem | wc -l
>168
>
>$ git grep -w dev_priv -- display | wc -l
>6241

thanks, will do.

Lucas De Marchi

>
>
>BR,
>Jani.
>
>
>>
>>>
>>> Lucas De Marchi
>>>
>>> >
>>> > Lucas De Marchi
>>> >
>>> > >
>>> > > BR,
>>> > > Jani.
>>> > >
>>> > >
>>> > > --
>>> > > Jani Nikula, Intel Open Source Graphics Center
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center