On Fri, May 24, 2024 at 11:17:32AM +0300, Jani Nikula wrote:
> On Thu, 23 May 2024, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> .../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! :)
As an emacs user I also tend to do that right alignment hitting the 'tab' key.
But I really never care if it is all to the right or all the 1 space.
>
> >> 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.
yeap, indeed... let's go with this then.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel
