Re: [PATCH 1/2] drm/{i915,xe}: Move intel_pch under display

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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?

>