Re: [PATCH v6 10/26] drm/i915/psr: Print Panel Replay status instead of frame lock status

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

On Fri, 2024-06-07 at 10:09 +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Friday, June 7, 2024 3:34 PM
> > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: Kahola, Mika <mika.kahola@intel.com>
> > Subject: Re: [PATCH v6 10/26] drm/i915/psr: Print Panel Replay
> > status instead
> > of frame lock status
> > 
> > On Fri, 2024-06-07 at 09:59 +0000, Manna, Animesh wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Hogander, Jouni <jouni.hogander@intel.com>
> > > > Sent: Thursday, June 6, 2024 9:08 PM
> > > > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > > > gfx@lists.freedesktop.org
> > > > Cc: Kahola, Mika <mika.kahola@intel.com>
> > > > Subject: Re: [PATCH v6 10/26] drm/i915/psr: Print Panel Replay
> > > > status instead of frame lock status
> > > > 
> > > > On Thu, 2024-06-06 at 14:35 +0000, Manna, Animesh wrote:
> > > > > 
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Hogander, Jouni <jouni.hogander@intel.com>
> > > > > > Sent: Wednesday, June 5, 2024 3:56 PM
> > > > > > To: intel-gfx@lists.freedesktop.org
> > > > > > Cc: Manna, Animesh <animesh.manna@intel.com>; Kahola, Mika
> > > > > > <mika.kahola@intel.com>; Hogander, Jouni
> > > > > > <jouni.hogander@intel.com>
> > > > > > Subject: [PATCH v6 10/26] drm/i915/psr: Print Panel Replay
> > > > > > status instead of frame lock status
> > > > > > 
> > > > > > Currently Panel Replay status printout is printing frame
> > > > > > lock
> > > > > > status. It should print Panel Replay status instead. Panel
> > > > > > Replay status register field follows PSR status register
> > > > > > field.
> > > > > > Use existing PSR code for that.
> > > > > > 
> > > > > > Fixes: ef75c25e8fed ("drm/i915/panelreplay: Debugfs support
> > > > > > for
> > > > > > panel
> > > > > > replay")
> > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 22 +++++-------
> > > > > > ----
> > > > > > ----
> > > > > > --
> > > > > >  1 file changed, 5 insertions(+), 17 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > index 7bdae0d0ea45..3530e5f44096 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > @@ -3579,16 +3579,9 @@ 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;
> > > > > > -       u32 idx;
> > > > > > 
> > > > > >         if (!(CAN_PSR(intel_dp) ||
> > > > > > CAN_PANEL_REPLAY(intel_dp)))
> > > > > > {
> > > > > >                 seq_puts(m, "PSR/Panel-Replay
> > > > > > Unsupported\n");
> > > > > > @@
> > > > > > -3602,16 +3595,11 @@ static int
> > > > > > i915_psr_sink_status_show(struct seq_file *m, void *data)
> > > > > >         if (ret)
> > > > > >                 return ret;
> > > > > > 
> > > > > > -       str = "unknown";
> > > > > > -       if (intel_dp->psr.panel_replay_enabled) {
> > > > > > -               idx = (status & DP_SINK_FRAME_LOCKED_MASK)
> > > > > > >>
> > > > > > DP_SINK_FRAME_LOCKED_SHIFT;
> > > > > > -               if (idx < ARRAY_SIZE(panel_replay_status))
> > > > > > -                       str = panel_replay_status[idx];
> > > > > > -       } else if (intel_dp->psr.enabled) {
> > > > > > -               idx = status & DP_PSR_SINK_STATE_MASK;
> > > > > > -               if (idx < ARRAY_SIZE(sink_status))
> > > > > > -                       str = sink_status[idx];
> > > > > > -       }
> > > > > > +       status &= DP_PSR_SINK_STATE_MASK;
> > > > > > +       if (status < ARRAY_SIZE(sink_status))
> > > > > > +               str = sink_status[status];
> > > > > > +       else
> > > > > > +               str = "unknown";
> > > > > 
> > > > > psr_get_status_and_error_status() is returning frame-locked-
> > > > > status for panel replay, Its different dpcd
> > > > > DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS, not same like psr.
> > > > 
> > > > Panel Replay STATUS ~= PSR STATUS if you look at description of
> > > > the
> > > > registers. Frame lock status is completely different thing. I
> > > > don't
> > > > understand why psr sink status debugfs interface should print
> > > > frame
> > > > lock status for Panel Replay?
> > > 
> > > If we do not want to print
> > DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS
> > > the psr_get_status_and_error_status() need to be modified. Do you
> > > agree?
> > 
> > Yes, and this what I'm doing in this patch? Or can you elaborate a
> > bit what do
> > you mean?
> 
> I do not see any change in psr_get_status_and_error_status() in this
> patch.
> Just adding below the code-snippet where based on
> panel_replay_enabled flag offset is assigned to
> DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS.
> 
> static int psr_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;
>         unsigned int offset;
> 
>         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);
>         if (ret != 1)
>                 return ret;
> ...
> ...
> ...
> 

DP_SINK_DEVICE_PR_AND_FRAME_LOCK_STATUS contains two fields. "Sink
Device Panel Replay Status" and "SINK FRAME LOCKED". Currently we are
printing latter.   "SINK FRAME LOCKED" is not actually that much
related to psr status debugfs interface. This patch is changing the
interface to print out "Sink Device Panel Replay Status".

BR,

Jouni Högander

> Regards,
> Animesh
> 
> > 
> > BR,
> > 
> > Jouni Högander
> > > 
> > > Regards,
> > > Animesh
> > > > 
> > > > BR,
> > > > 
> > > > Jouni Högander
> > > > 
> > > > > 
> > > > > Regards,
> > > > > Animesh
> > > > > 
> > > > > > 
> > > > > >         seq_printf(m, "Sink %s status: 0x%x [%s]\n",
> > > > > > psr_mode_str(intel_dp), status, str);
> > > > > > 
> > > > > > --
> > > > > > 2.34.1
> > > > > 
> > > 
>