[PATCH] drm/i915/xelpg: Add fake PCH for xelpg
Kalvala, Haridhar
haridhar.kalvala at intel.com
Tue Dec 19 09:28:12 UTC 2023
On 12/18/2023 9:24 PM, Matt Roper wrote:
> 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
Sure, I will update.
> 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.
yeah, removed.
>>> 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
yeah, removed.
>>
>>> 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
sure, removed.
>> .
>>> - 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
sure, modified and removed unused macros.
Thanks & regards,
Haridhar Kalvala
>>> #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
--
Regards,
Haridhar Kalvala
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20231219/85ffd1d2/attachment-0001.htm>
More information about the Intel-gfx
mailing list