Re: [PATCH v12 13/14] drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver

From: Dmitry Baryshkov
Date: Wed Feb 08 2023 - 16:58:01 EST


On 07/02/2023 17:25, Dmitry Baryshkov wrote:
On 07/02/2023 16:26, Vinod Polimera wrote:


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

Ack, thank you for the explanations. I'd like to take another glance later today, but generally it look good to me.

After another glance it still looks good to me. Please send the last iteration of the series:
- moving all core patches to the first place, as it was asked previously. This will help us get them merged to drm core repo first
- dropping the patch 09 as agreed.

--
With best wishes
Dmitry