Re: [Intel-gfx] [PATCH v2 01/27] drm/i915/xelpdp: Add XE_LPDP_FEATURES

[Lucas De Marchi]

On Thu, Sep 07, 2023 at 09:04:03AM -0700, Matt Roper wrote:

On Thu, Sep 07, 2023 at 08:37:31AM -0700, Lucas De Marchi wrote:

Add a FEATURES macro for XE_LPD+ as this is expected to be the baseline
for Xe2_LPD and will allow to see the delta more easily.


Would it be simpler to do

   #define XE_LPDP_FEATURES \
   XE_LPD_FEATURES \
   /* additional deltas */

so that it's more obvious what the deltas between Xe_LPD -> Xe_LPD+ are
too?



remember the nightmare we had in i915 with multiple inheritance? In the
end we settled to have just one level - the struct has 1 _FEATURES and
overrides or set new fields. A _FEATURES is always the root, without
inheriting from others.

It may be more verbose, but it's easier to look for "is x enabled in
this platform?"

Lucas De Marchi

Re: [Intel-gfx] [PATCH v2 01/27] drm/i915/xelpdp: Add XE_LPDP_FEATURES

[Matt Roper]

On Thu, Sep 07, 2023 at 08:37:31AM -0700, Lucas De Marchi wrote:
> Add a FEATURES macro for XE_LPD+ as this is expected to be the baseline
> for Xe2_LPD and will allow to see the delta more easily.

Would it be simpler to do

#define XE_LPDP_FEATURES \
XE_LPD_FEATURES \
/* additional deltas */

so that it's more obvious what the deltas between Xe_LPD -> Xe_LPD+ are
too?

> 
> Signed-off-by: Lucas De Marchi 
> ---
>  .../drm/i915/display/intel_display_device.c   | 60 ---
>  1 file changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c 
> b/drivers/gpu/drm/i915/display/intel_display_device.c
> index c39f8a15d8aa..089674e2f1d2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -710,18 +710,62 @@ static const struct intel_display_device_info 
> xe_hpd_display = {
>   BIT(PORT_TC1),
>  };
>  
> +#define XE_LPDP_FEATURES 
> \
> + .abox_mask = GENMASK(1, 0), 
> \
> + .color = {  
> \
> + .degamma_lut_size = 129, .gamma_lut_size = 1024,
> \
> + .degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING | 
> \
> + DRM_COLOR_LUT_EQUAL_CHANNELS,   
> \
> + },  
> \
> + .dbuf.size = 4096,  
> \
> + .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | 
> \
> + BIT(DBUF_S4),   
> \
> + .has_cdclk_crawl = 1,   
> \
> + .has_cdclk_squash = 1,  
> \
> + .has_ddi = 1,   
> \
> + .has_dp_mst = 1,
> \
> + .has_dsb = 1,   
> \
> + .has_fpga_dbg = 1,  
> \
> + .has_hotplug = 1,   
> \
> + .has_ipc = 1,   
> \
> + .has_psr = 1,   
> \
> + .pipe_offsets = {   
> \
> + [TRANSCODER_A] = PIPE_A_OFFSET, 
> \
> + [TRANSCODER_B] = PIPE_B_OFFSET, 
> \
> + [TRANSCODER_C] = PIPE_C_OFFSET, 
> \
> + [TRANSCODER_D] = PIPE_D_OFFSET, 
> \
> + [TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET,  
> \
> + [TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET,  
> \

If we're expanding out the whole definition rather than defining this as
XE_LPD_FEATURES + deltas, then there's no need for the DSI here.

> + },  
> \
> + .trans_offsets = {  
> \
> + [TRANSCODER_A] = TRANSCODER_A_OFFSET,   
> \
> + [TRANSCODER_B] = TRANSCODER_B_OFFSET,   
> \
> + [TRANSCODER_C] = TRANSCODER_C_OFFSET,   
> \
> + [TRANSCODER_D] = TRANSCODER_D_OFFSET,   
> \
> + [TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET,
> \
> + [TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET,
> \

Or here.

> + },  
> \
> + TGL_CURSOR_OFFSETS, 
> \
> + 
> \
> + .__runtime_defaults.cpu_transcoder_mask =   
> \
> + BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | 
> \
> + BIT(TRANSCODER_C) | BIT(TRANSCODER_D),  
> \
> + .__runtime_defaults.ip.ver = 13,
> \

Xe_LPD+'s ip version is 14 rather than 13.

> + .__runtime_defaults.has_dmc = 1,
> \
> + .__runtime_defaults.has_dsc = 1,
> \
> + .__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A),
> \

And the FBC is on A + B.


Matt

> + .__runtime_defau