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