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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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...

-- 
Ville Syrjälä
Intel