[Intel-gfx] [RFC] drm/i915: make dev_priv usage explitic in some macros

[Lucas De Marchi]

On Wed, Feb 01, 2023 at 09:20:42AM -0800, Coelho, Luciano wrote:
>On Wed, 2023-02-01 at 19:09 +0200, Jani Nikula wrote:
>> On Wed, 01 Feb 2023, Luca Coelho <luciano.coelho at intel.com> wrote:
>> > There are a few macros (e.g. DPLL()) that implicitly use dev_priv, by
>> > using other macros that implcitily use dev_priv.
>> >
>> > In an effort to align all definitions of struct drm_i915_private to be
>> > declared as i915 instead of arbitrarily using either i915 or dev_priv,
>> > we need to make these macros explicitly use dev_priv, so that we can
>> > change them later to be defined as i915.
>>
>> Lucas posted a slightly related patch [1], and I think based on the
>> discussion we should probably add AUX and DPLL registers that are
>> VLV/CHV specific, and include the MMIO offset directly without dev_priv,
>> and non-VLV/CHV macros that will have MMIO offset 0. This would reduce
>> the implicit dev_priv considerably, and avoid the need to pass i915
>> pointer to those register macros altogether.
>
>Yes, I saw that.
>
>
>> > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi_regs.h b/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
>> > index abbe427e462e..d00e9321064a 100644
>> > --- a/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
>> > +++ b/drivers/gpu/drm/i915/display/vlv_dsi_regs.h
>> > @@ -100,7 +100,7 @@
>> >
>> >  #define _MIPIA_DEVICE_READY		(_MIPI_MMIO_BASE(dev_priv) + 0xb000)
>> >  #define _MIPIC_DEVICE_READY		(_MIPI_MMIO_BASE(dev_priv) + 0xb800)
>> > -#define MIPI_DEVICE_READY(port)		_MMIO_MIPI(port, _MIPIA_DEVICE_READY, _MIPIC_DEVICE_READY)
>> > +#define MIPI_DEVICE_READY(dev_priv, port) _MMIO_MIPI(port, _MIPIA_DEVICE_READY, _MIPIC_DEVICE_READY)
>>
>> While this kind of passes dev_priv as parameter, the dev_priv in
>> _MIPIA_DEVICE_READY and _MIPIC_DEVICE_READY is still implicit.
>
>Yes, this was on purpose and my second change is to modify the the
>calls to the inner macros to pass the parameter as well.
>
>In any case, this already works as is, even if we pass i915 to
>MIPI_DEVICE_READY() here, because the dev_priv that MIPI*_DEVICE_READY
>use will be expanded  to whatever we passed as dev_priv to the outer
>macro.
>
>
>> I think these could use a similar treatment as in [1], moving the
>> _MIPI_MMIO_BASE() part one level up.
>
>Yes, and this can also be done with more rules after my other patches.
>The problem is if we all start making changes in this area at the same
>time, then there will be conflict after conflict.

As I shared previously I think these manual changes need to come
*before* not after - why would you change the callers to pass dev_priv
and then ultimately remove? Let the scripted changes only do the
addition of dev_priv to the callers, after you removed the ones that
shouldn't have it at all

This makes the script easier to write and run and can be postponed to
when the branches are in sync if we are going to cross the display/
boundary.

Anyway since you are changing this, I'll hold off on more patches
related to it.

Lucas De Marchi

>
>--
>Cheers,
>Luca.