[PATCH] drm/i915/xelpg: Add fake PCH for xelpg

Matt Roper matthew.d.roper at intel.com
Mon Dec 18 15:54:37 UTC 2023


Oh, and one more thing I forgot to mention before hitting send...the
title for this patch doesn't make sense.  Xe_LPG is the graphics IP used
by MTL; that's completely unrelated to the display IP (which is
Xe_LPD+).

Since we're assigning the fake PCH value based off the platform
(IS_METEORLAKE) rather than the display version, this should just be
"drm/i915/mtl: Add fake PCH for Meteor Lake"


Matt

On Mon, Dec 18, 2023 at 07:52:12AM -0800, Matt Roper wrote:
> On Mon, Dec 18, 2023 at 04:33:13PM +0530, Haridhar Kalvala wrote:
> > Correct the implementation trying to detect MTL PCH with
> > the MTL fake PCH id.
> > 
> > On MTL, both the North Display (NDE) and South Display (SDE) functionality
> > reside on the same die (the SoC die in this case), unlike many past
> > platforms where the SDE was on a separate PCH die. The code is (badly)
> > structured today in a way that assumes the SDE is always on the PCH for
> > modern platforms, so on platforms where we don't actually need to identify
> > the PCH to figure out how the SDE behaves (i.e., all DG1/2 GPUs as well as
> > MTL and LNL),we've been assigning a "fake PCH" as a quickhack that allows
> > us to avoid restructuring a bunch of the code.we've been assigning a
> > "fake PCH" as a quick hack that allows us to avoid restructuring a bunch
> > of the code.
> > 
> > Signed-off-by: Haridhar Kalvala <haridhar.kalvala at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_backlight.c   |  2 +-
> >  drivers/gpu/drm/i915/display/intel_cdclk.c       |  2 +-
> >  drivers/gpu/drm/i915/display/intel_display_irq.c |  2 +-
> >  drivers/gpu/drm/i915/display/intel_gmbus.c       |  6 ++----
> >  drivers/gpu/drm/i915/display/intel_hotplug_irq.c |  7 +++----
> >  drivers/gpu/drm/i915/display/intel_pps.c         |  2 +-
> >  drivers/gpu/drm/i915/soc/intel_pch.c             | 12 +++++++-----
> >  drivers/gpu/drm/i915/soc/intel_pch.h             |  4 ++--
> >  8 files changed, 18 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> > index 612d4cd9dacb..696ae59874a9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> > +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> > @@ -1465,7 +1465,7 @@ static bool cnp_backlight_controller_is_valid(struct drm_i915_private *i915, int
> >  
> >  	if (controller == 1 &&
> >  	    INTEL_PCH_TYPE(i915) >= PCH_ICP &&
> > -	    INTEL_PCH_TYPE(i915) < PCH_MTP)
> > +	    INTEL_PCH_TYPE(i915) <= PCH_ADP)
> >  		return intel_de_read(i915, SOUTH_CHICKEN1) & ICP_SECOND_PPS_IO_SELECT;
> >  
> >  	return true;
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index c985ebb6831a..2e6e55d3e885 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -3469,7 +3469,7 @@ u32 intel_read_rawclk(struct drm_i915_private *dev_priv)
> >  
> >  	if (INTEL_PCH_TYPE(dev_priv) >= PCH_DG1)
> >  		freq = dg1_rawclk(dev_priv);
> > -	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_MTP)
> > +	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_MTL)
> >  		/*
> >  		 * MTL always uses a 38.4 MHz rawclk.  The bspec tells us
> >  		 * "RAWCLK_FREQ defaults to the values for 38.4 and does
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > index a7d8f3fc98de..e318e24d1efd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > @@ -986,7 +986,7 @@ static void gen8_read_and_ack_pch_irqs(struct drm_i915_private *i915, u32 *pch_i
> >  	 * their flags both in the PICA and SDE IIR.
> >  	 */
> >  	if (*pch_iir & SDE_PICAINTERRUPT) {
> > -		drm_WARN_ON(&i915->drm, INTEL_PCH_TYPE(i915) < PCH_MTP);
> > +		drm_WARN_ON(&i915->drm, INTEL_PCH_TYPE(i915) <= PCH_ADP);
> >  
> >  		pica_ier = intel_de_rmw(i915, PICAINTERRUPT_IER, ~0, 0);
> >  		*pica_iir = intel_de_read(i915, PICAINTERRUPT_IIR);
> > diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
> > index 40d7b6f3f489..2d9c740ba17e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
> > +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
> > @@ -155,7 +155,8 @@ static const struct gmbus_pin *get_gmbus_pin(struct drm_i915_private *i915,
> >  	const struct gmbus_pin *pins;
> >  	size_t size;
> >  
> > -	if (INTEL_PCH_TYPE(i915) >= PCH_LNL) {
> > +	if ((INTEL_PCH_TYPE(i915) >= PCH_LNL) ||
> > +	    (INTEL_PCH_TYPE(i915) >= PCH_MTL)) {
> 
> You only need the MTL condition here.  The LNL check becomes redundant.
> 
> >  		pins = gmbus_pins_mtp;
> >  		size = ARRAY_SIZE(gmbus_pins_mtp);
> >  	} else if (INTEL_PCH_TYPE(i915) >= PCH_DG2) {
> > @@ -164,9 +165,6 @@ static const struct gmbus_pin *get_gmbus_pin(struct drm_i915_private *i915,
> >  	} else if (INTEL_PCH_TYPE(i915) >= PCH_DG1) {
> >  		pins = gmbus_pins_dg1;
> >  		size = ARRAY_SIZE(gmbus_pins_dg1);
> > -	} else if (INTEL_PCH_TYPE(i915) >= PCH_MTP) {
> > -		pins = gmbus_pins_mtp;
> > -		size = ARRAY_SIZE(gmbus_pins_mtp);
> >  	} else if (INTEL_PCH_TYPE(i915) >= PCH_ICP) {
> >  		pins = gmbus_pins_icp;
> >  		size = ARRAY_SIZE(gmbus_pins_icp);
> > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c
> > index 04f62f27ad74..63f697383bf3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c
> > @@ -163,12 +163,11 @@ static void intel_hpd_init_pins(struct drm_i915_private *dev_priv)
> >  	    (!HAS_PCH_SPLIT(dev_priv) || HAS_PCH_NOP(dev_priv)))
> >  		return;
> >  
> > -	if (INTEL_PCH_TYPE(dev_priv) >= PCH_LNL)
> > +	if ((INTEL_PCH_TYPE(dev_priv) >= PCH_LNL) ||
> > +	    (INTEL_PCH_TYPE(dev_priv) >= PCH_MTL))
> 
> Ditto
> 
> >  		hpd->pch_hpd = hpd_mtp;
> >  	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_DG1)
> >  		hpd->pch_hpd = hpd_sde_dg1;
> > -	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_MTP)
> > -		hpd->pch_hpd = hpd_mtp;
> >  	else if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> >  		hpd->pch_hpd = hpd_icp;
> >  	else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_SPT(dev_priv))
> > @@ -1139,7 +1138,7 @@ static void xelpdp_hpd_irq_setup(struct drm_i915_private *i915)
> >  
> >  	if (INTEL_PCH_TYPE(i915) >= PCH_LNL)
> >  		xe2lpd_sde_hpd_irq_setup(i915);
> > -	else if (INTEL_PCH_TYPE(i915) >= PCH_MTP)
> > +	else if (INTEL_PCH_TYPE(i915) >= PCH_MTL)
> >  		mtp_hpd_irq_setup(i915);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> > index a8fa3a20990e..2d65a538f83e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > @@ -366,7 +366,7 @@ static bool intel_pps_is_valid(struct intel_dp *intel_dp)
> >  
> >  	if (intel_dp->pps.pps_idx == 1 &&
> >  	    INTEL_PCH_TYPE(i915) >= PCH_ICP &&
> > -	    INTEL_PCH_TYPE(i915) < PCH_MTP)
> > +	    INTEL_PCH_TYPE(i915) <= PCH_ADP)
> >  		return intel_de_read(i915, SOUTH_CHICKEN1) & ICP_SECOND_PPS_IO_SELECT;
> >  
> >  	return true;
> > diff --git a/drivers/gpu/drm/i915/soc/intel_pch.c b/drivers/gpu/drm/i915/soc/intel_pch.c
> > index 240beafb38ed..f693c1ffbeee 100644
> > --- a/drivers/gpu/drm/i915/soc/intel_pch.c
> > +++ b/drivers/gpu/drm/i915/soc/intel_pch.c
> > @@ -140,11 +140,6 @@ intel_pch_type(const struct drm_i915_private *dev_priv, unsigned short id)
> >  		drm_WARN_ON(&dev_priv->drm, !IS_ALDERLAKE_S(dev_priv) &&
> >  			    !IS_ALDERLAKE_P(dev_priv));
> >  		return PCH_ADP;
> > -	case INTEL_PCH_MTP_DEVICE_ID_TYPE:
> > -	case INTEL_PCH_MTP2_DEVICE_ID_TYPE:
> 
> The #define's for these should also be removed from intel_pch.h
> 
> > -		drm_dbg_kms(&dev_priv->drm, "Found Meteor Lake PCH\n");
> > -		drm_WARN_ON(&dev_priv->drm, !IS_METEORLAKE(dev_priv));
> > -		return PCH_MTP;
> >  	default:
> >  		return PCH_NONE;
> >  	}
> > @@ -225,6 +220,13 @@ void intel_detect_pch(struct drm_i915_private *dev_priv)
> >  	if (DISPLAY_VER(dev_priv) >= 20) {
> >  		dev_priv->pch_type = PCH_LNL;
> >  		return;
> > +	} else if (IS_METEORLAKE(dev_priv)) {
> > +		/*
> > +		 * Both north display and south display are on the SoC die.
> > +		 * The real PCH is uninvolved in display.
> > +		 */
> > +		dev_priv->pch_type = PCH_MTL;
> > +		return;
> >  	} else if (IS_DG2(dev_priv)) {
> >  		dev_priv->pch_type = PCH_DG2;
> >  		return;
> > diff --git a/drivers/gpu/drm/i915/soc/intel_pch.h b/drivers/gpu/drm/i915/soc/intel_pch.h
> > index 1b03ea60a7a8..b044bb0a77ae 100644
> > --- a/drivers/gpu/drm/i915/soc/intel_pch.h
> > +++ b/drivers/gpu/drm/i915/soc/intel_pch.h
> > @@ -25,11 +25,11 @@ enum intel_pch {
> >  	PCH_ICP,	/* Ice Lake/Jasper Lake PCH */
> >  	PCH_TGP,	/* Tiger Lake/Mule Creek Canyon PCH */
> >  	PCH_ADP,	/* Alder Lake PCH */
> > -	PCH_MTP,	/* Meteor Lake PCH */
> >  
> >  	/* Fake PCHs, functionality handled on the same PCI dev */
> >  	PCH_DG1 = 1024,
> >  	PCH_DG2,
> > +	PCH_MTL,
> >  	PCH_LNL,
> >  };
> >  
> > @@ -68,7 +68,7 @@ enum intel_pch {
> >  #define INTEL_PCH_TYPE(dev_priv)		((dev_priv)->pch_type)
> >  #define INTEL_PCH_ID(dev_priv)			((dev_priv)->pch_id)
> >  #define HAS_PCH_LNL(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_LNL)
> > -#define HAS_PCH_MTP(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_MTP)
> > +#define HAS_PCH_MTP(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_MTL)
> 
> Since this is a fake PCH, this macro should be HAS_PCH_MTL() to match
> the naming of the LNL fake PCH.
> 
> It looks like this macro only gets used in a single place in
> map_ddc_pin(), and that's another case where the MTL + LNL conditions
> can be consolidated now that the PCH enum has been reordered.  So maybe
> you can just remove this macro entirely once you make that change.  In
> fact, it doesn't look like HAS_PCH_LNL is used anywhere today either, so
> a follow-up patch to remove that unused definition might be a good idea
> as well.
> 
> 
> Matt
> 
> >  #define HAS_PCH_DG2(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_DG2)
> >  #define HAS_PCH_ADP(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_ADP)
> >  #define HAS_PCH_DG1(dev_priv)			(INTEL_PCH_TYPE(dev_priv) == PCH_DG1)
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation


More information about the Intel-gfx mailing list