* [PATCH v4 0/4] Move TE setup to prepare_for_kickoff()
@ 2023-02-21 18:42 ` Jessica Zhang
0 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, dmitry.baryshkov, quic_abhinavk, marijn.suijten
Move TE setup to prepare_for_kickoff() and remove empty prepare_commit()
functions in both MDP4 and DPU drivers.
Changes in V2:
- Added changes to remove empty prepare_commit() functions
Changes in V3:
- Reordered "drm/msm/dpu: Move TE setup to prepare_for_kickoff()" for
clarity
- Fixed spelling mistakes and wording issues
- Picked up "Reviewed-by" tags for patches [2/4] and [4/4]
Changes in V4:
- Reworded commit messages in [1/4] and [3/4] for clarity
- Removed dpu_encoder_phys_cmd_is_ongoing_pptx() function prototype
Jessica Zhang (4):
drm/msm/dpu: Move TE setup to prepare_for_kickoff()
drm/msm: Check for NULL before calling prepare_commit()
drm/msm/dpu: Remove empty prepare_commit() function
drm/msm/mdp4: Remove empty prepare_commit() function
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -----------------
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 ++++---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 -------------------
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 -----
drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
6 files changed, 7 insertions(+), 56 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 0/4] Move TE setup to prepare_for_kickoff()
@ 2023-02-21 18:42 ` Jessica Zhang
0 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
marijn.suijten, dmitry.baryshkov, Jessica Zhang
Move TE setup to prepare_for_kickoff() and remove empty prepare_commit()
functions in both MDP4 and DPU drivers.
Changes in V2:
- Added changes to remove empty prepare_commit() functions
Changes in V3:
- Reordered "drm/msm/dpu: Move TE setup to prepare_for_kickoff()" for
clarity
- Fixed spelling mistakes and wording issues
- Picked up "Reviewed-by" tags for patches [2/4] and [4/4]
Changes in V4:
- Reworded commit messages in [1/4] and [3/4] for clarity
- Removed dpu_encoder_phys_cmd_is_ongoing_pptx() function prototype
Jessica Zhang (4):
drm/msm/dpu: Move TE setup to prepare_for_kickoff()
drm/msm: Check for NULL before calling prepare_commit()
drm/msm/dpu: Remove empty prepare_commit() function
drm/msm/mdp4: Remove empty prepare_commit() function
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -----------------
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 ++++---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 -------------------
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 -----
drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
6 files changed, 7 insertions(+), 56 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
2023-02-21 18:42 ` Jessica Zhang
@ 2023-02-21 18:42 ` Jessica Zhang
-1 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, dmitry.baryshkov, quic_abhinavk, marijn.suijten
Currently, DPU will enable TE during prepare_commit(). However, this
will cause a crash and reboot to sahara when trying to read/write to
register in get_autorefresh_config(), because the core clock rates
aren't set at that time.
This used to work because phys_enc->hw_pp is only initialized in mode
set [1], so the first prepare_commit() will return before any register
read/write as hw_pp would be NULL.
However, when we try to implement support for INTF TE, we will run into
the clock issue described above as hw_intf will *not* be NULL on the
first prepare_commit(). This is because the initialization of
dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
To avoid this issue, let's enable TE during prepare_for_kickoff()
instead as the core clock rates are guaranteed to be set then.
Depends on: "Implement tearcheck support on INTF block" [3]
Changes in V3:
- Added function prototypes
- Reordered function definitions to make change more legible
- Removed prepare_commit() function from dpu_encoder_phys_cmd
Changes in V4:
- Reworded commit message to be more specific
- Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
[2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
[3] https://patchwork.freedesktop.org/series/112332/
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
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 cb05036f2916..98958c75864a 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
@@ -40,6 +40,8 @@
#define DPU_ENC_MAX_POLL_TIMEOUT_US 2000
+static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc);
+
static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
{
return (phys_enc->split_role != ENC_ROLE_SLAVE);
@@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
phys_enc->hw_pp->idx - PINGPONG_0);
}
+ dpu_encoder_phys_cmd_enable_te(phys_enc);
+
DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
phys_enc->hw_pp->idx - PINGPONG_0,
atomic_read(&phys_enc->pending_kickoff_cnt));
@@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
return false;
}
-static void dpu_encoder_phys_cmd_prepare_commit(
- struct dpu_encoder_phys *phys_enc)
+static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
{
struct dpu_encoder_phys_cmd *cmd_enc =
to_dpu_encoder_phys_cmd(phys_enc);
@@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start(
static void dpu_encoder_phys_cmd_init_ops(
struct dpu_encoder_phys_ops *ops)
{
- ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
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;
--
2.39.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
@ 2023-02-21 18:42 ` Jessica Zhang
0 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
marijn.suijten, dmitry.baryshkov, Jessica Zhang
Currently, DPU will enable TE during prepare_commit(). However, this
will cause a crash and reboot to sahara when trying to read/write to
register in get_autorefresh_config(), because the core clock rates
aren't set at that time.
This used to work because phys_enc->hw_pp is only initialized in mode
set [1], so the first prepare_commit() will return before any register
read/write as hw_pp would be NULL.
However, when we try to implement support for INTF TE, we will run into
the clock issue described above as hw_intf will *not* be NULL on the
first prepare_commit(). This is because the initialization of
dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
To avoid this issue, let's enable TE during prepare_for_kickoff()
instead as the core clock rates are guaranteed to be set then.
Depends on: "Implement tearcheck support on INTF block" [3]
Changes in V3:
- Added function prototypes
- Reordered function definitions to make change more legible
- Removed prepare_commit() function from dpu_encoder_phys_cmd
Changes in V4:
- Reworded commit message to be more specific
- Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
[2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
[3] https://patchwork.freedesktop.org/series/112332/
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
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 cb05036f2916..98958c75864a 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
@@ -40,6 +40,8 @@
#define DPU_ENC_MAX_POLL_TIMEOUT_US 2000
+static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc);
+
static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
{
return (phys_enc->split_role != ENC_ROLE_SLAVE);
@@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
phys_enc->hw_pp->idx - PINGPONG_0);
}
+ dpu_encoder_phys_cmd_enable_te(phys_enc);
+
DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
phys_enc->hw_pp->idx - PINGPONG_0,
atomic_read(&phys_enc->pending_kickoff_cnt));
@@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
return false;
}
-static void dpu_encoder_phys_cmd_prepare_commit(
- struct dpu_encoder_phys *phys_enc)
+static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
{
struct dpu_encoder_phys_cmd *cmd_enc =
to_dpu_encoder_phys_cmd(phys_enc);
@@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start(
static void dpu_encoder_phys_cmd_init_ops(
struct dpu_encoder_phys_ops *ops)
{
- ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
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;
--
2.39.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v4 2/4] drm/msm: Check for NULL before calling prepare_commit()
2023-02-21 18:42 ` Jessica Zhang
@ 2023-02-21 18:42 ` Jessica Zhang
-1 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, dmitry.baryshkov, quic_abhinavk, marijn.suijten
Add a NULL check before calling prepare_commit() in
msm_atomic_commit_tail()
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 1686fbb611fd..c8a0a5cc5ca5 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -206,7 +206,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
* Now that there is no in-progress flush, prepare the
* current update:
*/
- kms->funcs->prepare_commit(kms, state);
+ if (kms->funcs->prepare_commit)
+ kms->funcs->prepare_commit(kms, state);
/*
* Push atomic updates down to hardware:
--
2.39.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v4 2/4] drm/msm: Check for NULL before calling prepare_commit()
@ 2023-02-21 18:42 ` Jessica Zhang
0 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
marijn.suijten, dmitry.baryshkov, Jessica Zhang
Add a NULL check before calling prepare_commit() in
msm_atomic_commit_tail()
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 1686fbb611fd..c8a0a5cc5ca5 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -206,7 +206,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
* Now that there is no in-progress flush, prepare the
* current update:
*/
- kms->funcs->prepare_commit(kms, state);
+ if (kms->funcs->prepare_commit)
+ kms->funcs->prepare_commit(kms, state);
/*
* Push atomic updates down to hardware:
--
2.39.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
2023-02-21 18:42 ` Jessica Zhang
@ 2023-02-21 18:42 ` Jessica Zhang
-1 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, dmitry.baryshkov, quic_abhinavk, marijn.suijten
Now that the TE setup has been moved to prepare_for_kickoff(), we have
not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
do nothing. Remove prepare_commit() from DPU driver.
Changes in V3:
- Reworded commit message to be more clear
- Corrected spelling mistake in commit message
Changes in V4:
- Reworded commit message for clarity
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
3 files changed, 47 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index dcceed91aed8..35e120b5ef53 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
ctl->ops.clear_pending_flush(ctl);
}
-void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
-{
- struct dpu_encoder_virt *dpu_enc;
- struct dpu_encoder_phys *phys;
- int i;
-
- if (!drm_enc) {
- DPU_ERROR("invalid encoder\n");
- return;
- }
- dpu_enc = to_dpu_encoder_virt(drm_enc);
-
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- phys = dpu_enc->phys_encs[i];
- if (phys->ops.prepare_commit)
- phys->ops.prepare_commit(phys);
- }
-}
-
#ifdef CONFIG_DEBUG_FS
static int _dpu_encoder_status_show(struct seq_file *s, void *data)
{
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236ef34e6..2c9ef8d1b877 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
struct msm_display_info *disp_info);
-/**
- * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
- * atomic commit, before any registers are written
- * @drm_enc: Pointer to previously created drm encoder structure
- */
-void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
-
/**
* dpu_encoder_set_idle_timeout - set the idle timeout for video
* and command mode encoders.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 165958d47ec6..6f7ddbf0d9b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
return ktime_get();
}
-static void dpu_kms_prepare_commit(struct msm_kms *kms,
- struct drm_atomic_state *state)
-{
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_encoder *encoder;
- int i;
-
- if (!kms)
- return;
-
- /* Call prepare_commit for all affected encoders */
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
- drm_for_each_encoder_mask(encoder, crtc->dev,
- crtc_state->encoder_mask) {
- dpu_encoder_prepare_commit(encoder);
- }
- }
-}
-
static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
{
struct dpu_kms *dpu_kms = to_dpu_kms(kms);
@@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
.enable_commit = dpu_kms_enable_commit,
.disable_commit = dpu_kms_disable_commit,
.vsync_time = dpu_kms_vsync_time,
- .prepare_commit = dpu_kms_prepare_commit,
.flush_commit = dpu_kms_flush_commit,
.wait_flush = dpu_kms_wait_flush,
.complete_commit = dpu_kms_complete_commit,
--
2.39.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
@ 2023-02-21 18:42 ` Jessica Zhang
0 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
marijn.suijten, dmitry.baryshkov, Jessica Zhang
Now that the TE setup has been moved to prepare_for_kickoff(), we have
not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
do nothing. Remove prepare_commit() from DPU driver.
Changes in V3:
- Reworded commit message to be more clear
- Corrected spelling mistake in commit message
Changes in V4:
- Reworded commit message for clarity
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
3 files changed, 47 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index dcceed91aed8..35e120b5ef53 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
ctl->ops.clear_pending_flush(ctl);
}
-void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
-{
- struct dpu_encoder_virt *dpu_enc;
- struct dpu_encoder_phys *phys;
- int i;
-
- if (!drm_enc) {
- DPU_ERROR("invalid encoder\n");
- return;
- }
- dpu_enc = to_dpu_encoder_virt(drm_enc);
-
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- phys = dpu_enc->phys_encs[i];
- if (phys->ops.prepare_commit)
- phys->ops.prepare_commit(phys);
- }
-}
-
#ifdef CONFIG_DEBUG_FS
static int _dpu_encoder_status_show(struct seq_file *s, void *data)
{
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236ef34e6..2c9ef8d1b877 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
struct msm_display_info *disp_info);
-/**
- * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
- * atomic commit, before any registers are written
- * @drm_enc: Pointer to previously created drm encoder structure
- */
-void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
-
/**
* dpu_encoder_set_idle_timeout - set the idle timeout for video
* and command mode encoders.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 165958d47ec6..6f7ddbf0d9b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
return ktime_get();
}
-static void dpu_kms_prepare_commit(struct msm_kms *kms,
- struct drm_atomic_state *state)
-{
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_encoder *encoder;
- int i;
-
- if (!kms)
- return;
-
- /* Call prepare_commit for all affected encoders */
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
- drm_for_each_encoder_mask(encoder, crtc->dev,
- crtc_state->encoder_mask) {
- dpu_encoder_prepare_commit(encoder);
- }
- }
-}
-
static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
{
struct dpu_kms *dpu_kms = to_dpu_kms(kms);
@@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
.enable_commit = dpu_kms_enable_commit,
.disable_commit = dpu_kms_disable_commit,
.vsync_time = dpu_kms_vsync_time,
- .prepare_commit = dpu_kms_prepare_commit,
.flush_commit = dpu_kms_flush_commit,
.wait_flush = dpu_kms_wait_flush,
.complete_commit = dpu_kms_complete_commit,
--
2.39.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v4 4/4] drm/msm/mdp4: Remove empty prepare_commit() function
2023-02-21 18:42 ` Jessica Zhang
@ 2023-02-21 18:42 ` Jessica Zhang
-1 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, dmitry.baryshkov, quic_abhinavk, marijn.suijten
Remove empty prepare_commit() function from MDP4 driver.
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 9a1a0769575d..6e37072ed302 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -84,10 +84,6 @@ static void mdp4_disable_commit(struct msm_kms *kms)
mdp4_disable(mdp4_kms);
}
-static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
-{
-}
-
static void mdp4_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
{
/* TODO */
@@ -154,7 +150,6 @@ static const struct mdp_kms_funcs kms_funcs = {
.disable_vblank = mdp4_disable_vblank,
.enable_commit = mdp4_enable_commit,
.disable_commit = mdp4_disable_commit,
- .prepare_commit = mdp4_prepare_commit,
.flush_commit = mdp4_flush_commit,
.wait_flush = mdp4_wait_flush,
.complete_commit = mdp4_complete_commit,
--
2.39.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v4 4/4] drm/msm/mdp4: Remove empty prepare_commit() function
@ 2023-02-21 18:42 ` Jessica Zhang
0 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-02-21 18:42 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
marijn.suijten, dmitry.baryshkov, Jessica Zhang
Remove empty prepare_commit() function from MDP4 driver.
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 9a1a0769575d..6e37072ed302 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -84,10 +84,6 @@ static void mdp4_disable_commit(struct msm_kms *kms)
mdp4_disable(mdp4_kms);
}
-static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
-{
-}
-
static void mdp4_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
{
/* TODO */
@@ -154,7 +150,6 @@ static const struct mdp_kms_funcs kms_funcs = {
.disable_vblank = mdp4_disable_vblank,
.enable_commit = mdp4_enable_commit,
.disable_commit = mdp4_disable_commit,
- .prepare_commit = mdp4_prepare_commit,
.flush_commit = mdp4_flush_commit,
.wait_flush = mdp4_wait_flush,
.complete_commit = mdp4_complete_commit,
--
2.39.2
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
2023-02-21 18:42 ` Jessica Zhang
@ 2023-03-01 3:21 ` Dmitry Baryshkov
-1 siblings, 0 replies; 38+ messages in thread
From: Dmitry Baryshkov @ 2023-03-01 3:21 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_abhinavk, marijn.suijten
On 21/02/2023 20:42, Jessica Zhang wrote:
> Currently, DPU will enable TE during prepare_commit(). However, this
> will cause a crash and reboot to sahara when trying to read/write to
> register in get_autorefresh_config(), because the core clock rates
> aren't set at that time.
>
> This used to work because phys_enc->hw_pp is only initialized in mode
> set [1], so the first prepare_commit() will return before any register
> read/write as hw_pp would be NULL.
>
> However, when we try to implement support for INTF TE, we will run into
> the clock issue described above as hw_intf will *not* be NULL on the
> first prepare_commit(). This is because the initialization of
> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>
> To avoid this issue, let's enable TE during prepare_for_kickoff()
> instead as the core clock rates are guaranteed to be set then.
>
> Depends on: "Implement tearcheck support on INTF block" [3]
>
> Changes in V3:
> - Added function prototypes
> - Reordered function definitions to make change more legible
> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>
> Changes in V4:
> - Reworded commit message to be more specific
> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
>
> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
> [3] https://patchwork.freedesktop.org/series/112332/
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
@ 2023-03-01 3:21 ` Dmitry Baryshkov
0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Baryshkov @ 2023-03-01 3:21 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
marijn.suijten
On 21/02/2023 20:42, Jessica Zhang wrote:
> Currently, DPU will enable TE during prepare_commit(). However, this
> will cause a crash and reboot to sahara when trying to read/write to
> register in get_autorefresh_config(), because the core clock rates
> aren't set at that time.
>
> This used to work because phys_enc->hw_pp is only initialized in mode
> set [1], so the first prepare_commit() will return before any register
> read/write as hw_pp would be NULL.
>
> However, when we try to implement support for INTF TE, we will run into
> the clock issue described above as hw_intf will *not* be NULL on the
> first prepare_commit(). This is because the initialization of
> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>
> To avoid this issue, let's enable TE during prepare_for_kickoff()
> instead as the core clock rates are guaranteed to be set then.
>
> Depends on: "Implement tearcheck support on INTF block" [3]
>
> Changes in V3:
> - Added function prototypes
> - Reordered function definitions to make change more legible
> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>
> Changes in V4:
> - Reworded commit message to be more specific
> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
>
> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
> [3] https://patchwork.freedesktop.org/series/112332/
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
2023-02-21 18:42 ` Jessica Zhang
@ 2023-03-01 3:22 ` Dmitry Baryshkov
-1 siblings, 0 replies; 38+ messages in thread
From: Dmitry Baryshkov @ 2023-03-01 3:22 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_abhinavk, marijn.suijten
On 21/02/2023 20:42, Jessica Zhang wrote:
> Now that the TE setup has been moved to prepare_for_kickoff(), we have
> not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
> do nothing. Remove prepare_commit() from DPU driver.
>
> Changes in V3:
> - Reworded commit message to be more clear
> - Corrected spelling mistake in commit message
>
> Changes in V4:
> - Reworded commit message for clarity
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
> 3 files changed, 47 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
@ 2023-03-01 3:22 ` Dmitry Baryshkov
0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Baryshkov @ 2023-03-01 3:22 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
marijn.suijten
On 21/02/2023 20:42, Jessica Zhang wrote:
> Now that the TE setup has been moved to prepare_for_kickoff(), we have
> not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
> do nothing. Remove prepare_commit() from DPU driver.
>
> Changes in V3:
> - Reworded commit message to be more clear
> - Corrected spelling mistake in commit message
>
> Changes in V4:
> - Reworded commit message for clarity
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
> 3 files changed, 47 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
2023-02-21 18:42 ` Jessica Zhang
@ 2023-03-01 10:03 ` Marijn Suijten
-1 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:03 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
dmitry.baryshkov, quic_abhinavk
On 2023-02-21 10:42:53, Jessica Zhang wrote:
> Currently, DPU will enable TE during prepare_commit(). However, this
> will cause a crash and reboot to sahara when trying to read/write to
> register in get_autorefresh_config(), because the core clock rates
> aren't set at that time.
Haven't seeen a crash like this on any of my devices (after implementing
INTF TE). get_autorefresh_config() always reads zero (or 1 for
frame_count) except the first time it is called (autorefresh is left
enabled by our bootloader on SM6125) and triggers the disable codepath.
It does look like prepare_for_kickoff() is much more susceptible to
delays in code than prepare_commit(). The debug logs used to figure
this out together with this series result in FPS drops on SM6125 and
SM8150. Not an issue with this series, just something to keep in mind.
> This used to work because phys_enc->hw_pp is only initialized in mode
> set [1], so the first prepare_commit() will return before any register
> read/write as hw_pp would be NULL.
>
> However, when we try to implement support for INTF TE, we will run into
> the clock issue described above as hw_intf will *not* be NULL on the
> first prepare_commit(). This is because the initialization of
> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>
> To avoid this issue, let's enable TE during prepare_for_kickoff()
> instead as the core clock rates are guaranteed to be set then.
For the change itself:
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
And tested on SM6125, SM8150 (INTF TE) and SDM845 (PP TE).
Then, for some patch hygiene, starting here:
> Depends on: "Implement tearcheck support on INTF block" [3]
>
> Changes in V3:
> - Added function prototypes
> - Reordered function definitions to make change more legible
> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>
> Changes in V4:
> - Reworded commit message to be more specific
> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
... until here: all this info belongs /below the cut/ outside of the
messge that becomes part of the commit when this patch is applied to the
tree.
> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
Please replace these with "permalinks" (to a commit hash): a branch with
line number annotation will fall out of date soon as more patches are
applied that touch these files.
> [3] https://patchwork.freedesktop.org/series/112332/
Is this a hard dependency? It seems this series applies cleanly on
-next and - from a cursory view - should be applicable and testable
without my INTF TE series. However, Dmitry asked me to move some code
around in review resulting in separate callbacks in the encoder, rather
than having various if(has_intf_te) within those callbacks. That'll
cause conflicts when I eventually get to respin a v2.
- Marijn
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> 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 cb05036f2916..98958c75864a 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
> @@ -40,6 +40,8 @@
>
> #define DPU_ENC_MAX_POLL_TIMEOUT_US 2000
>
> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc);
> +
> static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
> {
> return (phys_enc->split_role != ENC_ROLE_SLAVE);
> @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
> phys_enc->hw_pp->idx - PINGPONG_0);
> }
>
> + dpu_encoder_phys_cmd_enable_te(phys_enc);
> +
> DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
> phys_enc->hw_pp->idx - PINGPONG_0,
> atomic_read(&phys_enc->pending_kickoff_cnt));
> @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
> return false;
> }
>
> -static void dpu_encoder_phys_cmd_prepare_commit(
> - struct dpu_encoder_phys *phys_enc)
> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
> {
> struct dpu_encoder_phys_cmd *cmd_enc =
> to_dpu_encoder_phys_cmd(phys_enc);
> @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start(
> static void dpu_encoder_phys_cmd_init_ops(
> struct dpu_encoder_phys_ops *ops)
> {
> - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
> 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;
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
@ 2023-03-01 10:03 ` Marijn Suijten
0 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:03 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, freedreno
On 2023-02-21 10:42:53, Jessica Zhang wrote:
> Currently, DPU will enable TE during prepare_commit(). However, this
> will cause a crash and reboot to sahara when trying to read/write to
> register in get_autorefresh_config(), because the core clock rates
> aren't set at that time.
Haven't seeen a crash like this on any of my devices (after implementing
INTF TE). get_autorefresh_config() always reads zero (or 1 for
frame_count) except the first time it is called (autorefresh is left
enabled by our bootloader on SM6125) and triggers the disable codepath.
It does look like prepare_for_kickoff() is much more susceptible to
delays in code than prepare_commit(). The debug logs used to figure
this out together with this series result in FPS drops on SM6125 and
SM8150. Not an issue with this series, just something to keep in mind.
> This used to work because phys_enc->hw_pp is only initialized in mode
> set [1], so the first prepare_commit() will return before any register
> read/write as hw_pp would be NULL.
>
> However, when we try to implement support for INTF TE, we will run into
> the clock issue described above as hw_intf will *not* be NULL on the
> first prepare_commit(). This is because the initialization of
> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>
> To avoid this issue, let's enable TE during prepare_for_kickoff()
> instead as the core clock rates are guaranteed to be set then.
For the change itself:
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
And tested on SM6125, SM8150 (INTF TE) and SDM845 (PP TE).
Then, for some patch hygiene, starting here:
> Depends on: "Implement tearcheck support on INTF block" [3]
>
> Changes in V3:
> - Added function prototypes
> - Reordered function definitions to make change more legible
> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>
> Changes in V4:
> - Reworded commit message to be more specific
> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
... until here: all this info belongs /below the cut/ outside of the
messge that becomes part of the commit when this patch is applied to the
tree.
> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
Please replace these with "permalinks" (to a commit hash): a branch with
line number annotation will fall out of date soon as more patches are
applied that touch these files.
> [3] https://patchwork.freedesktop.org/series/112332/
Is this a hard dependency? It seems this series applies cleanly on
-next and - from a cursory view - should be applicable and testable
without my INTF TE series. However, Dmitry asked me to move some code
around in review resulting in separate callbacks in the encoder, rather
than having various if(has_intf_te) within those callbacks. That'll
cause conflicts when I eventually get to respin a v2.
- Marijn
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> 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 cb05036f2916..98958c75864a 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
> @@ -40,6 +40,8 @@
>
> #define DPU_ENC_MAX_POLL_TIMEOUT_US 2000
>
> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc);
> +
> static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
> {
> return (phys_enc->split_role != ENC_ROLE_SLAVE);
> @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
> phys_enc->hw_pp->idx - PINGPONG_0);
> }
>
> + dpu_encoder_phys_cmd_enable_te(phys_enc);
> +
> DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
> phys_enc->hw_pp->idx - PINGPONG_0,
> atomic_read(&phys_enc->pending_kickoff_cnt));
> @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
> return false;
> }
>
> -static void dpu_encoder_phys_cmd_prepare_commit(
> - struct dpu_encoder_phys *phys_enc)
> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
> {
> struct dpu_encoder_phys_cmd *cmd_enc =
> to_dpu_encoder_phys_cmd(phys_enc);
> @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start(
> static void dpu_encoder_phys_cmd_init_ops(
> struct dpu_encoder_phys_ops *ops)
> {
> - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
> 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;
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 2/4] drm/msm: Check for NULL before calling prepare_commit()
2023-02-21 18:42 ` Jessica Zhang
@ 2023-03-01 10:03 ` Marijn Suijten
-1 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:03 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
dmitry.baryshkov, quic_abhinavk
On 2023-02-21 10:42:54, Jessica Zhang wrote:
> Add a NULL check before calling prepare_commit() in
> msm_atomic_commit_tail()
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
> drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 1686fbb611fd..c8a0a5cc5ca5 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -206,7 +206,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
> * Now that there is no in-progress flush, prepare the
> * current update:
> */
> - kms->funcs->prepare_commit(kms, state);
> + if (kms->funcs->prepare_commit)
> + kms->funcs->prepare_commit(kms, state);
>
> /*
> * Push atomic updates down to hardware:
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 2/4] drm/msm: Check for NULL before calling prepare_commit()
@ 2023-03-01 10:03 ` Marijn Suijten
0 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:03 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, freedreno
On 2023-02-21 10:42:54, Jessica Zhang wrote:
> Add a NULL check before calling prepare_commit() in
> msm_atomic_commit_tail()
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
> drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 1686fbb611fd..c8a0a5cc5ca5 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -206,7 +206,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
> * Now that there is no in-progress flush, prepare the
> * current update:
> */
> - kms->funcs->prepare_commit(kms, state);
> + if (kms->funcs->prepare_commit)
> + kms->funcs->prepare_commit(kms, state);
>
> /*
> * Push atomic updates down to hardware:
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 4/4] drm/msm/mdp4: Remove empty prepare_commit() function
2023-02-21 18:42 ` Jessica Zhang
@ 2023-03-01 10:06 ` Marijn Suijten
-1 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:06 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
dmitry.baryshkov, quic_abhinavk
On 2023-02-21 10:42:56, Jessica Zhang wrote:
> Remove empty prepare_commit() function from MDP4 driver.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 9a1a0769575d..6e37072ed302 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -84,10 +84,6 @@ static void mdp4_disable_commit(struct msm_kms *kms)
> mdp4_disable(mdp4_kms);
> }
>
> -static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
> -{
> -}
> -
> static void mdp4_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> {
> /* TODO */
> @@ -154,7 +150,6 @@ static const struct mdp_kms_funcs kms_funcs = {
> .disable_vblank = mdp4_disable_vblank,
> .enable_commit = mdp4_enable_commit,
> .disable_commit = mdp4_disable_commit,
> - .prepare_commit = mdp4_prepare_commit,
> .flush_commit = mdp4_flush_commit,
> .wait_flush = mdp4_wait_flush,
> .complete_commit = mdp4_complete_commit,
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 4/4] drm/msm/mdp4: Remove empty prepare_commit() function
@ 2023-03-01 10:06 ` Marijn Suijten
0 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:06 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, freedreno
On 2023-02-21 10:42:56, Jessica Zhang wrote:
> Remove empty prepare_commit() function from MDP4 driver.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 9a1a0769575d..6e37072ed302 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -84,10 +84,6 @@ static void mdp4_disable_commit(struct msm_kms *kms)
> mdp4_disable(mdp4_kms);
> }
>
> -static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
> -{
> -}
> -
> static void mdp4_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> {
> /* TODO */
> @@ -154,7 +150,6 @@ static const struct mdp_kms_funcs kms_funcs = {
> .disable_vblank = mdp4_disable_vblank,
> .enable_commit = mdp4_enable_commit,
> .disable_commit = mdp4_disable_commit,
> - .prepare_commit = mdp4_prepare_commit,
> .flush_commit = mdp4_flush_commit,
> .wait_flush = mdp4_wait_flush,
> .complete_commit = mdp4_complete_commit,
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
2023-02-21 18:42 ` Jessica Zhang
@ 2023-03-01 10:08 ` Marijn Suijten
-1 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:08 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, freedreno
On 2023-02-21 10:42:55, Jessica Zhang wrote:
> Now that the TE setup has been moved to prepare_for_kickoff(), we have
> not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
s/not/no
> do nothing. Remove prepare_commit() from DPU driver.
And again, this:
> Changes in V3:
> - Reworded commit message to be more clear
> - Corrected spelling mistake in commit message
>
> Changes in V4:
> - Reworded commit message for clarity
... should go below the cut.
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
With the above two issues fixed:
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
> 3 files changed, 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index dcceed91aed8..35e120b5ef53 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
> ctl->ops.clear_pending_flush(ctl);
> }
>
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> -{
> - struct dpu_encoder_virt *dpu_enc;
> - struct dpu_encoder_phys *phys;
> - int i;
> -
> - if (!drm_enc) {
> - DPU_ERROR("invalid encoder\n");
> - return;
> - }
> - dpu_enc = to_dpu_encoder_virt(drm_enc);
> -
> - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> - phys = dpu_enc->phys_encs[i];
> - if (phys->ops.prepare_commit)
> - phys->ops.prepare_commit(phys);
> - }
> -}
> -
> #ifdef CONFIG_DEBUG_FS
> static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236ef34e6..2c9ef8d1b877 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
> int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> struct msm_display_info *disp_info);
>
> -/**
> - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
> - * atomic commit, before any registers are written
> - * @drm_enc: Pointer to previously created drm encoder structure
> - */
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> -
> /**
> * dpu_encoder_set_idle_timeout - set the idle timeout for video
> * and command mode encoders.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 165958d47ec6..6f7ddbf0d9b7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
> return ktime_get();
> }
>
> -static void dpu_kms_prepare_commit(struct msm_kms *kms,
> - struct drm_atomic_state *state)
> -{
> - struct drm_crtc *crtc;
> - struct drm_crtc_state *crtc_state;
> - struct drm_encoder *encoder;
> - int i;
> -
> - if (!kms)
> - return;
> -
> - /* Call prepare_commit for all affected encoders */
> - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> - drm_for_each_encoder_mask(encoder, crtc->dev,
> - crtc_state->encoder_mask) {
> - dpu_encoder_prepare_commit(encoder);
> - }
> - }
> -}
> -
> static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> {
> struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
> .enable_commit = dpu_kms_enable_commit,
> .disable_commit = dpu_kms_disable_commit,
> .vsync_time = dpu_kms_vsync_time,
> - .prepare_commit = dpu_kms_prepare_commit,
> .flush_commit = dpu_kms_flush_commit,
> .wait_flush = dpu_kms_wait_flush,
> .complete_commit = dpu_kms_complete_commit,
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
@ 2023-03-01 10:08 ` Marijn Suijten
0 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:08 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
dmitry.baryshkov, quic_abhinavk
On 2023-02-21 10:42:55, Jessica Zhang wrote:
> Now that the TE setup has been moved to prepare_for_kickoff(), we have
> not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
s/not/no
> do nothing. Remove prepare_commit() from DPU driver.
And again, this:
> Changes in V3:
> - Reworded commit message to be more clear
> - Corrected spelling mistake in commit message
>
> Changes in V4:
> - Reworded commit message for clarity
... should go below the cut.
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
With the above two issues fixed:
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
> 3 files changed, 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index dcceed91aed8..35e120b5ef53 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
> ctl->ops.clear_pending_flush(ctl);
> }
>
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> -{
> - struct dpu_encoder_virt *dpu_enc;
> - struct dpu_encoder_phys *phys;
> - int i;
> -
> - if (!drm_enc) {
> - DPU_ERROR("invalid encoder\n");
> - return;
> - }
> - dpu_enc = to_dpu_encoder_virt(drm_enc);
> -
> - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> - phys = dpu_enc->phys_encs[i];
> - if (phys->ops.prepare_commit)
> - phys->ops.prepare_commit(phys);
> - }
> -}
> -
> #ifdef CONFIG_DEBUG_FS
> static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236ef34e6..2c9ef8d1b877 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
> int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> struct msm_display_info *disp_info);
>
> -/**
> - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
> - * atomic commit, before any registers are written
> - * @drm_enc: Pointer to previously created drm encoder structure
> - */
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> -
> /**
> * dpu_encoder_set_idle_timeout - set the idle timeout for video
> * and command mode encoders.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 165958d47ec6..6f7ddbf0d9b7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
> return ktime_get();
> }
>
> -static void dpu_kms_prepare_commit(struct msm_kms *kms,
> - struct drm_atomic_state *state)
> -{
> - struct drm_crtc *crtc;
> - struct drm_crtc_state *crtc_state;
> - struct drm_encoder *encoder;
> - int i;
> -
> - if (!kms)
> - return;
> -
> - /* Call prepare_commit for all affected encoders */
> - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> - drm_for_each_encoder_mask(encoder, crtc->dev,
> - crtc_state->encoder_mask) {
> - dpu_encoder_prepare_commit(encoder);
> - }
> - }
> -}
> -
> static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> {
> struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
> .enable_commit = dpu_kms_enable_commit,
> .disable_commit = dpu_kms_disable_commit,
> .vsync_time = dpu_kms_vsync_time,
> - .prepare_commit = dpu_kms_prepare_commit,
> .flush_commit = dpu_kms_flush_commit,
> .wait_flush = dpu_kms_wait_flush,
> .complete_commit = dpu_kms_complete_commit,
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
2023-03-01 10:08 ` Marijn Suijten
@ 2023-03-01 10:13 ` Marijn Suijten
-1 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:13 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
dmitry.baryshkov, quic_abhinavk
On 2023-03-01 11:08:16, Marijn Suijten wrote:
> On 2023-02-21 10:42:55, Jessica Zhang wrote:
> > Now that the TE setup has been moved to prepare_for_kickoff(), we have
> > not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
>
> s/not/no
>
> > do nothing. Remove prepare_commit() from DPU driver.
>
> And again, this:
>
> > Changes in V3:
> > - Reworded commit message to be more clear
> > - Corrected spelling mistake in commit message
> >
> > Changes in V4:
> > - Reworded commit message for clarity
>
> ... should go below the cut.
>
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>
> With the above two issues fixed:
>
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
> > 3 files changed, 47 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index dcceed91aed8..35e120b5ef53 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
> > ctl->ops.clear_pending_flush(ctl);
> > }
> >
> > -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> > -{
> > - struct dpu_encoder_virt *dpu_enc;
> > - struct dpu_encoder_phys *phys;
> > - int i;
> > -
> > - if (!drm_enc) {
> > - DPU_ERROR("invalid encoder\n");
> > - return;
> > - }
> > - dpu_enc = to_dpu_encoder_virt(drm_enc);
> > -
> > - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > - phys = dpu_enc->phys_encs[i];
> > - if (phys->ops.prepare_commit)
> > - phys->ops.prepare_commit(phys);
In hindsight, Dmitry asked in v2 to remove prepare_commit from
dpu_encoder_phys_ops (and its documentation comment) in
dpu_encoder_phys.h, but that has not happened yet. Can we do that in a
v5?
- Marijn
> > - }
> > -}
> > -
> > #ifdef CONFIG_DEBUG_FS
> > static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> > {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index 9e7236ef34e6..2c9ef8d1b877 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
> > int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> > struct msm_display_info *disp_info);
> >
> > -/**
> > - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
> > - * atomic commit, before any registers are written
> > - * @drm_enc: Pointer to previously created drm encoder structure
> > - */
> > -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> > -
> > /**
> > * dpu_encoder_set_idle_timeout - set the idle timeout for video
> > * and command mode encoders.
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 165958d47ec6..6f7ddbf0d9b7 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
> > return ktime_get();
> > }
> >
> > -static void dpu_kms_prepare_commit(struct msm_kms *kms,
> > - struct drm_atomic_state *state)
> > -{
> > - struct drm_crtc *crtc;
> > - struct drm_crtc_state *crtc_state;
> > - struct drm_encoder *encoder;
> > - int i;
> > -
> > - if (!kms)
> > - return;
> > -
> > - /* Call prepare_commit for all affected encoders */
> > - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > - drm_for_each_encoder_mask(encoder, crtc->dev,
> > - crtc_state->encoder_mask) {
> > - dpu_encoder_prepare_commit(encoder);
> > - }
> > - }
> > -}
> > -
> > static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> > {
> > struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> > @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
> > .enable_commit = dpu_kms_enable_commit,
> > .disable_commit = dpu_kms_disable_commit,
> > .vsync_time = dpu_kms_vsync_time,
> > - .prepare_commit = dpu_kms_prepare_commit,
> > .flush_commit = dpu_kms_flush_commit,
> > .wait_flush = dpu_kms_wait_flush,
> > .complete_commit = dpu_kms_complete_commit,
> > --
> > 2.39.2
> >
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
@ 2023-03-01 10:13 ` Marijn Suijten
0 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 10:13 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, freedreno
On 2023-03-01 11:08:16, Marijn Suijten wrote:
> On 2023-02-21 10:42:55, Jessica Zhang wrote:
> > Now that the TE setup has been moved to prepare_for_kickoff(), we have
> > not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
>
> s/not/no
>
> > do nothing. Remove prepare_commit() from DPU driver.
>
> And again, this:
>
> > Changes in V3:
> > - Reworded commit message to be more clear
> > - Corrected spelling mistake in commit message
> >
> > Changes in V4:
> > - Reworded commit message for clarity
>
> ... should go below the cut.
>
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>
> With the above two issues fixed:
>
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
> > 3 files changed, 47 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index dcceed91aed8..35e120b5ef53 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
> > ctl->ops.clear_pending_flush(ctl);
> > }
> >
> > -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> > -{
> > - struct dpu_encoder_virt *dpu_enc;
> > - struct dpu_encoder_phys *phys;
> > - int i;
> > -
> > - if (!drm_enc) {
> > - DPU_ERROR("invalid encoder\n");
> > - return;
> > - }
> > - dpu_enc = to_dpu_encoder_virt(drm_enc);
> > -
> > - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > - phys = dpu_enc->phys_encs[i];
> > - if (phys->ops.prepare_commit)
> > - phys->ops.prepare_commit(phys);
In hindsight, Dmitry asked in v2 to remove prepare_commit from
dpu_encoder_phys_ops (and its documentation comment) in
dpu_encoder_phys.h, but that has not happened yet. Can we do that in a
v5?
- Marijn
> > - }
> > -}
> > -
> > #ifdef CONFIG_DEBUG_FS
> > static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> > {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index 9e7236ef34e6..2c9ef8d1b877 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
> > int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> > struct msm_display_info *disp_info);
> >
> > -/**
> > - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
> > - * atomic commit, before any registers are written
> > - * @drm_enc: Pointer to previously created drm encoder structure
> > - */
> > -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> > -
> > /**
> > * dpu_encoder_set_idle_timeout - set the idle timeout for video
> > * and command mode encoders.
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 165958d47ec6..6f7ddbf0d9b7 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
> > return ktime_get();
> > }
> >
> > -static void dpu_kms_prepare_commit(struct msm_kms *kms,
> > - struct drm_atomic_state *state)
> > -{
> > - struct drm_crtc *crtc;
> > - struct drm_crtc_state *crtc_state;
> > - struct drm_encoder *encoder;
> > - int i;
> > -
> > - if (!kms)
> > - return;
> > -
> > - /* Call prepare_commit for all affected encoders */
> > - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > - drm_for_each_encoder_mask(encoder, crtc->dev,
> > - crtc_state->encoder_mask) {
> > - dpu_encoder_prepare_commit(encoder);
> > - }
> > - }
> > -}
> > -
> > static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> > {
> > struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> > @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
> > .enable_commit = dpu_kms_enable_commit,
> > .disable_commit = dpu_kms_disable_commit,
> > .vsync_time = dpu_kms_vsync_time,
> > - .prepare_commit = dpu_kms_prepare_commit,
> > .flush_commit = dpu_kms_flush_commit,
> > .wait_flush = dpu_kms_wait_flush,
> > .complete_commit = dpu_kms_complete_commit,
> > --
> > 2.39.2
> >
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
2023-03-01 10:03 ` Marijn Suijten
@ 2023-03-01 16:23 ` Abhinav Kumar
-1 siblings, 0 replies; 38+ messages in thread
From: Abhinav Kumar @ 2023-03-01 16:23 UTC (permalink / raw)
To: Marijn Suijten, Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
dmitry.baryshkov
On 3/1/2023 2:03 AM, Marijn Suijten wrote:
> On 2023-02-21 10:42:53, Jessica Zhang wrote:
>> Currently, DPU will enable TE during prepare_commit(). However, this
>> will cause a crash and reboot to sahara when trying to read/write to
>> register in get_autorefresh_config(), because the core clock rates
>> aren't set at that time.
>
> Haven't seeen a crash like this on any of my devices (after implementing
> INTF TE). get_autorefresh_config() always reads zero (or 1 for
> frame_count) except the first time it is called (autorefresh is left
> enabled by our bootloader on SM6125) and triggers the disable codepath.
>
I feel that the fact that bootloader keeps things on for you is the
reason you dont see the issue. With continuoush splash, clocks are kept
enabled. We dont have it enabled (confirmed that).
> It does look like prepare_for_kickoff() is much more susceptible to
> delays in code than prepare_commit(). The debug logs used to figure
> this out together with this series result in FPS drops on SM6125 and
> SM8150. Not an issue with this series, just something to keep in mind.
>
>> This used to work because phys_enc->hw_pp is only initialized in mode
>> set [1], so the first prepare_commit() will return before any register
>> read/write as hw_pp would be NULL.
>>
>> However, when we try to implement support for INTF TE, we will run into
>> the clock issue described above as hw_intf will *not* be NULL on the
>> first prepare_commit(). This is because the initialization of
>> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>>
>> To avoid this issue, let's enable TE during prepare_for_kickoff()
>> instead as the core clock rates are guaranteed to be set then.
>
> For the change itself:
>
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>
> And tested on SM6125, SM8150 (INTF TE) and SDM845 (PP TE).
>
Thanks.
> Then, for some patch hygiene, starting here:
>
>> Depends on: "Implement tearcheck support on INTF block" [3]
>>
>> Changes in V3:
>> - Added function prototypes
>> - Reordered function definitions to make change more legible
>> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>>
>> Changes in V4:
>> - Reworded commit message to be more specific
>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
>
> ... until here: all this info belongs /below the cut/ outside of the
> messge that becomes part of the commit when this patch is applied to the
> tree.
For DRM, I thought we are keeping the change log above the ---- ?
Which means its allowed in the commit message.
>
>> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
>> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
>
> Please replace these with "permalinks" (to a commit hash): a branch with
> line number annotation will fall out of date soon as more patches are
> applied that touch these files.
>
>> [3] https://patchwork.freedesktop.org/series/112332/
>
> Is this a hard dependency? It seems this series applies cleanly on
> -next and - from a cursory view - should be applicable and testable
> without my INTF TE series. However, Dmitry asked me to move some code
> around in review resulting in separate callbacks in the encoder, rather
> than having various if(has_intf_te) within those callbacks. That'll
> cause conflicts when I eventually get to respin a v2.
>
I guess Jessica listed this because without intf_te series there is no
crash because hw_pp would be NULL and autorefresh() would return early.
So dependency is from the standpoint of when this series is needed and
not from compilation point of view.
> - Marijn
>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> 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 cb05036f2916..98958c75864a 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
>> @@ -40,6 +40,8 @@
>>
>> #define DPU_ENC_MAX_POLL_TIMEOUT_US 2000
>>
>> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc);
>> +
>> static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
>> {
>> return (phys_enc->split_role != ENC_ROLE_SLAVE);
>> @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>> phys_enc->hw_pp->idx - PINGPONG_0);
>> }
>>
>> + dpu_encoder_phys_cmd_enable_te(phys_enc);
>> +
>> DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
>> phys_enc->hw_pp->idx - PINGPONG_0,
>> atomic_read(&phys_enc->pending_kickoff_cnt));
>> @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>> return false;
>> }
>>
>> -static void dpu_encoder_phys_cmd_prepare_commit(
>> - struct dpu_encoder_phys *phys_enc)
>> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
>> {
>> struct dpu_encoder_phys_cmd *cmd_enc =
>> to_dpu_encoder_phys_cmd(phys_enc);
>> @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start(
>> static void dpu_encoder_phys_cmd_init_ops(
>> struct dpu_encoder_phys_ops *ops)
>> {
>> - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>> 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;
>> --
>> 2.39.2
>>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
@ 2023-03-01 16:23 ` Abhinav Kumar
0 siblings, 0 replies; 38+ messages in thread
From: Abhinav Kumar @ 2023-03-01 16:23 UTC (permalink / raw)
To: Marijn Suijten, Jessica Zhang
Cc: linux-arm-msm, dri-devel, swboyd, seanpaul, dmitry.baryshkov, freedreno
On 3/1/2023 2:03 AM, Marijn Suijten wrote:
> On 2023-02-21 10:42:53, Jessica Zhang wrote:
>> Currently, DPU will enable TE during prepare_commit(). However, this
>> will cause a crash and reboot to sahara when trying to read/write to
>> register in get_autorefresh_config(), because the core clock rates
>> aren't set at that time.
>
> Haven't seeen a crash like this on any of my devices (after implementing
> INTF TE). get_autorefresh_config() always reads zero (or 1 for
> frame_count) except the first time it is called (autorefresh is left
> enabled by our bootloader on SM6125) and triggers the disable codepath.
>
I feel that the fact that bootloader keeps things on for you is the
reason you dont see the issue. With continuoush splash, clocks are kept
enabled. We dont have it enabled (confirmed that).
> It does look like prepare_for_kickoff() is much more susceptible to
> delays in code than prepare_commit(). The debug logs used to figure
> this out together with this series result in FPS drops on SM6125 and
> SM8150. Not an issue with this series, just something to keep in mind.
>
>> This used to work because phys_enc->hw_pp is only initialized in mode
>> set [1], so the first prepare_commit() will return before any register
>> read/write as hw_pp would be NULL.
>>
>> However, when we try to implement support for INTF TE, we will run into
>> the clock issue described above as hw_intf will *not* be NULL on the
>> first prepare_commit(). This is because the initialization of
>> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>>
>> To avoid this issue, let's enable TE during prepare_for_kickoff()
>> instead as the core clock rates are guaranteed to be set then.
>
> For the change itself:
>
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>
> And tested on SM6125, SM8150 (INTF TE) and SDM845 (PP TE).
>
Thanks.
> Then, for some patch hygiene, starting here:
>
>> Depends on: "Implement tearcheck support on INTF block" [3]
>>
>> Changes in V3:
>> - Added function prototypes
>> - Reordered function definitions to make change more legible
>> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>>
>> Changes in V4:
>> - Reworded commit message to be more specific
>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
>
> ... until here: all this info belongs /below the cut/ outside of the
> messge that becomes part of the commit when this patch is applied to the
> tree.
For DRM, I thought we are keeping the change log above the ---- ?
Which means its allowed in the commit message.
>
>> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
>> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
>
> Please replace these with "permalinks" (to a commit hash): a branch with
> line number annotation will fall out of date soon as more patches are
> applied that touch these files.
>
>> [3] https://patchwork.freedesktop.org/series/112332/
>
> Is this a hard dependency? It seems this series applies cleanly on
> -next and - from a cursory view - should be applicable and testable
> without my INTF TE series. However, Dmitry asked me to move some code
> around in review resulting in separate callbacks in the encoder, rather
> than having various if(has_intf_te) within those callbacks. That'll
> cause conflicts when I eventually get to respin a v2.
>
I guess Jessica listed this because without intf_te series there is no
crash because hw_pp would be NULL and autorefresh() would return early.
So dependency is from the standpoint of when this series is needed and
not from compilation point of view.
> - Marijn
>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> 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 cb05036f2916..98958c75864a 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
>> @@ -40,6 +40,8 @@
>>
>> #define DPU_ENC_MAX_POLL_TIMEOUT_US 2000
>>
>> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc);
>> +
>> static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
>> {
>> return (phys_enc->split_role != ENC_ROLE_SLAVE);
>> @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>> phys_enc->hw_pp->idx - PINGPONG_0);
>> }
>>
>> + dpu_encoder_phys_cmd_enable_te(phys_enc);
>> +
>> DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
>> phys_enc->hw_pp->idx - PINGPONG_0,
>> atomic_read(&phys_enc->pending_kickoff_cnt));
>> @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>> return false;
>> }
>>
>> -static void dpu_encoder_phys_cmd_prepare_commit(
>> - struct dpu_encoder_phys *phys_enc)
>> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
>> {
>> struct dpu_encoder_phys_cmd *cmd_enc =
>> to_dpu_encoder_phys_cmd(phys_enc);
>> @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start(
>> static void dpu_encoder_phys_cmd_init_ops(
>> struct dpu_encoder_phys_ops *ops)
>> {
>> - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>> 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;
>> --
>> 2.39.2
>>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
2023-03-01 16:23 ` Abhinav Kumar
@ 2023-03-01 17:08 ` Marijn Suijten
-1 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 17:08 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Jessica Zhang, freedreno, linux-arm-msm, dri-devel, robdclark,
seanpaul, swboyd, dmitry.baryshkov
On 2023-03-01 08:23:28, Abhinav Kumar wrote:
>
> On 3/1/2023 2:03 AM, Marijn Suijten wrote:
> > On 2023-02-21 10:42:53, Jessica Zhang wrote:
> >> Currently, DPU will enable TE during prepare_commit(). However, this
> >> will cause a crash and reboot to sahara when trying to read/write to
> >> register in get_autorefresh_config(), because the core clock rates
> >> aren't set at that time.
> >
> > Haven't seeen a crash like this on any of my devices (after implementing
> > INTF TE). get_autorefresh_config() always reads zero (or 1 for
> > frame_count) except the first time it is called (autorefresh is left
> > enabled by our bootloader on SM6125) and triggers the disable codepath.
> >
>
> I feel that the fact that bootloader keeps things on for you is the
> reason you dont see the issue. With continuoush splash, clocks are kept
> enabled. We dont have it enabled (confirmed that).
That is quite likely, we may even have them enabled because of
simple-framebuffer in DTs; turning those off likely won't have any
effect for testing this.
For what it's worth, my SM8150 reads 0 for autorefresh.
<snip>
> > Then, for some patch hygiene, starting here:
> >
> >> Depends on: "Implement tearcheck support on INTF block" [3]
> >>
> >> Changes in V3:
> >> - Added function prototypes
> >> - Reordered function definitions to make change more legible
> >> - Removed prepare_commit() function from dpu_encoder_phys_cmd
> >>
> >> Changes in V4:
> >> - Reworded commit message to be more specific
> >> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
> >
> > ... until here: all this info belongs /below the cut/ outside of the
> > messge that becomes part of the commit when this patch is applied to the
> > tree.
>
> For DRM, I thought we are keeping the change log above the ---- ?
> Which means its allowed in the commit message.
I hope not, seems unlikely to have different rules across kernel
subsystems. The main point is that this changelog and dependency chain
isn't of any value when the final patch is applied, regardless of
whether it is "allowed".
> >> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> >> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
> >
> > Please replace these with "permalinks" (to a commit hash): a branch with
> > line number annotation will fall out of date soon as more patches are
> > applied that touch these files.
> >
> >> [3] https://patchwork.freedesktop.org/series/112332/
> >
> > Is this a hard dependency? It seems this series applies cleanly on
> > -next and - from a cursory view - should be applicable and testable
> > without my INTF TE series. However, Dmitry asked me to move some code
> > around in review resulting in separate callbacks in the encoder, rather
> > than having various if(has_intf_te) within those callbacks. That'll
> > cause conflicts when I eventually get to respin a v2.
> >
>
> I guess Jessica listed this because without intf_te series there is no
> crash because hw_pp would be NULL and autorefresh() would return early.
> So dependency is from the standpoint of when this series is needed and
> not from compilation point of view.
That is indeed the question. I'll leave it to the maintainers to decide
what order to apply these in, which we should be made aware of before
submitting v2 so that one of us can resolve the conflicts.
- Marijn
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
@ 2023-03-01 17:08 ` Marijn Suijten
0 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-01 17:08 UTC (permalink / raw)
To: Abhinav Kumar
Cc: linux-arm-msm, dri-devel, swboyd, seanpaul, dmitry.baryshkov,
Jessica Zhang, freedreno
On 2023-03-01 08:23:28, Abhinav Kumar wrote:
>
> On 3/1/2023 2:03 AM, Marijn Suijten wrote:
> > On 2023-02-21 10:42:53, Jessica Zhang wrote:
> >> Currently, DPU will enable TE during prepare_commit(). However, this
> >> will cause a crash and reboot to sahara when trying to read/write to
> >> register in get_autorefresh_config(), because the core clock rates
> >> aren't set at that time.
> >
> > Haven't seeen a crash like this on any of my devices (after implementing
> > INTF TE). get_autorefresh_config() always reads zero (or 1 for
> > frame_count) except the first time it is called (autorefresh is left
> > enabled by our bootloader on SM6125) and triggers the disable codepath.
> >
>
> I feel that the fact that bootloader keeps things on for you is the
> reason you dont see the issue. With continuoush splash, clocks are kept
> enabled. We dont have it enabled (confirmed that).
That is quite likely, we may even have them enabled because of
simple-framebuffer in DTs; turning those off likely won't have any
effect for testing this.
For what it's worth, my SM8150 reads 0 for autorefresh.
<snip>
> > Then, for some patch hygiene, starting here:
> >
> >> Depends on: "Implement tearcheck support on INTF block" [3]
> >>
> >> Changes in V3:
> >> - Added function prototypes
> >> - Reordered function definitions to make change more legible
> >> - Removed prepare_commit() function from dpu_encoder_phys_cmd
> >>
> >> Changes in V4:
> >> - Reworded commit message to be more specific
> >> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
> >
> > ... until here: all this info belongs /below the cut/ outside of the
> > messge that becomes part of the commit when this patch is applied to the
> > tree.
>
> For DRM, I thought we are keeping the change log above the ---- ?
> Which means its allowed in the commit message.
I hope not, seems unlikely to have different rules across kernel
subsystems. The main point is that this changelog and dependency chain
isn't of any value when the final patch is applied, regardless of
whether it is "allowed".
> >> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> >> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
> >
> > Please replace these with "permalinks" (to a commit hash): a branch with
> > line number annotation will fall out of date soon as more patches are
> > applied that touch these files.
> >
> >> [3] https://patchwork.freedesktop.org/series/112332/
> >
> > Is this a hard dependency? It seems this series applies cleanly on
> > -next and - from a cursory view - should be applicable and testable
> > without my INTF TE series. However, Dmitry asked me to move some code
> > around in review resulting in separate callbacks in the encoder, rather
> > than having various if(has_intf_te) within those callbacks. That'll
> > cause conflicts when I eventually get to respin a v2.
> >
>
> I guess Jessica listed this because without intf_te series there is no
> crash because hw_pp would be NULL and autorefresh() would return early.
> So dependency is from the standpoint of when this series is needed and
> not from compilation point of view.
That is indeed the question. I'll leave it to the maintainers to decide
what order to apply these in, which we should be made aware of before
submitting v2 so that one of us can resolve the conflicts.
- Marijn
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
2023-03-01 17:08 ` Marijn Suijten
@ 2023-03-01 21:42 ` Abhinav Kumar
-1 siblings, 0 replies; 38+ messages in thread
From: Abhinav Kumar @ 2023-03-01 21:42 UTC (permalink / raw)
To: Marijn Suijten
Cc: Jessica Zhang, freedreno, linux-arm-msm, dri-devel, robdclark,
seanpaul, swboyd, dmitry.baryshkov
On 3/1/2023 9:08 AM, Marijn Suijten wrote:
> On 2023-03-01 08:23:28, Abhinav Kumar wrote:
>>
>> On 3/1/2023 2:03 AM, Marijn Suijten wrote:
>>> On 2023-02-21 10:42:53, Jessica Zhang wrote:
>>>> Currently, DPU will enable TE during prepare_commit(). However, this
>>>> will cause a crash and reboot to sahara when trying to read/write to
>>>> register in get_autorefresh_config(), because the core clock rates
>>>> aren't set at that time.
>>>
>>> Haven't seeen a crash like this on any of my devices (after implementing
>>> INTF TE). get_autorefresh_config() always reads zero (or 1 for
>>> frame_count) except the first time it is called (autorefresh is left
>>> enabled by our bootloader on SM6125) and triggers the disable codepath.
>>>
>>
>> I feel that the fact that bootloader keeps things on for you is the
>> reason you dont see the issue. With continuoush splash, clocks are kept
>> enabled. We dont have it enabled (confirmed that).
>
> That is quite likely, we may even have them enabled because of
> simple-framebuffer in DTs; turning those off likely won't have any
> effect for testing this.
>
> For what it's worth, my SM8150 reads 0 for autorefresh.
>
the value shouldnt really matter. The fact that you are able to read
that register without crashing like we are means your clocks are on and
ours arent. Thats what this change is fixing.
> <snip>
>
>>> Then, for some patch hygiene, starting here:
>>>
>>>> Depends on: "Implement tearcheck support on INTF block" [3]
>>>>
>>>> Changes in V3:
>>>> - Added function prototypes
>>>> - Reordered function definitions to make change more legible
>>>> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>>>>
>>>> Changes in V4:
>>>> - Reworded commit message to be more specific
>>>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
>>>
>>> ... until here: all this info belongs /below the cut/ outside of the
>>> messge that becomes part of the commit when this patch is applied to the
>>> tree.
>>
>> For DRM, I thought we are keeping the change log above the ---- ?
>> Which means its allowed in the commit message.
>
> I hope not, seems unlikely to have different rules across kernel
> subsystems. The main point is that this changelog and dependency chain
> isn't of any value when the final patch is applied, regardless of
> whether it is "allowed".
>
I looked at a recently posted change by Rob and change log is above the ---
https://patchwork.kernel.org/project/dri-devel/patch/20230301185432.3010939-1-robdclark@gmail.com/
So we will follow that.
>>>> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
>>>> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
>>>
>>> Please replace these with "permalinks" (to a commit hash): a branch with
>>> line number annotation will fall out of date soon as more patches are
>>> applied that touch these files.
>>>
>>>> [3] https://patchwork.freedesktop.org/series/112332/
>>>
>>> Is this a hard dependency? It seems this series applies cleanly on
>>> -next and - from a cursory view - should be applicable and testable
>>> without my INTF TE series. However, Dmitry asked me to move some code
>>> around in review resulting in separate callbacks in the encoder, rather
>>> than having various if(has_intf_te) within those callbacks. That'll
>>> cause conflicts when I eventually get to respin a v2.
>>>
>>
>> I guess Jessica listed this because without intf_te series there is no
>> crash because hw_pp would be NULL and autorefresh() would return early.
>> So dependency is from the standpoint of when this series is needed and
>> not from compilation point of view.
>
> That is indeed the question. I'll leave it to the maintainers to decide
> what order to apply these in, which we should be made aware of before
> submitting v2 so that one of us can resolve the conflicts.
>
It should be first the intf TE series and then this one. You can go
ahead and post your v2, we will rebase on top of yours.
> - Marijn
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
@ 2023-03-01 21:42 ` Abhinav Kumar
0 siblings, 0 replies; 38+ messages in thread
From: Abhinav Kumar @ 2023-03-01 21:42 UTC (permalink / raw)
To: Marijn Suijten
Cc: linux-arm-msm, dri-devel, swboyd, seanpaul, dmitry.baryshkov,
Jessica Zhang, freedreno
On 3/1/2023 9:08 AM, Marijn Suijten wrote:
> On 2023-03-01 08:23:28, Abhinav Kumar wrote:
>>
>> On 3/1/2023 2:03 AM, Marijn Suijten wrote:
>>> On 2023-02-21 10:42:53, Jessica Zhang wrote:
>>>> Currently, DPU will enable TE during prepare_commit(). However, this
>>>> will cause a crash and reboot to sahara when trying to read/write to
>>>> register in get_autorefresh_config(), because the core clock rates
>>>> aren't set at that time.
>>>
>>> Haven't seeen a crash like this on any of my devices (after implementing
>>> INTF TE). get_autorefresh_config() always reads zero (or 1 for
>>> frame_count) except the first time it is called (autorefresh is left
>>> enabled by our bootloader on SM6125) and triggers the disable codepath.
>>>
>>
>> I feel that the fact that bootloader keeps things on for you is the
>> reason you dont see the issue. With continuoush splash, clocks are kept
>> enabled. We dont have it enabled (confirmed that).
>
> That is quite likely, we may even have them enabled because of
> simple-framebuffer in DTs; turning those off likely won't have any
> effect for testing this.
>
> For what it's worth, my SM8150 reads 0 for autorefresh.
>
the value shouldnt really matter. The fact that you are able to read
that register without crashing like we are means your clocks are on and
ours arent. Thats what this change is fixing.
> <snip>
>
>>> Then, for some patch hygiene, starting here:
>>>
>>>> Depends on: "Implement tearcheck support on INTF block" [3]
>>>>
>>>> Changes in V3:
>>>> - Added function prototypes
>>>> - Reordered function definitions to make change more legible
>>>> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>>>>
>>>> Changes in V4:
>>>> - Reworded commit message to be more specific
>>>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
>>>
>>> ... until here: all this info belongs /below the cut/ outside of the
>>> messge that becomes part of the commit when this patch is applied to the
>>> tree.
>>
>> For DRM, I thought we are keeping the change log above the ---- ?
>> Which means its allowed in the commit message.
>
> I hope not, seems unlikely to have different rules across kernel
> subsystems. The main point is that this changelog and dependency chain
> isn't of any value when the final patch is applied, regardless of
> whether it is "allowed".
>
I looked at a recently posted change by Rob and change log is above the ---
https://patchwork.kernel.org/project/dri-devel/patch/20230301185432.3010939-1-robdclark@gmail.com/
So we will follow that.
>>>> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
>>>> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
>>>
>>> Please replace these with "permalinks" (to a commit hash): a branch with
>>> line number annotation will fall out of date soon as more patches are
>>> applied that touch these files.
>>>
>>>> [3] https://patchwork.freedesktop.org/series/112332/
>>>
>>> Is this a hard dependency? It seems this series applies cleanly on
>>> -next and - from a cursory view - should be applicable and testable
>>> without my INTF TE series. However, Dmitry asked me to move some code
>>> around in review resulting in separate callbacks in the encoder, rather
>>> than having various if(has_intf_te) within those callbacks. That'll
>>> cause conflicts when I eventually get to respin a v2.
>>>
>>
>> I guess Jessica listed this because without intf_te series there is no
>> crash because hw_pp would be NULL and autorefresh() would return early.
>> So dependency is from the standpoint of when this series is needed and
>> not from compilation point of view.
>
> That is indeed the question. I'll leave it to the maintainers to decide
> what order to apply these in, which we should be made aware of before
> submitting v2 so that one of us can resolve the conflicts.
>
It should be first the intf TE series and then this one. You can go
ahead and post your v2, we will rebase on top of yours.
> - Marijn
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
2023-03-01 17:08 ` Marijn Suijten
@ 2023-03-01 21:46 ` Dmitry Baryshkov
-1 siblings, 0 replies; 38+ messages in thread
From: Dmitry Baryshkov @ 2023-03-01 21:46 UTC (permalink / raw)
To: Marijn Suijten, Abhinav Kumar
Cc: Jessica Zhang, freedreno, linux-arm-msm, dri-devel, robdclark,
seanpaul, swboyd
On 01/03/2023 19:08, Marijn Suijten wrote:
> On 2023-03-01 08:23:28, Abhinav Kumar wrote:
>>
>> On 3/1/2023 2:03 AM, Marijn Suijten wrote:
>>> On 2023-02-21 10:42:53, Jessica Zhang wrote:
>>> Then, for some patch hygiene, starting here:
>>>
>>>> Depends on: "Implement tearcheck support on INTF block" [3]
>>>>
>>>> Changes in V3:
>>>> - Added function prototypes
>>>> - Reordered function definitions to make change more legible
>>>> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>>>>
>>>> Changes in V4:
>>>> - Reworded commit message to be more specific
>>>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
>>>
>>> ... until here: all this info belongs /below the cut/ outside of the
>>> messge that becomes part of the commit when this patch is applied to the
>>> tree.
>>
>> For DRM, I thought we are keeping the change log above the ---- ?
>> Which means its allowed in the commit message.
>
> I hope not, seems unlikely to have different rules across kernel
> subsystems. The main point is that this changelog and dependency chain
> isn't of any value when the final patch is applied, regardless of
> whether it is "allowed".
Unfortunately this is one of DRM peculiarities. So, some of the patches
have changelog in commit message.
I myself prefer to have changelog in the cover letter, but I don't
enforce that.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
@ 2023-03-01 21:46 ` Dmitry Baryshkov
0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Baryshkov @ 2023-03-01 21:46 UTC (permalink / raw)
To: Marijn Suijten, Abhinav Kumar
Cc: linux-arm-msm, dri-devel, swboyd, seanpaul, Jessica Zhang, freedreno
On 01/03/2023 19:08, Marijn Suijten wrote:
> On 2023-03-01 08:23:28, Abhinav Kumar wrote:
>>
>> On 3/1/2023 2:03 AM, Marijn Suijten wrote:
>>> On 2023-02-21 10:42:53, Jessica Zhang wrote:
>>> Then, for some patch hygiene, starting here:
>>>
>>>> Depends on: "Implement tearcheck support on INTF block" [3]
>>>>
>>>> Changes in V3:
>>>> - Added function prototypes
>>>> - Reordered function definitions to make change more legible
>>>> - Removed prepare_commit() function from dpu_encoder_phys_cmd
>>>>
>>>> Changes in V4:
>>>> - Reworded commit message to be more specific
>>>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
>>>
>>> ... until here: all this info belongs /below the cut/ outside of the
>>> messge that becomes part of the commit when this patch is applied to the
>>> tree.
>>
>> For DRM, I thought we are keeping the change log above the ---- ?
>> Which means its allowed in the commit message.
>
> I hope not, seems unlikely to have different rules across kernel
> subsystems. The main point is that this changelog and dependency chain
> isn't of any value when the final patch is applied, regardless of
> whether it is "allowed".
Unfortunately this is one of DRM peculiarities. So, some of the patches
have changelog in commit message.
I myself prefer to have changelog in the cover letter, but I don't
enforce that.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
2023-03-01 10:13 ` Marijn Suijten
@ 2023-03-02 21:25 ` Jessica Zhang
-1 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-03-02 21:25 UTC (permalink / raw)
To: Marijn Suijten
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, freedreno
On 3/1/2023 2:13 AM, Marijn Suijten wrote:
> On 2023-03-01 11:08:16, Marijn Suijten wrote:
>> On 2023-02-21 10:42:55, Jessica Zhang wrote:
>>> Now that the TE setup has been moved to prepare_for_kickoff(), we have
>>> not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
>>
>> s/not/no
>>
>>> do nothing. Remove prepare_commit() from DPU driver.
>>
>> And again, this:
>>
>>> Changes in V3:
>>> - Reworded commit message to be more clear
>>> - Corrected spelling mistake in commit message
>>>
>>> Changes in V4:
>>> - Reworded commit message for clarity
>>
>> ... should go below the cut.
>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> With the above two issues fixed:
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
>>> 3 files changed, 47 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index dcceed91aed8..35e120b5ef53 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
>>> ctl->ops.clear_pending_flush(ctl);
>>> }
>>>
>>> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
>>> -{
>>> - struct dpu_encoder_virt *dpu_enc;
>>> - struct dpu_encoder_phys *phys;
>>> - int i;
>>> -
>>> - if (!drm_enc) {
>>> - DPU_ERROR("invalid encoder\n");
>>> - return;
>>> - }
>>> - dpu_enc = to_dpu_encoder_virt(drm_enc);
>>> -
>>> - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>> - phys = dpu_enc->phys_encs[i];
>>> - if (phys->ops.prepare_commit)
>>> - phys->ops.prepare_commit(phys);
>
> In hindsight, Dmitry asked in v2 to remove prepare_commit from
> dpu_encoder_phys_ops (and its documentation comment) in
> dpu_encoder_phys.h, but that has not happened yet. Can we do that in a
> v5?
Ah, forgot to include that change. Will add it in the v5. Thanks for
catching it!
- Jessica Zhang
>
> - Marijn
>
>>> - }
>>> -}
>>> -
>>> #ifdef CONFIG_DEBUG_FS
>>> static int _dpu_encoder_status_show(struct seq_file *s, void *data)
>>> {
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> index 9e7236ef34e6..2c9ef8d1b877 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
>>> int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>>> struct msm_display_info *disp_info);
>>>
>>> -/**
>>> - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
>>> - * atomic commit, before any registers are written
>>> - * @drm_enc: Pointer to previously created drm encoder structure
>>> - */
>>> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
>>> -
>>> /**
>>> * dpu_encoder_set_idle_timeout - set the idle timeout for video
>>> * and command mode encoders.
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 165958d47ec6..6f7ddbf0d9b7 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
>>> return ktime_get();
>>> }
>>>
>>> -static void dpu_kms_prepare_commit(struct msm_kms *kms,
>>> - struct drm_atomic_state *state)
>>> -{
>>> - struct drm_crtc *crtc;
>>> - struct drm_crtc_state *crtc_state;
>>> - struct drm_encoder *encoder;
>>> - int i;
>>> -
>>> - if (!kms)
>>> - return;
>>> -
>>> - /* Call prepare_commit for all affected encoders */
>>> - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>> - drm_for_each_encoder_mask(encoder, crtc->dev,
>>> - crtc_state->encoder_mask) {
>>> - dpu_encoder_prepare_commit(encoder);
>>> - }
>>> - }
>>> -}
>>> -
>>> static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
>>> {
>>> struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>> @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
>>> .enable_commit = dpu_kms_enable_commit,
>>> .disable_commit = dpu_kms_disable_commit,
>>> .vsync_time = dpu_kms_vsync_time,
>>> - .prepare_commit = dpu_kms_prepare_commit,
>>> .flush_commit = dpu_kms_flush_commit,
>>> .wait_flush = dpu_kms_wait_flush,
>>> .complete_commit = dpu_kms_complete_commit,
>>> --
>>> 2.39.2
>>>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
@ 2023-03-02 21:25 ` Jessica Zhang
0 siblings, 0 replies; 38+ messages in thread
From: Jessica Zhang @ 2023-03-02 21:25 UTC (permalink / raw)
To: Marijn Suijten
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
dmitry.baryshkov, quic_abhinavk
On 3/1/2023 2:13 AM, Marijn Suijten wrote:
> On 2023-03-01 11:08:16, Marijn Suijten wrote:
>> On 2023-02-21 10:42:55, Jessica Zhang wrote:
>>> Now that the TE setup has been moved to prepare_for_kickoff(), we have
>>> not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
>>
>> s/not/no
>>
>>> do nothing. Remove prepare_commit() from DPU driver.
>>
>> And again, this:
>>
>>> Changes in V3:
>>> - Reworded commit message to be more clear
>>> - Corrected spelling mistake in commit message
>>>
>>> Changes in V4:
>>> - Reworded commit message for clarity
>>
>> ... should go below the cut.
>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> With the above two issues fixed:
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 -------
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 ---------------------
>>> 3 files changed, 47 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index dcceed91aed8..35e120b5ef53 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
>>> ctl->ops.clear_pending_flush(ctl);
>>> }
>>>
>>> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
>>> -{
>>> - struct dpu_encoder_virt *dpu_enc;
>>> - struct dpu_encoder_phys *phys;
>>> - int i;
>>> -
>>> - if (!drm_enc) {
>>> - DPU_ERROR("invalid encoder\n");
>>> - return;
>>> - }
>>> - dpu_enc = to_dpu_encoder_virt(drm_enc);
>>> -
>>> - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>> - phys = dpu_enc->phys_encs[i];
>>> - if (phys->ops.prepare_commit)
>>> - phys->ops.prepare_commit(phys);
>
> In hindsight, Dmitry asked in v2 to remove prepare_commit from
> dpu_encoder_phys_ops (and its documentation comment) in
> dpu_encoder_phys.h, but that has not happened yet. Can we do that in a
> v5?
Ah, forgot to include that change. Will add it in the v5. Thanks for
catching it!
- Jessica Zhang
>
> - Marijn
>
>>> - }
>>> -}
>>> -
>>> #ifdef CONFIG_DEBUG_FS
>>> static int _dpu_encoder_status_show(struct seq_file *s, void *data)
>>> {
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> index 9e7236ef34e6..2c9ef8d1b877 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
>>> int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>>> struct msm_display_info *disp_info);
>>>
>>> -/**
>>> - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
>>> - * atomic commit, before any registers are written
>>> - * @drm_enc: Pointer to previously created drm encoder structure
>>> - */
>>> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
>>> -
>>> /**
>>> * dpu_encoder_set_idle_timeout - set the idle timeout for video
>>> * and command mode encoders.
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 165958d47ec6..6f7ddbf0d9b7 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
>>> return ktime_get();
>>> }
>>>
>>> -static void dpu_kms_prepare_commit(struct msm_kms *kms,
>>> - struct drm_atomic_state *state)
>>> -{
>>> - struct drm_crtc *crtc;
>>> - struct drm_crtc_state *crtc_state;
>>> - struct drm_encoder *encoder;
>>> - int i;
>>> -
>>> - if (!kms)
>>> - return;
>>> -
>>> - /* Call prepare_commit for all affected encoders */
>>> - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>> - drm_for_each_encoder_mask(encoder, crtc->dev,
>>> - crtc_state->encoder_mask) {
>>> - dpu_encoder_prepare_commit(encoder);
>>> - }
>>> - }
>>> -}
>>> -
>>> static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
>>> {
>>> struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>> @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
>>> .enable_commit = dpu_kms_enable_commit,
>>> .disable_commit = dpu_kms_disable_commit,
>>> .vsync_time = dpu_kms_vsync_time,
>>> - .prepare_commit = dpu_kms_prepare_commit,
>>> .flush_commit = dpu_kms_flush_commit,
>>> .wait_flush = dpu_kms_wait_flush,
>>> .complete_commit = dpu_kms_complete_commit,
>>> --
>>> 2.39.2
>>>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
2023-03-01 21:42 ` Abhinav Kumar
@ 2023-03-04 10:29 ` Marijn Suijten
-1 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-04 10:29 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Rob Clark, Jessica Zhang, freedreno, linux-arm-msm, dri-devel,
seanpaul, swboyd, dmitry.baryshkov
On 2023-03-01 13:42:55, Abhinav Kumar wrote:
<snip>
> >>> Then, for some patch hygiene, starting here:
> >>>
> >>>> Depends on: "Implement tearcheck support on INTF block" [3]
> >>>>
> >>>> Changes in V3:
> >>>> - Added function prototypes
> >>>> - Reordered function definitions to make change more legible
> >>>> - Removed prepare_commit() function from dpu_encoder_phys_cmd
> >>>>
> >>>> Changes in V4:
> >>>> - Reworded commit message to be more specific
> >>>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
> >>>
> >>> ... until here: all this info belongs /below the cut/ outside of the
> >>> messge that becomes part of the commit when this patch is applied to the
> >>> tree.
> >>
> >> For DRM, I thought we are keeping the change log above the ---- ?
> >> Which means its allowed in the commit message.
> >
> > I hope not, seems unlikely to have different rules across kernel
> > subsystems. The main point is that this changelog and dependency chain
> > isn't of any value when the final patch is applied, regardless of
> > whether it is "allowed".
> >
>
> I looked at a recently posted change by Rob and change log is above the ---
>
> https://patchwork.kernel.org/project/dri-devel/patch/20230301185432.3010939-1-robdclark@gmail.com/
>
> So we will follow that.
I hope that was in error, or no-one pointed it out to Rob. As said
before there is no use to having this information in the applied patch,
even the kernel guidelines state so:
https://docs.kernel.org/process/submitting-patches.html
Other comments relevant only to the moment or the maintainer, not
suitable for the permanent changelog, should also go here. **A good
example of such comments might be patch changelogs which describe
what has changed between the v1 and v2 version of the patch.**
**Please put this information after the --- line** which separates
the changelog from the rest of the patch. The version information is
not part of the changelog which gets committed to the git tree. It
is additional information for the reviewers. If it’s placed above
the commit tags, it needs manual interaction to remove it. If it is
below the separator line, it gets automatically stripped off when
applying the patch:
<snip>
> It should be first the intf TE series and then this one. You can go
> ahead and post your v2, we will rebase on top of yours.
Sounds good; though as said before I'm extremely short on time making it
hard to actively commit to this, especially as the catalog changes are
really hard to juggle between various "local" branches to test on the
many (Sony) devices we are working on. As usual, a preview of v2 is
still available at:
https://github.com/SoMainline/linux/commits/marijn/dpu
And I will do my best to get the last comments worked out.
- Marijn
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
@ 2023-03-04 10:29 ` Marijn Suijten
0 siblings, 0 replies; 38+ messages in thread
From: Marijn Suijten @ 2023-03-04 10:29 UTC (permalink / raw)
To: Abhinav Kumar
Cc: linux-arm-msm, dri-devel, swboyd, seanpaul, dmitry.baryshkov,
Jessica Zhang, freedreno
On 2023-03-01 13:42:55, Abhinav Kumar wrote:
<snip>
> >>> Then, for some patch hygiene, starting here:
> >>>
> >>>> Depends on: "Implement tearcheck support on INTF block" [3]
> >>>>
> >>>> Changes in V3:
> >>>> - Added function prototypes
> >>>> - Reordered function definitions to make change more legible
> >>>> - Removed prepare_commit() function from dpu_encoder_phys_cmd
> >>>>
> >>>> Changes in V4:
> >>>> - Reworded commit message to be more specific
> >>>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype
> >>>
> >>> ... until here: all this info belongs /below the cut/ outside of the
> >>> messge that becomes part of the commit when this patch is applied to the
> >>> tree.
> >>
> >> For DRM, I thought we are keeping the change log above the ---- ?
> >> Which means its allowed in the commit message.
> >
> > I hope not, seems unlikely to have different rules across kernel
> > subsystems. The main point is that this changelog and dependency chain
> > isn't of any value when the final patch is applied, regardless of
> > whether it is "allowed".
> >
>
> I looked at a recently posted change by Rob and change log is above the ---
>
> https://patchwork.kernel.org/project/dri-devel/patch/20230301185432.3010939-1-robdclark@gmail.com/
>
> So we will follow that.
I hope that was in error, or no-one pointed it out to Rob. As said
before there is no use to having this information in the applied patch,
even the kernel guidelines state so:
https://docs.kernel.org/process/submitting-patches.html
Other comments relevant only to the moment or the maintainer, not
suitable for the permanent changelog, should also go here. **A good
example of such comments might be patch changelogs which describe
what has changed between the v1 and v2 version of the patch.**
**Please put this information after the --- line** which separates
the changelog from the rest of the patch. The version information is
not part of the changelog which gets committed to the git tree. It
is additional information for the reviewers. If it’s placed above
the commit tags, it needs manual interaction to remove it. If it is
below the separator line, it gets automatically stripped off when
applying the patch:
<snip>
> It should be first the intf TE series and then this one. You can go
> ahead and post your v2, we will rebase on top of yours.
Sounds good; though as said before I'm extremely short on time making it
hard to actively commit to this, especially as the catalog changes are
really hard to juggle between various "local" branches to test on the
many (Sony) devices we are working on. As usual, a preview of v2 is
still available at:
https://github.com/SoMainline/linux/commits/marijn/dpu
And I will do my best to get the last comments worked out.
- Marijn
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 0/4] Move TE setup to prepare_for_kickoff()
2023-02-21 18:42 ` Jessica Zhang
@ 2023-03-28 22:37 ` Dmitry Baryshkov
-1 siblings, 0 replies; 38+ messages in thread
From: Dmitry Baryshkov @ 2023-03-28 22:37 UTC (permalink / raw)
To: freedreno, Jessica Zhang
Cc: linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_abhinavk, marijn.suijten
On Tue, 21 Feb 2023 10:42:52 -0800, Jessica Zhang wrote:
> Move TE setup to prepare_for_kickoff() and remove empty prepare_commit()
> functions in both MDP4 and DPU drivers.
>
> Changes in V2:
> - Added changes to remove empty prepare_commit() functions
>
> Changes in V3:
> - Reordered "drm/msm/dpu: Move TE setup to prepare_for_kickoff()" for
> clarity
> - Fixed spelling mistakes and wording issues
> - Picked up "Reviewed-by" tags for patches [2/4] and [4/4]
>
> [...]
Applied, thanks!
[1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
https://gitlab.freedesktop.org/lumag/msm/-/commit/dd7904e0f824
[2/4] drm/msm: Check for NULL before calling prepare_commit()
https://gitlab.freedesktop.org/lumag/msm/-/commit/63c3df12d13a
[3/4] drm/msm/dpu: Remove empty prepare_commit() function
https://gitlab.freedesktop.org/lumag/msm/-/commit/f4d83f101233
[4/4] drm/msm/mdp4: Remove empty prepare_commit() function
https://gitlab.freedesktop.org/lumag/msm/-/commit/191604898585
Best regards,
--
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 0/4] Move TE setup to prepare_for_kickoff()
@ 2023-03-28 22:37 ` Dmitry Baryshkov
0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Baryshkov @ 2023-03-28 22:37 UTC (permalink / raw)
To: freedreno, Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
marijn.suijten
On Tue, 21 Feb 2023 10:42:52 -0800, Jessica Zhang wrote:
> Move TE setup to prepare_for_kickoff() and remove empty prepare_commit()
> functions in both MDP4 and DPU drivers.
>
> Changes in V2:
> - Added changes to remove empty prepare_commit() functions
>
> Changes in V3:
> - Reordered "drm/msm/dpu: Move TE setup to prepare_for_kickoff()" for
> clarity
> - Fixed spelling mistakes and wording issues
> - Picked up "Reviewed-by" tags for patches [2/4] and [4/4]
>
> [...]
Applied, thanks!
[1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
https://gitlab.freedesktop.org/lumag/msm/-/commit/dd7904e0f824
[2/4] drm/msm: Check for NULL before calling prepare_commit()
https://gitlab.freedesktop.org/lumag/msm/-/commit/63c3df12d13a
[3/4] drm/msm/dpu: Remove empty prepare_commit() function
https://gitlab.freedesktop.org/lumag/msm/-/commit/f4d83f101233
[4/4] drm/msm/mdp4: Remove empty prepare_commit() function
https://gitlab.freedesktop.org/lumag/msm/-/commit/191604898585
Best regards,
--
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2023-03-28 22:39 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-21 18:42 [PATCH v4 0/4] Move TE setup to prepare_for_kickoff() Jessica Zhang
2023-02-21 18:42 ` Jessica Zhang
2023-02-21 18:42 ` [PATCH v4 1/4] drm/msm/dpu: " Jessica Zhang
2023-02-21 18:42 ` Jessica Zhang
2023-03-01 3:21 ` Dmitry Baryshkov
2023-03-01 3:21 ` Dmitry Baryshkov
2023-03-01 10:03 ` Marijn Suijten
2023-03-01 10:03 ` Marijn Suijten
2023-03-01 16:23 ` Abhinav Kumar
2023-03-01 16:23 ` Abhinav Kumar
2023-03-01 17:08 ` Marijn Suijten
2023-03-01 17:08 ` Marijn Suijten
2023-03-01 21:42 ` Abhinav Kumar
2023-03-01 21:42 ` Abhinav Kumar
2023-03-04 10:29 ` Marijn Suijten
2023-03-04 10:29 ` Marijn Suijten
2023-03-01 21:46 ` Dmitry Baryshkov
2023-03-01 21:46 ` Dmitry Baryshkov
2023-02-21 18:42 ` [PATCH v4 2/4] drm/msm: Check for NULL before calling prepare_commit() Jessica Zhang
2023-02-21 18:42 ` Jessica Zhang
2023-03-01 10:03 ` Marijn Suijten
2023-03-01 10:03 ` Marijn Suijten
2023-02-21 18:42 ` [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function Jessica Zhang
2023-02-21 18:42 ` Jessica Zhang
2023-03-01 3:22 ` Dmitry Baryshkov
2023-03-01 3:22 ` Dmitry Baryshkov
2023-03-01 10:08 ` Marijn Suijten
2023-03-01 10:08 ` Marijn Suijten
2023-03-01 10:13 ` Marijn Suijten
2023-03-01 10:13 ` Marijn Suijten
2023-03-02 21:25 ` Jessica Zhang
2023-03-02 21:25 ` Jessica Zhang
2023-02-21 18:42 ` [PATCH v4 4/4] drm/msm/mdp4: " Jessica Zhang
2023-02-21 18:42 ` Jessica Zhang
2023-03-01 10:06 ` Marijn Suijten
2023-03-01 10:06 ` Marijn Suijten
2023-03-28 22:37 ` [PATCH v4 0/4] Move TE setup to prepare_for_kickoff() Dmitry Baryshkov
2023-03-28 22:37 ` Dmitry Baryshkov
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.