Re: [Intel-gfx] [PATCH v7 6/6] drm/i915/panelreplay: Debugfs support for panel replay

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Fri, 2023-11-03 at 06:10 +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Thursday, November 2, 2023 1:08 PM
> > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R
> > <arun.r.murthy@intel.com>; Nikula, Jani <jani.nikula@intel.com>
> > Subject: Re: [PATCH v7 6/6] drm/i915/panelreplay: Debugfs support
> > for
> > panel replay
> > 
> > On Wed, 2023-10-11 at 16:39 +0530, Animesh Manna wrote:
> > > Add debugfs support which will print source and sink status per
> > > connector basis.
> > 
> > Sorry for late review. Noticed only by now that you have added this
> > patch
> > into you set.
> 
> Added from v5.
> 
> > 
> > Can you please describe in commit message how you see the output of
> > debugfs interface will look like after your changes?
> 
> Sure.
> 
> > 
> > > 
> > > v1: Initial version. [rb-ed by Arun]
> > > v2: Added check for DP 2.0 and connector type in
> > > connector_debugfs_add().
> > > 
> > > Cc: Jouni Högander <jouni.hogander@intel.com>
> > > Cc: Arun R Murthy <arun.r.murthy@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_psr.c | 136
> > > +++++++++++++++++----
> > > --
> > >  1 file changed, 102 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 80de831c2f60..399fc0a8e636 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -2823,6 +2823,25 @@ static int
> > > psr_get_status_and_error_status(struct intel_dp *intel_dp,
> > >         return 0;
> > >  }
> > > 
> > > +static int panel_replay_get_status_and_error_status(struct
> > > intel_dp
> > > *intel_dp,
> > > +                                                   u8 *status,
> > > u8
> > > *error_status)
> > > +{
> > > +       struct drm_dp_aux *aux = &intel_dp->aux;
> > > +       int ret;
> > > +
> > > +       ret = drm_dp_dpcd_readb(aux,
> > > DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS, status);
> > > +       if (ret != 1)
> > > +               return ret;
> > > +
> > > +       ret = drm_dp_dpcd_readb(aux,
> > > DP_PANEL_REPLAY_ERROR_STATUS,
> > > error_status);
> > > +       if (ret != 1)
> > > +               return ret;
> > > +
> > > +       *status = *status & DP_PSR_SINK_STATE_MASK;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > 
> > I think you should modify  psr_get_status_and_error_status instead
> > of
> > duplicating most of it.
> 
> DPCD addresses are different for panel replay, I did not get the need
> of it. 

I would like to see:

unsigned int offset = intel_dp->psr.panel_replay_enabled ?
DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS : DP_PSR_STATUS;

ret = drm_dp_dpcd_readb(aux, offset, status);

rather than duplicating it.

>  
> > 
> > >  static void psr_alpm_check(struct intel_dp *intel_dp)
> > >  {
> > >         struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > @@
> > > -3035,7 +3054,7 @@ psr_source_status(struct intel_dp *intel_dp,
> > > struct
> > > seq_file *m)
> > >                         status = live_status[status_val];
> > >         }
> > > 
> > > -       seq_printf(m, "Source PSR status: %s [0x%08x]\n", status,
> > > val);
> > > +       seq_printf(m, "Source PSR/PanelReplay status: %s
> > > [0x%08x]\n",
> > > status, val);
> > >  }
> > > 
> > >  static int intel_psr_status(struct seq_file *m, struct intel_dp
> > > *intel_dp)
> > > @@ -3048,18 +3067,23 @@ static int intel_psr_status(struct
> > > seq_file
> > > *m, struct intel_dp *intel_dp)
> > >         bool enabled;
> > >         u32 val;
> > > 
> > > -       seq_printf(m, "Sink support: %s", str_yes_no(psr-
> > > > sink_support));
> > > -       if (psr->sink_support)
> > > +       seq_printf(m, "Sink support: PSR = %s, Panel Replay =
> > > %s",
> > > +                  str_yes_no(psr->sink_support),
> > > +                  str_yes_no(psr->sink_panel_replay_support));
> > > +
> > > +       if (psr->sink_support || psr->sink_panel_replay_support)
> > >                 seq_printf(m, " [0x%02x]", intel_dp-
> > > >psr_dpcd[0]);
> > >         seq_puts(m, "\n");
> > > 
> > > -       if (!psr->sink_support)
> > > +       if (!(psr->sink_support || psr-
> > > >sink_panel_replay_support))
> > >                 return 0;
> > > 
> > >         wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> > >         mutex_lock(&psr->lock);
> > > 
> > > -       if (psr->enabled)
> > > +       if (psr->panel_replay_enabled)
> > > +               status = "Panel Replay Enabled";
> > > +       else if (psr->enabled)
> > >                 status = psr->psr2_enabled ? "PSR2 enabled" :
> > > "PSR1
> > > enabled";
> > >         else
> > >                 status = "disabled";
> > > @@ -3072,14 +3096,17 @@ static int intel_psr_status(struct
> > > seq_file
> > > *m, struct intel_dp *intel_dp)
> > >                 goto unlock;
> > >         }
> > > 
> > > -       if (psr->psr2_enabled) {
> > > +       if (psr->panel_replay_enabled) {
> > > +               val = intel_de_read(dev_priv,
> > > TRANS_DP2_CTL(cpu_transcoder));
> > > +               enabled = val & TRANS_DP2_PANEL_REPLAY_ENABLE;
> > > +       } else if (psr->psr2_enabled) {
> > >                 val = intel_de_read(dev_priv,
> > > EDP_PSR2_CTL(cpu_transcoder));
> > > 
> > >                 enabled = val & EDP_PSR2_ENABLE;
> > >         } else {
> > >                 val = intel_de_read(dev_priv,
> > > psr_ctl_reg(dev_priv,
> > > cpu_transcoder));
> > >                 enabled = val & EDP_PSR_ENABLE;
> > >         }
> > > -       seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > > +       seq_printf(m, "Source PSR/PanelReplay ctl: %s
> > > [0x%08x]\n",
> > >                    str_enabled_disabled(enabled), val);
> > >         psr_source_status(intel_dp, m);
> > >         seq_printf(m, "Busy frontbuffer bits: 0x%08x\n", @@ -
> > > 3221,6
> > > +3248,7 @@ static int i915_psr_sink_status_show(struct seq_file
> > > *m,
> > > void *data)
> > >  {
> > >         struct intel_connector *connector = m->private;
> > >         struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > +       struct intel_psr *psr = &intel_dp->psr;
> > >         static const char * const sink_status[] = {
> > >                 "inactive",
> > >                 "transition to active, capture and display", @@
> > > -3231,45 +3259,82 @@ static int i915_psr_sink_status_show(struct
> > > seq_file *m, void *data)
> > >                 "reserved",
> > >                 "sink internal error",
> > >         };
> > > +       static const char * const panel_replay_status[] = {
> > > +               "Sink device frame is locked to the Source
> > > device",
> > > +               "Sink device is coasting, using the VTotal
> > > target",
> > > +               "Sink device is governing the frame rate (frame
> > > rate
> > > unlock is granted)",
> > > +               "Sink device in the process of re-locking with
> > > the
> > > Source device",
> > > +       };
> > >         const char *str;
> > >         int ret;
> > >         u8 status, error_status;
> > > 
> > > -       if (!CAN_PSR(intel_dp)) {
> > > -               seq_puts(m, "PSR Unsupported\n");
> > > +       if (!(CAN_PSR(intel_dp) || CAN_PANEL_REPLAY(intel_dp))) {
> > > +               seq_puts(m, "PSR/Panel-Replay Unsupported\n");
> > >                 return -ENODEV;
> > >         }
> > > 
> > >         if (connector->base.status != connector_status_connected)
> > >                 return -ENODEV;
> > > 
> > > -       ret = psr_get_status_and_error_status(intel_dp, &status,
> > > &error_status);
> > > -       if (ret)
> > > -               return ret;
> > > +       if (psr->panel_replay_enabled) {
> > > +               u32 temp;
> > > 
> > > -       status &= DP_PSR_SINK_STATE_MASK;
> > > -       if (status < ARRAY_SIZE(sink_status))
> > > -               str = sink_status[status];
> > > -       else
> > > -               str = "unknown";
> > > +               ret =
> > > panel_replay_get_status_and_error_status(intel_dp, &status,
> > > &error_status);
> > > +               if (ret)
> > > +                       return ret;
> > > 
> > > -       seq_printf(m, "Sink PSR status: 0x%x [%s]\n", status,
> > > str);
> > > +               temp = status & DP_SINK_FRAME_LOCKED_MASK;
> > > +               temp >>= DP_SINK_FRAME_LOCKED_SHIFT;
> > > +               if (temp < ARRAY_SIZE(panel_replay_status))
> > > +                       str = panel_replay_status[temp];
> > > +               else
> > > +                       str = "unknown";
> > > 
> > > -       seq_printf(m, "Sink PSR error status: 0x%x",
> > > error_status);
> > > +               seq_printf(m, "Sink Panel Replay status: 0x%x
> > > [%s]\n", status, str);
> > > 
> > > -       if (error_status & (DP_PSR_RFB_STORAGE_ERROR |
> > > -                           DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> > > -                           DP_PSR_LINK_CRC_ERROR))
> > > -               seq_puts(m, ":\n");
> > > -       else
> > > -               seq_puts(m, "\n");
> > > -       if (error_status & DP_PSR_RFB_STORAGE_ERROR)
> > > -               seq_puts(m, "\tPSR RFB storage error\n");
> > > -       if (error_status & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> > > -               seq_puts(m, "\tPSR VSC SDP uncorrectable
> > > error\n");
> > > -       if (error_status & DP_PSR_LINK_CRC_ERROR)
> > > -               seq_puts(m, "\tPSR Link CRC error\n");
> > > +               seq_printf(m, "Sink Panel Replay error status:
> > > 0x%x",
> > > error_status);
> > > +
> > > +               if (error_status &
> > > (DP_PANEL_REPLAY_RFB_STORAGE_ERROR
> > > > 
> > > +
> > > DP_PANEL_REPLAY_VSC_SDP_UNCORRECTABLE_ERROR |
> > > +                                  
> > > DP_PANEL_REPLAY_LINK_CRC_ERROR))
> > > +                       seq_puts(m, ":\n");
> > > +               else
> > > +                       seq_puts(m, "\n");
> > > +               if (error_status &
> > > DP_PANEL_REPLAY_RFB_STORAGE_ERROR)
> > > +                       seq_puts(m, "\tPANEL-REPLAY RFB storage
> > > error\n");
> > > +               if (error_status &
> > > DP_PANEL_REPLAY_VSC_SDP_UNCORRECTABLE_ERROR)
> > > +                       seq_puts(m, "\tPANEL-REPLAY VSC SDP
> > > uncorrectable error\n");
> > > +               if (error_status &
> > > DP_PANEL_REPLAY_LINK_CRC_ERROR)
> > > +                       seq_puts(m, "\tPANEL-REPLAY Link CRC
> > > error\n");
> > > +       } else {
> > > +               ret = psr_get_status_and_error_status(intel_dp,
> > > &status, &error_status);
> > > +               if (ret)
> > > +                       return ret;
> > > +
> > > +               status &= DP_PSR_SINK_STATE_MASK;
> > > +               if (status < ARRAY_SIZE(sink_status))
> > > +                       str = sink_status[status];
> > > +               else
> > > +                       str = "unknown";
> > > +
> > > +               seq_printf(m, "Sink PSR status: 0x%x [%s]\n",
> > > status,
> > > str);
> > > +
> > > +               seq_printf(m, "Sink PSR error status: 0x%x",
> > > error_status);
> > > 
> > > +               if (error_status & (DP_PSR_RFB_STORAGE_ERROR |
> > > +
> > > DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> > > +                                   DP_PSR_LINK_CRC_ERROR))
> > > +                       seq_puts(m, ":\n");
> > > +               else
> > > +                       seq_puts(m, "\n");
> > > +               if (error_status & DP_PSR_RFB_STORAGE_ERROR)
> > > +                       seq_puts(m, "\tPSR RFB storage error\n");
> > > +               if (error_status &
> > > DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> > > +                       seq_puts(m, "\tPSR VSC SDP uncorrectable
> > > error\n");
> > > +               if (error_status & DP_PSR_LINK_CRC_ERROR)
> > > +                       seq_puts(m, "\tPSR Link CRC error\n");
> > > +       }
> > 
> > Instead of duplicating so much here I think it woukd be ok to just
> > drop PSR
> > and do (same applies for all the statuses):
> > 
> > 
> > if (error_status & (DP_PSR_LINK_CRC_ERROR |
> > DP_PANEL_REPLAY_LINK_CRC_ERROR))
> >     seq_puts(m, "\tLink CRC error\n");
> > 
> > What do you think?
> 
> Thinking of backward compatibility, I have added separately for panel
> replay.
> I feel good to have separate cleanup patch for psr and panel-replay
> together, not part of this patch.

If you want to keep the format as it is, then you should solve it by
changing just the mode in printout rather than duplicating everything.
One way:

static const char *psr_mode_str(struct intel_dp *intel_dp)
{
	if (intel_dp->psr.panel_replay_enabled)
		return "PANEL-REPLAY";
	else if(intel_dp->psr.psr_enabled)
		return "PSR";

	return "unknown";
}
...
seq_puts(m, "\t%s RFB storage error\n", psr_mode_str(intel_dp));
...

> 
> > 
> > 
> > >         return ret;
> > >  }
> > >  DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
> > > @@ -3288,13 +3353,16 @@ void
> > > intel_psr_connector_debugfs_add(struct
> > > intel_connector *connector)
> > >         struct drm_i915_private *i915 = to_i915(connector-
> > > >base.dev);
> > >         struct dentry *root = connector->base.debugfs_entry;
> > > 
> > > -       if (connector->base.connector_type !=
> > DRM_MODE_CONNECTOR_eDP)
> > > -               return;
> > > +       if (connector->base.connector_type !=
> > DRM_MODE_CONNECTOR_eDP)
> > > {
> > > +               if (!(HAS_DP20(i915) &&
> > > +                     connector->base.connector_type ==
> > > DRM_MODE_CONNECTOR_DisplayPort))
> > > +                       return;
> > > +       }
> > 
> > Why do you need to check DRM_MODE_CONNECTOR_DisplayPort ? I think
> > connector->base.connector_type != DRM_MODE_CONNECTOR_eDP &&
> > !HAS_DP20(i915) is enough.
> 
> As per your suggestion, the code will look like below:
> 
>         if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP
> && !HAS_DP20(i915))
>                 return;
> 
> For example in case of hdmi connector type in MTL still we go ahead
> and create debugfs entry... rt? Please let me if I am missing
> anything.

Ok, I got this now.

BR,

Jouni Högander
> 
> Regards,
> Animesh
>  
> > 
> > BR,
> > 
> > Jouni Högander
> > > 
> > >         debugfs_create_file("i915_psr_sink_status", 0444, root,
> > >                             connector,
> > > &i915_psr_sink_status_fops);
> > > 
> > > -       if (HAS_PSR(i915))
> > > +       if (HAS_PSR(i915) || HAS_DP20(i915))
> > >                 debugfs_create_file("i915_psr_status", 0444,
> > > root,
> > >                                     connector,
> > > &i915_psr_status_fops);
> > >  }
>