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

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

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.

<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.

> +#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.

-- 
Ville Syrjälä
Intel