Re: [PATCH] Revert "drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set"

2024-05-14 Thread Caleb Connolly 




On 14/05/2024 09:56, Dmitry Baryshkov wrote:

In the DPU driver blank IRQ handling is called from a vblank worker and
can happen outside of the irq_enable / irq_disable pair. Revert commit
d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
to fix vblank IRQ assignment for CMD DSI panels.

Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
Signed-off-by: Dmitry Baryshkov 


Hi Dmitry,

This fixes the regular DRM irq errors for me!

Tested-by: Caleb Connolly  # sm8250 OnePlus 8

---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c|  2 ++
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  5 
  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   | 32 --
  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 13 +++--
  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 11 +++-
  5 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..a7d8ecf3f5be 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct 
drm_encoder *drm_enc,
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
  
  		phys->cached_mode = crtc_state->adjusted_mode;

+   if (phys->ops.atomic_mode_set)
+   phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
}
  }
  
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h

index 002e89cc1705..30470cd15a48 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -69,6 +69,8 @@ struct dpu_encoder_phys;
   * @is_master:Whether this phys_enc is the current 
master
   *encoder. Can be switched at enable time. Based
   *on split_role and current mode (CMD/VID).
+ * @atomic_mode_set:   DRM Call. Set a DRM mode.
+ * This likely caches the mode, for use at enable.
   * @enable:   DRM Call. Enable a DRM mode.
   * @disable:  DRM Call. Disable mode.
   * @control_vblank_irqRegister/Deregister for VBLANK IRQ
@@ -93,6 +95,9 @@ struct dpu_encoder_phys;
  struct dpu_encoder_phys_ops {
void (*prepare_commit)(struct dpu_encoder_phys *encoder);
bool (*is_master)(struct dpu_encoder_phys *encoder);
+   void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
+   struct drm_crtc_state *crtc_state,
+   struct drm_connector_state *conn_state);
void (*enable)(struct dpu_encoder_phys *encoder);
void (*disable)(struct dpu_encoder_phys *encoder);
int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 489be1c0c704..95cd39b49668 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg)
dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
  }
  
+static void dpu_encoder_phys_cmd_atomic_mode_set(

+   struct dpu_encoder_phys *phys_enc,
+   struct drm_crtc_state *crtc_state,
+   struct drm_connector_state *conn_state)
+{
+   phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
+
+   phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
+
+   if (phys_enc->has_intf_te)
+   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_intf->cap->intr_tear_rd_ptr;
+   else
+   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_pp->caps->intr_rdptr;
+
+   phys_enc->irq[INTR_IDX_UNDERRUN] = 
phys_enc->hw_intf->cap->intr_underrun;
+}
+
  static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
struct dpu_encoder_phys *phys_enc)
  {
@@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct 
dpu_encoder_phys *phys_enc)
  phys_enc->hw_pp->idx - PINGPONG_0,
  phys_enc->vblank_refcount);
  
-	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;

-   phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
-
-   if (phys_enc->has_intf_te)
-   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_intf->cap->intr_tear_rd_ptr;
-   else
-   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_pp->caps->intr_rdptr;
-
dpu_core_irq_register_callback(phys_enc->dpu_kms,
   phys_enc->irq[INTR_IDX_PINGPONG],

Re: [PATCH] Revert "drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set"

2024-05-14 Thread Abhinav Kumar 




On 5/14/2024 12:56 AM, Dmitry Baryshkov wrote:

In the DPU driver blank IRQ handling is called from a vblank worker and
can happen outside of the irq_enable / irq_disable pair. Revert commit
d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
to fix vblank IRQ assignment for CMD DSI panels.



Can you please explain the sequence of events causing an issue due to 
moving the irq assignment to init and how is moving it back to modeset 
helping?



Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c|  2 ++
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  5 
  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   | 32 --
  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 13 +++--
  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 11 +++-
  5 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..a7d8ecf3f5be 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct 
drm_encoder *drm_enc,
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
  
  		phys->cached_mode = crtc_state->adjusted_mode;

+   if (phys->ops.atomic_mode_set)
+   phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
}
  }
  
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h

index 002e89cc1705..30470cd15a48 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -69,6 +69,8 @@ struct dpu_encoder_phys;
   * @is_master:Whether this phys_enc is the current 
master
   *encoder. Can be switched at enable time. Based
   *on split_role and current mode (CMD/VID).
+ * @atomic_mode_set:   DRM Call. Set a DRM mode.
+ * This likely caches the mode, for use at enable.
   * @enable:   DRM Call. Enable a DRM mode.
   * @disable:  DRM Call. Disable mode.
   * @control_vblank_irqRegister/Deregister for VBLANK IRQ
