RE: [PATCH v7 6/6] drm/i915/panelreplay: Debugfs support for panel replay

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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. 
 
> 
> >  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.

> 
> 
> >         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.

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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux