Re: [PATCH v4 12/21] drm/i915/dmc: Reload PIPEDMC MMIO registers for pipe C/D on PTL+

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

On Tue, Jun 10, 2025 at 11:24:58PM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Monday, June 9, 2025 7:41 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: intel-xe@lists.freedesktop.org
> > Subject: [PATCH v4 12/21] drm/i915/dmc: Reload PIPEDMC MMIO registers for
> > pipe C/D on PTL+
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On PTL+ the PIPEDMC on pipes C/D loses its MMIO state occasionally.
> > Not quite sure what the specific sequence is that makes this happen (eg. simply
> > disbling PG2 doesn't seem to be enough to trigger this on its own).
> 
> Nit: Typo in disabling
> 
> > Reload the MMIO registers for the affected pipes when enabling the PIPEDMC. So
> > far I've not see this happen on PTL pipe A/B, nor on any pipe on any other
> > platform.
> > 
> > The DMC program RAM doesn't appear to need manual restoring, though Windows
> > appears to be doing exactly that on most platforms (for some of the pipes). None
> > of this is properly documented anywhere it seems.
> 
> Yeah can't find any documentation for the same. Lets go with the empirical behaviour,
> will try to get this updated in spec as well.

CI did catch the fact that TGL/derivatives lose the entire pipe DMC state
when PG1 is disabled, and the main DMC does not restore any of it. I'll
cook up some extra patches to deal with that. The pipe DMC is only
needed for LACE on these platforms, so could perhaps just not load it at
all, but I think I'll keep it around just in case we ever want to
implement some LACE stuff at some point.

> 
> Looks Good to me.
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dmc.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c
> > b/drivers/gpu/drm/i915/display/intel_dmc.c
> > index 5a43298cd0e7..247e88265cf3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > @@ -578,6 +578,17 @@ static u32 dmc_mmiodata(struct intel_display *display,
> >  		return dmc->dmc_info[dmc_id].mmiodata[i];
> >  }
> > 
> > +static void intel_dmc_load_mmio(struct intel_display *display, enum
> > +intel_dmc_id dmc_id) {
> > +	struct intel_dmc *dmc = display_to_dmc(display);
> > +	int i;
> > +
> > +	for (i = 0; i < dmc->dmc_info[dmc_id].mmio_count; i++) {
> > +		intel_de_write(display, dmc->dmc_info[dmc_id].mmioaddr[i],
> > +			       dmc_mmiodata(display, dmc, dmc_id, i));
> > +	}
> > +}
> > +
> >  void intel_dmc_enable_pipe(struct intel_display *display, enum pipe pipe)  {
> >  	enum intel_dmc_id dmc_id = PIPE_TO_DMC_ID(pipe); @@ -585,6
> > +596,10 @@ void intel_dmc_enable_pipe(struct intel_display *display, enum pipe
> > pipe)
> >  	if (!is_valid_dmc_id(dmc_id) || !has_dmc_id_fw(display, dmc_id))
> >  		return;
> > 
> > +	/* on PTL pipe C/D PIPEDMC MMIO state is lost sometimes */
> > +	if (DISPLAY_VER(display) >= 30 && pipe >= PIPE_C)
> > +		intel_dmc_load_mmio(display, dmc_id);
> > +
> >  	if (DISPLAY_VER(display) >= 20) {
> >  		intel_de_write(display, PIPEDMC_INTERRUPT(pipe),
> > pipedmc_interrupt_mask(display));
> >  		intel_de_write(display, PIPEDMC_INTERRUPT_MASK(pipe),
> > ~pipedmc_interrupt_mask(display));
> > @@ -710,12 +725,8 @@ void intel_dmc_load_program(struct intel_display
> > *display)
> > 
> >  	preempt_enable();
> > 
> > -	for_each_dmc_id(dmc_id) {
> > -		for (i = 0; i < dmc->dmc_info[dmc_id].mmio_count; i++) {
> > -			intel_de_write(display, dmc-
> > >dmc_info[dmc_id].mmioaddr[i],
> > -				       dmc_mmiodata(display, dmc, dmc_id, i));
> > -		}
> > -	}
> > +	for_each_dmc_id(dmc_id)
> > +		intel_dmc_load_mmio(display, dmc_id);
> > 
> >  	power_domains->dc_state = 0;
> > 
> > --
> > 2.49.0
> 

-- 
Ville Syrjälä
Intel