[Intel-gfx] [PATCH 21/23] drm/i915/dmc: MTL DMC debugfs entries

[Srivatsa, Anusha]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Matt
> Roper
> Sent: Tuesday, August 2, 2022 11:23 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 21/23] drm/i915/dmc: MTL DMC debugfs
> entries
> 
> On Wed, Jul 27, 2022 at 06:34:18PM -0700, Radhakrishna Sripada wrote:
> > From: Anusha Srivatsa <anusha.srivatsa at intel.com>
> >
> > MTL needs both Pipe A and Pipe B DMC to be loaded along with Main DMC.
> > Patch also adds
> 
> That's true, but it's unrelated to this patch.  intel_dmc_load_program()
> always loads all of the pipe firmwares (including pipe C and pipe D) assuming
> it found them in the firmware file.
> 
> > DMC debug register for MTL.
> >
> > BSpec: 49788
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dmc.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c
> > b/drivers/gpu/drm/i915/display/intel_dmc.c
> > index 9c4f442fa407..2fabb2760474 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > @@ -1005,7 +1005,7 @@ static int intel_dmc_debugfs_status_show(struct
> seq_file *m, void *unused)
> >  	seq_printf(m, "Pipe A fw loaded: %s\n",
> >  		   str_yes_no(dmc->dmc_info[DMC_FW_PIPEA].payload));
> >  	seq_printf(m, "Pipe B fw support: %s\n",
> > -		   str_yes_no(IS_ALDERLAKE_P(i915)));
> > +		   str_yes_no(DISPLAY_VER(i915) >= 13));
> 
> What is this debugfs trying to tell us?  Pipe DMC fw for all four pipes has
> been supported since TGL.  So the output here is misleading (and incomplete
> since it doesn't include C/D).
> 
> The thing that changed in DG2 was that we were required to upload the pipe
> A firmware along with the main firmware (other pipes optional).
> The thing that further changed in ADL-P was that we were required to upload
> *both* pipe A and pipe B along with the main firmware (other two pipes still
> optional).
> 
> Even if the output here was trying to indicate which pipe firmware(s) need to
> be uploaded at the same time as the main firmware (rather than being
> uploaded later), the change here wouldn't be correct since as noted above,
> DG2 (which has display version 13) only required pipe A and not B.
> 
> I think we probably need to decide what the purpose of this debugfs is
> supposed to be and then rework it accordingly.
> 

IMO the debugfs should give a gist of the different firmwares that was loaded and which ones the platform actually needs. At this point the driver loads all available firmwares even if the platform doesn't need them. Debugfs should clearly state wat Is needed and if that is loaded or not.

Something like:
Pipe A loaded: yes/no
Pipe B loaded: yes/no
And so on.....
As well as:
Pipe A needed: yes/no
Pipe B needed: yes/no

Does this sound like the right way forward?

Anusha
> Matt
> 
> >  	seq_printf(m, "Pipe B fw loaded: %s\n",
> >  		   str_yes_no(dmc->dmc_info[DMC_FW_PIPEB].payload));
> >
> > @@ -1029,9 +1029,9 @@ static int intel_dmc_debugfs_status_show(struct
> seq_file *m, void *unused)
> >  		 * reg for DC3CO debugging and validation,
> >  		 * but TGL DMC f/w is using DMC_DEBUG3 reg for DC3CO
> counter.
> >  		 */
> > -		seq_printf(m, "DC3CO count: %d\n",
> > -			   intel_de_read(i915, IS_DGFX(i915) ?
> > -					 DG1_DMC_DEBUG3 :
> TGL_DMC_DEBUG3));
> > +		seq_printf(m, "DC3CO count: %d\n", intel_de_read(i915,
> > +			   (IS_DGFX(i915) || DISPLAY_VER(i915) >= 14) ?
> > +			    DG1_DMC_DEBUG3 : TGL_DMC_DEBUG3));
> >  	} else {
> >  		dc5_reg = IS_BROXTON(i915) ? BXT_DMC_DC3_DC5_COUNT :
> >  			SKL_DMC_DC3_DC5_COUNT;
> > --
> > 2.25.1
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation