Re: [PATCH 05/10] drm/i915/display: add platform descriptors

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 23 May 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Wed, May 22, 2024 at 08:33:42PM +0300, Jani Nikula wrote:
>> We'll need to start identifying the platforms independently in display
>> code in order to break free from the i915 and xe IS_<PLATFORM>()
>> macros. This is fairly straightforward, as we already identify most
>> platforms by PCI ID in display probe anyway.
>> 
>> As the first step, add platform descriptors with pointers to display
>> info. We'll have more platforms than display info, so minimize
>> duplication:
>> 
>> - Add separate skl/kbl/cfl/cml descriptors while they share the display
>>   info.
>> 
>> - Add separate jsl/ehl descriptors while they share the display info.
>> 
>> Identify ADL-P (and derivatives) and DG2 descriptors by their names even
>> though their display info is Xe LPD or HPD.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  .../drm/i915/display/intel_display_device.c   | 558 ++++++++++--------
>>  1 file changed, 326 insertions(+), 232 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
>> index 56b27546d1b3..d1e03437abb3 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
>> @@ -20,6 +20,10 @@
>>  __diag_push();
>>  __diag_ignore_all("-Woverride-init", "Allow field initialization overrides for display info");
>>  
>> +struct platform_desc {
>> +	const struct intel_display_device_info *info;
>> +};
>
> I had to jump to the latest patch to understand why this single item
> in a new struct... later it makes sense...

Yeah...

>> -#define GEN3_DISPLAY \
>> +#define GEN3_DISPLAY   \
>
> I had noticed a trend in all of your recent series, to replace the long tab
> or space before '\' with a single space. But then here you change the single
> space to multiple spaces. Intentional?

Accidental.

Emacs wants to indent and align \'s in a specific way, in a nice column
towards the right. Usually I follow that when adding new stuff manually.

Here, that happened on a line I didn't mean to change.

In the PCI ID patches I intentionally used a single space because I
scripted the whole thing, and I couldn't be bothered to figure out how
to align the \'s any other way! :)

>>  static const struct {
>>  	u32 devid;
>> -	const struct intel_display_device_info *info;
>> +	const struct platform_desc *desc;
>>  } intel_display_ids[] = {
>> -	INTEL_I830_IDS(INTEL_DISPLAY_DEVICE, &i830_display),
>> -	INTEL_I845G_IDS(INTEL_DISPLAY_DEVICE, &i845_display),
>> -	INTEL_I85X_IDS(INTEL_DISPLAY_DEVICE, &i85x_display),
>> -	INTEL_I865G_IDS(INTEL_DISPLAY_DEVICE, &i865g_display),
>> -	INTEL_I915G_IDS(INTEL_DISPLAY_DEVICE, &i915g_display),
>> -	INTEL_I915GM_IDS(INTEL_DISPLAY_DEVICE, &i915gm_display),
>> -	INTEL_I945G_IDS(INTEL_DISPLAY_DEVICE, &i945g_display),
>> -	INTEL_I945GM_IDS(INTEL_DISPLAY_DEVICE, &i945gm_display),
>> -	INTEL_I965G_IDS(INTEL_DISPLAY_DEVICE, &i965g_display),
>> -	INTEL_G33_IDS(INTEL_DISPLAY_DEVICE, &g33_display),
>> -	INTEL_I965GM_IDS(INTEL_DISPLAY_DEVICE, &i965gm_display),
>> -	INTEL_GM45_IDS(INTEL_DISPLAY_DEVICE, &gm45_display),
>> -	INTEL_G45_IDS(INTEL_DISPLAY_DEVICE, &g45_display),
>> -	INTEL_PNV_IDS(INTEL_DISPLAY_DEVICE, &pnv_display),
>> -	INTEL_ILK_D_IDS(INTEL_DISPLAY_DEVICE, &ilk_d_display),
>> -	INTEL_ILK_M_IDS(INTEL_DISPLAY_DEVICE, &ilk_m_display),
>> -	INTEL_SNB_IDS(INTEL_DISPLAY_DEVICE, &snb_display),
>> -	INTEL_IVB_IDS(INTEL_DISPLAY_DEVICE, &ivb_display),
>> -	INTEL_HSW_IDS(INTEL_DISPLAY_DEVICE, &hsw_display),
>> -	INTEL_VLV_IDS(INTEL_DISPLAY_DEVICE, &vlv_display),
>> -	INTEL_BDW_IDS(INTEL_DISPLAY_DEVICE, &bdw_display),
>> -	INTEL_CHV_IDS(INTEL_DISPLAY_DEVICE, &chv_display),
>> -	INTEL_SKL_IDS(INTEL_DISPLAY_DEVICE, &skl_display),
>> -	INTEL_BXT_IDS(INTEL_DISPLAY_DEVICE, &bxt_display),
>> -	INTEL_GLK_IDS(INTEL_DISPLAY_DEVICE, &glk_display),
>> -	INTEL_KBL_IDS(INTEL_DISPLAY_DEVICE, &skl_display),
>> -	INTEL_CFL_IDS(INTEL_DISPLAY_DEVICE, &skl_display),
>> -	INTEL_WHL_IDS(INTEL_DISPLAY_DEVICE, &skl_display),
>> -	INTEL_CML_IDS(INTEL_DISPLAY_DEVICE, &skl_display),
>> -	INTEL_ICL_IDS(INTEL_DISPLAY_DEVICE, &icl_display),
>> -	INTEL_EHL_IDS(INTEL_DISPLAY_DEVICE, &jsl_ehl_display),
>> -	INTEL_JSL_IDS(INTEL_DISPLAY_DEVICE, &jsl_ehl_display),
>> -	INTEL_TGL_IDS(INTEL_DISPLAY_DEVICE, &tgl_display),
>> -	INTEL_DG1_IDS(INTEL_DISPLAY_DEVICE, &dg1_display),
>> -	INTEL_RKL_IDS(INTEL_DISPLAY_DEVICE, &rkl_display),
>> -	INTEL_ADLS_IDS(INTEL_DISPLAY_DEVICE, &adl_s_display),
>> -	INTEL_RPLS_IDS(INTEL_DISPLAY_DEVICE, &adl_s_display),
>> -	INTEL_ADLP_IDS(INTEL_DISPLAY_DEVICE, &xe_lpd_display),
>> -	INTEL_ADLN_IDS(INTEL_DISPLAY_DEVICE, &xe_lpd_display),
>> -	INTEL_RPLU_IDS(INTEL_DISPLAY_DEVICE, &xe_lpd_display),
>> -	INTEL_RPLP_IDS(INTEL_DISPLAY_DEVICE, &xe_lpd_display),
>> -	INTEL_DG2_IDS(INTEL_DISPLAY_DEVICE, &xe_hpd_display),
>> +	INTEL_I830_IDS(INTEL_DISPLAY_DEVICE, &i830_desc),
>
> But here is what I'm not sure if I completely understand/agree...
> before this patch is a display device with a display struct
> but then it becomes a display device with a platform description
> but a platform that is not used by the driver...
>
> I'm probably missing some later jump there.

Yeah, I did not want to put too much stuff in the same patch. I think
easier to review this way, though I guess I should've made my intentions
more clear in the commit message! Also, easy to squash if so desired.

So this one adds the platform descs with just the display struct, and
later patches add more content in the descs.


BR,
Jani.


-- 
Jani Nikula, Intel