| Message ID | 20250218010224.761209-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] drm/{i915,xe}: Move intel_pch under display | expand |
On Mon, 17 Feb 2025, Rodrigo Vivi <rodrigo.vivi@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. 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. 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? It would be logical and great to do PCH detection near the end of intel_display_device_probe(). Cc: Ville BR, Jani. > > No functional or code change. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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
On Tue, Feb 18, 2025 at 02:19:38PM +0200, Jani Nikula wrote: > On Mon, 17 Feb 2025, Rodrigo Vivi <rodrigo.vivi@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.vivi@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
On Tue, Feb 18, 2025 at 09:45:46AM -0500, Rodrigo Vivi wrote: > On Tue, Feb 18, 2025 at 02:19:38PM +0200, Jani Nikula wrote: > > On Mon, 17 Feb 2025, Rodrigo Vivi <rodrigo.vivi@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. Mostly, but it still has bunch of gt workarounds in it. Those all should be moved into the gt w/a framework. > 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. It's hidden behind some confusing macro stuff. > > 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. That seems like a random set of stuff no one actually cares about. Should just nuke the whole thing, apart from DERRMR because that one is actually relevant for GPU hangs. Though that one doesn't exist on VLV/CHV so currently some random garbage is being read into it on those platforms.
On Tue, 2025-02-18 at 17:27 +0200, Ville Syrjälä wrote: > On Tue, Feb 18, 2025 at 09:45:46AM -0500, Rodrigo Vivi wrote: > > On Tue, Feb 18, 2025 at 02:19:38PM +0200, Jani Nikula wrote: > > > On Mon, 17 Feb 2025, Rodrigo Vivi <rodrigo.vivi@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. > > Mostly, but it still has bunch of gt workarounds in it. > Those all should be moved into the gt w/a framework. hmm... indeed. But anyway, all that I saw using PCH is related to display. > > > 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. > > It's hidden behind some confusing macro stuff. oh no! someone is reading too much i915-guc related code :( > > > > > 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. > > That seems like a random set of stuff no one actually cares about. > Should just nuke the whole thing, apart from DERRMR because that > one is actually relevant for GPU hangs. Though that one doesn't > exist on VLV/CHV so currently some random garbage is being read > into it on those platforms. Indeed. Let's see what we can kill without breaking decode tools. But moving the function entirely to display is the easiest for now. So, about the PCH stuff itself moving to inside display, no objection from your side, right Ville? >
On Tue, Feb 18, 2025 at 05:27:41PM +0200, Ville Syrjälä wrote: >On Tue, Feb 18, 2025 at 09:45:46AM -0500, Rodrigo Vivi wrote: >> On Tue, Feb 18, 2025 at 02:19:38PM +0200, Jani Nikula wrote: >> > On Mon, 17 Feb 2025, Rodrigo Vivi <rodrigo.vivi@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. > >Mostly, but it still has bunch of gt workarounds in it. >Those all should be moved into the gt w/a framework. > >> 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. > >It's hidden behind some confusing macro stuff. yep. See CG_FUNCS in drivers/gpu/drm/i915/intel_clock_gating.c The callers are the ones calling intel_clock_gating_init(), which is both on display and gem sides. On the GEM side there's already and eternal TODO comment: * FIXME: break up the workarounds and apply them at the right time! Lucas De Marchi > >> >> 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. > >That seems like a random set of stuff no one actually cares about. >Should just nuke the whole thing, apart from DERRMR because that >one is actually relevant for GPU hangs. Though that one doesn't >exist on VLV/CHV so currently some random garbage is being read >into it on those platforms. > >-- >Ville Syrjälä >Intel
On Tue, Feb 18, 2025 at 03:34:14PM +0000, Vivi, Rodrigo wrote: > On Tue, 2025-02-18 at 17:27 +0200, Ville Syrjälä wrote: > > On Tue, Feb 18, 2025 at 09:45:46AM -0500, Rodrigo Vivi wrote: > > > On Tue, Feb 18, 2025 at 02:19:38PM +0200, Jani Nikula wrote: > > > > On Mon, 17 Feb 2025, Rodrigo Vivi <rodrigo.vivi@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. > > > > Mostly, but it still has bunch of gt workarounds in it. > > Those all should be moved into the gt w/a framework. > > hmm... indeed. But anyway, all that I saw using PCH is related > to display. > > > > > > 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. > > > > It's hidden behind some confusing macro stuff. > > oh no! someone is reading too much i915-guc related code :( > > > > > > > > > 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. > > > > That seems like a random set of stuff no one actually cares about. > > Should just nuke the whole thing, apart from DERRMR because that > > one is actually relevant for GPU hangs. Though that one doesn't > > exist on VLV/CHV so currently some random garbage is being read > > into it on those platforms. > > Indeed. Let's see what we can kill without breaking decode tools. > But moving the function entirely to display is the easiest for now. > > So, about the PCH stuff itself moving to inside display, no objection > from your side, right Ville? Seems mostly doable. The one thing I'm not quite sure how to deal with is the SDEIER hack in ilk_irq_handler(). Maybe we can move it into {ilk,ivb}_display_irq_handler(), just need to think about the ordering a bit. Hmm, the whole ilk+ irq handling still looks fairly dodgy. I should probably resurrect my old irq ack+handle split series and finally get that code into proper form...
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
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. No functional or code change. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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