On Tue, Feb 18, 2025 at 02:19:38PM +0200, Jani Nikula wrote:
> On Mon, 17 Feb 2025, Rodrigo Vivi <rodrigo.v...@intel.com> wrote:
> > The only usage of the "PCH" infra is to detect which South Display
> > Engine we should be using. Move it under display so we can convert
> > all its callers towards intel_display struct later.
>
> Yeah, PCH is becoming a blocker to finishing the conversions of many
> files from drm_i915_private to intel_display. We'll need to do something
> like this.
First of all, I'm sorry for not sending a cover letter explaining more about
my thoughts here and also for not tagging this as an RFC. And thank you very
much for jumping in the discussion.
I started this, exactly because my consolidation of display pm now is
blocked in some HPD calls. Then I checked the IRQ code and I have some
ideas do organize that, but that got blocked on the PCH, then I moved
towards seeing what could and should be done to PCH and these 2 patches
is the initial of my conclusion.
>
> I was eyeing the PCH checks outside of display, and frankly many of them
> are plain wrong (done to check stuff that's unrelated to PCH, but
> happens to match desired platforms), or should be in display
> (e.g. gt_record_display_regs()). But there are also legitimate checks, I
> think in clock gating.
Yes, this one in GPU hang was one of the most strange ones, but then
I noticed it is inside this function that should be moved to the display
side anyway.
Other cases are:
drivers/gpu/drm/i915/intel_clock_gating.c:
This entire file should be in the display side. But I got super
scared now because it looks like it is not getting called from nowhere.
So we might be missing many display workarounds. I will investigate
this more later.
drivers/gpu/drm/i915/i915_irq.c:
This is exactly where I got blocked. All the PCH calls inside it
are display related that I need to move to the display side in
order to have a proper split and make the display to stop using
the irq spin locks directly.
drivers/gpu/drm/i915/i915_gpu_error.c:
good idea on moving that entire function to display anyway.
>
> The thing that gives me an uneasy feeling about this patch series at
> this point in time is that i915 core still depends on the PCH
> detection. This won't work for a future independent display module, so
> what's the story for that? How will that pan out?
No, it doesn't. What depends on the PCH check to be done earlier
are the IRQs setup. My thoughts now is to move that to an early display
probe function. Unless I find something else better to do with the
IRQ that we could delay it.
>
> It would be logical and great to do PCH detection near the end of
> intel_display_device_probe().
That would be the ideal indeed. I will try to check it out.
>
> Cc: Ville
>
> BR,
> Jani.
Oh, one last thing that I'm not proud of this series.
We have the existing intel_display_pch.c, and with this patch
series we get this duplicated.
I had tried to merge all of these here inside the intel_display_pch.c
but that broke xe compilation and I preffered to start with the easy
path and to consolidate them as a next step.
Thank you so much,
Rodrigo.
>
>
> >
> > No functional or code change.
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> > ---
> > drivers/gpu/drm/i915/Makefile | 2 +-
> > drivers/gpu/drm/i915/{soc => display}/intel_pch.c | 2 +-
> > drivers/gpu/drm/i915/{soc => display}/intel_pch.h | 2 +-
> > drivers/gpu/drm/i915/i915_drv.h | 3 +--
> > drivers/gpu/drm/xe/Makefile | 2 +-
> > drivers/gpu/drm/xe/compat-i915-headers/soc/intel_pch.h | 6 ------
> > drivers/gpu/drm/xe/xe_device_types.h | 2 +-
> > 7 files changed, 6 insertions(+), 13 deletions(-)
> > rename drivers/gpu/drm/i915/{soc => display}/intel_pch.c (99%)
> > rename drivers/gpu/drm/i915/{soc => display}/intel_pch.h (98%)
> > delete mode 100644 drivers/gpu/drm/xe/compat-i915-headers/soc/intel_pch.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index ed05b131ed3a..3bac76059ba9 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -52,7 +52,6 @@ i915-y += \
> > i915-y += \
> > soc/intel_dram.o \
> > soc/intel_gmch.o \
> > - soc/intel_pch.o \
> > soc/intel_rom.o
> >
> > # core library code
> > @@ -281,6 +280,7 @@ i915-y += \
> > display/intel_modeset_setup.o \
> > display/intel_modeset_verify.o \
> > display/intel_overlay.o \
> > + display/intel_pch.o \
> > display/intel_pch_display.o \
> > display/intel_pch_refclk.o \
> > display/intel_plane_initial.o \
> > diff --git a/drivers/gpu/drm/i915/soc/intel_pch.c
> > b/drivers/gpu/drm/i915/display/intel_pch.c
> > similarity index 99%
> > rename from drivers/gpu/drm/i915/soc/intel_pch.c
> > rename to drivers/gpu/drm/i915/display/intel_pch.c
> > index 82dc7fbd1a3e..37766e40fd1c 100644
> > --- a/drivers/gpu/drm/i915/soc/intel_pch.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pch.c
> > @@ -1,6 +1,6 @@
> > // SPDX-License-Identifier: MIT
> > /*
> > - * Copyright 2019 Intel Corporation.
> > + * Copyright 2025 Intel Corporation.
> > */
> >
> > #include "i915_drv.h"
> > diff --git a/drivers/gpu/drm/i915/soc/intel_pch.h
> > b/drivers/gpu/drm/i915/display/intel_pch.h
> > similarity index 98%
> > rename from drivers/gpu/drm/i915/soc/intel_pch.h
> > rename to drivers/gpu/drm/i915/display/intel_pch.h
> > index 635aea7a5539..b9fa2b2f07bc 100644
> > --- a/drivers/gpu/drm/i915/soc/intel_pch.h
> > +++ b/drivers/gpu/drm/i915/display/intel_pch.h
> > @@ -1,6 +1,6 @@
> > /* SPDX-License-Identifier: MIT */
> > /*
> > - * Copyright 2019 Intel Corporation.
> > + * Copyright 2025 Intel Corporation.
> > */
> >
> > #ifndef __INTEL_PCH__
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index ffc346379cc2..2a2db0a6859e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -38,6 +38,7 @@
> >
> > #include "display/intel_display_limits.h"
> > #include "display/intel_display_core.h"
> > +#include "display/intel_pch.h"
> >
> > #include "gem/i915_gem_context_types.h"
> > #include "gem/i915_gem_shrinker.h"
> > @@ -49,8 +50,6 @@
> > #include "gt/intel_workarounds.h"
> > #include "gt/uc/intel_uc.h"
> >
> > -#include "soc/intel_pch.h"
> > -
> > #include "i915_drm_client.h"
> > #include "i915_gem.h"
> > #include "i915_gpu_error.h"
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index 5ce65ccb3c08..df30c4385403 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -192,7 +192,6 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> > # SOC code shared with i915
> > xe-$(CONFIG_DRM_XE_DISPLAY) += \
> > i915-soc/intel_dram.o \
> > - i915-soc/intel_pch.o \
> > i915-soc/intel_rom.o
> >
> > # Display code shared with i915
> > @@ -267,6 +266,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> > i915-display/intel_panel.o \
> > i915-display/intel_pfit.o \
> > i915-display/intel_pmdemand.o \
> > + i915-display/intel_pch.o \
> > i915-display/intel_pps.o \
> > i915-display/intel_psr.o \
> > i915-display/intel_qp_tables.o \
> > diff --git a/drivers/gpu/drm/xe/compat-i915-headers/soc/intel_pch.h
> > b/drivers/gpu/drm/xe/compat-i915-headers/soc/intel_pch.h
> > deleted file mode 100644
> > index 9c46556d33a4..000000000000
> > --- a/drivers/gpu/drm/xe/compat-i915-headers/soc/intel_pch.h
> > +++ /dev/null
> > @@ -1,6 +0,0 @@
> > -/* SPDX-License-Identifier: MIT */
> > -/*
> > - * Copyright © 2023 Intel Corporation
> > - */
> > -
> > -#include "../../../i915/soc/intel_pch.h"
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> > b/drivers/gpu/drm/xe/xe_device_types.h
> > index 4656305dd45a..2586ffc4909b 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -30,7 +30,7 @@
> > #endif
> >
> > #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> > -#include "soc/intel_pch.h"
> > +#include "intel_pch.h"
> > #include "intel_display_core.h"
> > #include "intel_display_device.h"
> > #endif
>
> --
> Jani Nikula, Intel
