[PATCH v12 13/14] drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
Vinod Polimera
vpolimer at qti.qualcomm.com
Tue Feb 7 14:26:03 UTC 2023
> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> Sent: Tuesday, January 31, 2023 6:29 PM
> To: Vinod Polimera (QUIC) <quic_vpolimer at quicinc.com>; dri-
> devel at lists.freedesktop.org; linux-arm-msm at vger.kernel.org;
> freedreno at lists.freedesktop.org; devicetree at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org; robdclark at gmail.com;
> dianders at chromium.org; swboyd at chromium.org; Kalyan Thota (QUIC)
> <quic_kalyant at quicinc.com>; Kuogee Hsieh (QUIC)
> <quic_khsieh at quicinc.com>; Vishnuvardhan Prodduturi (QUIC)
> <quic_vproddut at quicinc.com>; Bjorn Andersson (QUIC)
> <quic_bjorande at quicinc.com>; Abhinav Kumar (QUIC)
> <quic_abhinavk at quicinc.com>; Sankeerth Billakanti (QUIC)
> <quic_sbillaka at quicinc.com>
> 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 at quicinc.com>
> > Signed-off-by: Vinod Polimera <quic_vpolimer at quicinc.com>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > ---
> > 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.
More information about the dri-devel
mailing list