@@ -93,6 +95,9 @@ struct dpu_encoder_phys;
  struct dpu_encoder_phys_ops {
void (*prepare_commit)(struct dpu_encoder_phys *encoder);
bool (*is_master)(struct dpu_encoder_phys *encoder);
+   void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
+   struct drm_crtc_state *crtc_state,
+   struct drm_connector_state *conn_state);
void (*enable)(struct dpu_encoder_phys *encoder);
void (*disable)(struct dpu_encoder_phys *encoder);
int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 489be1c0c704..95cd39b49668 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg)
dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
  }
  
+static void dpu_encoder_phys_cmd_atomic_mode_set(

+   struct dpu_encoder_phys *phys_enc,
+   struct drm_crtc_state *crtc_state,
+   struct drm_connector_state *conn_state)
+{
+   phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
+
+   phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
+
+   if (phys_enc->has_intf_te)
+   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_intf->cap->intr_tear_rd_ptr;
+   else
+   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_pp->caps->intr_rdptr;
+
+   phys_enc->irq[INTR_IDX_UNDERRUN] = 
phys_enc->hw_intf->cap->intr_underrun;
+}
+
  static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
struct dpu_encoder_phys *phys_enc)
  {
@@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct 
dpu_encoder_phys *phys_enc)
  phys_enc->hw_pp->idx - PINGPONG_0,
  phys_enc->vblank_refcount);
  
-	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;

-   phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
-
-   if (phys_enc->has_intf_te)
-   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_intf->cap->intr_tear_rd_ptr;
-   else
-   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_pp->caps->intr_rdptr;
-
dpu_core_irq_register_callback(phys_enc->dpu_kms,

[PATCH] Revert "drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set"

2024-05-14 Thread Dmitry Baryshkov 
In the DPU driver blank IRQ handling is called from a vblank worker and
can happen outside of the irq_enable / irq_disable pair. Revert commit
d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
to fix vblank IRQ assignment for CMD DSI panels.

Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c|  2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  5 
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   | 32 --
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 13 +++--
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 11 +++-
 5 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..a7d8ecf3f5be 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct 
drm_encoder *drm_enc,
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
 
phys->cached_mode = crtc_state->adjusted_mode;
+   if (phys->ops.atomic_mode_set)
+   phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
}
 }
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 002e89cc1705..30470cd15a48 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -69,6 +69,8 @@ struct dpu_encoder_phys;
  * @is_master: Whether this phys_enc is the current master
  * encoder. Can be switched at enable time. Based
  * on split_role and current mode (CMD/VID).
+ * @atomic_mode_set:   DRM Call. Set a DRM mode.
+ * This likely caches the mode, for use at enable.
  * @enable:DRM Call. Enable a DRM mode.
  * @disable:   DRM Call. Disable mode.
  * @control_vblank_irq Register/Deregister for VBLANK IRQ
@@ -93,6 +95,9 @@ struct dpu_encoder_phys;
 struct dpu_encoder_phys_ops {
void (*prepare_commit)(struct dpu_encoder_phys *encoder);
bool (*is_master)(struct dpu_encoder_phys *encoder);
+   void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
+   struct drm_crtc_state *crtc_state,
+   struct drm_connector_state *conn_state);
void (*enable)(struct dpu_encoder_phys *encoder);
void (*disable)(struct dpu_encoder_phys *encoder);
int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 489be1c0c704..95cd39b49668 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg)
dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
 }
 
+static void dpu_encoder_phys_cmd_atomic_mode_set(
+   struct dpu_encoder_phys *phys_enc,
+   struct drm_crtc_state *crtc_state,
+   struct drm_connector_state *conn_state)
+{
+   phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
+
+   phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
+
+   if (phys_enc->has_intf_te)
+   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_intf->cap->intr_tear_rd_ptr;
+   else
+   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_pp->caps->intr_rdptr;
+
+   phys_enc->irq[INTR_IDX_UNDERRUN] = 
phys_enc->hw_intf->cap->intr_underrun;
+}
+
 static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
struct dpu_encoder_phys *phys_enc)
 {
@@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct 
dpu_encoder_phys *phys_enc)
  phys_enc->hw_pp->idx - PINGPONG_0,
  phys_enc->vblank_refcount);
 
-   phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
-   phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
-
-   if (phys_enc->has_intf_te)
-   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_intf->cap->intr_tear_rd_ptr;
-   else
-   phys_enc->irq[INTR_IDX_RDPTR] = 
phys_enc->hw_pp->caps->intr_rdptr;
-
dpu_core_irq_register_callback(phys_enc->dpu_kms,
   phys_enc->irq[INTR_IDX_PINGPONG],
   dpu_encoder_phys_cmd_pp_tx_done_irq,
@@ -318,10 +327,6 @@ static void dpu_encoder_phys_cmd_irq_disable(struct 
dpu_encoder_phys *phys_enc)