On Wed, 2023-02-01 at 10:07 -0800, Lucas De Marchi wrote: > 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@xxxxxxxxx> 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. Yes, you're totally right. Sorry if my comment came out negative, that was not my intent. We're obviously all working on the same code base so we work in parallel and conflicts are a part of nature. :) > Anyway since you are changing this, I'll hold off on more patches > related to it. It's maybe a bit easier like that, but in any case the beauty of semantic patches is that they should "understand" other changes and act accordingly. With your changes, my rules wouldn't matchthose cases anymore, but would still match other ones. Regarding adding arguments just to remove them later, I think it's okay if we don't want to do everything in a gigantic patch. One patch is lying the foundation for the next that will actually make things cleaner. It's a bit easier to review, IMHO. We probably can, in the end, before actually merging the changes, squash everything together to avoid adding-just-to-remove. Let's see how it goes. Thanks for the comments! -- Cheers, Luca.
