diff mbox series

[21/23] drm/i915/dmc: MTL DMC debugfs entries

Message ID 20220728013420.3750388-22-radhakrishna.sripada@intel.com (mailing list archive)
State New, archived
Headers show
Series Initial Meteorlake Support | expand

Commit Message

Sripada, Radhakrishna July 28, 2022, 1:34 a.m. UTC 
From: Anusha Srivatsa <anusha.srivatsa@intel.com>

MTL needs both Pipe A and Pipe B DMC to be loaded
along with Main DMC. Patch also adds
DMC debug register for MTL.

BSpec: 49788
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Matt Roper Aug. 2, 2022, 6:22 p.m. UTC | #1 
On Wed, Jul 27, 2022 at 06:34:18PM -0700, Radhakrishna Sripada wrote:
> From: Anusha Srivatsa <anusha.srivatsa@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@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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.


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
>
Srivatsa, Anusha Aug. 9, 2022, 6:06 p.m. UTC | #2 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Matt
> Roper
> Sent: Tuesday, August 2, 2022 11:23 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> Cc: intel-gfx@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@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@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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
diff mbox series

Patch

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));
 	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;