RE: [PATCH v12 13/14] drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
From: Vinod Polimera
Date: Tue Feb 07 2023 - 09:26:41 EST
> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Sent: Tuesday, January 31, 2023 6:29 PM
> To: Vinod Polimera (QUIC) <quic_vpolimer@xxxxxxxxxxx>; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx;
> freedreno@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxx;
> dianders@xxxxxxxxxxxx; swboyd@xxxxxxxxxxxx; Kalyan Thota (QUIC)
> <quic_kalyant@xxxxxxxxxxx>; Kuogee Hsieh (QUIC)
> <quic_khsieh@xxxxxxxxxxx>; Vishnuvardhan Prodduturi (QUIC)
> <quic_vproddut@xxxxxxxxxxx>; Bjorn Andersson (QUIC)
> <quic_bjorande@xxxxxxxxxxx>; Abhinav Kumar (QUIC)
> <quic_abhinavk@xxxxxxxxxxx>; Sankeerth Billakanti (QUIC)
> <quic_sbillaka@xxxxxxxxxxx>
> Subject: Re: [PATCH v12 13/14] drm/msm/disp/dpu: add PSR support for eDP
> interface in dpu driver
>
>
> On 30/01/2023 17:11, Vinod Polimera wrote:
> > Enable PSR on eDP interface using drm self-refresh librabry.
> > This patch uses a trigger from self-refresh library to enter/exit
> > into PSR, when there are no updates from framework.
> >
> > Signed-off-by: Kalyan Thota <quic_kalyant@xxxxxxxxxxx>
> > Signed-off-by: Vinod Polimera <quic_vpolimer@xxxxxxxxxxx>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 ++++++++++++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++++++++++++++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
> > 3 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index f29a339..60e5984 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -21,6 +21,7 @@
> > #include <drm/drm_probe_helper.h>
> > #include <drm/drm_rect.h>
> > #include <drm/drm_vblank.h>
> > +#include <drm/drm_self_refresh_helper.h>
> >
> > #include "dpu_kms.h"
> > #include "dpu_hw_lm.h"
> > @@ -1021,6 +1022,9 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >
> > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
> >
> > + if (old_crtc_state->self_refresh_active)
> > + return;
> > +
>
> I have been looking at the crtc_needs_disable(). It explicitly mentions
> that 'We also need to run through the crtc_funcs->disable() function
> [..] if it's transitioning to self refresh mode...'. Don't we need to
> perform some cleanup here (like disabling the vblank irq handling,
> freeing the bandwidth, etc)?
When self refresh active is enabled, then we will clean up irq handling and bandwidth etc.
The above case is to handle display off commit triggered when we are in psr as all
the resources are already cleaned up . we just need to do an early return.
>
> > /* Disable/save vblank irq handling */
> > drm_crtc_vblank_off(crtc);
> >
> > @@ -1577,7 +1581,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
> *dev, struct drm_plane *plane,
> > {
> > struct drm_crtc *crtc = NULL;
> > struct dpu_crtc *dpu_crtc = NULL;
> > - int i;
> > + int i, ret;
> >
> > dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL);
> > if (!dpu_crtc)
> > @@ -1614,6 +1618,13 @@ struct drm_crtc *dpu_crtc_init(struct
> drm_device *dev, struct drm_plane *plane,
> > /* initialize event handling */
> > spin_lock_init(&dpu_crtc->event_lock);
> >
> > + ret = drm_self_refresh_helper_init(crtc);
> > + if (ret) {
> > + DPU_ERROR("Failed to initialize %s with self-refresh helpers %d\n",
> > + crtc->name, ret);
> > + return ERR_PTR(ret);
> > + }
> > +
> > DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc-
> >name);
> > return crtc;
> > }
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 01b7509..450abb1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -12,6 +12,7 @@
> > #include <linux/kthread.h>
> > #include <linux/seq_file.h>
> >
> > +#include <drm/drm_atomic.h>
> > #include <drm/drm_crtc.h>
> > #include <drm/drm_file.h>
> > #include <drm/drm_probe_helper.h>
> > @@ -1212,11 +1213,24 @@ static void
> dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
> > struct drm_atomic_state *state)
> > {
> > struct dpu_encoder_virt *dpu_enc = NULL;
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *old_state = NULL;
> > int i = 0;
> >
> > dpu_enc = to_dpu_encoder_virt(drm_enc);
> > DPU_DEBUG_ENC(dpu_enc, "\n");
> >
> > + crtc = drm_atomic_get_old_crtc_for_encoder(state, drm_enc);
> > + if (crtc)
> > + old_state = drm_atomic_get_old_crtc_state(state, crtc);
> > +
> > + /*
> > + * The encoder is already disabled if self refresh mode was set earlier,
> > + * in the old_state for the corresponding crtc.
> > + */
> > + if (old_state && old_state->self_refresh_active)
> > + return;
> > +
>
> Again the same question here, doesn't crtc_needs_disable() take care of
> this clause? I might be missing something in the PSR state transitions.
> Could you please add some explanation here?
Same usecase as above, applies to encoder disable also when triggered via disable commit
When driver is in psr state.
>
> > mutex_lock(&dpu_enc->enc_lock);
> > dpu_enc->enabled = false;
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index a683bd9..681dd2e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -491,7 +491,7 @@ static void dpu_kms_wait_for_commit_done(struct
> msm_kms *kms,
> > return;
> > }
> >
> > - if (!crtc->state->active) {
> > + if (!drm_atomic_crtc_effectively_active(crtc->state)) {
> > DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id);
> > return;
> > }
>
> --
> With best wishes
> Dmitry
Thanks,
Vinod P.