Re: [PATCH 11/11] drm/i915: split out display regs from i915_reg.h

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 10, 2024 at 12:18:44AM +0300, Jani Nikula wrote:
> On Mon, 09 Sep 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Sep 09, 2024 at 09:59:02PM +0300, Jani Nikula wrote:
> >> Split out display/intel_display_regs.h from i915_reg.h. This is done
> >> programmatically.
> >> 
> >> Register macros in i915_reg.h are considered in chunks separated by
> >> blank lines. If all macros in the chunk are only referenced by display
> >> or gvt, and there's at least one reference in display, move the chunk to
> >> intel_display_regs.h. Otherwise, keep it in i915_reg.h.
> >> 
> >> This is a fairly good approximation, but the are some small hiccups here
> >> and there that need to be fixed manually.
> >> 
> >> Add the includes where needed, and sort includes where modified, also
> >> programmatically. Unnecessary i915_reg.h are not cleaned up at this
> >> point.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> > <snip>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h b/drivers/gpu/drm/i915/display/intel_display_regs.h
> >> new file mode 100644
> >> index 000000000000..97346a0e3373
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
> >> @@ -0,0 +1,2986 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/* Copyright © 2024 Intel Corporation */
> >> +
> >> +#ifndef __INTEL_DISPLAY_REGS_H__
> >> +#define __INTEL_DISPLAY_REGS_H__
> >> +
> >> +#include "intel_display_reg_defs.h"
> >> +
> >> +#define GU_CNTL_PROTECTED		_MMIO(0x10100C)
> >> +#define   DEPRESENT			REG_BIT(9)
> >
> > That doesn't really look like a display register to me.
> 
> Aye. It's just that doing the split manually is a daunting task. I've
> started and backed away a number of times. This is purely based on where
> the stuff is referenced.
> 
> If it's only referenced in display or gvt -> it's display.
> 
> If there's even one reference outside of display -> it's not display.
> 
> And this is on a newline separate chunk basis, not macros individually,
> because there are so many things that are not used and would be left
> behind.
> 
> So it's not perfect. But you need to strike a balance between polishing
> the scripts for the one time you run it vs. cleaning up the results
> manually.
> 
> > <snip>
> >> +#define _PS_COEF_SET0_DATA_1A	   0x6819C
> >> +#define _PS_COEF_SET0_DATA_2A	   0x6829C
> >> +#define _PS_COEF_SET0_DATA_1B	   0x6899C
> >> +#define _PS_COEF_SET0_DATA_2B	   0x68A9C
> >> +#define GLK_PS_COEF_DATA_SET(pipe, id, set)  _MMIO_PIPE(pipe,     \
> >> +			_ID(id, _PS_COEF_SET0_DATA_1A, _PS_COEF_SET0_DATA_2A) + (set) * 8, \
> >> +			_ID(id, _PS_COEF_SET0_DATA_1B, _PS_COEF_SET0_DATA_2B) + (set) * 8)
> >> +
> >> +/* More Ivybridge lolz */
> >
> > Where did the pre-ivb interrupt bits go? The relevant
> > register offset definitions also seem to have been left
> > behind.
> 
> I presume there's a reference in i915_irq.c to one of the macros, which
> breaks down the approximation. :(
> 
> I err on the side of leaving stuff behind, so I don't have to include
> intel_display_regs.h from i915 core code.
> 
> The best thing to do would be to move the stuff that needs them to
> intel_display_irq.c.

Right. I guess for now it'd be easier to just back out the 
movement of the ivb bits below so that it all stays in one 
place.

> 
> >> +#define DE_ERR_INT_IVB			(1 << 30)
> >> +#define DE_GSE_IVB			(1 << 29)
> >> +#define DE_PCH_EVENT_IVB		(1 << 28)
> >> +#define DE_DP_A_HOTPLUG_IVB		(1 << 27)
> >> +#define DE_AUX_CHANNEL_A_IVB		(1 << 26)
> >> +#define DE_EDP_PSR_INT_HSW		(1 << 19)
> >> +#define DE_SPRITEC_FLIP_DONE_IVB	(1 << 14)
> >> +#define DE_PLANEC_FLIP_DONE_IVB		(1 << 13)
> >> +#define DE_PIPEC_VBLANK_IVB		(1 << 10)
> >> +#define DE_SPRITEB_FLIP_DONE_IVB	(1 << 9)
> >> +#define DE_PLANEB_FLIP_DONE_IVB		(1 << 8)
> >> +#define DE_PIPEB_VBLANK_IVB		(1 << 5)
> >> +#define DE_SPRITEA_FLIP_DONE_IVB	(1 << 4)
> >> +#define DE_PLANEA_FLIP_DONE_IVB		(1 << 3)
> >> +#define DE_PLANE_FLIP_DONE_IVB(plane)	(1 << (3 + 5 * (plane)))
> >> +#define DE_PIPEA_VBLANK_IVB		(1 << 0)
> >> +#define DE_PIPE_VBLANK_IVB(pipe)	(1 << ((pipe) * 5))
> >> +
> > <snip>
> >> +#define PRIMARY_SPI_TRIGGER			_MMIO(0x102040)
> >> +#define PRIMARY_SPI_ADDRESS			_MMIO(0x102080)
> >> +#define PRIMARY_SPI_REGIONID			_MMIO(0x102084)
> >> +#define SPI_STATIC_REGIONS			_MMIO(0x102090)
> >> +#define   OPTIONROM_SPI_REGIONID_MASK		REG_GENMASK(7, 0)
> >> +#define OROM_OFFSET				_MMIO(0x1020c0)
> >> +#define   OROM_OFFSET_MASK			REG_GENMASK(20, 16)
> >
> > The SPI stuff doesn't really feel like it belongs here either.
> > I suppose we should just extract it into its own thing. But
> > that could be done later too. 
> >
> > And now that I looked at the SPI vs. PCI ROM stuff I'm
> > a bit annoyed at the duplicated code. I guess I'll take a quick
> > stab at abstracting away the differences... Ideally I'd like to
> > move all that stuff to soc/ but then I'd need to figure out what
> > to do about xe, so I guess I'll leave it under display/ for now.
> 
> soc/ is fine for xe. There's a makefile rule to build stuff from there,
> e.g. intel_dram.c and intel_pch.c are covered.

Right. Seems to work fine (after adding a new wrapper header into xe).
I'll need to run a few smoke tests to make sure I didn't break anything
though. I'll leave the SPI registers alone for now so it won't conflict
with this stuff. We can follow up with that afterwards.

-- 
Ville Syrjälä
Intel