* [PATCH v3 0/5] drm/msm/dpu: remove dpu_encoder_phys_ops::atomic_mode_set callback
@ 2023-12-25 13:08 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
The dpu_encoder_phys_ops::atomic_mode_set() callback is mostly
redundant. Implementations only set the IRQ indices there. Move
statically allocated IRQs to dpu_encoder_phys_*_init() and set
dynamically allocated IRQs in the irq_enable() callback.
The writeback backend of the dpu_encoder is the only user of the
dpu_encoder_phys_ops::atomic_check() callback. Move corresponding code
to the DPU's drm_writeback_connector implementation (dpu_writeback.c)
and drop corresponding callback code.
Changes since v2:
- Rebase on top of linux-next
- Also incorporate the dpu_encoder_phys::atomic_check series
Changes since v1:
- Split trace events into enable/disable pairs (Abhinav).
Dmitry Baryshkov (5):
drm/msm/dpu: split irq_control into irq_enable and _disable
drm/msm/dpu: split _dpu_encoder_resource_control_helper()
drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set
drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
drm/msm/dpu: drop dpu_encoder_phys_ops::atomic_check()
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 94 ++++++++++--------
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 15 +--
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 95 +++++++++----------
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 59 ++++++------
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 88 ++++-------------
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 74 +++++++++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
9 files changed, 270 insertions(+), 224 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 0/5] drm/msm/dpu: remove dpu_encoder_phys_ops::atomic_mode_set callback
@ 2023-12-25 13:08 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel,
Stephen Boyd, David Airlie
The dpu_encoder_phys_ops::atomic_mode_set() callback is mostly
redundant. Implementations only set the IRQ indices there. Move
statically allocated IRQs to dpu_encoder_phys_*_init() and set
dynamically allocated IRQs in the irq_enable() callback.
The writeback backend of the dpu_encoder is the only user of the
dpu_encoder_phys_ops::atomic_check() callback. Move corresponding code
to the DPU's drm_writeback_connector implementation (dpu_writeback.c)
and drop corresponding callback code.
Changes since v2:
- Rebase on top of linux-next
- Also incorporate the dpu_encoder_phys::atomic_check series
Changes since v1:
- Split trace events into enable/disable pairs (Abhinav).
Dmitry Baryshkov (5):
drm/msm/dpu: split irq_control into irq_enable and _disable
drm/msm/dpu: split _dpu_encoder_resource_control_helper()
drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set
drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
drm/msm/dpu: drop dpu_encoder_phys_ops::atomic_check()
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 94 ++++++++++--------
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 15 +--
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 95 +++++++++----------
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 59 ++++++------
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 88 ++++-------------
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 74 +++++++++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
9 files changed, 270 insertions(+), 224 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 1/5] drm/msm/dpu: split irq_control into irq_enable and _disable
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2023-12-25 13:08 ` Dmitry Baryshkov
-1 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
The single helper for both enable and disable cases is too complicated,
especially if we start adding more code to these helpers. Split it into
irq_enable and irq_disable cases.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 36 +++++++---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 6 +-
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 65 ++++++++++---------
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 46 +++++++------
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 29 ++++++---
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 62 ++++++++++++++----
6 files changed, 158 insertions(+), 86 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f2b82ca5efb3..5ab32ace7707 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -714,7 +714,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
}
}
-static void _dpu_encoder_irq_control(struct drm_encoder *drm_enc, bool enable)
+static void _dpu_encoder_irq_enable(struct drm_encoder *drm_enc)
{
struct dpu_encoder_virt *dpu_enc;
int i;
@@ -726,14 +726,32 @@ static void _dpu_encoder_irq_control(struct drm_encoder *drm_enc, bool enable)
dpu_enc = to_dpu_encoder_virt(drm_enc);
- DPU_DEBUG_ENC(dpu_enc, "enable:%d\n", enable);
+ DPU_DEBUG_ENC(dpu_enc, "\n");
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
- if (phys->ops.irq_control)
- phys->ops.irq_control(phys, enable);
+ phys->ops.irq_enable(phys);
+ }
+}
+
+static void _dpu_encoder_irq_disable(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+ int i;
+
+ if (!drm_enc) {
+ DPU_ERROR("invalid encoder\n");
+ return;
}
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ DPU_DEBUG_ENC(dpu_enc, "\n");
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+ phys->ops.irq_disable(phys);
+ }
}
static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
@@ -759,11 +777,11 @@ static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
pm_runtime_get_sync(&dpu_kms->pdev->dev);
/* enable all the irq */
- _dpu_encoder_irq_control(drm_enc, true);
+ _dpu_encoder_irq_enable(drm_enc);
} else {
/* disable all the irq */
- _dpu_encoder_irq_control(drm_enc, false);
+ _dpu_encoder_irq_disable(drm_enc);
/* disable DPU core clks */
pm_runtime_put_sync(&dpu_kms->pdev->dev);
@@ -824,7 +842,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
}
if (is_vid_mode && dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE)
- _dpu_encoder_irq_control(drm_enc, true);
+ _dpu_encoder_irq_enable(drm_enc);
else
_dpu_encoder_resource_control_helper(drm_enc, true);
@@ -879,7 +897,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
if (is_vid_mode &&
dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE) {
- _dpu_encoder_irq_control(drm_enc, true);
+ _dpu_encoder_irq_enable(drm_enc);
}
/* skip if is already OFF or IDLE, resources are off already */
else if (dpu_enc->rc_state == DPU_ENC_RC_STATE_OFF ||
@@ -954,7 +972,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
}
if (is_vid_mode)
- _dpu_encoder_irq_control(drm_enc, false);
+ _dpu_encoder_irq_disable(drm_enc);
else
_dpu_encoder_resource_control_helper(drm_enc, false);
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 993f26343331..8c5b0c853572 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -85,7 +85,8 @@ struct dpu_encoder_phys;
* @handle_post_kickoff: Do any work necessary post-kickoff work
* @trigger_start: Process start event on physical encoder
* @needs_single_flush: Whether encoder slaves need to be flushed
- * @irq_control: Handler to enable/disable all the encoder IRQs
+ * @irq_enable: Handler to enable all the encoder IRQs
+ * @irq_disable: Handler to disable all the encoder IRQs
* @prepare_idle_pc: phys encoder can update the vsync_enable status
* on idle power collapse prepare
* @restore: Restore all the encoder configs.
@@ -110,7 +111,8 @@ struct dpu_encoder_phys_ops {
void (*handle_post_kickoff)(struct dpu_encoder_phys *phys_enc);
void (*trigger_start)(struct dpu_encoder_phys *phys_enc);
bool (*needs_single_flush)(struct dpu_encoder_phys *phys_enc);
- void (*irq_control)(struct dpu_encoder_phys *phys, bool enable);
+ void (*irq_enable)(struct dpu_encoder_phys *phys);
+ void (*irq_disable)(struct dpu_encoder_phys *phys);
void (*prepare_idle_pc)(struct dpu_encoder_phys *phys_enc);
void (*restore)(struct dpu_encoder_phys *phys);
int (*get_line_count)(struct dpu_encoder_phys *phys);
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 a301e2833177..de826f9745e5 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
@@ -291,40 +291,42 @@ static int dpu_encoder_phys_cmd_control_vblank_irq(
return ret;
}
-static void dpu_encoder_phys_cmd_irq_control(struct dpu_encoder_phys *phys_enc,
- bool enable)
+static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
{
- trace_dpu_enc_phys_cmd_irq_ctrl(DRMID(phys_enc->parent),
- phys_enc->hw_pp->idx - PINGPONG_0,
- enable, phys_enc->vblank_refcount);
-
- if (enable) {
- dpu_core_irq_register_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_PINGPONG],
- dpu_encoder_phys_cmd_pp_tx_done_irq,
- phys_enc);
+ trace_dpu_enc_phys_cmd_irq_enable(DRMID(phys_enc->parent),
+ phys_enc->hw_pp->idx - PINGPONG_0,
+ phys_enc->vblank_refcount);
+
+ dpu_core_irq_register_callback(phys_enc->dpu_kms,
+ phys_enc->irq[INTR_IDX_PINGPONG],
+ dpu_encoder_phys_cmd_pp_tx_done_irq,
+ phys_enc);
+ dpu_core_irq_register_callback(phys_enc->dpu_kms,
+ phys_enc->irq[INTR_IDX_UNDERRUN],
+ dpu_encoder_phys_cmd_underrun_irq,
+ phys_enc);
+ dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
+
+ if (dpu_encoder_phys_cmd_is_master(phys_enc))
dpu_core_irq_register_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_UNDERRUN],
- dpu_encoder_phys_cmd_underrun_irq,
- phys_enc);
- dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
-
- if (dpu_encoder_phys_cmd_is_master(phys_enc))
- dpu_core_irq_register_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_CTL_START],
- dpu_encoder_phys_cmd_ctl_start_irq,
- phys_enc);
- } else {
- if (dpu_encoder_phys_cmd_is_master(phys_enc))
- dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_CTL_START]);
+ phys_enc->irq[INTR_IDX_CTL_START],
+ dpu_encoder_phys_cmd_ctl_start_irq,
+ phys_enc);
+}
+static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
+{
+ trace_dpu_enc_phys_cmd_irq_disable(DRMID(phys_enc->parent),
+ phys_enc->hw_pp->idx - PINGPONG_0,
+ phys_enc->vblank_refcount);
+
+ if (dpu_encoder_phys_cmd_is_master(phys_enc))
dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_UNDERRUN]);
- dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
- dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_PINGPONG]);
- }
+ phys_enc->irq[INTR_IDX_CTL_START]);
+
+ dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
+ dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
+ dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
}
static void dpu_encoder_phys_cmd_tearcheck_config(
@@ -713,7 +715,8 @@ static void dpu_encoder_phys_cmd_init_ops(
ops->wait_for_tx_complete = dpu_encoder_phys_cmd_wait_for_tx_complete;
ops->trigger_start = dpu_encoder_phys_cmd_trigger_start;
ops->needs_single_flush = dpu_encoder_phys_cmd_needs_single_flush;
- ops->irq_control = dpu_encoder_phys_cmd_irq_control;
+ ops->irq_enable = dpu_encoder_phys_cmd_irq_enable;
+ ops->irq_disable = dpu_encoder_phys_cmd_irq_disable;
ops->restore = dpu_encoder_phys_cmd_enable_helper;
ops->prepare_idle_pc = dpu_encoder_phys_cmd_prepare_idle_pc;
ops->handle_post_kickoff = dpu_encoder_phys_cmd_handle_post_kickoff;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index d0f56c5c4cce..9743ec43d862 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -615,30 +615,33 @@ static void dpu_encoder_phys_vid_handle_post_kickoff(
}
}
-static void dpu_encoder_phys_vid_irq_control(struct dpu_encoder_phys *phys_enc,
- bool enable)
+static void dpu_encoder_phys_vid_irq_enable(struct dpu_encoder_phys *phys_enc)
{
int ret;
- trace_dpu_enc_phys_vid_irq_ctrl(DRMID(phys_enc->parent),
- phys_enc->hw_intf->idx - INTF_0,
- enable,
- phys_enc->vblank_refcount);
+ trace_dpu_enc_phys_vid_irq_enable(DRMID(phys_enc->parent),
+ phys_enc->hw_intf->idx - INTF_0,
+ phys_enc->vblank_refcount);
- if (enable) {
- ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
- if (WARN_ON(ret))
- return;
-
- dpu_core_irq_register_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_UNDERRUN],
- dpu_encoder_phys_vid_underrun_irq,
- phys_enc);
- } else {
- dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
- dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_UNDERRUN]);
- }
+ ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
+ if (WARN_ON(ret))
+ return;
+
+ dpu_core_irq_register_callback(phys_enc->dpu_kms,
+ phys_enc->irq[INTR_IDX_UNDERRUN],
+ dpu_encoder_phys_vid_underrun_irq,
+ phys_enc);
+}
+
+static void dpu_encoder_phys_vid_irq_disable(struct dpu_encoder_phys *phys_enc)
+{
+ trace_dpu_enc_phys_vid_irq_disable(DRMID(phys_enc->parent),
+ phys_enc->hw_intf->idx - INTF_0,
+ phys_enc->vblank_refcount);
+
+ dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
+ dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
+ phys_enc->irq[INTR_IDX_UNDERRUN]);
}
static int dpu_encoder_phys_vid_get_line_count(
@@ -689,7 +692,8 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
ops->wait_for_commit_done = dpu_encoder_phys_vid_wait_for_commit_done;
ops->wait_for_tx_complete = dpu_encoder_phys_vid_wait_for_tx_complete;
- ops->irq_control = dpu_encoder_phys_vid_irq_control;
+ ops->irq_enable = dpu_encoder_phys_vid_irq_enable;
+ ops->irq_disable = dpu_encoder_phys_vid_irq_disable;
ops->prepare_for_kickoff = dpu_encoder_phys_vid_prepare_for_kickoff;
ops->handle_post_kickoff = dpu_encoder_phys_vid_handle_post_kickoff;
ops->needs_single_flush = dpu_encoder_phys_vid_needs_single_flush;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 4cd2d9e3131a..602013725484 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -511,21 +511,31 @@ static void dpu_encoder_phys_wb_done_irq(void *arg)
}
/**
- * dpu_encoder_phys_wb_irq_ctrl - irq control of WB
+ * dpu_encoder_phys_wb_irq_enable - irq control of WB
* @phys: Pointer to physical encoder
- * @enable: indicates enable or disable interrupts
*/
-static void dpu_encoder_phys_wb_irq_ctrl(
- struct dpu_encoder_phys *phys, bool enable)
+static void dpu_encoder_phys_wb_irq_enable(struct dpu_encoder_phys *phys)
{
struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
- if (enable && atomic_inc_return(&wb_enc->wbirq_refcount) == 1)
+ if (atomic_inc_return(&wb_enc->wbirq_refcount) == 1)
dpu_core_irq_register_callback(phys->dpu_kms,
- phys->irq[INTR_IDX_WB_DONE], dpu_encoder_phys_wb_done_irq, phys);
- else if (!enable &&
- atomic_dec_return(&wb_enc->wbirq_refcount) == 0)
+ phys->irq[INTR_IDX_WB_DONE],
+ dpu_encoder_phys_wb_done_irq,
+ phys);
+}
+
+/**
+ * dpu_encoder_phys_wb_irq_disable - irq control of WB
+ * @phys: Pointer to physical encoder
+ */
+static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
+{
+
+ struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
+
+ if (atomic_dec_return(&wb_enc->wbirq_refcount) == 0)
dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
}
@@ -785,7 +795,8 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
ops->trigger_start = dpu_encoder_helper_trigger_start;
ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
- ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
+ ops->irq_enable = dpu_encoder_phys_wb_irq_enable;
+ ops->irq_disable = dpu_encoder_phys_wb_irq_disable;
ops->is_valid_for_commit = dpu_encoder_phys_wb_is_valid_for_commit;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 35d03b121a0b..95ce7647ff76 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -514,24 +514,41 @@ TRACE_EVENT(dpu_enc_wait_event_timeout,
__entry->expected_time, __entry->atomic_cnt)
);
-TRACE_EVENT(dpu_enc_phys_cmd_irq_ctrl,
- TP_PROTO(uint32_t drm_id, enum dpu_pingpong pp, bool enable,
+TRACE_EVENT(dpu_enc_phys_cmd_irq_enable,
+ TP_PROTO(uint32_t drm_id, enum dpu_pingpong pp,
int refcnt),
- TP_ARGS(drm_id, pp, enable, refcnt),
+ TP_ARGS(drm_id, pp, refcnt),
+ TP_STRUCT__entry(
+ __field( uint32_t, drm_id )
+ __field( enum dpu_pingpong, pp )
+ __field( int, refcnt )
+ ),
+ TP_fast_assign(
+ __entry->drm_id = drm_id;
+ __entry->pp = pp;
+ __entry->refcnt = refcnt;
+ ),
+ TP_printk("id=%u, pp=%d, refcnt=%d", __entry->drm_id,
+ __entry->pp,
+ __entry->refcnt)
+);
+
+TRACE_EVENT(dpu_enc_phys_cmd_irq_disable,
+ TP_PROTO(uint32_t drm_id, enum dpu_pingpong pp,
+ int refcnt),
+ TP_ARGS(drm_id, pp, refcnt),
TP_STRUCT__entry(
__field( uint32_t, drm_id )
__field( enum dpu_pingpong, pp )
- __field( bool, enable )
__field( int, refcnt )
),
TP_fast_assign(
__entry->drm_id = drm_id;
__entry->pp = pp;
- __entry->enable = enable;
__entry->refcnt = refcnt;
),
- TP_printk("id=%u, pp=%d, enable=%s, refcnt=%d", __entry->drm_id,
- __entry->pp, __entry->enable ? "true" : "false",
+ TP_printk("id=%u, pp=%d, refcnt=%d", __entry->drm_id,
+ __entry->pp,
__entry->refcnt)
);
@@ -592,24 +609,41 @@ TRACE_EVENT(dpu_enc_phys_vid_post_kickoff,
TP_printk("id=%u, intf_idx=%d", __entry->drm_id, __entry->intf_idx)
);
-TRACE_EVENT(dpu_enc_phys_vid_irq_ctrl,
- TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx, bool enable,
+TRACE_EVENT(dpu_enc_phys_vid_irq_enable,
+ TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx,
int refcnt),
- TP_ARGS(drm_id, intf_idx, enable, refcnt),
+ TP_ARGS(drm_id, intf_idx, refcnt),
+ TP_STRUCT__entry(
+ __field( uint32_t, drm_id )
+ __field( enum dpu_intf, intf_idx )
+ __field( int, refcnt )
+ ),
+ TP_fast_assign(
+ __entry->drm_id = drm_id;
+ __entry->intf_idx = intf_idx;
+ __entry->refcnt = refcnt;
+ ),
+ TP_printk("id=%u, intf_idx=%d refcnt=%d", __entry->drm_id,
+ __entry->intf_idx,
+ __entry->drm_id)
+);
+
+TRACE_EVENT(dpu_enc_phys_vid_irq_disable,
+ TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx,
+ int refcnt),
+ TP_ARGS(drm_id, intf_idx, refcnt),
TP_STRUCT__entry(
__field( uint32_t, drm_id )
__field( enum dpu_intf, intf_idx )
- __field( bool, enable )
__field( int, refcnt )
),
TP_fast_assign(
__entry->drm_id = drm_id;
__entry->intf_idx = intf_idx;
- __entry->enable = enable;
__entry->refcnt = refcnt;
),
- TP_printk("id=%u, intf_idx=%d enable=%s refcnt=%d", __entry->drm_id,
- __entry->intf_idx, __entry->enable ? "true" : "false",
+ TP_printk("id=%u, intf_idx=%d refcnt=%d", __entry->drm_id,
+ __entry->intf_idx,
__entry->drm_id)
);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 1/5] drm/msm/dpu: split irq_control into irq_enable and _disable
@ 2023-12-25 13:08 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel,
Stephen Boyd, David Airlie
The single helper for both enable and disable cases is too complicated,
especially if we start adding more code to these helpers. Split it into
irq_enable and irq_disable cases.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 36 +++++++---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 6 +-
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 65 ++++++++++---------
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 46 +++++++------
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 29 ++++++---
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 62 ++++++++++++++----
6 files changed, 158 insertions(+), 86 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f2b82ca5efb3..5ab32ace7707 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -714,7 +714,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
}
}
-static void _dpu_encoder_irq_control(struct drm_encoder *drm_enc, bool enable)
+static void _dpu_encoder_irq_enable(struct drm_encoder *drm_enc)
{
struct dpu_encoder_virt *dpu_enc;
int i;
@@ -726,14 +726,32 @@ static void _dpu_encoder_irq_control(struct drm_encoder *drm_enc, bool enable)
dpu_enc = to_dpu_encoder_virt(drm_enc);
- DPU_DEBUG_ENC(dpu_enc, "enable:%d\n", enable);
+ DPU_DEBUG_ENC(dpu_enc, "\n");
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
- if (phys->ops.irq_control)
- phys->ops.irq_control(phys, enable);
+ phys->ops.irq_enable(phys);
+ }
+}
+
+static void _dpu_encoder_irq_disable(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+ int i;
+
+ if (!drm_enc) {
+ DPU_ERROR("invalid encoder\n");
+ return;
}
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ DPU_DEBUG_ENC(dpu_enc, "\n");
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+ phys->ops.irq_disable(phys);
+ }
}
static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
@@ -759,11 +777,11 @@ static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
pm_runtime_get_sync(&dpu_kms->pdev->dev);
/* enable all the irq */
- _dpu_encoder_irq_control(drm_enc, true);
+ _dpu_encoder_irq_enable(drm_enc);
} else {
/* disable all the irq */
- _dpu_encoder_irq_control(drm_enc, false);
+ _dpu_encoder_irq_disable(drm_enc);
/* disable DPU core clks */
pm_runtime_put_sync(&dpu_kms->pdev->dev);
@@ -824,7 +842,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
}
if (is_vid_mode && dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE)
- _dpu_encoder_irq_control(drm_enc, true);
+ _dpu_encoder_irq_enable(drm_enc);
else
_dpu_encoder_resource_control_helper(drm_enc, true);
@@ -879,7 +897,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
if (is_vid_mode &&
dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE) {
- _dpu_encoder_irq_control(drm_enc, true);
+ _dpu_encoder_irq_enable(drm_enc);
}
/* skip if is already OFF or IDLE, resources are off already */
else if (dpu_enc->rc_state == DPU_ENC_RC_STATE_OFF ||
@@ -954,7 +972,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
}
if (is_vid_mode)
- _dpu_encoder_irq_control(drm_enc, false);
+ _dpu_encoder_irq_disable(drm_enc);
else
_dpu_encoder_resource_control_helper(drm_enc, false);
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 993f26343331..8c5b0c853572 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -85,7 +85,8 @@ struct dpu_encoder_phys;
* @handle_post_kickoff: Do any work necessary post-kickoff work
* @trigger_start: Process start event on physical encoder
* @needs_single_flush: Whether encoder slaves need to be flushed
- * @irq_control: Handler to enable/disable all the encoder IRQs
+ * @irq_enable: Handler to enable all the encoder IRQs
+ * @irq_disable: Handler to disable all the encoder IRQs
* @prepare_idle_pc: phys encoder can update the vsync_enable status
* on idle power collapse prepare
* @restore: Restore all the encoder configs.
@@ -110,7 +111,8 @@ struct dpu_encoder_phys_ops {
void (*handle_post_kickoff)(struct dpu_encoder_phys *phys_enc);
void (*trigger_start)(struct dpu_encoder_phys *phys_enc);
bool (*needs_single_flush)(struct dpu_encoder_phys *phys_enc);
- void (*irq_control)(struct dpu_encoder_phys *phys, bool enable);
+ void (*irq_enable)(struct dpu_encoder_phys *phys);
+ void (*irq_disable)(struct dpu_encoder_phys *phys);
void (*prepare_idle_pc)(struct dpu_encoder_phys *phys_enc);
void (*restore)(struct dpu_encoder_phys *phys);
int (*get_line_count)(struct dpu_encoder_phys *phys);
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 a301e2833177..de826f9745e5 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
@@ -291,40 +291,42 @@ static int dpu_encoder_phys_cmd_control_vblank_irq(
return ret;
}
-static void dpu_encoder_phys_cmd_irq_control(struct dpu_encoder_phys *phys_enc,
- bool enable)
+static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
{
- trace_dpu_enc_phys_cmd_irq_ctrl(DRMID(phys_enc->parent),
- phys_enc->hw_pp->idx - PINGPONG_0,
- enable, phys_enc->vblank_refcount);
-
- if (enable) {
- dpu_core_irq_register_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_PINGPONG],
- dpu_encoder_phys_cmd_pp_tx_done_irq,
- phys_enc);
+ trace_dpu_enc_phys_cmd_irq_enable(DRMID(phys_enc->parent),
+ phys_enc->hw_pp->idx - PINGPONG_0,
+ phys_enc->vblank_refcount);
+
+ dpu_core_irq_register_callback(phys_enc->dpu_kms,
+ phys_enc->irq[INTR_IDX_PINGPONG],
+ dpu_encoder_phys_cmd_pp_tx_done_irq,
+ phys_enc);
+ dpu_core_irq_register_callback(phys_enc->dpu_kms,
+ phys_enc->irq[INTR_IDX_UNDERRUN],
+ dpu_encoder_phys_cmd_underrun_irq,
+ phys_enc);
+ dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
+
+ if (dpu_encoder_phys_cmd_is_master(phys_enc))
dpu_core_irq_register_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_UNDERRUN],
- dpu_encoder_phys_cmd_underrun_irq,
- phys_enc);
- dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
-
- if (dpu_encoder_phys_cmd_is_master(phys_enc))
- dpu_core_irq_register_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_CTL_START],
- dpu_encoder_phys_cmd_ctl_start_irq,
- phys_enc);
- } else {
- if (dpu_encoder_phys_cmd_is_master(phys_enc))
- dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_CTL_START]);
+ phys_enc->irq[INTR_IDX_CTL_START],
+ dpu_encoder_phys_cmd_ctl_start_irq,
+ phys_enc);
+}
+static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
+{
+ trace_dpu_enc_phys_cmd_irq_disable(DRMID(phys_enc->parent),
+ phys_enc->hw_pp->idx - PINGPONG_0,
+ phys_enc->vblank_refcount);
+
+ if (dpu_encoder_phys_cmd_is_master(phys_enc))
dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_UNDERRUN]);
- dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
- dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_PINGPONG]);
- }
+ phys_enc->irq[INTR_IDX_CTL_START]);
+
+ dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
+ dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
+ dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
}
static void dpu_encoder_phys_cmd_tearcheck_config(
@@ -713,7 +715,8 @@ static void dpu_encoder_phys_cmd_init_ops(
ops->wait_for_tx_complete = dpu_encoder_phys_cmd_wait_for_tx_complete;
ops->trigger_start = dpu_encoder_phys_cmd_trigger_start;
ops->needs_single_flush = dpu_encoder_phys_cmd_needs_single_flush;
- ops->irq_control = dpu_encoder_phys_cmd_irq_control;
+ ops->irq_enable = dpu_encoder_phys_cmd_irq_enable;
+ ops->irq_disable = dpu_encoder_phys_cmd_irq_disable;
ops->restore = dpu_encoder_phys_cmd_enable_helper;
ops->prepare_idle_pc = dpu_encoder_phys_cmd_prepare_idle_pc;
ops->handle_post_kickoff = dpu_encoder_phys_cmd_handle_post_kickoff;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index d0f56c5c4cce..9743ec43d862 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -615,30 +615,33 @@ static void dpu_encoder_phys_vid_handle_post_kickoff(
}
}
-static void dpu_encoder_phys_vid_irq_control(struct dpu_encoder_phys *phys_enc,
- bool enable)
+static void dpu_encoder_phys_vid_irq_enable(struct dpu_encoder_phys *phys_enc)
{
int ret;
- trace_dpu_enc_phys_vid_irq_ctrl(DRMID(phys_enc->parent),
- phys_enc->hw_intf->idx - INTF_0,
- enable,
- phys_enc->vblank_refcount);
+ trace_dpu_enc_phys_vid_irq_enable(DRMID(phys_enc->parent),
+ phys_enc->hw_intf->idx - INTF_0,
+ phys_enc->vblank_refcount);
- if (enable) {
- ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
- if (WARN_ON(ret))
- return;
-
- dpu_core_irq_register_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_UNDERRUN],
- dpu_encoder_phys_vid_underrun_irq,
- phys_enc);
- } else {
- dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
- dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
- phys_enc->irq[INTR_IDX_UNDERRUN]);
- }
+ ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
+ if (WARN_ON(ret))
+ return;
+
+ dpu_core_irq_register_callback(phys_enc->dpu_kms,
+ phys_enc->irq[INTR_IDX_UNDERRUN],
+ dpu_encoder_phys_vid_underrun_irq,
+ phys_enc);
+}
+
+static void dpu_encoder_phys_vid_irq_disable(struct dpu_encoder_phys *phys_enc)
+{
+ trace_dpu_enc_phys_vid_irq_disable(DRMID(phys_enc->parent),
+ phys_enc->hw_intf->idx - INTF_0,
+ phys_enc->vblank_refcount);
+
+ dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
+ dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
+ phys_enc->irq[INTR_IDX_UNDERRUN]);
}
static int dpu_encoder_phys_vid_get_line_count(
@@ -689,7 +692,8 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
ops->wait_for_commit_done = dpu_encoder_phys_vid_wait_for_commit_done;
ops->wait_for_tx_complete = dpu_encoder_phys_vid_wait_for_tx_complete;
- ops->irq_control = dpu_encoder_phys_vid_irq_control;
+ ops->irq_enable = dpu_encoder_phys_vid_irq_enable;
+ ops->irq_disable = dpu_encoder_phys_vid_irq_disable;
ops->prepare_for_kickoff = dpu_encoder_phys_vid_prepare_for_kickoff;
ops->handle_post_kickoff = dpu_encoder_phys_vid_handle_post_kickoff;
ops->needs_single_flush = dpu_encoder_phys_vid_needs_single_flush;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 4cd2d9e3131a..602013725484 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -511,21 +511,31 @@ static void dpu_encoder_phys_wb_done_irq(void *arg)
}
/**
- * dpu_encoder_phys_wb_irq_ctrl - irq control of WB
+ * dpu_encoder_phys_wb_irq_enable - irq control of WB
* @phys: Pointer to physical encoder
- * @enable: indicates enable or disable interrupts
*/
-static void dpu_encoder_phys_wb_irq_ctrl(
- struct dpu_encoder_phys *phys, bool enable)
+static void dpu_encoder_phys_wb_irq_enable(struct dpu_encoder_phys *phys)
{
struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
- if (enable && atomic_inc_return(&wb_enc->wbirq_refcount) == 1)
+ if (atomic_inc_return(&wb_enc->wbirq_refcount) == 1)
dpu_core_irq_register_callback(phys->dpu_kms,
- phys->irq[INTR_IDX_WB_DONE], dpu_encoder_phys_wb_done_irq, phys);
- else if (!enable &&
- atomic_dec_return(&wb_enc->wbirq_refcount) == 0)
+ phys->irq[INTR_IDX_WB_DONE],
+ dpu_encoder_phys_wb_done_irq,
+ phys);
+}
+
+/**
+ * dpu_encoder_phys_wb_irq_disable - irq control of WB
+ * @phys: Pointer to physical encoder
+ */
+static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
+{
+
+ struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
+
+ if (atomic_dec_return(&wb_enc->wbirq_refcount) == 0)
dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
}
@@ -785,7 +795,8 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
ops->trigger_start = dpu_encoder_helper_trigger_start;
ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
- ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
+ ops->irq_enable = dpu_encoder_phys_wb_irq_enable;
+ ops->irq_disable = dpu_encoder_phys_wb_irq_disable;
ops->is_valid_for_commit = dpu_encoder_phys_wb_is_valid_for_commit;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 35d03b121a0b..95ce7647ff76 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -514,24 +514,41 @@ TRACE_EVENT(dpu_enc_wait_event_timeout,
__entry->expected_time, __entry->atomic_cnt)
);
-TRACE_EVENT(dpu_enc_phys_cmd_irq_ctrl,
- TP_PROTO(uint32_t drm_id, enum dpu_pingpong pp, bool enable,
+TRACE_EVENT(dpu_enc_phys_cmd_irq_enable,
+ TP_PROTO(uint32_t drm_id, enum dpu_pingpong pp,
int refcnt),
- TP_ARGS(drm_id, pp, enable, refcnt),
+ TP_ARGS(drm_id, pp, refcnt),
+ TP_STRUCT__entry(
+ __field( uint32_t, drm_id )
+ __field( enum dpu_pingpong, pp )
+ __field( int, refcnt )
+ ),
+ TP_fast_assign(
+ __entry->drm_id = drm_id;
+ __entry->pp = pp;
+ __entry->refcnt = refcnt;
+ ),
+ TP_printk("id=%u, pp=%d, refcnt=%d", __entry->drm_id,
+ __entry->pp,
+ __entry->refcnt)
+);
+
+TRACE_EVENT(dpu_enc_phys_cmd_irq_disable,
+ TP_PROTO(uint32_t drm_id, enum dpu_pingpong pp,
+ int refcnt),
+ TP_ARGS(drm_id, pp, refcnt),
TP_STRUCT__entry(
__field( uint32_t, drm_id )
__field( enum dpu_pingpong, pp )
- __field( bool, enable )
__field( int, refcnt )
),
TP_fast_assign(
__entry->drm_id = drm_id;
__entry->pp = pp;
- __entry->enable = enable;
__entry->refcnt = refcnt;
),
- TP_printk("id=%u, pp=%d, enable=%s, refcnt=%d", __entry->drm_id,
- __entry->pp, __entry->enable ? "true" : "false",
+ TP_printk("id=%u, pp=%d, refcnt=%d", __entry->drm_id,
+ __entry->pp,
__entry->refcnt)
);
@@ -592,24 +609,41 @@ TRACE_EVENT(dpu_enc_phys_vid_post_kickoff,
TP_printk("id=%u, intf_idx=%d", __entry->drm_id, __entry->intf_idx)
);
-TRACE_EVENT(dpu_enc_phys_vid_irq_ctrl,
- TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx, bool enable,
+TRACE_EVENT(dpu_enc_phys_vid_irq_enable,
+ TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx,
int refcnt),
- TP_ARGS(drm_id, intf_idx, enable, refcnt),
+ TP_ARGS(drm_id, intf_idx, refcnt),
+ TP_STRUCT__entry(
+ __field( uint32_t, drm_id )
+ __field( enum dpu_intf, intf_idx )
+ __field( int, refcnt )
+ ),
+ TP_fast_assign(
+ __entry->drm_id = drm_id;
+ __entry->intf_idx = intf_idx;
+ __entry->refcnt = refcnt;
+ ),
+ TP_printk("id=%u, intf_idx=%d refcnt=%d", __entry->drm_id,
+ __entry->intf_idx,
+ __entry->drm_id)
+);
+
+TRACE_EVENT(dpu_enc_phys_vid_irq_disable,
+ TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx,
+ int refcnt),
+ TP_ARGS(drm_id, intf_idx, refcnt),
TP_STRUCT__entry(
__field( uint32_t, drm_id )
__field( enum dpu_intf, intf_idx )
- __field( bool, enable )
__field( int, refcnt )
),
TP_fast_assign(
__entry->drm_id = drm_id;
__entry->intf_idx = intf_idx;
- __entry->enable = enable;
__entry->refcnt = refcnt;
),
- TP_printk("id=%u, intf_idx=%d enable=%s refcnt=%d", __entry->drm_id,
- __entry->intf_idx, __entry->enable ? "true" : "false",
+ TP_printk("id=%u, intf_idx=%d refcnt=%d", __entry->drm_id,
+ __entry->intf_idx,
__entry->drm_id)
);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 2/5] drm/msm/dpu: split _dpu_encoder_resource_control_helper()
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2023-12-25 13:08 ` Dmitry Baryshkov
-1 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
Follow the _dpu_encoder_irq_control() change and split the
_dpu_encoder_resource_control_helper() into enable and disable parts.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +++++++++++++--------
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 12 ++++--
2 files changed, 37 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5ab32ace7707..7b79fa3a79a3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -754,8 +754,7 @@ static void _dpu_encoder_irq_disable(struct drm_encoder *drm_enc)
}
}
-static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
- bool enable)
+static void _dpu_encoder_resource_enable(struct drm_encoder *drm_enc)
{
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms;
@@ -765,28 +764,42 @@ static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
priv = drm_enc->dev->dev_private;
dpu_kms = to_dpu_kms(priv->kms);
- trace_dpu_enc_rc_helper(DRMID(drm_enc), enable);
+ trace_dpu_enc_rc_enable(DRMID(drm_enc));
if (!dpu_enc->cur_master) {
DPU_ERROR("encoder master not set\n");
return;
}
- if (enable) {
- /* enable DPU core clks */
- pm_runtime_get_sync(&dpu_kms->pdev->dev);
+ /* enable DPU core clks */
+ pm_runtime_get_sync(&dpu_kms->pdev->dev);
- /* enable all the irq */
- _dpu_encoder_irq_enable(drm_enc);
+ /* enable all the irq */
+ _dpu_encoder_irq_enable(drm_enc);
+}
- } else {
- /* disable all the irq */
- _dpu_encoder_irq_disable(drm_enc);
+static void _dpu_encoder_resource_disable(struct drm_encoder *drm_enc)
+{
+ struct msm_drm_private *priv;
+ struct dpu_kms *dpu_kms;
+ struct dpu_encoder_virt *dpu_enc;
- /* disable DPU core clks */
- pm_runtime_put_sync(&dpu_kms->pdev->dev);
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ priv = drm_enc->dev->dev_private;
+ dpu_kms = to_dpu_kms(priv->kms);
+
+ trace_dpu_enc_rc_disable(DRMID(drm_enc));
+
+ if (!dpu_enc->cur_master) {
+ DPU_ERROR("encoder master not set\n");
+ return;
}
+ /* disable all the irq */
+ _dpu_encoder_irq_disable(drm_enc);
+
+ /* disable DPU core clks */
+ pm_runtime_put_sync(&dpu_kms->pdev->dev);
}
static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
@@ -844,7 +857,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
if (is_vid_mode && dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE)
_dpu_encoder_irq_enable(drm_enc);
else
- _dpu_encoder_resource_control_helper(drm_enc, true);
+ _dpu_encoder_resource_enable(drm_enc);
dpu_enc->rc_state = DPU_ENC_RC_STATE_ON;
@@ -939,7 +952,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
* and in IDLE state the resources are already disabled
*/
if (dpu_enc->rc_state == DPU_ENC_RC_STATE_PRE_OFF)
- _dpu_encoder_resource_control_helper(drm_enc, false);
+ _dpu_encoder_resource_disable(drm_enc);
dpu_enc->rc_state = DPU_ENC_RC_STATE_OFF;
@@ -974,7 +987,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
if (is_vid_mode)
_dpu_encoder_irq_disable(drm_enc);
else
- _dpu_encoder_resource_control_helper(drm_enc, false);
+ _dpu_encoder_resource_disable(drm_enc);
dpu_enc->rc_state = DPU_ENC_RC_STATE_IDLE;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 95ce7647ff76..bd92fb2979aa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -273,6 +273,14 @@ DEFINE_EVENT(dpu_drm_obj_template, dpu_crtc_runtime_resume,
TP_PROTO(uint32_t drm_id),
TP_ARGS(drm_id)
);
+DEFINE_EVENT(dpu_drm_obj_template, dpu_enc_rc_enable,
+ TP_PROTO(uint32_t drm_id),
+ TP_ARGS(drm_id)
+);
+DEFINE_EVENT(dpu_drm_obj_template, dpu_enc_rc_disable,
+ TP_PROTO(uint32_t drm_id),
+ TP_ARGS(drm_id)
+);
TRACE_EVENT(dpu_enc_enable,
TP_PROTO(uint32_t drm_id, int hdisplay, int vdisplay),
@@ -342,10 +350,6 @@ DECLARE_EVENT_CLASS(dpu_enc_id_enable_template,
TP_printk("id=%u, enable=%s",
__entry->drm_id, __entry->enable ? "true" : "false")
);
-DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_rc_helper,
- TP_PROTO(uint32_t drm_id, bool enable),
- TP_ARGS(drm_id, enable)
-);
DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_vblank_cb,
TP_PROTO(uint32_t drm_id, bool enable),
TP_ARGS(drm_id, enable)
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 2/5] drm/msm/dpu: split _dpu_encoder_resource_control_helper()
@ 2023-12-25 13:08 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel,
Stephen Boyd, David Airlie
Follow the _dpu_encoder_irq_control() change and split the
_dpu_encoder_resource_control_helper() into enable and disable parts.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +++++++++++++--------
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 12 ++++--
2 files changed, 37 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5ab32ace7707..7b79fa3a79a3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -754,8 +754,7 @@ static void _dpu_encoder_irq_disable(struct drm_encoder *drm_enc)
}
}
-static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
- bool enable)
+static void _dpu_encoder_resource_enable(struct drm_encoder *drm_enc)
{
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms;
@@ -765,28 +764,42 @@ static void _dpu_encoder_resource_control_helper(struct drm_encoder *drm_enc,
priv = drm_enc->dev->dev_private;
dpu_kms = to_dpu_kms(priv->kms);
- trace_dpu_enc_rc_helper(DRMID(drm_enc), enable);
+ trace_dpu_enc_rc_enable(DRMID(drm_enc));
if (!dpu_enc->cur_master) {
DPU_ERROR("encoder master not set\n");
return;
}
- if (enable) {
- /* enable DPU core clks */
- pm_runtime_get_sync(&dpu_kms->pdev->dev);
+ /* enable DPU core clks */
+ pm_runtime_get_sync(&dpu_kms->pdev->dev);
- /* enable all the irq */
- _dpu_encoder_irq_enable(drm_enc);
+ /* enable all the irq */
+ _dpu_encoder_irq_enable(drm_enc);
+}
- } else {
- /* disable all the irq */
- _dpu_encoder_irq_disable(drm_enc);
+static void _dpu_encoder_resource_disable(struct drm_encoder *drm_enc)
+{
+ struct msm_drm_private *priv;
+ struct dpu_kms *dpu_kms;
+ struct dpu_encoder_virt *dpu_enc;
- /* disable DPU core clks */
- pm_runtime_put_sync(&dpu_kms->pdev->dev);
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ priv = drm_enc->dev->dev_private;
+ dpu_kms = to_dpu_kms(priv->kms);
+
+ trace_dpu_enc_rc_disable(DRMID(drm_enc));
+
+ if (!dpu_enc->cur_master) {
+ DPU_ERROR("encoder master not set\n");
+ return;
}
+ /* disable all the irq */
+ _dpu_encoder_irq_disable(drm_enc);
+
+ /* disable DPU core clks */
+ pm_runtime_put_sync(&dpu_kms->pdev->dev);
}
static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
@@ -844,7 +857,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
if (is_vid_mode && dpu_enc->rc_state == DPU_ENC_RC_STATE_IDLE)
_dpu_encoder_irq_enable(drm_enc);
else
- _dpu_encoder_resource_control_helper(drm_enc, true);
+ _dpu_encoder_resource_enable(drm_enc);
dpu_enc->rc_state = DPU_ENC_RC_STATE_ON;
@@ -939,7 +952,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
* and in IDLE state the resources are already disabled
*/
if (dpu_enc->rc_state == DPU_ENC_RC_STATE_PRE_OFF)
- _dpu_encoder_resource_control_helper(drm_enc, false);
+ _dpu_encoder_resource_disable(drm_enc);
dpu_enc->rc_state = DPU_ENC_RC_STATE_OFF;
@@ -974,7 +987,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
if (is_vid_mode)
_dpu_encoder_irq_disable(drm_enc);
else
- _dpu_encoder_resource_control_helper(drm_enc, false);
+ _dpu_encoder_resource_disable(drm_enc);
dpu_enc->rc_state = DPU_ENC_RC_STATE_IDLE;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 95ce7647ff76..bd92fb2979aa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -273,6 +273,14 @@ DEFINE_EVENT(dpu_drm_obj_template, dpu_crtc_runtime_resume,
TP_PROTO(uint32_t drm_id),
TP_ARGS(drm_id)
);
+DEFINE_EVENT(dpu_drm_obj_template, dpu_enc_rc_enable,
+ TP_PROTO(uint32_t drm_id),
+ TP_ARGS(drm_id)
+);
+DEFINE_EVENT(dpu_drm_obj_template, dpu_enc_rc_disable,
+ TP_PROTO(uint32_t drm_id),
+ TP_ARGS(drm_id)
+);
TRACE_EVENT(dpu_enc_enable,
TP_PROTO(uint32_t drm_id, int hdisplay, int vdisplay),
@@ -342,10 +350,6 @@ DECLARE_EVENT_CLASS(dpu_enc_id_enable_template,
TP_printk("id=%u, enable=%s",
__entry->drm_id, __entry->enable ? "true" : "false")
);
-DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_rc_helper,
- TP_PROTO(uint32_t drm_id, bool enable),
- TP_ARGS(drm_id, enable)
-);
DEFINE_EVENT(dpu_enc_id_enable_template, dpu_enc_vblank_cb,
TP_PROTO(uint32_t drm_id, bool enable),
TP_ARGS(drm_id, enable)
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 3/5] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2023-12-25 13:08 ` Dmitry Baryshkov
-1 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
The atomic_mode_set() callback only sets the phys_enc's IRQ data. As the
INTF and WB are statically allocated to each encoder/phys_enc, drop the
atomic_mode_set callback and set the IRQs during encoder init.
For the CMD panel usecase some of IRQ indexes depend on the selected
resources. Move setting them to the irq_enable() callback.
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Tested-by: Abhinav Kumar <quic_abhinavk@quicinc.com> # sc7280
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 --
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 ---
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 ++++++++-----------
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 ++------
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 11 +------
5 files changed, 17 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 7b79fa3a79a3..5022d0b9b4b4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1152,8 +1152,6 @@ 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 8c5b0c853572..7eb8bdfe6bbe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -69,8 +69,6 @@ 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.
* @atomic_check: DRM Call. Atomic check new DRM state.
@@ -96,9 +94,6 @@ 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 (*atomic_check)(struct dpu_encoder_phys *encoder,
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 de826f9745e5..fc1d5736d7fc 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,23 +142,6 @@ 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)
{
@@ -297,6 +280,14 @@ 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,
@@ -327,6 +318,10 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
+
+ phys_enc->irq[INTR_IDX_CTL_START] = 0;
+ phys_enc->irq[INTR_IDX_PINGPONG] = 0;
+ phys_enc->irq[INTR_IDX_RDPTR] = 0;
}
static void dpu_encoder_phys_cmd_tearcheck_config(
@@ -706,7 +701,6 @@ static void dpu_encoder_phys_cmd_init_ops(
struct dpu_encoder_phys_ops *ops)
{
ops->is_master = dpu_encoder_phys_cmd_is_master;
- ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
ops->enable = dpu_encoder_phys_cmd_enable;
ops->disable = dpu_encoder_phys_cmd_disable;
ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq;
@@ -745,6 +739,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(struct drm_device *dev,
dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
phys_enc->intf_mode = INTF_MODE_CMD;
+ phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
+
cmd_enc->stream_sel = 0;
if (!phys_enc->hw_intf) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 9743ec43d862..bc4ac7e2e4ba 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -349,16 +349,6 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
return phys_enc->split_role != ENC_ROLE_SOLO;
}
-static void dpu_encoder_phys_vid_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_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
-
- phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
-}
-
static int dpu_encoder_phys_vid_control_vblank_irq(
struct dpu_encoder_phys *phys_enc,
bool enable)
@@ -686,7 +676,6 @@ static int dpu_encoder_phys_vid_get_frame_count(
static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
{
ops->is_master = dpu_encoder_phys_vid_is_master;
- ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
ops->enable = dpu_encoder_phys_vid_enable;
ops->disable = dpu_encoder_phys_vid_disable;
ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
@@ -725,6 +714,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(struct drm_device *dev,
dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
phys_enc->intf_mode = INTF_MODE_VIDEO;
+ phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
+ phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 602013725484..a0a28230fc31 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -539,15 +539,6 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
}
-static void dpu_encoder_phys_wb_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_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
-}
-
static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
struct dpu_encoder_phys *phys_enc)
{
@@ -784,7 +775,6 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
{
ops->is_master = dpu_encoder_phys_wb_is_master;
- ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
ops->enable = dpu_encoder_phys_wb_enable;
ops->disable = dpu_encoder_phys_wb_disable;
ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
@@ -831,6 +821,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(struct drm_device *dev,
dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
phys_enc->intf_mode = INTF_MODE_WB_LINE;
+ phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
atomic_set(&wb_enc->wbirq_refcount, 0);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 3/5] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set
@ 2023-12-25 13:08 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel,
Stephen Boyd, David Airlie
The atomic_mode_set() callback only sets the phys_enc's IRQ data. As the
INTF and WB are statically allocated to each encoder/phys_enc, drop the
atomic_mode_set callback and set the IRQs during encoder init.
For the CMD panel usecase some of IRQ indexes depend on the selected
resources. Move setting them to the irq_enable() callback.
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Tested-by: Abhinav Kumar <quic_abhinavk@quicinc.com> # sc7280
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 --
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 ---
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 ++++++++-----------
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 ++------
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 11 +------
5 files changed, 17 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 7b79fa3a79a3..5022d0b9b4b4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1152,8 +1152,6 @@ 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 8c5b0c853572..7eb8bdfe6bbe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -69,8 +69,6 @@ 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.
* @atomic_check: DRM Call. Atomic check new DRM state.
@@ -96,9 +94,6 @@ 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 (*atomic_check)(struct dpu_encoder_phys *encoder,
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 de826f9745e5..fc1d5736d7fc 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,23 +142,6 @@ 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)
{
@@ -297,6 +280,14 @@ 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,
@@ -327,6 +318,10 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
+
+ phys_enc->irq[INTR_IDX_CTL_START] = 0;
+ phys_enc->irq[INTR_IDX_PINGPONG] = 0;
+ phys_enc->irq[INTR_IDX_RDPTR] = 0;
}
static void dpu_encoder_phys_cmd_tearcheck_config(
@@ -706,7 +701,6 @@ static void dpu_encoder_phys_cmd_init_ops(
struct dpu_encoder_phys_ops *ops)
{
ops->is_master = dpu_encoder_phys_cmd_is_master;
- ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
ops->enable = dpu_encoder_phys_cmd_enable;
ops->disable = dpu_encoder_phys_cmd_disable;
ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq;
@@ -745,6 +739,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(struct drm_device *dev,
dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
phys_enc->intf_mode = INTF_MODE_CMD;
+ phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
+
cmd_enc->stream_sel = 0;
if (!phys_enc->hw_intf) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 9743ec43d862..bc4ac7e2e4ba 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -349,16 +349,6 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
return phys_enc->split_role != ENC_ROLE_SOLO;
}
-static void dpu_encoder_phys_vid_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_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
-
- phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
-}
-
static int dpu_encoder_phys_vid_control_vblank_irq(
struct dpu_encoder_phys *phys_enc,
bool enable)
@@ -686,7 +676,6 @@ static int dpu_encoder_phys_vid_get_frame_count(
static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
{
ops->is_master = dpu_encoder_phys_vid_is_master;
- ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
ops->enable = dpu_encoder_phys_vid_enable;
ops->disable = dpu_encoder_phys_vid_disable;
ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
@@ -725,6 +714,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(struct drm_device *dev,
dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
phys_enc->intf_mode = INTF_MODE_VIDEO;
+ phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
+ phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 602013725484..a0a28230fc31 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -539,15 +539,6 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
}
-static void dpu_encoder_phys_wb_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_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
-}
-
static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
struct dpu_encoder_phys *phys_enc)
{
@@ -784,7 +775,6 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
{
ops->is_master = dpu_encoder_phys_wb_is_master;
- ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
ops->enable = dpu_encoder_phys_wb_enable;
ops->disable = dpu_encoder_phys_wb_disable;
ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
@@ -831,6 +821,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(struct drm_device *dev,
dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
phys_enc->intf_mode = INTF_MODE_WB_LINE;
+ phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
atomic_set(&wb_enc->wbirq_refcount, 0);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2023-12-25 13:08 ` Dmitry Baryshkov
-1 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
Move corresponding checks to drm_writeback_connector's implementation
and drop the dpu_encoder_phys_wb_atomic_check() function.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
4 files changed, 64 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index a0a28230fc31..8220cd920e6f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -354,59 +354,6 @@ static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
}
}
-/**
- * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
- * @phys_enc: Pointer to physical encoder
- * @crtc_state: Pointer to CRTC atomic state
- * @conn_state: Pointer to connector atomic state
- */
-static int dpu_encoder_phys_wb_atomic_check(
- struct dpu_encoder_phys *phys_enc,
- struct drm_crtc_state *crtc_state,
- struct drm_connector_state *conn_state)
-{
- struct drm_framebuffer *fb;
- const struct drm_display_mode *mode = &crtc_state->mode;
-
- DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
- phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
-
- if (!conn_state || !conn_state->connector) {
- DPU_ERROR("invalid connector state\n");
- return -EINVAL;
- } else if (conn_state->connector->status !=
- connector_status_connected) {
- DPU_ERROR("connector not connected %d\n",
- conn_state->connector->status);
- return -EINVAL;
- }
-
- if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
- return 0;
-
- fb = conn_state->writeback_job->fb;
-
- DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
- fb->width, fb->height);
-
- if (fb->width != mode->hdisplay) {
- DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
- mode->hdisplay);
- return -EINVAL;
- } else if (fb->height != mode->vdisplay) {
- DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
- mode->vdisplay);
- return -EINVAL;
- } else if (fb->width > phys_enc->hw_wb->caps->maxlinewidth) {
- DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
- fb->width, phys_enc->hw_wb->caps->maxlinewidth);
- return -EINVAL;
- }
-
- return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
-}
-
-
/**
* _dpu_encoder_phys_wb_update_flush - flush hardware update
* @phys_enc: Pointer to physical encoder
@@ -777,7 +724,6 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
ops->is_master = dpu_encoder_phys_wb_is_master;
ops->enable = dpu_encoder_phys_wb_enable;
ops->disable = dpu_encoder_phys_wb_disable;
- ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done;
ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 723cc1d82143..48728be27e15 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -630,23 +630,26 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
{
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
+ const enum dpu_wb wb_idx = WB_2;
+ u32 maxlinewidth;
int rc;
memset(&info, 0, sizeof(info));
info.num_of_h_tiles = 1;
/* use only WB idx 2 instance for DPU */
- info.h_tile_instance[0] = WB_2;
+ info.h_tile_instance[0] = wb_idx;
info.intf_type = INTF_WB;
+ maxlinewidth = dpu_rm_get_wb(&dpu_kms->rm, info.h_tile_instance[0])->caps->maxlinewidth;
+
encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
if (IS_ERR(encoder)) {
DPU_ERROR("encoder init failed for dsi display\n");
return PTR_ERR(encoder);
}
- rc = dpu_writeback_init(dev, encoder, wb_formats,
- n_formats);
+ rc = dpu_writeback_init(dev, encoder, wb_formats, n_formats, maxlinewidth);
if (rc) {
DPU_ERROR("dpu_writeback_init, rc = %d\n", rc);
return rc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
index 2a5a68366582..232b5f410de8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
@@ -4,6 +4,7 @@
*/
#include <drm/drm_edid.h>
+#include <drm/drm_framebuffer.h>
#include "dpu_writeback.h"
@@ -24,6 +25,57 @@ static int dpu_wb_conn_get_modes(struct drm_connector *connector)
dev->mode_config.max_height);
}
+static int dpu_wb_conn_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+ struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
+ struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(wb_conn);
+ struct drm_connector_state *conn_state =
+ drm_atomic_get_new_connector_state(state, connector);
+ struct drm_crtc *crtc = conn_state->crtc;
+ struct drm_crtc_state *crtc_state;
+ const struct drm_display_mode *mode;
+ struct drm_framebuffer *fb;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ mode = &crtc_state->mode;
+
+ DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
+ connector->base.id, mode->name, mode->hdisplay, mode->vdisplay);
+
+ if (!conn_state || !conn_state->connector) {
+ DPU_ERROR("invalid connector state\n");
+ return -EINVAL;
+ } else if (conn_state->connector->status != connector_status_connected) {
+ DPU_ERROR("connector not connected %d\n", conn_state->connector->status);
+ return -EINVAL;
+ }
+
+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+ return 0;
+
+ fb = conn_state->writeback_job->fb;
+
+ DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id, fb->width, fb->height);
+
+ if (fb->width != mode->hdisplay) {
+ DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width, mode->hdisplay);
+ return -EINVAL;
+ } else if (fb->height != mode->vdisplay) {
+ DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height, mode->vdisplay);
+ return -EINVAL;
+ } else if (fb->width > dpu_wb_conn->maxlinewidth) {
+ DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
+ fb->width, dpu_wb_conn->maxlinewidth);
+ return -EINVAL;
+ }
+
+ return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
+}
+
static const struct drm_connector_funcs dpu_wb_conn_funcs = {
.reset = drm_atomic_helper_connector_reset,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -59,12 +111,13 @@ static void dpu_wb_conn_cleanup_job(struct drm_writeback_connector *connector,
static const struct drm_connector_helper_funcs dpu_wb_conn_helper_funcs = {
.get_modes = dpu_wb_conn_get_modes,
+ .atomic_check = dpu_wb_conn_atomic_check,
.prepare_writeback_job = dpu_wb_conn_prepare_job,
.cleanup_writeback_job = dpu_wb_conn_cleanup_job,
};
int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
- const u32 *format_list, u32 num_formats)
+ const u32 *format_list, u32 num_formats, u32 maxlinewidth)
{
struct dpu_wb_connector *dpu_wb_conn;
int rc = 0;
@@ -73,6 +126,8 @@ int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
if (!dpu_wb_conn)
return -ENOMEM;
+ dpu_wb_conn->maxlinewidth = maxlinewidth;
+
drm_connector_helper_add(&dpu_wb_conn->base.base, &dpu_wb_conn_helper_funcs);
/* DPU initializes the encoder and sets it up completely for writeback
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
index 5a75ea916101..4b11cca8014c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
@@ -18,6 +18,7 @@
struct dpu_wb_connector {
struct drm_writeback_connector base;
struct drm_encoder *wb_enc;
+ u32 maxlinewidth;
};
static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_connector *conn)
@@ -26,6 +27,6 @@ static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_conne
}
int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
- const u32 *format_list, u32 num_formats);
+ const u32 *format_list, u32 num_formats, u32 maxlinewidth);
#endif /*_DPU_WRITEBACK_H */
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
@ 2023-12-25 13:08 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel,
Stephen Boyd, David Airlie
dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
Move corresponding checks to drm_writeback_connector's implementation
and drop the dpu_encoder_phys_wb_atomic_check() function.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
4 files changed, 64 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index a0a28230fc31..8220cd920e6f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -354,59 +354,6 @@ static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
}
}
-/**
- * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
- * @phys_enc: Pointer to physical encoder
- * @crtc_state: Pointer to CRTC atomic state
- * @conn_state: Pointer to connector atomic state
- */
-static int dpu_encoder_phys_wb_atomic_check(
- struct dpu_encoder_phys *phys_enc,
- struct drm_crtc_state *crtc_state,
- struct drm_connector_state *conn_state)
-{
- struct drm_framebuffer *fb;
- const struct drm_display_mode *mode = &crtc_state->mode;
-
- DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
- phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
-
- if (!conn_state || !conn_state->connector) {
- DPU_ERROR("invalid connector state\n");
- return -EINVAL;
- } else if (conn_state->connector->status !=
- connector_status_connected) {
- DPU_ERROR("connector not connected %d\n",
- conn_state->connector->status);
- return -EINVAL;
- }
-
- if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
- return 0;
-
- fb = conn_state->writeback_job->fb;
-
- DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
- fb->width, fb->height);
-
- if (fb->width != mode->hdisplay) {
- DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
- mode->hdisplay);
- return -EINVAL;
- } else if (fb->height != mode->vdisplay) {
- DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
- mode->vdisplay);
- return -EINVAL;
- } else if (fb->width > phys_enc->hw_wb->caps->maxlinewidth) {
- DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
- fb->width, phys_enc->hw_wb->caps->maxlinewidth);
- return -EINVAL;
- }
-
- return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
-}
-
-
/**
* _dpu_encoder_phys_wb_update_flush - flush hardware update
* @phys_enc: Pointer to physical encoder
@@ -777,7 +724,6 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
ops->is_master = dpu_encoder_phys_wb_is_master;
ops->enable = dpu_encoder_phys_wb_enable;
ops->disable = dpu_encoder_phys_wb_disable;
- ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done;
ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 723cc1d82143..48728be27e15 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -630,23 +630,26 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
{
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
+ const enum dpu_wb wb_idx = WB_2;
+ u32 maxlinewidth;
int rc;
memset(&info, 0, sizeof(info));
info.num_of_h_tiles = 1;
/* use only WB idx 2 instance for DPU */
- info.h_tile_instance[0] = WB_2;
+ info.h_tile_instance[0] = wb_idx;
info.intf_type = INTF_WB;
+ maxlinewidth = dpu_rm_get_wb(&dpu_kms->rm, info.h_tile_instance[0])->caps->maxlinewidth;
+
encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
if (IS_ERR(encoder)) {
DPU_ERROR("encoder init failed for dsi display\n");
return PTR_ERR(encoder);
}
- rc = dpu_writeback_init(dev, encoder, wb_formats,
- n_formats);
+ rc = dpu_writeback_init(dev, encoder, wb_formats, n_formats, maxlinewidth);
if (rc) {
DPU_ERROR("dpu_writeback_init, rc = %d\n", rc);
return rc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
index 2a5a68366582..232b5f410de8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
@@ -4,6 +4,7 @@
*/
#include <drm/drm_edid.h>
+#include <drm/drm_framebuffer.h>
#include "dpu_writeback.h"
@@ -24,6 +25,57 @@ static int dpu_wb_conn_get_modes(struct drm_connector *connector)
dev->mode_config.max_height);
}
+static int dpu_wb_conn_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+ struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
+ struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(wb_conn);
+ struct drm_connector_state *conn_state =
+ drm_atomic_get_new_connector_state(state, connector);
+ struct drm_crtc *crtc = conn_state->crtc;
+ struct drm_crtc_state *crtc_state;
+ const struct drm_display_mode *mode;
+ struct drm_framebuffer *fb;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ mode = &crtc_state->mode;
+
+ DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
+ connector->base.id, mode->name, mode->hdisplay, mode->vdisplay);
+
+ if (!conn_state || !conn_state->connector) {
+ DPU_ERROR("invalid connector state\n");
+ return -EINVAL;
+ } else if (conn_state->connector->status != connector_status_connected) {
+ DPU_ERROR("connector not connected %d\n", conn_state->connector->status);
+ return -EINVAL;
+ }
+
+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+ return 0;
+
+ fb = conn_state->writeback_job->fb;
+
+ DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id, fb->width, fb->height);
+
+ if (fb->width != mode->hdisplay) {
+ DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width, mode->hdisplay);
+ return -EINVAL;
+ } else if (fb->height != mode->vdisplay) {
+ DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height, mode->vdisplay);
+ return -EINVAL;
+ } else if (fb->width > dpu_wb_conn->maxlinewidth) {
+ DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
+ fb->width, dpu_wb_conn->maxlinewidth);
+ return -EINVAL;
+ }
+
+ return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
+}
+
static const struct drm_connector_funcs dpu_wb_conn_funcs = {
.reset = drm_atomic_helper_connector_reset,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -59,12 +111,13 @@ static void dpu_wb_conn_cleanup_job(struct drm_writeback_connector *connector,
static const struct drm_connector_helper_funcs dpu_wb_conn_helper_funcs = {
.get_modes = dpu_wb_conn_get_modes,
+ .atomic_check = dpu_wb_conn_atomic_check,
.prepare_writeback_job = dpu_wb_conn_prepare_job,
.cleanup_writeback_job = dpu_wb_conn_cleanup_job,
};
int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
- const u32 *format_list, u32 num_formats)
+ const u32 *format_list, u32 num_formats, u32 maxlinewidth)
{
struct dpu_wb_connector *dpu_wb_conn;
int rc = 0;
@@ -73,6 +126,8 @@ int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
if (!dpu_wb_conn)
return -ENOMEM;
+ dpu_wb_conn->maxlinewidth = maxlinewidth;
+
drm_connector_helper_add(&dpu_wb_conn->base.base, &dpu_wb_conn_helper_funcs);
/* DPU initializes the encoder and sets it up completely for writeback
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
index 5a75ea916101..4b11cca8014c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
@@ -18,6 +18,7 @@
struct dpu_wb_connector {
struct drm_writeback_connector base;
struct drm_encoder *wb_enc;
+ u32 maxlinewidth;
};
static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_connector *conn)
@@ -26,6 +27,6 @@ static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_conne
}
int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
- const u32 *format_list, u32 num_formats);
+ const u32 *format_list, u32 num_formats, u32 maxlinewidth);
#endif /*_DPU_WRITEBACK_H */
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 5/5] drm/msm/dpu: drop dpu_encoder_phys_ops::atomic_check()
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2023-12-25 13:08 ` Dmitry Baryshkov
-1 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
Writeback was the last user of dpu_encoder_phys_ops's atomic_check()
callback. As the code was moved to the dpu_writeback.c, the callback
becomes unused. Drop it now.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ---------------
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ----
2 files changed, 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5022d0b9b4b4..8a8aae850a3b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -584,7 +584,6 @@ static int dpu_encoder_virt_atomic_check(
struct dpu_global_state *global_state;
struct drm_framebuffer *fb;
struct drm_dsc_config *dsc;
- int i = 0;
int ret = 0;
if (!drm_enc || !crtc_state || !conn_state) {
@@ -605,20 +604,6 @@ static int dpu_encoder_virt_atomic_check(
trace_dpu_enc_atomic_check(DRMID(drm_enc));
- /* perform atomic check on the first physical encoder (master) */
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
- if (phys->ops.atomic_check)
- ret = phys->ops.atomic_check(phys, crtc_state,
- conn_state);
- if (ret) {
- DPU_ERROR_ENC(dpu_enc,
- "mode unsupported, phys idx %d\n", i);
- return ret;
- }
- }
-
dsc = dpu_encoder_get_dsc_config(drm_enc);
topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
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 7eb8bdfe6bbe..dd9e3603d120 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -71,7 +71,6 @@ struct dpu_encoder_phys;
* on split_role and current mode (CMD/VID).
* @enable: DRM Call. Enable a DRM mode.
* @disable: DRM Call. Disable mode.
- * @atomic_check: DRM Call. Atomic check new DRM state.
* @control_vblank_irq Register/Deregister for VBLANK IRQ
* @wait_for_commit_done: Wait for hardware to have flushed the
* current pending frames to hardware
@@ -96,9 +95,6 @@ struct dpu_encoder_phys_ops {
bool (*is_master)(struct dpu_encoder_phys *encoder);
void (*enable)(struct dpu_encoder_phys *encoder);
void (*disable)(struct dpu_encoder_phys *encoder);
- int (*atomic_check)(struct dpu_encoder_phys *encoder,
- struct drm_crtc_state *crtc_state,
- struct drm_connector_state *conn_state);
int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 5/5] drm/msm/dpu: drop dpu_encoder_phys_ops::atomic_check()
@ 2023-12-25 13:08 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2023-12-25 13:08 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel,
Stephen Boyd, David Airlie
Writeback was the last user of dpu_encoder_phys_ops's atomic_check()
callback. As the code was moved to the dpu_writeback.c, the callback
becomes unused. Drop it now.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ---------------
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ----
2 files changed, 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5022d0b9b4b4..8a8aae850a3b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -584,7 +584,6 @@ static int dpu_encoder_virt_atomic_check(
struct dpu_global_state *global_state;
struct drm_framebuffer *fb;
struct drm_dsc_config *dsc;
- int i = 0;
int ret = 0;
if (!drm_enc || !crtc_state || !conn_state) {
@@ -605,20 +604,6 @@ static int dpu_encoder_virt_atomic_check(
trace_dpu_enc_atomic_check(DRMID(drm_enc));
- /* perform atomic check on the first physical encoder (master) */
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
- if (phys->ops.atomic_check)
- ret = phys->ops.atomic_check(phys, crtc_state,
- conn_state);
- if (ret) {
- DPU_ERROR_ENC(dpu_enc,
- "mode unsupported, phys idx %d\n", i);
- return ret;
- }
- }
-
dsc = dpu_encoder_get_dsc_config(drm_enc);
topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
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 7eb8bdfe6bbe..dd9e3603d120 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -71,7 +71,6 @@ struct dpu_encoder_phys;
* on split_role and current mode (CMD/VID).
* @enable: DRM Call. Enable a DRM mode.
* @disable: DRM Call. Disable mode.
- * @atomic_check: DRM Call. Atomic check new DRM state.
* @control_vblank_irq Register/Deregister for VBLANK IRQ
* @wait_for_commit_done: Wait for hardware to have flushed the
* current pending frames to hardware
@@ -96,9 +95,6 @@ struct dpu_encoder_phys_ops {
bool (*is_master)(struct dpu_encoder_phys *encoder);
void (*enable)(struct dpu_encoder_phys *encoder);
void (*disable)(struct dpu_encoder_phys *encoder);
- int (*atomic_check)(struct dpu_encoder_phys *encoder,
- struct drm_crtc_state *crtc_state,
- struct drm_connector_state *conn_state);
int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 1/5] drm/msm/dpu: split irq_control into irq_enable and _disable
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2024-01-08 21:26 ` Abhinav Kumar
-1 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:26 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> The single helper for both enable and disable cases is too complicated,
> especially if we start adding more code to these helpers. Split it into
> irq_enable and irq_disable cases.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 36 +++++++---
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 6 +-
> .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 65 ++++++++++---------
> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 46 +++++++------
> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 29 ++++++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 62 ++++++++++++++----
> 6 files changed, 158 insertions(+), 86 deletions(-)
>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 1/5] drm/msm/dpu: split irq_control into irq_enable and _disable
@ 2024-01-08 21:26 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:26 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> The single helper for both enable and disable cases is too complicated,
> especially if we start adding more code to these helpers. Split it into
> irq_enable and irq_disable cases.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 36 +++++++---
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 6 +-
> .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 65 ++++++++++---------
> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 46 +++++++------
> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 29 ++++++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 62 ++++++++++++++----
> 6 files changed, 158 insertions(+), 86 deletions(-)
>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/5] drm/msm/dpu: split _dpu_encoder_resource_control_helper()
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2024-01-08 21:29 ` Abhinav Kumar
-1 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:29 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> Follow the _dpu_encoder_irq_control() change and split the
> _dpu_encoder_resource_control_helper() into enable and disable parts.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +++++++++++++--------
> drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 12 ++++--
> 2 files changed, 37 insertions(+), 20 deletions(-)
>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/5] drm/msm/dpu: split _dpu_encoder_resource_control_helper()
@ 2024-01-08 21:29 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:29 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> Follow the _dpu_encoder_irq_control() change and split the
> _dpu_encoder_resource_control_helper() into enable and disable parts.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45 +++++++++++++--------
> drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 12 ++++--
> 2 files changed, 37 insertions(+), 20 deletions(-)
>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2024-01-08 21:39 ` Abhinav Kumar
-1 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:39 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
> Move corresponding checks to drm_writeback_connector's implementation
> and drop the dpu_encoder_phys_wb_atomic_check() function.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
> 4 files changed, 64 insertions(+), 59 deletions(-)
>
I am fine with this change with respect to how the code is today.
Hence,
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
But if we start noticing a pattern like below in dpu_encoder.c's
atomic_check,
if (INTF_WB)
.....
else if (INTF_DP || INTF_DSI)
.....
then, it will demand bringing back a phys specific callback.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
@ 2024-01-08 21:39 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:39 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
> Move corresponding checks to drm_writeback_connector's implementation
> and drop the dpu_encoder_phys_wb_atomic_check() function.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
> 4 files changed, 64 insertions(+), 59 deletions(-)
>
I am fine with this change with respect to how the code is today.
Hence,
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
But if we start noticing a pattern like below in dpu_encoder.c's
atomic_check,
if (INTF_WB)
.....
else if (INTF_DP || INTF_DSI)
.....
then, it will demand bringing back a phys specific callback.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] drm/msm/dpu: drop dpu_encoder_phys_ops::atomic_check()
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2024-01-08 21:40 ` Abhinav Kumar
-1 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:40 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> Writeback was the last user of dpu_encoder_phys_ops's atomic_check()
> callback. As the code was moved to the dpu_writeback.c, the callback
> becomes unused. Drop it now.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ---------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ----
> 2 files changed, 19 deletions(-)
>
Same comment as prev change but otherwise
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] drm/msm/dpu: drop dpu_encoder_phys_ops::atomic_check()
@ 2024-01-08 21:40 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:40 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> Writeback was the last user of dpu_encoder_phys_ops's atomic_check()
> callback. As the code was moved to the dpu_writeback.c, the callback
> becomes unused. Drop it now.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 ---------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ----
> 2 files changed, 19 deletions(-)
>
Same comment as prev change but otherwise
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 0/5] drm/msm/dpu: remove dpu_encoder_phys_ops::atomic_mode_set callback
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2024-01-08 21:41 ` Abhinav Kumar
-1 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:41 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> The dpu_encoder_phys_ops::atomic_mode_set() callback is mostly
> redundant. Implementations only set the IRQ indices there. Move
> statically allocated IRQs to dpu_encoder_phys_*_init() and set
> dynamically allocated IRQs in the irq_enable() callback.
>
> The writeback backend of the dpu_encoder is the only user of the
> dpu_encoder_phys_ops::atomic_check() callback. Move corresponding code
> to the DPU's drm_writeback_connector implementation (dpu_writeback.c)
> and drop corresponding callback code.
>
> Changes since v2:
> - Rebase on top of linux-next
> - Also incorporate the dpu_encoder_phys::atomic_check series
>
> Changes since v1:
> - Split trace events into enable/disable pairs (Abhinav).
>
We will provide a Tested-by for this series soon after re-validating WB
and CDM with WB with this.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 0/5] drm/msm/dpu: remove dpu_encoder_phys_ops::atomic_mode_set callback
@ 2024-01-08 21:41 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-08 21:41 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> The dpu_encoder_phys_ops::atomic_mode_set() callback is mostly
> redundant. Implementations only set the IRQ indices there. Move
> statically allocated IRQs to dpu_encoder_phys_*_init() and set
> dynamically allocated IRQs in the irq_enable() callback.
>
> The writeback backend of the dpu_encoder is the only user of the
> dpu_encoder_phys_ops::atomic_check() callback. Move corresponding code
> to the DPU's drm_writeback_connector implementation (dpu_writeback.c)
> and drop corresponding callback code.
>
> Changes since v2:
> - Rebase on top of linux-next
> - Also incorporate the dpu_encoder_phys::atomic_check series
>
> Changes since v1:
> - Split trace events into enable/disable pairs (Abhinav).
>
We will provide a Tested-by for this series soon after re-validating WB
and CDM with WB with this.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
2024-01-08 21:39 ` Abhinav Kumar
@ 2024-01-08 22:43 ` Dmitry Baryshkov
-1 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-01-08 22:43 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Marijn Suijten, Stephen Boyd, David Airlie,
Daniel Vetter, Bjorn Andersson, linux-arm-msm, dri-devel,
freedreno
On Mon, 8 Jan 2024 at 23:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> > dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
> > Move corresponding checks to drm_writeback_connector's implementation
> > and drop the dpu_encoder_phys_wb_atomic_check() function.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
> > 4 files changed, 64 insertions(+), 59 deletions(-)
> >
>
> I am fine with this change with respect to how the code is today.
>
> Hence,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> But if we start noticing a pattern like below in dpu_encoder.c's
> atomic_check,
>
> if (INTF_WB)
> .....
> else if (INTF_DP || INTF_DSI)
> .....
>
> then, it will demand bringing back a phys specific callback.
The problem is that INTF_DP || INTF_DSI does not have the
phys-specific implementation. So while I agree about the INTF_WB part,
INTF_DP || INTF_DSI either should go as is, or it should be refactored
into output-specific handlers.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
@ 2024-01-08 22:43 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-01-08 22:43 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, Bjorn Andersson, dri-devel, Stephen Boyd,
linux-arm-msm, Marijn Suijten, Sean Paul
On Mon, 8 Jan 2024 at 23:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> > dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
> > Move corresponding checks to drm_writeback_connector's implementation
> > and drop the dpu_encoder_phys_wb_atomic_check() function.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
> > 4 files changed, 64 insertions(+), 59 deletions(-)
> >
>
> I am fine with this change with respect to how the code is today.
>
> Hence,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> But if we start noticing a pattern like below in dpu_encoder.c's
> atomic_check,
>
> if (INTF_WB)
> .....
> else if (INTF_DP || INTF_DSI)
> .....
>
> then, it will demand bringing back a phys specific callback.
The problem is that INTF_DP || INTF_DSI does not have the
phys-specific implementation. So while I agree about the INTF_WB part,
INTF_DP || INTF_DSI either should go as is, or it should be refactored
into output-specific handlers.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
2023-12-25 13:08 ` Dmitry Baryshkov
@ 2024-01-23 22:23 ` Abhinav Kumar
-1 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-23 22:23 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
> Move corresponding checks to drm_writeback_connector's implementation
> and drop the dpu_encoder_phys_wb_atomic_check() function.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
> 4 files changed, 64 insertions(+), 59 deletions(-)
>
While validating this change with kms_writeback, we found that this is
breaking back to back validate of kms_writeback with a NULL ptr
dereference in below stack:
[ 86.701062] Call trace:
[ 86.701067] dpu_wb_conn_atomic_check+0x118/0x18c
[ 86.701076] drm_atomic_helper_check_modeset+0x2d8/0x688
[ 86.701084] drm_atomic_helper_check+0x24/0x98
[ 86.701095] msm_atomic_check+0x90/0x9c
[ 86.701103] drm_atomic_check_only+0x4f4/0x8e8
[ 86.701111] drm_atomic_commit+0x64/0xd8
[ 86.701120] drm_mode_atomic_ioctl+0xbfc/0xe74
[ 86.701129] drm_ioctl_kernel+0xd4/0x114
[ 86.701137] drm_ioctl+0x274/0x508
[ 86.701143] __arm64_sys_ioctl+0x98/0xd0
[ 86.701152] invoke_syscall+0x48/0xfc
[ 86.701161] el0_svc_common+0x88/0xe4
[ 86.701167] do_el0_svc+0x24/0x30
[ 86.701175] el0_svc+0x34/0x80
[ 86.701184] el0t_64_sync_handler+0x44/0xec
[ 86.701192] el0t_64_sync+0x1a8/0x1ac
[ 86.701200] ---[ end trace 0000000000000000 ]---
We analysed this and found why. Please see below.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index a0a28230fc31..8220cd920e6f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -354,59 +354,6 @@ static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
> }
> }
>
> -/**
> - * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
> - * @phys_enc: Pointer to physical encoder
> - * @crtc_state: Pointer to CRTC atomic state
> - * @conn_state: Pointer to connector atomic state
> - */
> -static int dpu_encoder_phys_wb_atomic_check(
> - struct dpu_encoder_phys *phys_enc,
> - struct drm_crtc_state *crtc_state,
> - struct drm_connector_state *conn_state)
> -{
> - struct drm_framebuffer *fb;
> - const struct drm_display_mode *mode = &crtc_state->mode;
> -
> - DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> - phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
> -
> - if (!conn_state || !conn_state->connector) {
> - DPU_ERROR("invalid connector state\n");
> - return -EINVAL;
> - } else if (conn_state->connector->status !=
> - connector_status_connected) {
> - DPU_ERROR("connector not connected %d\n",
> - conn_state->connector->status);
> - return -EINVAL;
> - }
> -
> - if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> - return 0;
> -
> - fb = conn_state->writeback_job->fb;
> -
> - DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
> - fb->width, fb->height);
> -
> - if (fb->width != mode->hdisplay) {
> - DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
> - mode->hdisplay);
> - return -EINVAL;
> - } else if (fb->height != mode->vdisplay) {
> - DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
> - mode->vdisplay);
> - return -EINVAL;
> - } else if (fb->width > phys_enc->hw_wb->caps->maxlinewidth) {
> - DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
> - fb->width, phys_enc->hw_wb->caps->maxlinewidth);
> - return -EINVAL;
> - }
> -
> - return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> -}
> -
> -
> /**
> * _dpu_encoder_phys_wb_update_flush - flush hardware update
> * @phys_enc: Pointer to physical encoder
> @@ -777,7 +724,6 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
> ops->is_master = dpu_encoder_phys_wb_is_master;
> ops->enable = dpu_encoder_phys_wb_enable;
> ops->disable = dpu_encoder_phys_wb_disable;
> - ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
> ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done;
> ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
> ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 723cc1d82143..48728be27e15 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -630,23 +630,26 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
> {
> struct drm_encoder *encoder = NULL;
> struct msm_display_info info;
> + const enum dpu_wb wb_idx = WB_2;
> + u32 maxlinewidth;
> int rc;
>
> memset(&info, 0, sizeof(info));
>
> info.num_of_h_tiles = 1;
> /* use only WB idx 2 instance for DPU */
> - info.h_tile_instance[0] = WB_2;
> + info.h_tile_instance[0] = wb_idx;
> info.intf_type = INTF_WB;
>
> + maxlinewidth = dpu_rm_get_wb(&dpu_kms->rm, info.h_tile_instance[0])->caps->maxlinewidth;
> +
> encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
> if (IS_ERR(encoder)) {
> DPU_ERROR("encoder init failed for dsi display\n");
> return PTR_ERR(encoder);
> }
>
> - rc = dpu_writeback_init(dev, encoder, wb_formats,
> - n_formats);
> + rc = dpu_writeback_init(dev, encoder, wb_formats, n_formats, maxlinewidth);
> if (rc) {
> DPU_ERROR("dpu_writeback_init, rc = %d\n", rc);
> return rc;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> index 2a5a68366582..232b5f410de8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> @@ -4,6 +4,7 @@
> */
>
> #include <drm/drm_edid.h>
> +#include <drm/drm_framebuffer.h>
>
> #include "dpu_writeback.h"
>
> @@ -24,6 +25,57 @@ static int dpu_wb_conn_get_modes(struct drm_connector *connector)
> dev->mode_config.max_height);
> }
>
> +static int dpu_wb_conn_atomic_check(struct drm_connector *connector,
> + struct drm_atomic_state *state)
> +{
> + struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
> + struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(wb_conn);
> + struct drm_connector_state *conn_state =
> + drm_atomic_get_new_connector_state(state, connector);
> + struct drm_crtc *crtc = conn_state->crtc;
> + struct drm_crtc_state *crtc_state;
> + const struct drm_display_mode *mode;
> + struct drm_framebuffer *fb;
> +
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
To detach the CRTC associated with the connector, IGT will set the
associated CRTC_ID to 0 and the associated conn_state->crtc will be NULL.
This is valid as val will be 0 in this case:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_uapi.c#L722
Before this patch, for these cases, we used to call the encoder's
atomic_check which gets skipped when there is no valid crtc:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L440
But now with connector atomic check, these calls are allowed by the DRM
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L712
So questions:
1) Should we add protection in DRM to check if conn_state->crtc is valid
before calling connector's atomic_check()?
OR
2) Is it incorrect for us to dereference conn->crtc in connector's
atomic_check as its not guaranteed to be valid.
We cannot fail atomic_check for !crtc, because if we add a !crtc check
and fail those checks, it bails out these disable commit calls thus
failing those commits.
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> +
> + mode = &crtc_state->mode;
> +
> + DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> + connector->base.id, mode->name, mode->hdisplay, mode->vdisplay);
> +
> + if (!conn_state || !conn_state->connector) {
> + DPU_ERROR("invalid connector state\n");
> + return -EINVAL;
> + } else if (conn_state->connector->status != connector_status_connected) {
> + DPU_ERROR("connector not connected %d\n", conn_state->connector->status);
> + return -EINVAL;
> + }
> +
> + if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> + return 0;
> +
> + fb = conn_state->writeback_job->fb;
> +
> + DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id, fb->width, fb->height);
> +
> + if (fb->width != mode->hdisplay) {
> + DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width, mode->hdisplay);
> + return -EINVAL;
> + } else if (fb->height != mode->vdisplay) {
> + DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height, mode->vdisplay);
> + return -EINVAL;
> + } else if (fb->width > dpu_wb_conn->maxlinewidth) {
> + DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
> + fb->width, dpu_wb_conn->maxlinewidth);
> + return -EINVAL;
> + }
> +
> + return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> +}
> +
> static const struct drm_connector_funcs dpu_wb_conn_funcs = {
> .reset = drm_atomic_helper_connector_reset,
> .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -59,12 +111,13 @@ static void dpu_wb_conn_cleanup_job(struct drm_writeback_connector *connector,
>
> static const struct drm_connector_helper_funcs dpu_wb_conn_helper_funcs = {
> .get_modes = dpu_wb_conn_get_modes,
> + .atomic_check = dpu_wb_conn_atomic_check,
> .prepare_writeback_job = dpu_wb_conn_prepare_job,
> .cleanup_writeback_job = dpu_wb_conn_cleanup_job,
> };
>
> int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
> - const u32 *format_list, u32 num_formats)
> + const u32 *format_list, u32 num_formats, u32 maxlinewidth)
> {
> struct dpu_wb_connector *dpu_wb_conn;
> int rc = 0;
> @@ -73,6 +126,8 @@ int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
> if (!dpu_wb_conn)
> return -ENOMEM;
>
> + dpu_wb_conn->maxlinewidth = maxlinewidth;
> +
> drm_connector_helper_add(&dpu_wb_conn->base.base, &dpu_wb_conn_helper_funcs);
>
> /* DPU initializes the encoder and sets it up completely for writeback
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> index 5a75ea916101..4b11cca8014c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> @@ -18,6 +18,7 @@
> struct dpu_wb_connector {
> struct drm_writeback_connector base;
> struct drm_encoder *wb_enc;
> + u32 maxlinewidth;
> };
>
> static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_connector *conn)
> @@ -26,6 +27,6 @@ static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_conne
> }
>
> int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
> - const u32 *format_list, u32 num_formats);
> + const u32 *format_list, u32 num_formats, u32 maxlinewidth);
>
> #endif /*_DPU_WRITEBACK_H */
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
@ 2024-01-23 22:23 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-01-23 22:23 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel,
Stephen Boyd, Daniel Vetter, David Airlie
On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
> Move corresponding checks to drm_writeback_connector's implementation
> and drop the dpu_encoder_phys_wb_atomic_check() function.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
> 4 files changed, 64 insertions(+), 59 deletions(-)
>
While validating this change with kms_writeback, we found that this is
breaking back to back validate of kms_writeback with a NULL ptr
dereference in below stack:
[ 86.701062] Call trace:
[ 86.701067] dpu_wb_conn_atomic_check+0x118/0x18c
[ 86.701076] drm_atomic_helper_check_modeset+0x2d8/0x688
[ 86.701084] drm_atomic_helper_check+0x24/0x98
[ 86.701095] msm_atomic_check+0x90/0x9c
[ 86.701103] drm_atomic_check_only+0x4f4/0x8e8
[ 86.701111] drm_atomic_commit+0x64/0xd8
[ 86.701120] drm_mode_atomic_ioctl+0xbfc/0xe74
[ 86.701129] drm_ioctl_kernel+0xd4/0x114
[ 86.701137] drm_ioctl+0x274/0x508
[ 86.701143] __arm64_sys_ioctl+0x98/0xd0
[ 86.701152] invoke_syscall+0x48/0xfc
[ 86.701161] el0_svc_common+0x88/0xe4
[ 86.701167] do_el0_svc+0x24/0x30
[ 86.701175] el0_svc+0x34/0x80
[ 86.701184] el0t_64_sync_handler+0x44/0xec
[ 86.701192] el0t_64_sync+0x1a8/0x1ac
[ 86.701200] ---[ end trace 0000000000000000 ]---
We analysed this and found why. Please see below.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index a0a28230fc31..8220cd920e6f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -354,59 +354,6 @@ static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
> }
> }
>
> -/**
> - * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
> - * @phys_enc: Pointer to physical encoder
> - * @crtc_state: Pointer to CRTC atomic state
> - * @conn_state: Pointer to connector atomic state
> - */
> -static int dpu_encoder_phys_wb_atomic_check(
> - struct dpu_encoder_phys *phys_enc,
> - struct drm_crtc_state *crtc_state,
> - struct drm_connector_state *conn_state)
> -{
> - struct drm_framebuffer *fb;
> - const struct drm_display_mode *mode = &crtc_state->mode;
> -
> - DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> - phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
> -
> - if (!conn_state || !conn_state->connector) {
> - DPU_ERROR("invalid connector state\n");
> - return -EINVAL;
> - } else if (conn_state->connector->status !=
> - connector_status_connected) {
> - DPU_ERROR("connector not connected %d\n",
> - conn_state->connector->status);
> - return -EINVAL;
> - }
> -
> - if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> - return 0;
> -
> - fb = conn_state->writeback_job->fb;
> -
> - DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
> - fb->width, fb->height);
> -
> - if (fb->width != mode->hdisplay) {
> - DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
> - mode->hdisplay);
> - return -EINVAL;
> - } else if (fb->height != mode->vdisplay) {
> - DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
> - mode->vdisplay);
> - return -EINVAL;
> - } else if (fb->width > phys_enc->hw_wb->caps->maxlinewidth) {
> - DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
> - fb->width, phys_enc->hw_wb->caps->maxlinewidth);
> - return -EINVAL;
> - }
> -
> - return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> -}
> -
> -
> /**
> * _dpu_encoder_phys_wb_update_flush - flush hardware update
> * @phys_enc: Pointer to physical encoder
> @@ -777,7 +724,6 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
> ops->is_master = dpu_encoder_phys_wb_is_master;
> ops->enable = dpu_encoder_phys_wb_enable;
> ops->disable = dpu_encoder_phys_wb_disable;
> - ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
> ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done;
> ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
> ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 723cc1d82143..48728be27e15 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -630,23 +630,26 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
> {
> struct drm_encoder *encoder = NULL;
> struct msm_display_info info;
> + const enum dpu_wb wb_idx = WB_2;
> + u32 maxlinewidth;
> int rc;
>
> memset(&info, 0, sizeof(info));
>
> info.num_of_h_tiles = 1;
> /* use only WB idx 2 instance for DPU */
> - info.h_tile_instance[0] = WB_2;
> + info.h_tile_instance[0] = wb_idx;
> info.intf_type = INTF_WB;
>
> + maxlinewidth = dpu_rm_get_wb(&dpu_kms->rm, info.h_tile_instance[0])->caps->maxlinewidth;
> +
> encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
> if (IS_ERR(encoder)) {
> DPU_ERROR("encoder init failed for dsi display\n");
> return PTR_ERR(encoder);
> }
>
> - rc = dpu_writeback_init(dev, encoder, wb_formats,
> - n_formats);
> + rc = dpu_writeback_init(dev, encoder, wb_formats, n_formats, maxlinewidth);
> if (rc) {
> DPU_ERROR("dpu_writeback_init, rc = %d\n", rc);
> return rc;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> index 2a5a68366582..232b5f410de8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> @@ -4,6 +4,7 @@
> */
>
> #include <drm/drm_edid.h>
> +#include <drm/drm_framebuffer.h>
>
> #include "dpu_writeback.h"
>
> @@ -24,6 +25,57 @@ static int dpu_wb_conn_get_modes(struct drm_connector *connector)
> dev->mode_config.max_height);
> }
>
> +static int dpu_wb_conn_atomic_check(struct drm_connector *connector,
> + struct drm_atomic_state *state)
> +{
> + struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
> + struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(wb_conn);
> + struct drm_connector_state *conn_state =
> + drm_atomic_get_new_connector_state(state, connector);
> + struct drm_crtc *crtc = conn_state->crtc;
> + struct drm_crtc_state *crtc_state;
> + const struct drm_display_mode *mode;
> + struct drm_framebuffer *fb;
> +
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
To detach the CRTC associated with the connector, IGT will set the
associated CRTC_ID to 0 and the associated conn_state->crtc will be NULL.
This is valid as val will be 0 in this case:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_uapi.c#L722
Before this patch, for these cases, we used to call the encoder's
atomic_check which gets skipped when there is no valid crtc:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L440
But now with connector atomic check, these calls are allowed by the DRM
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L712
So questions:
1) Should we add protection in DRM to check if conn_state->crtc is valid
before calling connector's atomic_check()?
OR
2) Is it incorrect for us to dereference conn->crtc in connector's
atomic_check as its not guaranteed to be valid.
We cannot fail atomic_check for !crtc, because if we add a !crtc check
and fail those checks, it bails out these disable commit calls thus
failing those commits.
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> +
> + mode = &crtc_state->mode;
> +
> + DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> + connector->base.id, mode->name, mode->hdisplay, mode->vdisplay);
> +
> + if (!conn_state || !conn_state->connector) {
> + DPU_ERROR("invalid connector state\n");
> + return -EINVAL;
> + } else if (conn_state->connector->status != connector_status_connected) {
> + DPU_ERROR("connector not connected %d\n", conn_state->connector->status);
> + return -EINVAL;
> + }
> +
> + if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> + return 0;
> +
> + fb = conn_state->writeback_job->fb;
> +
> + DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id, fb->width, fb->height);
> +
> + if (fb->width != mode->hdisplay) {
> + DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width, mode->hdisplay);
> + return -EINVAL;
> + } else if (fb->height != mode->vdisplay) {
> + DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height, mode->vdisplay);
> + return -EINVAL;
> + } else if (fb->width > dpu_wb_conn->maxlinewidth) {
> + DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
> + fb->width, dpu_wb_conn->maxlinewidth);
> + return -EINVAL;
> + }
> +
> + return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> +}
> +
> static const struct drm_connector_funcs dpu_wb_conn_funcs = {
> .reset = drm_atomic_helper_connector_reset,
> .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -59,12 +111,13 @@ static void dpu_wb_conn_cleanup_job(struct drm_writeback_connector *connector,
>
> static const struct drm_connector_helper_funcs dpu_wb_conn_helper_funcs = {
> .get_modes = dpu_wb_conn_get_modes,
> + .atomic_check = dpu_wb_conn_atomic_check,
> .prepare_writeback_job = dpu_wb_conn_prepare_job,
> .cleanup_writeback_job = dpu_wb_conn_cleanup_job,
> };
>
> int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
> - const u32 *format_list, u32 num_formats)
> + const u32 *format_list, u32 num_formats, u32 maxlinewidth)
> {
> struct dpu_wb_connector *dpu_wb_conn;
> int rc = 0;
> @@ -73,6 +126,8 @@ int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
> if (!dpu_wb_conn)
> return -ENOMEM;
>
> + dpu_wb_conn->maxlinewidth = maxlinewidth;
> +
> drm_connector_helper_add(&dpu_wb_conn->base.base, &dpu_wb_conn_helper_funcs);
>
> /* DPU initializes the encoder and sets it up completely for writeback
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> index 5a75ea916101..4b11cca8014c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> @@ -18,6 +18,7 @@
> struct dpu_wb_connector {
> struct drm_writeback_connector base;
> struct drm_encoder *wb_enc;
> + u32 maxlinewidth;
> };
>
> static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_connector *conn)
> @@ -26,6 +27,6 @@ static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_conne
> }
>
> int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
> - const u32 *format_list, u32 num_formats);
> + const u32 *format_list, u32 num_formats, u32 maxlinewidth);
>
> #endif /*_DPU_WRITEBACK_H */
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
2024-01-23 22:23 ` Abhinav Kumar
@ 2024-01-24 19:03 ` Dmitry Baryshkov
-1 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-01-24 19:03 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Marijn Suijten, Stephen Boyd, David Airlie,
Daniel Vetter, Bjorn Andersson, linux-arm-msm, dri-devel,
freedreno
On Wed, 24 Jan 2024 at 00:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> > dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
> > Move corresponding checks to drm_writeback_connector's implementation
> > and drop the dpu_encoder_phys_wb_atomic_check() function.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
> > 4 files changed, 64 insertions(+), 59 deletions(-)
> >
>
> While validating this change with kms_writeback, we found that this is
> breaking back to back validate of kms_writeback with a NULL ptr
> dereference in below stack:
>
> [ 86.701062] Call trace:
> [ 86.701067] dpu_wb_conn_atomic_check+0x118/0x18c
> [ 86.701076] drm_atomic_helper_check_modeset+0x2d8/0x688
> [ 86.701084] drm_atomic_helper_check+0x24/0x98
> [ 86.701095] msm_atomic_check+0x90/0x9c
> [ 86.701103] drm_atomic_check_only+0x4f4/0x8e8
> [ 86.701111] drm_atomic_commit+0x64/0xd8
> [ 86.701120] drm_mode_atomic_ioctl+0xbfc/0xe74
> [ 86.701129] drm_ioctl_kernel+0xd4/0x114
> [ 86.701137] drm_ioctl+0x274/0x508
> [ 86.701143] __arm64_sys_ioctl+0x98/0xd0
> [ 86.701152] invoke_syscall+0x48/0xfc
> [ 86.701161] el0_svc_common+0x88/0xe4
> [ 86.701167] do_el0_svc+0x24/0x30
> [ 86.701175] el0_svc+0x34/0x80
> [ 86.701184] el0t_64_sync_handler+0x44/0xec
> [ 86.701192] el0t_64_sync+0x1a8/0x1ac
> [ 86.701200] ---[ end trace 0000000000000000 ]---
>
> We analysed this and found why. Please see below.
>
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > index a0a28230fc31..8220cd920e6f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > @@ -354,59 +354,6 @@ static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
> > }
> > }
> >
> > -/**
> > - * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
> > - * @phys_enc: Pointer to physical encoder
> > - * @crtc_state: Pointer to CRTC atomic state
> > - * @conn_state: Pointer to connector atomic state
> > - */
> > -static int dpu_encoder_phys_wb_atomic_check(
> > - struct dpu_encoder_phys *phys_enc,
> > - struct drm_crtc_state *crtc_state,
> > - struct drm_connector_state *conn_state)
> > -{
> > - struct drm_framebuffer *fb;
> > - const struct drm_display_mode *mode = &crtc_state->mode;
> > -
> > - DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> > - phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
> > -
> > - if (!conn_state || !conn_state->connector) {
> > - DPU_ERROR("invalid connector state\n");
> > - return -EINVAL;
> > - } else if (conn_state->connector->status !=
> > - connector_status_connected) {
> > - DPU_ERROR("connector not connected %d\n",
> > - conn_state->connector->status);
> > - return -EINVAL;
> > - }
> > -
> > - if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > - return 0;
> > -
> > - fb = conn_state->writeback_job->fb;
> > -
> > - DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
> > - fb->width, fb->height);
> > -
> > - if (fb->width != mode->hdisplay) {
> > - DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
> > - mode->hdisplay);
> > - return -EINVAL;
> > - } else if (fb->height != mode->vdisplay) {
> > - DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
> > - mode->vdisplay);
> > - return -EINVAL;
> > - } else if (fb->width > phys_enc->hw_wb->caps->maxlinewidth) {
> > - DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
> > - fb->width, phys_enc->hw_wb->caps->maxlinewidth);
> > - return -EINVAL;
> > - }
> > -
> > - return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> > -}
> > -
> > -
> > /**
> > * _dpu_encoder_phys_wb_update_flush - flush hardware update
> > * @phys_enc: Pointer to physical encoder
> > @@ -777,7 +724,6 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
> > ops->is_master = dpu_encoder_phys_wb_is_master;
> > ops->enable = dpu_encoder_phys_wb_enable;
> > ops->disable = dpu_encoder_phys_wb_disable;
> > - ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
> > ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done;
> > ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
> > ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 723cc1d82143..48728be27e15 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -630,23 +630,26 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
> > {
> > struct drm_encoder *encoder = NULL;
> > struct msm_display_info info;
> > + const enum dpu_wb wb_idx = WB_2;
> > + u32 maxlinewidth;
> > int rc;
> >
> > memset(&info, 0, sizeof(info));
> >
> > info.num_of_h_tiles = 1;
> > /* use only WB idx 2 instance for DPU */
> > - info.h_tile_instance[0] = WB_2;
> > + info.h_tile_instance[0] = wb_idx;
> > info.intf_type = INTF_WB;
> >
> > + maxlinewidth = dpu_rm_get_wb(&dpu_kms->rm, info.h_tile_instance[0])->caps->maxlinewidth;
> > +
> > encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
> > if (IS_ERR(encoder)) {
> > DPU_ERROR("encoder init failed for dsi display\n");
> > return PTR_ERR(encoder);
> > }
> >
> > - rc = dpu_writeback_init(dev, encoder, wb_formats,
> > - n_formats);
> > + rc = dpu_writeback_init(dev, encoder, wb_formats, n_formats, maxlinewidth);
> > if (rc) {
> > DPU_ERROR("dpu_writeback_init, rc = %d\n", rc);
> > return rc;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > index 2a5a68366582..232b5f410de8 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > @@ -4,6 +4,7 @@
> > */
> >
> > #include <drm/drm_edid.h>
> > +#include <drm/drm_framebuffer.h>
> >
> > #include "dpu_writeback.h"
> >
> > @@ -24,6 +25,57 @@ static int dpu_wb_conn_get_modes(struct drm_connector *connector)
> > dev->mode_config.max_height);
> > }
> >
> > +static int dpu_wb_conn_atomic_check(struct drm_connector *connector,
> > + struct drm_atomic_state *state)
> > +{
> > + struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
> > + struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(wb_conn);
> > + struct drm_connector_state *conn_state =
> > + drm_atomic_get_new_connector_state(state, connector);
> > + struct drm_crtc *crtc = conn_state->crtc;
> > + struct drm_crtc_state *crtc_state;
> > + const struct drm_display_mode *mode;
> > + struct drm_framebuffer *fb;
> > +
> > + crtc_state = drm_atomic_get_crtc_state(state, crtc);
>
> To detach the CRTC associated with the connector, IGT will set the
> associated CRTC_ID to 0 and the associated conn_state->crtc will be NULL.
>
> This is valid as val will be 0 in this case:
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_uapi.c#L722
>
> Before this patch, for these cases, we used to call the encoder's
> atomic_check which gets skipped when there is no valid crtc:
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L440
>
> But now with connector atomic check, these calls are allowed by the DRM
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L712
>
> So questions:
>
> 1) Should we add protection in DRM to check if conn_state->crtc is valid
> before calling connector's atomic_check()?
I think this is correct. So if !crtc, just return 0. I'll send next
iteration in the next few days.
>
> OR
>
> 2) Is it incorrect for us to dereference conn->crtc in connector's
> atomic_check as its not guaranteed to be valid.
>
> We cannot fail atomic_check for !crtc, because if we add a !crtc check
> and fail those checks, it bails out these disable commit calls thus
> failing those commits.
>
> > + if (IS_ERR(crtc_state))
> > + return PTR_ERR(crtc_state);
> > +
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c
@ 2024-01-24 19:03 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-01-24 19:03 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, David Airlie, Bjorn Andersson, dri-devel,
Stephen Boyd, Daniel Vetter, linux-arm-msm, Marijn Suijten,
Sean Paul
On Wed, 24 Jan 2024 at 00:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 12/25/2023 5:08 AM, Dmitry Baryshkov wrote:
> > dpu_encoder_phys_wb is the only user of encoder's atomic_check callback.
> > Move corresponding checks to drm_writeback_connector's implementation
> > and drop the dpu_encoder_phys_wb_atomic_check() function.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 54 ------------------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 57 ++++++++++++++++++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 3 +-
> > 4 files changed, 64 insertions(+), 59 deletions(-)
> >
>
> While validating this change with kms_writeback, we found that this is
> breaking back to back validate of kms_writeback with a NULL ptr
> dereference in below stack:
>
> [ 86.701062] Call trace:
> [ 86.701067] dpu_wb_conn_atomic_check+0x118/0x18c
> [ 86.701076] drm_atomic_helper_check_modeset+0x2d8/0x688
> [ 86.701084] drm_atomic_helper_check+0x24/0x98
> [ 86.701095] msm_atomic_check+0x90/0x9c
> [ 86.701103] drm_atomic_check_only+0x4f4/0x8e8
> [ 86.701111] drm_atomic_commit+0x64/0xd8
> [ 86.701120] drm_mode_atomic_ioctl+0xbfc/0xe74
> [ 86.701129] drm_ioctl_kernel+0xd4/0x114
> [ 86.701137] drm_ioctl+0x274/0x508
> [ 86.701143] __arm64_sys_ioctl+0x98/0xd0
> [ 86.701152] invoke_syscall+0x48/0xfc
> [ 86.701161] el0_svc_common+0x88/0xe4
> [ 86.701167] do_el0_svc+0x24/0x30
> [ 86.701175] el0_svc+0x34/0x80
> [ 86.701184] el0t_64_sync_handler+0x44/0xec
> [ 86.701192] el0t_64_sync+0x1a8/0x1ac
> [ 86.701200] ---[ end trace 0000000000000000 ]---
>
> We analysed this and found why. Please see below.
>
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > index a0a28230fc31..8220cd920e6f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > @@ -354,59 +354,6 @@ static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
> > }
> > }
> >
> > -/**
> > - * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
> > - * @phys_enc: Pointer to physical encoder
> > - * @crtc_state: Pointer to CRTC atomic state
> > - * @conn_state: Pointer to connector atomic state
> > - */
> > -static int dpu_encoder_phys_wb_atomic_check(
> > - struct dpu_encoder_phys *phys_enc,
> > - struct drm_crtc_state *crtc_state,
> > - struct drm_connector_state *conn_state)
> > -{
> > - struct drm_framebuffer *fb;
> > - const struct drm_display_mode *mode = &crtc_state->mode;
> > -
> > - DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> > - phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
> > -
> > - if (!conn_state || !conn_state->connector) {
> > - DPU_ERROR("invalid connector state\n");
> > - return -EINVAL;
> > - } else if (conn_state->connector->status !=
> > - connector_status_connected) {
> > - DPU_ERROR("connector not connected %d\n",
> > - conn_state->connector->status);
> > - return -EINVAL;
> > - }
> > -
> > - if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > - return 0;
> > -
> > - fb = conn_state->writeback_job->fb;
> > -
> > - DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
> > - fb->width, fb->height);
> > -
> > - if (fb->width != mode->hdisplay) {
> > - DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
> > - mode->hdisplay);
> > - return -EINVAL;
> > - } else if (fb->height != mode->vdisplay) {
> > - DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
> > - mode->vdisplay);
> > - return -EINVAL;
> > - } else if (fb->width > phys_enc->hw_wb->caps->maxlinewidth) {
> > - DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
> > - fb->width, phys_enc->hw_wb->caps->maxlinewidth);
> > - return -EINVAL;
> > - }
> > -
> > - return drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> > -}
> > -
> > -
> > /**
> > * _dpu_encoder_phys_wb_update_flush - flush hardware update
> > * @phys_enc: Pointer to physical encoder
> > @@ -777,7 +724,6 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
> > ops->is_master = dpu_encoder_phys_wb_is_master;
> > ops->enable = dpu_encoder_phys_wb_enable;
> > ops->disable = dpu_encoder_phys_wb_disable;
> > - ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
> > ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done;
> > ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
> > ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 723cc1d82143..48728be27e15 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -630,23 +630,26 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
> > {
> > struct drm_encoder *encoder = NULL;
> > struct msm_display_info info;
> > + const enum dpu_wb wb_idx = WB_2;
> > + u32 maxlinewidth;
> > int rc;
> >
> > memset(&info, 0, sizeof(info));
> >
> > info.num_of_h_tiles = 1;
> > /* use only WB idx 2 instance for DPU */
> > - info.h_tile_instance[0] = WB_2;
> > + info.h_tile_instance[0] = wb_idx;
> > info.intf_type = INTF_WB;
> >
> > + maxlinewidth = dpu_rm_get_wb(&dpu_kms->rm, info.h_tile_instance[0])->caps->maxlinewidth;
> > +
> > encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
> > if (IS_ERR(encoder)) {
> > DPU_ERROR("encoder init failed for dsi display\n");
> > return PTR_ERR(encoder);
> > }
> >
> > - rc = dpu_writeback_init(dev, encoder, wb_formats,
> > - n_formats);
> > + rc = dpu_writeback_init(dev, encoder, wb_formats, n_formats, maxlinewidth);
> > if (rc) {
> > DPU_ERROR("dpu_writeback_init, rc = %d\n", rc);
> > return rc;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > index 2a5a68366582..232b5f410de8 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > @@ -4,6 +4,7 @@
> > */
> >
> > #include <drm/drm_edid.h>
> > +#include <drm/drm_framebuffer.h>
> >
> > #include "dpu_writeback.h"
> >
> > @@ -24,6 +25,57 @@ static int dpu_wb_conn_get_modes(struct drm_connector *connector)
> > dev->mode_config.max_height);
> > }
> >
> > +static int dpu_wb_conn_atomic_check(struct drm_connector *connector,
> > + struct drm_atomic_state *state)
> > +{
> > + struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
> > + struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(wb_conn);
> > + struct drm_connector_state *conn_state =
> > + drm_atomic_get_new_connector_state(state, connector);
> > + struct drm_crtc *crtc = conn_state->crtc;
> > + struct drm_crtc_state *crtc_state;
> > + const struct drm_display_mode *mode;
> > + struct drm_framebuffer *fb;
> > +
> > + crtc_state = drm_atomic_get_crtc_state(state, crtc);
>
> To detach the CRTC associated with the connector, IGT will set the
> associated CRTC_ID to 0 and the associated conn_state->crtc will be NULL.
>
> This is valid as val will be 0 in this case:
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_uapi.c#L722
>
> Before this patch, for these cases, we used to call the encoder's
> atomic_check which gets skipped when there is no valid crtc:
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L440
>
> But now with connector atomic check, these calls are allowed by the DRM
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L712
>
> So questions:
>
> 1) Should we add protection in DRM to check if conn_state->crtc is valid
> before calling connector's atomic_check()?
I think this is correct. So if !crtc, just return 0. I'll send next
iteration in the next few days.
>
> OR
>
> 2) Is it incorrect for us to dereference conn->crtc in connector's
> atomic_check as its not guaranteed to be valid.
>
> We cannot fail atomic_check for !crtc, because if we add a !crtc check
> and fail those checks, it bails out these disable commit calls thus
> failing those commits.
>
> > + if (IS_ERR(crtc_state))
> > + return PTR_ERR(crtc_state);
> > +
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2024-01-24 19:05 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-25 13:08 [PATCH v3 0/5] drm/msm/dpu: remove dpu_encoder_phys_ops::atomic_mode_set callback Dmitry Baryshkov
2023-12-25 13:08 ` Dmitry Baryshkov
2023-12-25 13:08 ` [PATCH v3 1/5] drm/msm/dpu: split irq_control into irq_enable and _disable Dmitry Baryshkov
2023-12-25 13:08 ` Dmitry Baryshkov
2024-01-08 21:26 ` Abhinav Kumar
2024-01-08 21:26 ` Abhinav Kumar
2023-12-25 13:08 ` [PATCH v3 2/5] drm/msm/dpu: split _dpu_encoder_resource_control_helper() Dmitry Baryshkov
2023-12-25 13:08 ` Dmitry Baryshkov
2024-01-08 21:29 ` Abhinav Kumar
2024-01-08 21:29 ` Abhinav Kumar
2023-12-25 13:08 ` [PATCH v3 3/5] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set Dmitry Baryshkov
2023-12-25 13:08 ` Dmitry Baryshkov
2023-12-25 13:08 ` [PATCH v3 4/5] drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c Dmitry Baryshkov
2023-12-25 13:08 ` Dmitry Baryshkov
2024-01-08 21:39 ` Abhinav Kumar
2024-01-08 21:39 ` Abhinav Kumar
2024-01-08 22:43 ` Dmitry Baryshkov
2024-01-08 22:43 ` Dmitry Baryshkov
2024-01-23 22:23 ` Abhinav Kumar
2024-01-23 22:23 ` Abhinav Kumar
2024-01-24 19:03 ` Dmitry Baryshkov
2024-01-24 19:03 ` Dmitry Baryshkov
2023-12-25 13:08 ` [PATCH v3 5/5] drm/msm/dpu: drop dpu_encoder_phys_ops::atomic_check() Dmitry Baryshkov
2023-12-25 13:08 ` Dmitry Baryshkov
2024-01-08 21:40 ` Abhinav Kumar
2024-01-08 21:40 ` Abhinav Kumar
2024-01-08 21:41 ` [PATCH v3 0/5] drm/msm/dpu: remove dpu_encoder_phys_ops::atomic_mode_set callback Abhinav Kumar
2024-01-08 21:41 ` Abhinav Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.