> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander@xxxxxxxxx>
> Sent: Friday, November 3, 2023 12:32 PM
> To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Murthy, Arun R
> <arun.r.murthy@xxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx>
> Subject: Re: [PATCH v7 6/6] drm/i915/panelreplay: Debugfs support for
> panel replay
>
> On Fri, 2023-11-03 at 06:10 +0000, Manna, Animesh wrote:
> >
> >
> > > -----Original Message-----
> > > From: Hogander, Jouni <jouni.hogander@xxxxxxxxx>
> > > Sent: Thursday, November 2, 2023 1:08 PM
> > > To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-
> > > gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Murthy, Arun R
> > > <arun.r.murthy@xxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx>
> > > 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@xxxxxxxxx>
> > > > Cc: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > > > ---
> > > > 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.
Added in v8.
>
> >
> > >
> > > > 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)); ...
>
Added in v8, please have a look if it looks good to you.
Regards,
Animesh
> >
> > >
> > >
> > > > 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);
> > > > }
> >
