Re: [Freedreno] [PATCH 11/12] drm/msm/dpu: gracefully handle null fb commits for writeback
On 15/04/2022 02:17, Abhinav Kumar wrote:
On 2/4/2022 2:43 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
kms_writeback test cases also verify with a null fb for the
writeback connector job. In addition there are also other
commit paths which can result in kickoffs without a valid
framebuffer like while closing the fb which results in the
callback to drm_atomic_helper_dirtyfb() which internally
triggers a commit.
Add protection in the dpu driver to ensure that commits for
writeback encoders without a valid fb are gracefully skipped.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21
+
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 6 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 12
5 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e7c9fe1..f7963b0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -869,6 +869,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
DPU_ATRACE_BEGIN("crtc_commit");
+ drm_for_each_encoder_mask(encoder, crtc->dev,
+ crtc->state->encoder_mask) {
+ if (!dpu_encoder_has_valid_fb(encoder)) {
Two small comments here. First, let's probably rename the function to
dpu_encoder_is_valid() or dpu_encoder_is_valid_for_commit() (ugh, too
long). There might be other cases (in theory), why encoder is invalid
at this moment.
dpu_encoder_is_valid_for_commit() seems fine to me even if long.
Good!
Also (a minor nit): I think that we should commit if at least one of
encoders is valid. So we might want to create an encoder_valid_mask
basing on the calls to dpu_encoder. And then use it later for calling
dpu_encoder_prepare_for_kickoff() and dpu_encoder_kickoff().
Its not just these two calls. These can be easily skipped within the
encoder itself. I had to bring this to the dpu_crtc level because of the
frame_pending.
I see. Let's settle down on a question which should have been aksed
initially. If we have a set of encoders, some of which are valid and
others are not (for whatever reason), should we proceed with kickoff of
the valid ones or should we skip it completely? Which encoder would call
the frame_done callbacks, in the end decrementing the frame_pending?
The issue is atomic_inc_return(_crtc->frame_pending)
We have to skip this call otherwise it leads to incorrect "frame done
timeouts" because CRTC thinks frame was kicked off but it was actually
skipped.
Maybe, what we can do is first prepare the mask.
if (hweight(crtc_encoder_mask)) {
if (atomic_inc_return(_crtc->frame_pending) == 1) {
/* acquire bandwidth and other resources */
DRM_DEBUG_ATOMIC("crtc%d first commit\n", crtc->base.id);
} else
DRM_DEBUG_ATOMIC("crtc%d commit\n", crtc->base.id);
dpu_crtc->play_count++;
dpu_vbif_clear_errors(dpu_kms);
}
do the encoder_kickoff
if (hweight(crtc_encoder_mask))
reinit_completion(_crtc->frame_done_comp);
calls to dpu_encoder_prepare_for_kickoff() and dpu_encoder_kickoff() can
be protected by dpu_encoder_is_valid_for_commit() checks.
This is probably the best we can do here.
Let me know what you think.
+ DRM_DEBUG_ATOMIC("invalid FB not kicking off crtc\n");
+ goto end;
+ }
+ }
/*
* Encoder will flush/start now, unless it has a tx pending. If
so, it
* may delay and flush at an irq event (e.g. ppdone)
@@ -891,6 +898,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
dpu_encoder_kickoff(encoder);
reinit_completion(_crtc->frame_done_comp);
+
+end:
DPU_ATRACE_END("crtc_commit");
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3746432..e990dbc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1832,6 +1832,27 @@ void dpu_encoder_prepare_for_kickoff(struct
drm_encoder *drm_enc)
}
}
+bool dpu_encoder_has_valid_fb(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+ unsigned int i;
+ struct dpu_encoder_phys *phys;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_VIRTUAL) {
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ phys = dpu_enc->phys_encs[i];
+ if (phys->ops.has_valid_output_fb &&
!phys->ops.has_valid_output_fb(phys)) {
+ DPU_DEBUG("invalid FB not kicking off\n");
+ return false;
+ }
+ }
+ }
+
+ return true;
+}
+
void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
{
Re: [Freedreno] [PATCH 04/12] drm/msm/dpu: add changes to support writeback in hw_ctl
Sorry for multiple replies on this, I missed responding to one question.
On 4/14/2022 5:27 PM, Abhinav Kumar wrote:
On 4/14/2022 5:19 PM, Dmitry Baryshkov wrote:
On 15/04/2022 03:01, Abhinav Kumar wrote:
On 4/14/2022 4:25 PM, Dmitry Baryshkov wrote:
On 15/04/2022 00:50, Abhinav Kumar wrote:
On 2/4/2022 2:19 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add changes to support writeback module in the dpu_hw_ctl
interface. In addition inroduce a reset_intf_cfg op to reset
the interface bits for the currently active interfaces in
the ctl path.
Signed-off-by: Abhinav Kumar
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 65
--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 27 -
4 files changed, 91 insertions(+), 10 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 34a6940..4cb72fa 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
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All
rights reserved.
* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
@@ -70,7 +71,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d =
dpu_encoder_helper_get_3d_blend_mode(phys_enc);
- ctl->ops.setup_intf_cfg(ctl, _cfg);
+ ctl->ops.setup_intf_cfg(ctl, _cfg, false);
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89..950fcd6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All
rights reserved.
+ * Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
@@ -290,7 +292,7 @@ static void
dpu_encoder_phys_vid_setup_timing_engine(
spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
_params, fmt);
- phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg);
+ phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg, false);
/* setup which pp blk will connect to this intf */
if (phys_enc->hw_intf->ops.bind_pingpong_blk)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 02da9ec..a2069af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights
reserved.
+/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All
rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights
reserved.
*/
#include
@@ -23,8 +24,10 @@
#define CTL_SW_RESET 0x030
#define CTL_LAYER_EXTN_OFFSET 0x40
#define CTL_MERGE_3D_ACTIVE 0x0E4
+#define CTL_WB_ACTIVE 0x0EC
#define CTL_INTF_ACTIVE 0x0F4
#define CTL_MERGE_3D_FLUSH 0x100
+#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH 0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
@@ -35,6 +38,7 @@
#define DPU_REG_RESET_TIMEOUT_US 2000
#define MERGE_3D_IDX 23
#define INTF_IDX 31
+#define WB_IDX 16
#define CTL_INVALID_BIT 0x
#define CTL_DEFAULT_GROUP_ID 0xf
@@ -128,6 +132,9 @@ static inline void
dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
if (ctx->pending_flush_mask & BIT(INTF_IDX))
DPU_REG_WRITE(>hw, CTL_INTF_FLUSH,
ctx->pending_intf_flush_mask);
+ if (ctx->pending_flush_mask & BIT(WB_IDX))
+ DPU_REG_WRITE(>hw, CTL_WB_FLUSH,
+ ctx->pending_wb_flush_mask);
DPU_REG_WRITE(>hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -248,6 +255,13 @@ static void
dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
}
}
+static void dpu_hw_ctl_update_pending_flush_wb_v1(struct
dpu_hw_ctl *ctx,
+ enum dpu_wb wb)
+{
+ ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
+
Re: [Freedreno] [PATCH 04/12] drm/msm/dpu: add changes to support writeback in hw_ctl
On 4/14/2022 5:19 PM, Dmitry Baryshkov wrote:
On 15/04/2022 03:01, Abhinav Kumar wrote:
On 4/14/2022 4:25 PM, Dmitry Baryshkov wrote:
On 15/04/2022 00:50, Abhinav Kumar wrote:
On 2/4/2022 2:19 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add changes to support writeback module in the dpu_hw_ctl
interface. In addition inroduce a reset_intf_cfg op to reset
the interface bits for the currently active interfaces in
the ctl path.
Signed-off-by: Abhinav Kumar
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 65
--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 27 -
4 files changed, 91 insertions(+), 10 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 34a6940..4cb72fa 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
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
@@ -70,7 +71,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d =
dpu_encoder_helper_get_3d_blend_mode(phys_enc);
- ctl->ops.setup_intf_cfg(ctl, _cfg);
+ ctl->ops.setup_intf_cfg(ctl, _cfg, false);
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89..950fcd6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All
rights reserved.
+ * Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
@@ -290,7 +292,7 @@ static void
dpu_encoder_phys_vid_setup_timing_engine(
spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
_params, fmt);
- phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg);
+ phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg, false);
/* setup which pp blk will connect to this intf */
if (phys_enc->hw_intf->ops.bind_pingpong_blk)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 02da9ec..a2069af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights
reserved.
+/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights
reserved.
*/
#include
@@ -23,8 +24,10 @@
#define CTL_SW_RESET 0x030
#define CTL_LAYER_EXTN_OFFSET 0x40
#define CTL_MERGE_3D_ACTIVE 0x0E4
+#define CTL_WB_ACTIVE 0x0EC
#define CTL_INTF_ACTIVE 0x0F4
#define CTL_MERGE_3D_FLUSH 0x100
+#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH 0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
@@ -35,6 +38,7 @@
#define DPU_REG_RESET_TIMEOUT_US 2000
#define MERGE_3D_IDX 23
#define INTF_IDX 31
+#define WB_IDX 16
#define CTL_INVALID_BIT 0x
#define CTL_DEFAULT_GROUP_ID 0xf
@@ -128,6 +132,9 @@ static inline void
dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
if (ctx->pending_flush_mask & BIT(INTF_IDX))
DPU_REG_WRITE(>hw, CTL_INTF_FLUSH,
ctx->pending_intf_flush_mask);
+ if (ctx->pending_flush_mask & BIT(WB_IDX))
+ DPU_REG_WRITE(>hw, CTL_WB_FLUSH,
+ ctx->pending_wb_flush_mask);
DPU_REG_WRITE(>hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -248,6 +255,13 @@ static void
dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
}
}
+static void dpu_hw_ctl_update_pending_flush_wb_v1(struct
dpu_hw_ctl *ctx,
+ enum dpu_wb wb)
+{
+ ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
+ ctx->pending_flush_mask |= BIT(WB_IDX);
+}
+
static void dpu_hw_ctl_update_pending_flush_intf_v1(struct
dpu_hw_ctl *ctx,
Re: [Freedreno] [PATCH 04/12] drm/msm/dpu: add changes to support writeback in hw_ctl
On 15/04/2022 03:01, Abhinav Kumar wrote:
On 4/14/2022 4:25 PM, Dmitry Baryshkov wrote:
On 15/04/2022 00:50, Abhinav Kumar wrote:
On 2/4/2022 2:19 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add changes to support writeback module in the dpu_hw_ctl
interface. In addition inroduce a reset_intf_cfg op to reset
the interface bits for the currently active interfaces in
the ctl path.
Signed-off-by: Abhinav Kumar
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 65
--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 27 -
4 files changed, 91 insertions(+), 10 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 34a6940..4cb72fa 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
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
@@ -70,7 +71,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d =
dpu_encoder_helper_get_3d_blend_mode(phys_enc);
- ctl->ops.setup_intf_cfg(ctl, _cfg);
+ ctl->ops.setup_intf_cfg(ctl, _cfg, false);
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89..950fcd6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
@@ -290,7 +292,7 @@ static void
dpu_encoder_phys_vid_setup_timing_engine(
spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
_params, fmt);
- phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg);
+ phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg, false);
/* setup which pp blk will connect to this intf */
if (phys_enc->hw_intf->ops.bind_pingpong_blk)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 02da9ec..a2069af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights
reserved.
+/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights
reserved.
*/
#include
@@ -23,8 +24,10 @@
#define CTL_SW_RESET 0x030
#define CTL_LAYER_EXTN_OFFSET 0x40
#define CTL_MERGE_3D_ACTIVE 0x0E4
+#define CTL_WB_ACTIVE 0x0EC
#define CTL_INTF_ACTIVE 0x0F4
#define CTL_MERGE_3D_FLUSH 0x100
+#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH 0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
@@ -35,6 +38,7 @@
#define DPU_REG_RESET_TIMEOUT_US 2000
#define MERGE_3D_IDX 23
#define INTF_IDX 31
+#define WB_IDX 16
#define CTL_INVALID_BIT 0x
#define CTL_DEFAULT_GROUP_ID 0xf
@@ -128,6 +132,9 @@ static inline void
dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
if (ctx->pending_flush_mask & BIT(INTF_IDX))
DPU_REG_WRITE(>hw, CTL_INTF_FLUSH,
ctx->pending_intf_flush_mask);
+ if (ctx->pending_flush_mask & BIT(WB_IDX))
+ DPU_REG_WRITE(>hw, CTL_WB_FLUSH,
+ ctx->pending_wb_flush_mask);
DPU_REG_WRITE(>hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -248,6 +255,13 @@ static void
dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
}
}
+static void dpu_hw_ctl_update_pending_flush_wb_v1(struct
dpu_hw_ctl *ctx,
+ enum dpu_wb wb)
+{
+ ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
+ ctx->pending_flush_mask |= BIT(WB_IDX);
+}
+
static void dpu_hw_ctl_update_pending_flush_intf_v1(struct
dpu_hw_ctl *ctx,
enum dpu_intf intf)
{
@@ -493,10 +507,11 @@
Re: [Freedreno] [PATCH 04/12] drm/msm/dpu: add changes to support writeback in hw_ctl
On 4/14/2022 4:25 PM, Dmitry Baryshkov wrote:
On 15/04/2022 00:50, Abhinav Kumar wrote:
On 2/4/2022 2:19 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add changes to support writeback module in the dpu_hw_ctl
interface. In addition inroduce a reset_intf_cfg op to reset
the interface bits for the currently active interfaces in
the ctl path.
Signed-off-by: Abhinav Kumar
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 65
--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 27 -
4 files changed, 91 insertions(+), 10 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 34a6940..4cb72fa 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
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
@@ -70,7 +71,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d =
dpu_encoder_helper_get_3d_blend_mode(phys_enc);
- ctl->ops.setup_intf_cfg(ctl, _cfg);
+ ctl->ops.setup_intf_cfg(ctl, _cfg, false);
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89..950fcd6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
@@ -290,7 +292,7 @@ static void
dpu_encoder_phys_vid_setup_timing_engine(
spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
_params, fmt);
- phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, _cfg);
+ phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg, false);
/* setup which pp blk will connect to this intf */
if (phys_enc->hw_intf->ops.bind_pingpong_blk)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 02da9ec..a2069af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#include
@@ -23,8 +24,10 @@
#define CTL_SW_RESET 0x030
#define CTL_LAYER_EXTN_OFFSET 0x40
#define CTL_MERGE_3D_ACTIVE 0x0E4
+#define CTL_WB_ACTIVE 0x0EC
#define CTL_INTF_ACTIVE 0x0F4
#define CTL_MERGE_3D_FLUSH 0x100
+#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH 0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
@@ -35,6 +38,7 @@
#define DPU_REG_RESET_TIMEOUT_US 2000
#define MERGE_3D_IDX 23
#define INTF_IDX 31
+#define WB_IDX 16
#define CTL_INVALID_BIT 0x
#define CTL_DEFAULT_GROUP_ID 0xf
@@ -128,6 +132,9 @@ static inline void
dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
if (ctx->pending_flush_mask & BIT(INTF_IDX))
DPU_REG_WRITE(>hw, CTL_INTF_FLUSH,
ctx->pending_intf_flush_mask);
+ if (ctx->pending_flush_mask & BIT(WB_IDX))
+ DPU_REG_WRITE(>hw, CTL_WB_FLUSH,
+ ctx->pending_wb_flush_mask);
DPU_REG_WRITE(>hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -248,6 +255,13 @@ static void
dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
}
}
+static void dpu_hw_ctl_update_pending_flush_wb_v1(struct dpu_hw_ctl
*ctx,
+ enum dpu_wb wb)
+{
+ ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
+ ctx->pending_flush_mask |= BIT(WB_IDX);
+}
+
static void dpu_hw_ctl_update_pending_flush_intf_v1(struct
dpu_hw_ctl *ctx,
enum dpu_intf intf)
{
@@ -493,10 +507,11 @@ static void dpu_hw_ctl_setup_blendstage(struct
Re: [Freedreno] [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
On 15/04/2022 01:06, Kuogee Hsieh wrote: On 4/8/2022 4:59 PM, Dmitry Baryshkov wrote: On Fri, 8 Apr 2022 at 23:30, Kuogee Hsieh wrote: On 4/8/2022 5:27 AM, Dmitry Baryshkov wrote: On 07/04/2022 00:28, Kuogee Hsieh wrote: dp_hpd_plug_handle() is responsible for setting up main link and send uevent to notify user space framework to start video stream. Similarly, dp_hdp_unplug_handle is responsible to send uevent to notify user space framework to stop video stream and then tear down main link. However there are rare cases, such as in the middle of system suspending, that uevent could not be delivered to user space framework. Therefore some kind of recover mechanism armed by timer need to be in place in the case of user space framework does not respond to uevent. Hmm, how does userpsace 'respond' to the uevent? The driver should send hotplug notifications to userspace, but it must not expect any particular reaction. The userspace might be as simple, as fbdev emulation, but the driver still should function correctly. yes, driver is function correctly by setting up main link. but it does not know which resolution to display. It send hotplug notification through uevent to framework after main link is ready. Framework is responsible to set up MDP timing engine to start video stream. However it does not know which It's of no concern to the driver. It is completely the userspace problem. After resuming, it should reread available video output properties. The display could have been changed while the system is suspended. From your description I still do not understand why you need the 'recovery' mechanism. I mean "recovery" means dp driver may leave stay at link ready mistakenly after dongle unplugged due to missing framework action to tear down main link so that next dongle plug in will not work. Currently it was armed with timeout function and it will fired if framework did not call msm_dp_display_disable() after 5 second. framework = userspace? Is my understanding correct? Currently DP driver sends a notification to userspace that DP is unplugged, waits for userspace to disable DP output, and only after that it will shutdown the link. Is this a correct description? If so, then yes, your change is correct. I mean that the kernel shouldn't wait for clients to stop using video pipeline if the sink gets unplugged. Instead it should tear the video stream down and let the DRM client cope with that. I'm not sure how should the driver react if the client doesn't disable the output, but then the sink gets reattached? And a bit more interesting question: how should the driver behave if the new sink is not compatible with the existing video stream. Anyway, we think this approach is not good. therefore it is replaced with below patch. drm/msm/dp: tear down main link at unplug handle immediately -- With best wishes Dmitry
Re: [Freedreno] [PATCH 04/12] drm/msm/dpu: add changes to support writeback in hw_ctl
On 15/04/2022 00:50, Abhinav Kumar wrote:
On 2/4/2022 2:19 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add changes to support writeback module in the dpu_hw_ctl
interface. In addition inroduce a reset_intf_cfg op to reset
the interface bits for the currently active interfaces in
the ctl path.
Signed-off-by: Abhinav Kumar
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 65
--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 27 -
4 files changed, 91 insertions(+), 10 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 34a6940..4cb72fa 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
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
@@ -70,7 +71,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
- ctl->ops.setup_intf_cfg(ctl, _cfg);
+ ctl->ops.setup_intf_cfg(ctl, _cfg, false);
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89..950fcd6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
@@ -290,7 +292,7 @@ static void
dpu_encoder_phys_vid_setup_timing_engine(
spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
_params, fmt);
- phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, _cfg);
+ phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg, false);
/* setup which pp blk will connect to this intf */
if (phys_enc->hw_intf->ops.bind_pingpong_blk)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 02da9ec..a2069af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#include
@@ -23,8 +24,10 @@
#define CTL_SW_RESET 0x030
#define CTL_LAYER_EXTN_OFFSET 0x40
#define CTL_MERGE_3D_ACTIVE 0x0E4
+#define CTL_WB_ACTIVE 0x0EC
#define CTL_INTF_ACTIVE 0x0F4
#define CTL_MERGE_3D_FLUSH 0x100
+#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH 0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
@@ -35,6 +38,7 @@
#define DPU_REG_RESET_TIMEOUT_US 2000
#define MERGE_3D_IDX 23
#define INTF_IDX 31
+#define WB_IDX 16
#define CTL_INVALID_BIT 0x
#define CTL_DEFAULT_GROUP_ID 0xf
@@ -128,6 +132,9 @@ static inline void
dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
if (ctx->pending_flush_mask & BIT(INTF_IDX))
DPU_REG_WRITE(>hw, CTL_INTF_FLUSH,
ctx->pending_intf_flush_mask);
+ if (ctx->pending_flush_mask & BIT(WB_IDX))
+ DPU_REG_WRITE(>hw, CTL_WB_FLUSH,
+ ctx->pending_wb_flush_mask);
DPU_REG_WRITE(>hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -248,6 +255,13 @@ static void
dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
}
}
+static void dpu_hw_ctl_update_pending_flush_wb_v1(struct dpu_hw_ctl
*ctx,
+ enum dpu_wb wb)
+{
+ ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
+ ctx->pending_flush_mask |= BIT(WB_IDX);
+}
+
static void dpu_hw_ctl_update_pending_flush_intf_v1(struct
dpu_hw_ctl *ctx,
enum dpu_intf intf)
{
@@ -493,10 +507,11 @@ static void dpu_hw_ctl_setup_blendstage(struct
dpu_hw_ctl *ctx,
static void
Re: [Freedreno] [PATCH 11/12] drm/msm/dpu: gracefully handle null fb commits for writeback
On 2/4/2022 2:43 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
kms_writeback test cases also verify with a null fb for the
writeback connector job. In addition there are also other
commit paths which can result in kickoffs without a valid
framebuffer like while closing the fb which results in the
callback to drm_atomic_helper_dirtyfb() which internally
triggers a commit.
Add protection in the dpu driver to ensure that commits for
writeback encoders without a valid fb are gracefully skipped.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21
+
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 6 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 12
5 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e7c9fe1..f7963b0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -869,6 +869,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
DPU_ATRACE_BEGIN("crtc_commit");
+ drm_for_each_encoder_mask(encoder, crtc->dev,
+ crtc->state->encoder_mask) {
+ if (!dpu_encoder_has_valid_fb(encoder)) {
Two small comments here. First, let's probably rename the function to
dpu_encoder_is_valid() or dpu_encoder_is_valid_for_commit() (ugh, too
long). There might be other cases (in theory), why encoder is invalid at
this moment.
dpu_encoder_is_valid_for_commit() seems fine to me even if long.
Also (a minor nit): I think that we should commit if at least one of
encoders is valid. So we might want to create an encoder_valid_mask
basing on the calls to dpu_encoder. And then use it later for calling
dpu_encoder_prepare_for_kickoff() and dpu_encoder_kickoff().
Its not just these two calls. These can be easily skipped within the
encoder itself. I had to bring this to the dpu_crtc level because of the
frame_pending.
The issue is atomic_inc_return(_crtc->frame_pending)
We have to skip this call otherwise it leads to incorrect "frame done
timeouts" because CRTC thinks frame was kicked off but it was actually
skipped.
Maybe, what we can do is first prepare the mask.
if (hweight(crtc_encoder_mask)) {
if (atomic_inc_return(_crtc->frame_pending) == 1) {
/* acquire bandwidth and other resources */
DRM_DEBUG_ATOMIC("crtc%d first commit\n", crtc->base.id);
} else
DRM_DEBUG_ATOMIC("crtc%d commit\n", crtc->base.id);
dpu_crtc->play_count++;
dpu_vbif_clear_errors(dpu_kms);
}
do the encoder_kickoff
if (hweight(crtc_encoder_mask))
reinit_completion(_crtc->frame_done_comp);
calls to dpu_encoder_prepare_for_kickoff() and dpu_encoder_kickoff() can
be protected by dpu_encoder_is_valid_for_commit() checks.
This is probably the best we can do here.
Let me know what you think.
+ DRM_DEBUG_ATOMIC("invalid FB not kicking off crtc\n");
+ goto end;
+ }
+ }
/*
* Encoder will flush/start now, unless it has a tx pending. If
so, it
* may delay and flush at an irq event (e.g. ppdone)
@@ -891,6 +898,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
dpu_encoder_kickoff(encoder);
reinit_completion(_crtc->frame_done_comp);
+
+end:
DPU_ATRACE_END("crtc_commit");
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3746432..e990dbc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1832,6 +1832,27 @@ void dpu_encoder_prepare_for_kickoff(struct
drm_encoder *drm_enc)
}
}
+bool dpu_encoder_has_valid_fb(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+ unsigned int i;
+ struct dpu_encoder_phys *phys;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_VIRTUAL) {
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ phys = dpu_enc->phys_encs[i];
+ if (phys->ops.has_valid_output_fb &&
!phys->ops.has_valid_output_fb(phys)) {
+ DPU_DEBUG("invalid FB not kicking off\n");
+ return false;
+ }
+ }
+ }
+
+ return true;
+}
+
void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
{
struct dpu_encoder_virt *dpu_enc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index da5b6d6..63d90b8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -187,4 +187,10 @@ void dpu_encoder_prepare_wb_job(struct
drm_encoder *drm_enc,
void
Re: [Freedreno] [PATCH 06/12] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder
Hi Marijn
Thank you for your suggestion.
I will address it and add your "Reported by".
Thanks
Abhinav
On 4/14/2022 3:26 PM, Marijn Suijten wrote:
On 2022-02-04 13:17:19, Abhinav Kumar wrote:
Make changes to dpu_encoder to support virtual encoder needed
to support writeback for dpu.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 +
1 file changed, 42 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e977c05..947069b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
+ enum dpu_hw_blk_type blk_type;
int num_lm, num_ctl, num_pp;
int i, j;
@@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
phys->hw_pp = dpu_enc->hw_pp[i];
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
+ if (phys->intf_mode == INTF_MODE_WB_LINE)
+ blk_type = DPU_HW_BLK_WB;
+ else
+ blk_type = DPU_HW_BLK_INTF;
+
num_blk = dpu_rm_get_assigned_resources(_kms->rm,
- global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
+ global_state, drm_enc->base.id, blk_type,
hw_blk, ARRAY_SIZE(hw_blk));
- for (j = 0; j < num_blk; j++) {
- struct dpu_hw_intf *hw_intf;
- hw_intf = to_dpu_hw_intf(hw_blk[i]);
- if (hw_intf->idx == phys->intf_idx)
- phys->hw_intf = hw_intf;
+ if (blk_type == DPU_HW_BLK_WB) {
+ for (j = 0; j < num_blk; j++) {
+ struct dpu_hw_wb *hw_wb;
+
+ hw_wb = to_dpu_hw_wb(hw_blk[i]);
+ if (hw_wb->idx == phys->intf_idx)
+ phys->hw_wb = hw_wb;
+ }
+ } else {
+ for (j = 0; j < num_blk; j++) {
+ struct dpu_hw_intf *hw_intf;
+
+ hw_intf = to_dpu_hw_intf(hw_blk[i]);
+ if (hw_intf->idx == phys->intf_idx)
+ phys->hw_intf = hw_intf;
+ }
It appears the original bit of code iterates j from 0 to num_blks yet
only uses i as iteration value. All of this code seems reentrant
meaning that executing it more than one times is redundant. You've
adopted this mistake, though I'd argue it should use j in hw_blk[i] as
that follows the number of elements in hw_blk returned by
dpu_rm_get_assigned_resources.
I suggest to address this in a separate patch (I can send that too, feel
free to add my Reported-by otherwise) and rebase this patch on top of
that, substituting i with j here as well. It seems to have been
introduced by b954fa6baaca ("drm/msm/dpu: Refactor rm iterator").
- Marijn
}
- if (!phys->hw_intf) {
+ if (!phys->hw_intf && !phys->hw_wb) {
DPU_ERROR_ENC(dpu_enc,
- "no intf block assigned at idx: %d\n", i);
+ "no intf or WB block assigned at idx:
%d\n", i);
return;
}
@@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
mutex_unlock(_enc->enc_lock);
}
-static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
+static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
enum dpu_intf_type type, u32 controller_id)
{
int i = 0;
- for (i = 0; i < catalog->intf_count; i++) {
- if (catalog->intf[i].type == type
- && catalog->intf[i].controller_id == controller_id) {
- return catalog->intf[i].id;
+ if (type != INTF_WB) {
+ for (i = 0; i < catalog->intf_count; i++) {
+ if (catalog->intf[i].type == type
+ && catalog->intf[i].controller_id ==
controller_id) {
+ return catalog->intf[i].id;
+ }
+ }
+ } else {
+ for (i = 0; i < catalog->wb_count; i++) {
+ if (catalog->wb[i].id == controller_id)
+ return catalog->wb[i].id;
}
}
@@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
Re: [Freedreno] [PATCH 06/12] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder
On 2022-02-04 13:17:19, Abhinav Kumar wrote:
> Make changes to dpu_encoder to support virtual encoder needed
> to support writeback for dpu.
>
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57
> +
> 1 file changed, 42 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index e977c05..947069b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
> *drm_enc,
> struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
> struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
> struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
> + enum dpu_hw_blk_type blk_type;
> int num_lm, num_ctl, num_pp;
> int i, j;
>
> @@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
> phys->hw_pp = dpu_enc->hw_pp[i];
> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>
> + if (phys->intf_mode == INTF_MODE_WB_LINE)
> + blk_type = DPU_HW_BLK_WB;
> + else
> + blk_type = DPU_HW_BLK_INTF;
> +
> num_blk = dpu_rm_get_assigned_resources(_kms->rm,
> - global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
> + global_state, drm_enc->base.id, blk_type,
> hw_blk, ARRAY_SIZE(hw_blk));
> - for (j = 0; j < num_blk; j++) {
> - struct dpu_hw_intf *hw_intf;
>
> - hw_intf = to_dpu_hw_intf(hw_blk[i]);
> - if (hw_intf->idx == phys->intf_idx)
> - phys->hw_intf = hw_intf;
> + if (blk_type == DPU_HW_BLK_WB) {
> + for (j = 0; j < num_blk; j++) {
> + struct dpu_hw_wb *hw_wb;
> +
> + hw_wb = to_dpu_hw_wb(hw_blk[i]);
> + if (hw_wb->idx == phys->intf_idx)
> + phys->hw_wb = hw_wb;
> + }
> + } else {
> + for (j = 0; j < num_blk; j++) {
> + struct dpu_hw_intf *hw_intf;
> +
> + hw_intf = to_dpu_hw_intf(hw_blk[i]);
> + if (hw_intf->idx == phys->intf_idx)
> + phys->hw_intf = hw_intf;
> + }
It appears the original bit of code iterates j from 0 to num_blks yet
only uses i as iteration value. All of this code seems reentrant
meaning that executing it more than one times is redundant. You've
adopted this mistake, though I'd argue it should use j in hw_blk[i] as
that follows the number of elements in hw_blk returned by
dpu_rm_get_assigned_resources.
I suggest to address this in a separate patch (I can send that too, feel
free to add my Reported-by otherwise) and rebase this patch on top of
that, substituting i with j here as well. It seems to have been
introduced by b954fa6baaca ("drm/msm/dpu: Refactor rm iterator").
- Marijn
> }
>
> - if (!phys->hw_intf) {
> + if (!phys->hw_intf && !phys->hw_wb) {
> DPU_ERROR_ENC(dpu_enc,
> - "no intf block assigned at idx: %d\n", i);
> + "no intf or WB block assigned at idx:
> %d\n", i);
> return;
> }
>
> @@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
> mutex_unlock(_enc->enc_lock);
> }
>
> -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> enum dpu_intf_type type, u32 controller_id)
> {
> int i = 0;
>
> - for (i = 0; i < catalog->intf_count; i++) {
> - if (catalog->intf[i].type == type
> - && catalog->intf[i].controller_id == controller_id) {
> - return catalog->intf[i].id;
> + if (type != INTF_WB) {
> + for (i = 0; i < catalog->intf_count; i++) {
> + if (catalog->intf[i].type == type
> + && catalog->intf[i].controller_id ==
> controller_id) {
> + return catalog->intf[i].id;
> + }
> + }
> + } else {
> + for (i = 0; i < catalog->wb_count; i++) {
> + if (catalog->wb[i].id == controller_id)
> + return catalog->wb[i].id;
> }
> }
>
> @@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct
> dpu_encoder_virt *dpu_enc,
> case DRM_MODE_ENCODER_TMDS:
>
Re: [Freedreno] [PATCH 10/12] drm/msm/dpu: initialize dpu encoder and connector for writeback
On 2/4/2022 2:34 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Initialize dpu encoder and connector for writeback if the
target supports it in the catalog.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 37 -
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 62
+
2 files changed, 88 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b51a677..3746432 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2066,7 +2066,7 @@ static void dpu_encoder_early_unregister(struct
drm_encoder *encoder)
}
static int dpu_encoder_virt_add_phys_encs(
- u32 display_caps,
+ struct msm_display_info *disp_info,
struct dpu_encoder_virt *dpu_enc,
struct dpu_enc_phys_init_params *params)
{
@@ -2085,7 +2085,7 @@ static int dpu_encoder_virt_add_phys_encs(
return -EINVAL;
}
- if (display_caps & MSM_DISPLAY_CAP_VID_MODE) {
+ if (disp_info->capabilities & MSM_DISPLAY_CAP_VID_MODE) {
enc = dpu_encoder_phys_vid_init(params);
if (IS_ERR_OR_NULL(enc)) {
@@ -2098,7 +2098,7 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}
- if (display_caps & MSM_DISPLAY_CAP_CMD_MODE) {
+ if (disp_info->capabilities & MSM_DISPLAY_CAP_CMD_MODE) {
enc = dpu_encoder_phys_cmd_init(params);
if (IS_ERR_OR_NULL(enc)) {
@@ -2111,6 +2111,19 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}
+ if (disp_info->intf_type == DRM_MODE_ENCODER_VIRTUAL) {
+ enc = dpu_encoder_phys_wb_init(params);
+
+ if (IS_ERR_OR_NULL(enc)) {
+ DPU_ERROR_ENC(dpu_enc, "failed to init wb enc: %ld\n",
+ PTR_ERR(enc));
+ return enc == NULL ? -EINVAL : PTR_ERR(enc);
+ }
+
+ dpu_enc->phys_encs[dpu_enc->num_phys_encs] = enc;
+ ++dpu_enc->num_phys_encs;
+ }
+
if (params->split_role == ENC_ROLE_SLAVE)
dpu_enc->cur_slave = enc;
else
@@ -2199,9 +2212,8 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
}
if (!ret) {
- ret =
dpu_encoder_virt_add_phys_encs(disp_info->capabilities,
- dpu_enc,
- _params);
+ ret = dpu_encoder_virt_add_phys_encs(disp_info,
+ dpu_enc, _params);
if (ret)
DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n");
}
@@ -2317,11 +2329,14 @@ struct drm_encoder *dpu_encoder_init(struct
drm_device *dev,
if (!dpu_enc)
return ERR_PTR(-ENOMEM);
- rc = drm_encoder_init(dev, _enc->base, _encoder_funcs,
- drm_enc_mode, NULL);
- if (rc) {
- devm_kfree(dev->dev, dpu_enc);
- return ERR_PTR(rc);
+ /* this is handled by drm_writeback_connector_init for virtual
encoder */
+ if (drm_enc_mode != DRM_MODE_ENCODER_VIRTUAL) {
+ rc = drm_encoder_init(dev, _enc->base, _encoder_funcs,
+ drm_enc_mode, NULL);
+ if (rc) {
+ devm_kfree(dev->dev, dpu_enc);
+ return ERR_PTR(rc);
+ }
}
drm_encoder_helper_add(_enc->base, _encoder_helper_funcs);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 47fe11a..6327ba9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark
@@ -15,6 +16,7 @@
#include
#include
#include
+#include
#include "msm_drv.h"
#include "msm_mmu.h"
@@ -29,6 +31,7 @@
#include "dpu_kms.h"
#include "dpu_plane.h"
#include "dpu_vbif.h"
+#include "dpu_writeback.h"
#define CREATE_TRACE_POINTS
#include "dpu_trace.h"
@@ -642,6 +645,56 @@ static int _dpu_kms_initialize_displayport(struct
drm_device *dev,
return 0;
}
+static int _dpu_kms_initialize_writeback(struct drm_device *dev,
+ struct msm_drm_private *priv, struct dpu_kms *dpu_kms)
+{
+ struct drm_encoder *encoder = NULL;
+ struct msm_display_info info;
+ int rc, i;
+ const u32 *wb_formats;
+ int n_formats;
+
+ encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
+ if (IS_ERR(encoder)) {
+ DPU_ERROR("encoder init failed for dsi display\n");
+ return PTR_ERR(encoder);
+ }
+
+ memset(, 0, sizeof(info));
+
+ for (i = 0; i < dpu_kms->catalog->wb_count; i++) {
+ if (dpu_kms->catalog->wb[i].id == WB_2)
Re: [Freedreno] [PATCH 08/12] drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback
On 2/4/2022 3:19 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Introduce the dpu_encoder_phys_* for the writeback interface
to handle writeback specific hardware programming.
Signed-off-by: Abhinav Kumar
Mostly looks ok, see minor comments bellow.
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 36 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 813
+
3 files changed, 849 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index c43ef35..3abaf84 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -53,6 +53,7 @@ msm-y := \
disp/dpu1/dpu_encoder.o \
disp/dpu1/dpu_encoder_phys_cmd.o \
disp/dpu1/dpu_encoder_phys_vid.o \
+ disp/dpu1/dpu_encoder_phys_wb.o \
disp/dpu1/dpu_formats.o \
disp/dpu1/dpu_hw_catalog.o \
disp/dpu1/dpu_hw_ctl.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 7b3354d..80da0a9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -159,6 +159,7 @@ struct dpu_encoder_phys_ops {
* @INTR_IDX_PINGPONG: Pingpong done unterrupt for cmd mode panel
* @INTR_IDX_UNDERRUN: Underrun unterrupt for video and cmd mode panel
* @INTR_IDX_RDPTR: Readpointer done unterrupt for cmd mode panel
+ * @INTR_IDX_WB_DONE: Writeback fone interrupt for virtual connector
*/
enum dpu_intr_idx {
INTR_IDX_VSYNC,
@@ -166,6 +167,7 @@ enum dpu_intr_idx {
INTR_IDX_UNDERRUN,
INTR_IDX_CTL_START,
INTR_IDX_RDPTR,
+ INTR_IDX_WB_DONE,
INTR_IDX_MAX,
};
@@ -196,7 +198,7 @@ struct dpu_encoder_irq {
* @hw_ctl: Hardware interface to the ctl registers
* @hw_pp: Hardware interface to the ping pong registers
* @hw_intf: Hardware interface to the intf registers
- * @hw_wb: Hardware interface to the wb registers
+ * @hw_wb: Hardware interface to the wb registers
* @dpu_kms: Pointer to the dpu_kms top level
* @cached_mode: DRM mode cached at mode_set time, acted on in
enable
* @enabled: Whether the encoder has enabled and running a mode
@@ -250,6 +252,31 @@ static inline int
dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
}
/**
+ * struct dpu_encoder_phys_wb - sub-class of dpu_encoder_phys to
handle command
+ * mode specific operations
+ * @base: Baseclass physical encoder structure
+ * @wbirq_refcount: Reference count of writeback interrupt
+ * @wb_done_timeout_cnt: number of wb done irq timeout errors
+ * @wb_cfg: writeback block config to store fb related details
+ * @cdp_cfg: chroma down prefetch block config for wb
+ * @aspace: address space to be used for wb framebuffer
+ * @wb_conn: backpointer to writeback connector
+ * @wb_job: backpointer to current writeback job
+ * @dest: dpu buffer layout for current writeback output buffer
+ */
+struct dpu_encoder_phys_wb {
+ struct dpu_encoder_phys base;
+ atomic_t wbirq_refcount;
+ int wb_done_timeout_cnt;
+ struct dpu_hw_wb_cfg wb_cfg;
+ struct dpu_hw_wb_cdp_cfg cdp_cfg;
What about moving them to the stack rather storing them in the structure?
Yes cdp_cfg can be moved to the stack. Will do it in the next version.
wb_cfg cannot because for drm_wb, prepare_wb_job has the job's fb
attributes which are used to setup the wb.
But we commit it only in the encoder_kickoff in
dpu_encoder_phys_wb_setup_fb().
So its shared across two methods.
+ struct msm_gem_address_space *aspace;
+ struct drm_writeback_connector *wb_conn;
+ struct drm_writeback_job *wb_job;
+ struct dpu_hw_fmt_layout dest;
+};
+
+/**
* struct dpu_encoder_phys_cmd - sub-class of dpu_encoder_phys to
handle command
* mode specific operations
* @base: Baseclass physical encoder structure
@@ -317,6 +344,13 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
struct dpu_enc_phys_init_params *p);
/**
+ * dpu_encoder_phys_wb_init - initialize writeback encoder
+ * @init: Pointer to init info structure with initialization params
+ */
+struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
+ struct dpu_enc_phys_init_params *p);
+
+/**
* dpu_encoder_helper_trigger_start - control start helper function
* This helper function may be optionally specified by physical
* encoders if they require ctl_start triggering.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
new file mode 100644
index 000..783f83e
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -0,0 +1,813 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022
Re: [Freedreno] [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
On 4/8/2022 4:59 PM, Dmitry Baryshkov wrote: On Fri, 8 Apr 2022 at 23:30, Kuogee Hsieh wrote: On 4/8/2022 5:27 AM, Dmitry Baryshkov wrote: On 07/04/2022 00:28, Kuogee Hsieh wrote: dp_hpd_plug_handle() is responsible for setting up main link and send uevent to notify user space framework to start video stream. Similarly, dp_hdp_unplug_handle is responsible to send uevent to notify user space framework to stop video stream and then tear down main link. However there are rare cases, such as in the middle of system suspending, that uevent could not be delivered to user space framework. Therefore some kind of recover mechanism armed by timer need to be in place in the case of user space framework does not respond to uevent. Hmm, how does userpsace 'respond' to the uevent? The driver should send hotplug notifications to userspace, but it must not expect any particular reaction. The userspace might be as simple, as fbdev emulation, but the driver still should function correctly. yes, driver is function correctly by setting up main link. but it does not know which resolution to display. It send hotplug notification through uevent to framework after main link is ready. Framework is responsible to set up MDP timing engine to start video stream. However it does not know which It's of no concern to the driver. It is completely the userspace problem. After resuming, it should reread available video output properties. The display could have been changed while the system is suspended. From your description I still do not understand why you need the 'recovery' mechanism. I mean "recovery" means dp driver may leave stay at link ready mistakenly after dongle unplugged due to missing framework action to tear down main link so that next dongle plug in will not work. Currently it was armed with timeout function and it will fired if framework did not call msm_dp_display_disable() after 5 second. Anyway, we think this approach is not good. therefore it is replaced with below patch. drm/msm/dp: tear down main link at unplug handle immediately
Re: [Freedreno] [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus
Hi, On Thu, Apr 14, 2022 at 2:16 PM Dmitry Baryshkov wrote: > > > Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work? > > It's definitely worth confirming but from my reading of the code it > > _probably_ wouldn't hurt. > > > > One thing someone would want to confirm would be what would happen if > > we move this code and the panel code to implement DRM_BRIDGE_OP_EDID > > properly. It looks as if both actually ought to be implementing that > > instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a > > future day. Could we get into trouble if one moved before the other? > > Then the panel would no longer override the eDP controller and the eDP > > controller would try to read from a possibly unpowered panel? > > That would depend on the way the get_edid would be implemented in DP > driver. Currently the edid is cached via the > dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain. > > With this patchset, the dp_hpd_plug_handle() -> > dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be > called too late for the get_modes/get_edid (from dp_bridge's enable() op). > > There is another issue. drm_panel has only get_modes() callback, so > panel_bridge can not implement get_edid() unless we extend the panel > interface (which might be a good idea). Ah, that makes sense and explains why the current panel code does the EDID reading in its get_modes() function even though get_modes() is _documented_ that it doesn't read the EDID. ;-) I guess it's another of the "let's move some people over to the new way but we'll keep the old code working". Definitely makes it hard to understand at times. > > For hotplug/detect I'm even less confident that setting the bits would > > be harmless. I haven't sat down and traced everything, but from what I > > can see the panel _doesn't_ set these bits, does it? I believe that > > the rule is that when every bridge in the chain _doesn't_ implement > > detect/hotplug that the panel is always present. The moment someone > > says "hey, I can detect" then it suddenly becomes _not_ always > > present. Yes, I guess we could have the panel implement "detect" and > > return true, but I'm not convinced that's actually better... > > I think it makes sense to implement OP_DETECT in panel bridge (that > always returns connector_status_connected) at least to override the > possible detect ops in previous bridges. So I truly don't know the right answer, but are you sure that's the best design? I _think_ that panel_bridge is used for all kinds of panels, right? So what if there's some type of display that uses a panel but there's still a mechanism that supports physical detection of the panel? By implementing "detect" in the generic panel_bridge then you're _preventing_ anyone higher up in the chain from implementing it and you're forcing it to be "always connected". For instance, we could come up with a new display standard called "pluggable eDP" that is just like eDP except that you can physically detect it. This imaginary new display standard is different from DP because it has eDP power sequencing (fully powers the display off when the screen is off) but it's hot pluggable! It introduces a new pin that goes to the DP controller called RT-HPD for "really, truly hot plug detect" that works even when the panel is off. The existing "HPD" pin continues to mean that the panel is read to communicate. If the drm_panel hardcodes "always connected" then I can't implement my "pluggable eDP" system, right? However, if we leave it just like it is today then my new system would be easy to implement. ;-) The above example is obviously not truly a real one but I guess my point is that I find it more intuitive / useful to say that we should only implement "detect" if we truly think we can detect and that if nobody says they can detect then we must be always connected. As an aside; I think in general it's not always easy to fit every possible graphics system into these "bridge chains" and the simple sequence of pre-enable, enable, etc, so we have to do our best and accept the fact that sometimes we'll need special cases. Dave Stephenson's patches [1] should tell us that, at least. [1] https://lore.kernel.org/all/cover.1646406653.git.dave.steven...@raspberrypi.com/ -Doug
Re: [Freedreno] [PATCH 06/12] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder
On 2/4/2022 3:36 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Make changes to dpu_encoder to support virtual encoder needed
to support writeback for dpu.
This patch will change significantly if
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57
+
1 file changed, 42 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e977c05..947069b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct
drm_encoder *drm_enc,
struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
+ enum dpu_hw_blk_type blk_type;
int num_lm, num_ctl, num_pp;
int i, j;
@@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct
drm_encoder *drm_enc,
phys->hw_pp = dpu_enc->hw_pp[i];
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
+ if (phys->intf_mode == INTF_MODE_WB_LINE)
+ blk_type = DPU_HW_BLK_WB;
+ else
+ blk_type = DPU_HW_BLK_INTF;
+
num_blk = dpu_rm_get_assigned_resources(_kms->rm,
- global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
+ global_state, drm_enc->base.id, blk_type,
hw_blk, ARRAY_SIZE(hw_blk));
- for (j = 0; j < num_blk; j++) {
- struct dpu_hw_intf *hw_intf;
- hw_intf = to_dpu_hw_intf(hw_blk[i]);
- if (hw_intf->idx == phys->intf_idx)
- phys->hw_intf = hw_intf;
+ if (blk_type == DPU_HW_BLK_WB) {
+ for (j = 0; j < num_blk; j++) {
+ struct dpu_hw_wb *hw_wb;
+
+ hw_wb = to_dpu_hw_wb(hw_blk[i]);
+ if (hw_wb->idx == phys->intf_idx)
+ phys->hw_wb = hw_wb;
+ }
+ } else {
+ for (j = 0; j < num_blk; j++) {
+ struct dpu_hw_intf *hw_intf;
+
+ hw_intf = to_dpu_hw_intf(hw_blk[i]);
+ if (hw_intf->idx == phys->intf_idx)
+ phys->hw_intf = hw_intf;
+ }
}
I think that if we sequentially call dpu_rm_get_assigned_resources(..,
DPU_HW_BLK_INTF, ...) and then dpu_rm_get_assigned_resources(..,
DPU_HW_BLK_WB, ...), the code would be cleaner.
Or even better get the WB direclty using the provided ID.
Yeah i think you have done something like this for INTF on msm-next.
Will follow that and post in the next version :)
- if (!phys->hw_intf) {
+ if (!phys->hw_intf && !phys->hw_wb) {
DPU_ERROR_ENC(dpu_enc,
- "no intf block assigned at idx: %d\n", i);
+ "no intf or WB block assigned at idx: %d\n", i);
return;
}
@@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
mutex_unlock(_enc->enc_lock);
}
-static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
+static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
*catalog,
enum dpu_intf_type type, u32 controller_id)
{
int i = 0;
- for (i = 0; i < catalog->intf_count; i++) {
- if (catalog->intf[i].type == type
- && catalog->intf[i].controller_id == controller_id) {
- return catalog->intf[i].id;
+ if (type != INTF_WB) {
+ for (i = 0; i < catalog->intf_count; i++) {
+ if (catalog->intf[i].type == type
+ && catalog->intf[i].controller_id == controller_id) {
+ return catalog->intf[i].id;
+ }
+ }
+ } else {
+ for (i = 0; i < catalog->wb_count; i++) {
+ if (catalog->wb[i].id == controller_id)
+ return catalog->wb[i].id;
}
}
@@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
case DRM_MODE_ENCODER_TMDS:
intf_type = INTF_DP;
break;
+ case DRM_MODE_ENCODER_VIRTUAL:
+ intf_type = INTF_WB;
+ break;
}
WARN_ON(disp_info->num_of_h_tiles < 1);
@@ -2128,11 +2155,11 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
i, controller_id, phys_params.split_role);
- phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
+ phys_params.intf_idx =
dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
intf_type,
controller_id);
if (phys_params.intf_idx == INTF_MAX) {
- DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id
%d\n",
+ DPU_ERROR_ENC(dpu_enc,
Re: [Freedreno] [PATCH 05/12] drm/msm/dpu: add an API to reset the encoder related hw blocks
On 2/4/2022 3:46 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add an API to reset the encoder related hw blocks to ensure
a proper teardown of the pipeline. At the moment this is being
used only for the writeback encoder but eventually we can start
using this for all interfaces.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 92
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 10 +++
2 files changed, 102 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db..e977c05 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2014-2018, 2020-2021 The Linux Foundation. All
rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark
@@ -21,6 +22,7 @@
#include "dpu_hw_intf.h"
#include "dpu_hw_ctl.h"
#include "dpu_hw_dspp.h"
+#include "dpu_hw_merge3d.h"
#include "dpu_formats.h"
#include "dpu_encoder_phys.h"
#include "dpu_crtc.h"
@@ -1813,6 +1815,96 @@ void dpu_encoder_kickoff(struct drm_encoder
*drm_enc)
DPU_ATRACE_END("encoder_kickoff");
}
+static void dpu_encoder_helper_reset_mixers(struct dpu_encoder_phys
*phys_enc)
+{
+ struct dpu_hw_mixer_cfg mixer;
+ int i, num_lm;
+ u32 flush_mask = 0;
+ struct dpu_global_state *global_state;
+ struct dpu_hw_blk *hw_lm[2];
+ struct dpu_hw_mixer *hw_mixer[2];
+ struct dpu_hw_ctl *ctl = phys_enc->hw_ctl;
+
+ memset(, 0, sizeof(mixer));
+
+ /* reset all mixers for this encoder */
+ if (phys_enc->hw_ctl->ops.clear_all_blendstages)
+ phys_enc->hw_ctl->ops.clear_all_blendstages(phys_enc->hw_ctl);
+
+ global_state = dpu_kms_get_existing_global_state(phys_enc->dpu_kms);
+
+ num_lm = dpu_rm_get_assigned_resources(_enc->dpu_kms->rm,
global_state,
+ phys_enc->parent->base.id, DPU_HW_BLK_LM, hw_lm,
ARRAY_SIZE(hw_lm));
+
+ for (i = 0; i < num_lm; i++) {
+ hw_mixer[i] = to_dpu_hw_mixer(hw_lm[i]);
+ flush_mask = phys_enc->hw_ctl->ops.get_bitmask_mixer(ctl,
hw_mixer[i]->idx);
+ if (phys_enc->hw_ctl->ops.update_pending_flush)
+ phys_enc->hw_ctl->ops.update_pending_flush(ctl, flush_mask);
+
+ /* clear all blendstages */
+ if (phys_enc->hw_ctl->ops.setup_blendstage)
+ phys_enc->hw_ctl->ops.setup_blendstage(ctl,
hw_mixer[i]->idx, NULL);
+ }
+}
+
+void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
+{
+ struct dpu_hw_ctl *ctl = phys_enc->hw_ctl;
+ struct dpu_hw_intf_cfg intf_cfg = { 0 };
+ int i;
+ struct dpu_encoder_virt *dpu_enc;
+
+ dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
+
+ phys_enc->hw_ctl->ops.reset(ctl);
+
+ dpu_encoder_helper_reset_mixers(phys_enc);
+
+ if (phys_enc->hw_wb) {
+ /* disable the PP block */
+ if (phys_enc->hw_wb->ops.bind_pingpong_blk)
+ phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb,
false,
+ phys_enc->hw_pp->idx);
+
+ /* mark WB flush as pending */
+ if (phys_enc->hw_ctl->ops.update_pending_flush_wb)
+ phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl,
phys_enc->hw_wb->idx);
+ } else {
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ if (dpu_enc->phys_encs[i] &&
phys_enc->hw_intf->ops.bind_pingpong_blk)
+ phys_enc->hw_intf->ops.bind_pingpong_blk(
+ dpu_enc->phys_encs[i]->hw_intf, false,
+ dpu_enc->phys_encs[i]->hw_pp->idx);
+ /* mark INTF flush as pending */
+ if (phys_enc->hw_ctl->ops.update_pending_flush_intf)
+
phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl,
+ dpu_enc->phys_encs[i]->hw_intf->idx);
+ }
+ }
+
+ /* reset the merge 3D HW block */
+ if (phys_enc->hw_pp->merge_3d) {
+
phys_enc->hw_pp->merge_3d->ops.setup_3d_mode(phys_enc->hw_pp->merge_3d,
+ BLEND_3D_NONE);
+ if (phys_enc->hw_ctl->ops.update_pending_flush_merge_3d)
+ phys_enc->hw_ctl->ops.update_pending_flush_merge_3d(ctl,
+ phys_enc->hw_pp->merge_3d->idx);
+ }
+
+ intf_cfg.stream_sel = 0; /* Don't care value for video mode */
+ intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ if (phys_enc->hw_pp->merge_3d)
+ intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
+
+ if (ctl->ops.reset_intf_cfg)
+ ctl->ops.reset_intf_cfg(ctl, _cfg, true);
+
+ ctl->ops.trigger_flush(ctl);
+ ctl->ops.trigger_start(ctl);
+ ctl->ops.clear_pending_flush(ctl);
+}
+
void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
{
struct dpu_encoder_virt
[Freedreno] [PATCH v5] drm/msm/dp: stop event kernel thread when DP unbind
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().
Changes in v2:
-- start event thread at dp_display_bind()
Changes in v3:
-- disable all HDP interrupts at unbind
-- replace dp_hpd_event_setup() with dp_hpd_event_thread_start()
-- replace dp_hpd_event_stop() with dp_hpd_event_thread_stop()
-- move init_waitqueue_head(>event_q) to probe()
-- move spin_lock_init(>event_lock) to probe()
Changes in v4:
-- relocate both dp_display_bind() and dp_display_unbind() to bottom of file
Changes in v5:
-- cancel relocation of both dp_display_bind() and dp_display_unbind()
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
Reported-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 35 ---
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..baba95d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
u32 hpd_state;
u32 event_pndx;
u32 event_gndx;
+ struct task_struct *ev_tsk;
struct dp_event event_list[DP_EVENT_Q_MAX];
spinlock_t event_lock;
@@ -230,6 +231,18 @@ void dp_display_signal_audio_complete(struct msm_dp
*dp_display)
complete_all(>audio_comp);
}
+
+static void dp_hpd_event_thread_stop(struct dp_display_private *dp_priv)
+{
+ kthread_stop(dp_priv->ev_tsk);
+
+ /* reset event q to empty */
+ dp_priv->event_gndx = 0;
+ dp_priv->event_pndx = 0;
+}
+
+static int dp_hpd_event_thread_start(struct dp_display_private *dp_priv);
+
static int dp_display_bind(struct device *dev, struct device *master,
void *data)
{
@@ -269,6 +282,7 @@ static int dp_display_bind(struct device *dev, struct
device *master,
if (rc)
DRM_ERROR("Audio registration Dp failed\n");
+ rc = dp_hpd_event_thread_start(dp);
end:
return rc;
}
@@ -280,6 +294,9 @@ static void dp_display_unbind(struct device *dev, struct
device *master,
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
+ /* disable all HPD interrupts */
+ dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
+ dp_hpd_event_thread_stop(dp);
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp[dp->id] = NULL;
@@ -1054,7 +1071,7 @@ static int hpd_event_thread(void *data)
dp_priv = (struct dp_display_private *)data;
- while (1) {
+ while (!kthread_should_stop()) {
if (timeout_mode) {
wait_event_timeout(dp_priv->event_q,
(dp_priv->event_pndx == dp_priv->event_gndx),
@@ -1132,12 +1149,15 @@ static int hpd_event_thread(void *data)
return 0;
}
-static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
+static int dp_hpd_event_thread_start(struct dp_display_private *dp_priv)
{
- init_waitqueue_head(_priv->event_q);
- spin_lock_init(_priv->event_lock);
+ dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv,
"dp_hpd_handler");
+ if (IS_ERR(dp_priv->ev_tsk)) {
+ DRM_ERROR("failed to create DP event thread\n");
+ return PTR_ERR(dp_priv->ev_tsk);
+ }
- kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
+ return 0;
}
static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
@@ -1266,7 +1286,10 @@ static int dp_display_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
}
+ /* setup event q */
mutex_init(>event_mutex);
+ init_waitqueue_head(>event_q);
+ spin_lock_init(>event_lock);
/* Store DP audio handle inside DP display */
dp->dp_display.dp_audio = dp->audio;
@@ -1441,8 +1464,6 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp = container_of(dp_display, struct dp_display_private, dp_display);
- dp_hpd_event_setup(dp);
-
dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [Freedreno] [PATCH 04/12] drm/msm/dpu: add changes to support writeback in hw_ctl
On 2/4/2022 2:19 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add changes to support writeback module in the dpu_hw_ctl
interface. In addition inroduce a reset_intf_cfg op to reset
the interface bits for the currently active interfaces in
the ctl path.
Signed-off-by: Abhinav Kumar
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 65
--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 27 -
4 files changed, 91 insertions(+), 10 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 34a6940..4cb72fa 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
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
@@ -70,7 +71,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
- ctl->ops.setup_intf_cfg(ctl, _cfg);
+ ctl->ops.setup_intf_cfg(ctl, _cfg, false);
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89..950fcd6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, 2020-2021 The Linux Foundation. All
rights reserved.
*/
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
@@ -290,7 +292,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
_params, fmt);
- phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, _cfg);
+ phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, _cfg,
false);
/* setup which pp blk will connect to this intf */
if (phys_enc->hw_intf->ops.bind_pingpong_blk)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 02da9ec..a2069af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#include
@@ -23,8 +24,10 @@
#define CTL_SW_RESET 0x030
#define CTL_LAYER_EXTN_OFFSET 0x40
#define CTL_MERGE_3D_ACTIVE 0x0E4
+#define CTL_WB_ACTIVE 0x0EC
#define CTL_INTF_ACTIVE 0x0F4
#define CTL_MERGE_3D_FLUSH 0x100
+#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH 0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
@@ -35,6 +38,7 @@
#define DPU_REG_RESET_TIMEOUT_US 2000
#define MERGE_3D_IDX 23
#define INTF_IDX 31
+#define WB_IDX 16
#define CTL_INVALID_BIT 0x
#define CTL_DEFAULT_GROUP_ID 0xf
@@ -128,6 +132,9 @@ static inline void
dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
if (ctx->pending_flush_mask & BIT(INTF_IDX))
DPU_REG_WRITE(>hw, CTL_INTF_FLUSH,
ctx->pending_intf_flush_mask);
+ if (ctx->pending_flush_mask & BIT(WB_IDX))
+ DPU_REG_WRITE(>hw, CTL_WB_FLUSH,
+ ctx->pending_wb_flush_mask);
DPU_REG_WRITE(>hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -248,6 +255,13 @@ static void
dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
}
}
+static void dpu_hw_ctl_update_pending_flush_wb_v1(struct dpu_hw_ctl
*ctx,
+ enum dpu_wb wb)
+{
+ ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
+ ctx->pending_flush_mask |= BIT(WB_IDX);
+}
+
static void dpu_hw_ctl_update_pending_flush_intf_v1(struct
dpu_hw_ctl *ctx,
enum dpu_intf intf)
{
@@ -493,10 +507,11 @@ static void dpu_hw_ctl_setup_blendstage(struct
dpu_hw_ctl *ctx,
static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
- struct
Re: [Freedreno] [PATCH v5 03/10] drm/hdcp: Update property value on content type and user changes
On Thu, Apr 14, 2022 at 03:58:02PM +, Sean Paul wrote:
> On Tue, Apr 12, 2022 at 09:25:59AM -0400, Rodrigo Vivi wrote:
> > On Mon, Apr 11, 2022 at 08:47:32PM +, Sean Paul wrote:
> > > From: Sean Paul
> > >
> > > This patch updates the connector's property value in 2 cases which were
> > > previously missed:
> > >
> > > 1- Content type changes. The value should revert back to DESIRED from
> > >ENABLED in case the driver must re-authenticate the link due to the
> > >new content type.
> > >
> > > 2- Userspace sets value to DESIRED while ENABLED. In this case, the
> > >value should be reset immediately to ENABLED since the link is
> > >actively being encrypted.
> > >
> > > To accommodate these changes, I've split up the conditionals to make
> > > things a bit more clear (as much as one can with this mess of state).
> > >
> > > Acked-by: Jani Nikula
> > > Reviewed-by: Abhinav Kumar
> > > Signed-off-by: Sean Paul
> > > Link:
> > > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-4-s...@poorly.run
> > > #v1
> > > Link:
> > > https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-4-s...@poorly.run
> > > #v2
> > > Link:
> > > https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-4-s...@poorly.run
> > > #v3
> > > Link:
> > > https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-4-s...@poorly.run
> > > #v4
> > >
> > > Changes in v2:
> > > -None
> > > Changes in v3:
> > > -Fixed indentation issue identified by 0-day
> > > Changes in v4:
> > > -None
> > > Changes in v5:
> > > -None
> > > ---
> > > drivers/gpu/drm/drm_hdcp.c | 26 +-
> > > 1 file changed, 17 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > > index dd8fa91c51d6..8c851d40cd45 100644
> > > --- a/drivers/gpu/drm/drm_hdcp.c
> > > +++ b/drivers/gpu/drm/drm_hdcp.c
> > > @@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector
> > > *connector,
> > > return true;
> > >
> > > /*
> > > - * Nothing to do if content type is unchanged and one of:
> > > - * - state didn't change
> > > + * Content type changes require an HDCP disable/enable cycle.
> > > + */
> > > + if (new_conn_state->hdcp_content_type !=
> > > old_conn_state->hdcp_content_type) {
> >
> > shouldn't we add some && ( old_hdcp ==
> > DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> > here?
>
> Thanks for your reviews Rodrigo.
>
> I don't think so since the content type is changing the current state of old
> content protection is immaterial (ie: if we need to enable HDCP 2.x, the state
> of HDCP 1.x doesn't really matter), we need to re-evaluate whether the current
> level of HDCP is sufficient.
>
> Hopefully that makes sense, but I could be missing something :-)
nope, I think I was just missing that the important is the content type change
as you pointed out in the item number 1 in the commit msg.
Reviewed-by: Rodrigo Vivi
>
> Sean
>
> >
> > > + new_conn_state->content_protection =
> > > + DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > + return true;
> > > + }
> > > +
> > > + /*
> > > + * Ignore meaningless state changes:
> > >* - HDCP was activated since the last commit
> > > - * - attempting to set to desired while already enabled
> > > + * - Attempting to set to desired while already enabled
> > >*/
> > > - if (old_hdcp == new_hdcp ||
> > > - (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > > + if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > >new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
> > > (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > >new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
> > > - if (old_conn_state->hdcp_content_type ==
> > > - new_conn_state->hdcp_content_type)
> > > - return false;
> > > + new_conn_state->content_protection =
> > > + DRM_MODE_CONTENT_PROTECTION_ENABLED;
> > > + return false;
> > > }
> > >
> > > - return true;
> > > + /* Finally, if state changes, we need action */
> > > + return old_hdcp != new_hdcp;
> > > }
> > > EXPORT_SYMBOL(drm_hdcp_atomic_check);
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > >
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
Re: [Freedreno] [PATCH v4] drm/msm/dp: stop event kernel thread when DP unbind
On 15/04/2022 00:26, Kuogee Hsieh wrote:
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().
Changes in v2:
-- start event thread at dp_display_bind()
Changes in v3:
-- disable all HDP interrupts at unbind
-- replace dp_hpd_event_setup() with dp_hpd_event_thread_start()
-- replace dp_hpd_event_stop() with dp_hpd_event_thread_stop()
-- move init_waitqueue_head(>event_q) to probe()
-- move spin_lock_init(>event_lock) to probe()
Changes in v4:
-- relocate both dp_display_bind() and dp_display_unbind() to bottom of file
Ugh, no. I think Stephen asked to do it in the followup (= additional)
patch. Doing it in the same patch squashes unrelated changes and makes
review harder. The previous one was nearly ideal.
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
Reported-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 345 +++-
1 file changed, 182 insertions(+), 163 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..198df6b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
u32 hpd_state;
u32 event_pndx;
u32 event_gndx;
+ struct task_struct *ev_tsk;
struct dp_event event_list[DP_EVENT_Q_MAX];
spinlock_t event_lock;
@@ -230,65 +231,6 @@ void dp_display_signal_audio_complete(struct msm_dp *dp_display)
complete_all(>audio_comp);
}
-static int dp_display_bind(struct device *dev, struct device *master,
- void *data)
-{
- int rc = 0;
- struct dp_display_private *dp = dev_get_dp_display_private(dev);
- struct msm_drm_private *priv;
- struct drm_device *drm;
-
- drm = dev_get_drvdata(master);
-
- dp->dp_display.drm_dev = drm;
- priv = drm->dev_private;
- priv->dp[dp->id] = >dp_display;
-
- rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
- if (rc) {
- DRM_ERROR("device tree parsing failed\n");
- goto end;
- }
-
- dp->dp_display.panel_bridge = dp->parser->panel_bridge;
-
- dp->aux->drm_dev = drm;
- rc = dp_aux_register(dp->aux);
- if (rc) {
- DRM_ERROR("DRM DP AUX register failed\n");
- goto end;
- }
-
- rc = dp_power_client_init(dp->power);
- if (rc) {
- DRM_ERROR("Power client create failed\n");
- goto end;
- }
-
- rc = dp_register_audio_driver(dev, dp->audio);
- if (rc)
- DRM_ERROR("Audio registration Dp failed\n");
-
-end:
- return rc;
-}
-
-static void dp_display_unbind(struct device *dev, struct device *master,
- void *data)
-{
- struct dp_display_private *dp = dev_get_dp_display_private(dev);
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
-
- dp_power_client_deinit(dp->power);
- dp_aux_unregister(dp->aux);
- priv->dp[dp->id] = NULL;
-}
-
-static const struct component_ops dp_display_comp_ops = {
- .bind = dp_display_bind,
- .unbind = dp_display_unbind,
-};
static bool dp_display_is_ds_bridge(struct dp_panel *panel)
{
@@ -1054,7 +996,7 @@ static int hpd_event_thread(void *data)
dp_priv = (struct dp_display_private *)data;
- while (1) {
+ while (!kthread_should_stop()) {
if (timeout_mode) {
wait_event_timeout(dp_priv->event_q,
(dp_priv->event_pndx == dp_priv->event_gndx),
@@ -1132,14 +1074,6 @@ static int hpd_event_thread(void *data)
return 0;
}
-static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
-{
- init_waitqueue_head(_priv->event_q);
- spin_lock_init(_priv->event_lock);
-
- kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
-}
-
static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
{
struct dp_display_private *dp = dev_id;
@@ -1237,65 +1171,6 @@ static const struct msm_dp_desc
*dp_display_get_desc(struct platform_device *pde
return NULL;
}
-static int dp_display_probe(struct platform_device *pdev)
-{
- int rc = 0;
- struct dp_display_private *dp;
- const struct msm_dp_desc *desc;
-
- if (!pdev || !pdev->dev.of_node) {
- DRM_ERROR("pdev not found\n");
- return -ENODEV;
- }
-
- dp = devm_kzalloc(>dev, sizeof(*dp), GFP_KERNEL);
- if (!dp)
- return -ENOMEM;
-
- desc = dp_display_get_desc(pdev, >id);
- if (!desc)
- return -EINVAL;
-
-
Re: [Freedreno] [PATCH 03/12] drm/msm/dpu: add writeback blocks to DPU RM
On 2/4/2022 3:43 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add writeback blocks to DPU resource manager so that
writeback encoders can request for writeback hardware blocks
through RM and their usage can be tracked.
Signed-off-by: Abhinav Kumar
[please excuse me for the duplicate, I've sent the email without the
proper distribution list]
We have WB blocks being allocated manually. Could you please consider
following the ideas from
https://patchwork.freedesktop.org/patch/470394/?series=99175=1 ?
I think it simplifies the code and shows exact correspondence between WB
and dpu_encoder.
Yes, will rebase on the latest msm-next which i think already has this
change and refactor this.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 71
+
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +
4 files changed, 78 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index e241914..cc10436 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark
@@ -21,9 +22,11 @@
/**
* Encoder functions and data types
* @intfs: Interfaces this encoder is using, INTF_MODE_NONE if
unused
+ * @wbs: Writeback blocks this encoder is using
*/
struct dpu_encoder_hw_resources {
enum dpu_intf_mode intfs[INTF_MAX];
+ enum dpu_intf_mode wbs[WB_MAX];
};
/**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 2d385b4..1e00804 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark
@@ -146,6 +147,7 @@ struct dpu_global_state {
uint32_t ctl_to_enc_id[CTL_MAX - CTL_0];
uint32_t intf_to_enc_id[INTF_MAX - INTF_0];
uint32_t dspp_to_enc_id[DSPP_MAX - DSPP_0];
+ uint32_t wb_to_enc_id[WB_MAX - WB_0];
};
struct dpu_global_state
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index f9c83d6..edd0b7a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved.
* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
*/
@@ -9,6 +10,7 @@
#include "dpu_hw_ctl.h"
#include "dpu_hw_pingpong.h"
#include "dpu_hw_intf.h"
+#include "dpu_hw_wb.h"
#include "dpu_hw_dspp.h"
#include "dpu_hw_merge3d.h"
#include "dpu_encoder.h"
@@ -75,6 +77,14 @@ int dpu_rm_destroy(struct dpu_rm *rm)
dpu_hw_intf_destroy(hw);
}
}
+ for (i = 0; i < ARRAY_SIZE(rm->wb_blks); i++) {
+ struct dpu_hw_wb *hw;
+
+ if (rm->wb_blks[i]) {
+ hw = to_dpu_hw_wb(rm->wb_blks[i]);
+ dpu_hw_wb_destroy(hw);
+ }
+ }
return 0;
}
@@ -187,6 +197,24 @@ int dpu_rm_init(struct dpu_rm *rm,
rm->intf_blks[intf->id - INTF_0] = >base;
}
+ for (i = 0; i < cat->wb_count; i++) {
+ struct dpu_hw_wb *hw;
+ const struct dpu_wb_cfg *wb = >wb[i];
+
+ if (wb->id < WB_0 || wb->id >= WB_MAX) {
+ DPU_ERROR("skip intf %d with invalid id\n", wb->id);
+ continue;
+ }
+
+ hw = dpu_hw_wb_init(wb->id, mmio, cat);
+ if (IS_ERR_OR_NULL(hw)) {
+ rc = PTR_ERR(hw);
+ DPU_ERROR("failed wb object creation: err %d\n", rc);
+ goto fail;
+ }
+ rm->wb_blks[wb->id - WB_0] = >base;
+ }
+
for (i = 0; i < cat->ctl_count; i++) {
struct dpu_hw_ctl *hw;
const struct dpu_ctl_cfg *ctl = >ctl[i];
@@ -479,6 +507,33 @@ static int _dpu_rm_reserve_intf(
return 0;
}
+static int _dpu_rm_reserve_wb(
+ struct dpu_rm *rm,
+ struct dpu_global_state *global_state,
+ uint32_t enc_id,
+ uint32_t id)
+{
+ int idx = id - WB_0;
+
+ if (idx < 0 || idx >= ARRAY_SIZE(rm->wb_blks)) {
+ DPU_ERROR("invalid intf id: %d", id);
+ return -EINVAL;
+ }
+
+ if (!rm->wb_blks[idx]) {
+ DPU_ERROR("couldn't find wb id %d\n", id);
+ return -EINVAL;
+ }
+
+ if (reserved_by_other(global_state->wb_to_enc_id, idx, enc_id)) {
+
Re: [Freedreno] [PATCH 02/12] drm/msm/dpu: add dpu_hw_wb abstraction for writeback blocks
Hi Dmitry
Thanks for the review.
Finally got back to this series after getting acks on the drm_writeback
core changes.
On 2/4/2022 2:56 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
Add the dpu_hw_wb abstraction to program registers related to the
writeback block. These will be invoked once all the configuration
is set and ready to be programmed to the registers.
Reviewed-by: Dmitry Baryshkov
Few minor comments bellow.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 267
++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 145
3 files changed, 413 insertions(+)
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 03ab55c..c43ef35 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -66,6 +66,7 @@ msm-y := \
disp/dpu1/dpu_hw_top.o \
disp/dpu1/dpu_hw_util.o \
disp/dpu1/dpu_hw_vbif.o \
+ disp/dpu1/dpu_hw_wb.o \
disp/dpu1/dpu_io_util.o \
disp/dpu1/dpu_kms.o \
disp/dpu1/dpu_mdss.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
new file mode 100644
index 000..d395475
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
@@ -0,0 +1,267 @@
+// SPDX-License-Identifier: GPL-2.0-only
+ /*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
reserved
+ */
+
+#include "dpu_hw_mdss.h"
+#include "dpu_hwio.h"
+#include "dpu_hw_catalog.h"
+#include "dpu_hw_wb.h"
+#include "dpu_formats.h"
+#include "dpu_kms.h"
+
+#define WB_DST_FORMAT 0x000
+#define WB_DST_OP_MODE 0x004
+#define WB_DST_PACK_PATTERN 0x008
+#define WB_DST0_ADDR 0x00C
+#define WB_DST1_ADDR 0x010
+#define WB_DST2_ADDR 0x014
+#define WB_DST3_ADDR 0x018
+#define WB_DST_YSTRIDE0 0x01C
+#define WB_DST_YSTRIDE1 0x020
+#define WB_DST_YSTRIDE1 0x020
+#define WB_DST_DITHER_BITDEPTH 0x024
+#define WB_DST_MATRIX_ROW0 0x030
+#define WB_DST_MATRIX_ROW1 0x034
+#define WB_DST_MATRIX_ROW2 0x038
+#define WB_DST_MATRIX_ROW3 0x03C
+#define WB_DST_WRITE_CONFIG 0x048
+#define WB_ROTATION_DNSCALER 0x050
+#define WB_ROTATOR_PIPE_DOWNSCALER 0x054
+#define WB_N16_INIT_PHASE_X_C03 0x060
+#define WB_N16_INIT_PHASE_X_C12 0x064
+#define WB_N16_INIT_PHASE_Y_C03 0x068
+#define WB_N16_INIT_PHASE_Y_C12 0x06C
+#define WB_OUT_SIZE 0x074
+#define WB_ALPHA_X_VALUE 0x078
+#define WB_DANGER_LUT 0x084
+#define WB_SAFE_LUT 0x088
+#define WB_QOS_CTRL 0x090
+#define WB_CREQ_LUT_0 0x098
+#define WB_CREQ_LUT_1 0x09C
+#define WB_UBWC_STATIC_CTRL 0x144
+#define WB_MUX 0x150
+#define WB_CROP_CTRL 0x154
+#define WB_CROP_OFFSET 0x158
+#define WB_CSC_BASE 0x260
+#define WB_DST_ADDR_SW_STATUS 0x2B0
+#define WB_CDP_CNTL 0x2B4
+#define WB_OUT_IMAGE_SIZE 0x2C0
+#define WB_OUT_XY 0x2C4
+
+/* WB_QOS_CTRL */
+#define WB_QOS_CTRL_DANGER_SAFE_EN BIT(0)
+
+static const struct dpu_wb_cfg *_wb_offset(enum dpu_wb wb,
+ const struct dpu_mdss_cfg *m, void __iomem *addr,
+ struct dpu_hw_blk_reg_map *b)
+{
+ int i;
+
+ for (i = 0; i < m->wb_count; i++) {
+ if (wb == m->wb[i].id) {
+ b->base_off = addr;
+ b->blk_off = m->wb[i].base;
+ b->length = m->wb[i].len;
+ b->hwversion = m->hwversion;
+ return >wb[i];
+ }
+ }
+ return ERR_PTR(-EINVAL);
+}
+
+static void dpu_hw_wb_setup_outaddress(struct dpu_hw_wb *ctx,
+ struct dpu_hw_wb_cfg *data)
+{
+ struct dpu_hw_blk_reg_map *c = >hw;
+
+ DPU_REG_WRITE(c, WB_DST0_ADDR, data->dest.plane_addr[0]);
+ DPU_REG_WRITE(c, WB_DST1_ADDR, data->dest.plane_addr[1]);
+ DPU_REG_WRITE(c, WB_DST2_ADDR, data->dest.plane_addr[2]);
+ DPU_REG_WRITE(c, WB_DST3_ADDR, data->dest.plane_addr[3]);
+}
+
+static void dpu_hw_wb_setup_format(struct dpu_hw_wb *ctx,
+ struct dpu_hw_wb_cfg *data)
+{
+ struct dpu_hw_blk_reg_map *c = >hw;
+ const struct dpu_format *fmt = data->dest.format;
+ u32 dst_format, pattern,
[Freedreno] [PATCH v4] drm/msm/dp: stop event kernel thread when DP unbind
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().
Changes in v2:
-- start event thread at dp_display_bind()
Changes in v3:
-- disable all HDP interrupts at unbind
-- replace dp_hpd_event_setup() with dp_hpd_event_thread_start()
-- replace dp_hpd_event_stop() with dp_hpd_event_thread_stop()
-- move init_waitqueue_head(>event_q) to probe()
-- move spin_lock_init(>event_lock) to probe()
Changes in v4:
-- relocate both dp_display_bind() and dp_display_unbind() to bottom of file
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
Reported-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 345 +++-
1 file changed, 182 insertions(+), 163 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..198df6b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
u32 hpd_state;
u32 event_pndx;
u32 event_gndx;
+ struct task_struct *ev_tsk;
struct dp_event event_list[DP_EVENT_Q_MAX];
spinlock_t event_lock;
@@ -230,65 +231,6 @@ void dp_display_signal_audio_complete(struct msm_dp
*dp_display)
complete_all(>audio_comp);
}
-static int dp_display_bind(struct device *dev, struct device *master,
- void *data)
-{
- int rc = 0;
- struct dp_display_private *dp = dev_get_dp_display_private(dev);
- struct msm_drm_private *priv;
- struct drm_device *drm;
-
- drm = dev_get_drvdata(master);
-
- dp->dp_display.drm_dev = drm;
- priv = drm->dev_private;
- priv->dp[dp->id] = >dp_display;
-
- rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
- if (rc) {
- DRM_ERROR("device tree parsing failed\n");
- goto end;
- }
-
- dp->dp_display.panel_bridge = dp->parser->panel_bridge;
-
- dp->aux->drm_dev = drm;
- rc = dp_aux_register(dp->aux);
- if (rc) {
- DRM_ERROR("DRM DP AUX register failed\n");
- goto end;
- }
-
- rc = dp_power_client_init(dp->power);
- if (rc) {
- DRM_ERROR("Power client create failed\n");
- goto end;
- }
-
- rc = dp_register_audio_driver(dev, dp->audio);
- if (rc)
- DRM_ERROR("Audio registration Dp failed\n");
-
-end:
- return rc;
-}
-
-static void dp_display_unbind(struct device *dev, struct device *master,
- void *data)
-{
- struct dp_display_private *dp = dev_get_dp_display_private(dev);
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
-
- dp_power_client_deinit(dp->power);
- dp_aux_unregister(dp->aux);
- priv->dp[dp->id] = NULL;
-}
-
-static const struct component_ops dp_display_comp_ops = {
- .bind = dp_display_bind,
- .unbind = dp_display_unbind,
-};
static bool dp_display_is_ds_bridge(struct dp_panel *panel)
{
@@ -1054,7 +996,7 @@ static int hpd_event_thread(void *data)
dp_priv = (struct dp_display_private *)data;
- while (1) {
+ while (!kthread_should_stop()) {
if (timeout_mode) {
wait_event_timeout(dp_priv->event_q,
(dp_priv->event_pndx == dp_priv->event_gndx),
@@ -1132,14 +1074,6 @@ static int hpd_event_thread(void *data)
return 0;
}
-static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
-{
- init_waitqueue_head(_priv->event_q);
- spin_lock_init(_priv->event_lock);
-
- kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
-}
-
static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
{
struct dp_display_private *dp = dev_id;
@@ -1237,65 +1171,6 @@ static const struct msm_dp_desc
*dp_display_get_desc(struct platform_device *pde
return NULL;
}
-static int dp_display_probe(struct platform_device *pdev)
-{
- int rc = 0;
- struct dp_display_private *dp;
- const struct msm_dp_desc *desc;
-
- if (!pdev || !pdev->dev.of_node) {
- DRM_ERROR("pdev not found\n");
- return -ENODEV;
- }
-
- dp = devm_kzalloc(>dev, sizeof(*dp), GFP_KERNEL);
- if (!dp)
- return -ENOMEM;
-
- desc = dp_display_get_desc(pdev, >id);
- if (!desc)
- return -EINVAL;
-
- dp->pdev = pdev;
- dp->name = "drm_dp";
- dp->dp_display.connector_type = desc->connector_type;
-
- rc = dp_init_sub_modules(dp);
- if (rc) {
- DRM_ERROR("init sub module failed\n");
- return
Re: [Freedreno] [PATCH v4 04/10] drm/msm/gem: Split out inuse helper
On 12/04/2022 00:58, Rob Clark wrote:
From: Rob Clark
Prep for a following patch, where it gets a bit more complicated.
Signed-off-by: Rob Clark
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_gem.c | 2 +-
drivers/gpu/drm/msm/msm_gem.h | 1 +
drivers/gpu/drm/msm/msm_gem_vma.c | 9 +++--
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index a4f61972667b..f96d1dc72021 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -938,7 +938,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct
seq_file *m,
name, comm ? ":" : "", comm ? comm : "",
vma->aspace, vma->iova,
vma->mapped ? "mapped" : "unmapped",
- vma->inuse);
+ msm_gem_vma_inuse(vma));
kfree(comm);
}
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 947ff7d9b471..1b7f0f0b88bf 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -61,6 +61,7 @@ struct msm_gem_vma {
int msm_gem_init_vma(struct msm_gem_address_space *aspace,
struct msm_gem_vma *vma, int npages,
u64 range_start, u64 range_end);
+bool msm_gem_vma_inuse(struct msm_gem_vma *vma);
void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
struct msm_gem_vma *vma);
void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c
b/drivers/gpu/drm/msm/msm_gem_vma.c
index 64906594fc65..dc2ae097805e 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -37,6 +37,11 @@ msm_gem_address_space_get(struct msm_gem_address_space
*aspace)
return aspace;
}
+bool msm_gem_vma_inuse(struct msm_gem_vma *vma)
+{
+ return !!vma->inuse;
+}
+
/* Actually unmap memory for the vma */
void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
struct msm_gem_vma *vma)
@@ -44,7 +49,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
unsigned size = vma->node.size << PAGE_SHIFT;
/* Print a message if we try to purge a vma in use */
- if (GEM_WARN_ON(vma->inuse > 0))
+ if (GEM_WARN_ON(msm_gem_vma_inuse(vma)))
return;
/* Don't do anything if the memory isn't mapped */
@@ -100,7 +105,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
void msm_gem_close_vma(struct msm_gem_address_space *aspace,
struct msm_gem_vma *vma)
{
- if (GEM_WARN_ON(vma->inuse > 0 || vma->mapped))
+ if (GEM_WARN_ON(msm_gem_vma_inuse(vma) || vma->mapped))
return;
spin_lock(>lock);
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v4 03/10] drm/msm/gem: Convert some missed GEM_WARN_ON()s
On 12/04/2022 00:58, Rob Clark wrote:
From: Rob Clark
Signed-off-by: Rob Clark
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_gem_vma.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c
b/drivers/gpu/drm/msm/msm_gem_vma.c
index f914ddbaea89..64906594fc65 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -44,7 +44,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
unsigned size = vma->node.size << PAGE_SHIFT;
/* Print a message if we try to purge a vma in use */
- if (WARN_ON(vma->inuse > 0))
+ if (GEM_WARN_ON(vma->inuse > 0))
return;
/* Don't do anything if the memory isn't mapped */
@@ -61,7 +61,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
struct msm_gem_vma *vma)
{
- if (!WARN_ON(!vma->iova))
+ if (!GEM_WARN_ON(!vma->iova))
vma->inuse--;
}
@@ -73,7 +73,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
unsigned size = npages << PAGE_SHIFT;
int ret = 0;
- if (WARN_ON(!vma->iova))
+ if (GEM_WARN_ON(!vma->iova))
return -EINVAL;
/* Increase the usage counter */
@@ -100,7 +100,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
void msm_gem_close_vma(struct msm_gem_address_space *aspace,
struct msm_gem_vma *vma)
{
- if (WARN_ON(vma->inuse > 0 || vma->mapped))
+ if (GEM_WARN_ON(vma->inuse > 0 || vma->mapped))
return;
spin_lock(>lock);
@@ -120,7 +120,7 @@ int msm_gem_init_vma(struct msm_gem_address_space *aspace,
{
int ret;
- if (WARN_ON(vma->iova))
+ if (GEM_WARN_ON(vma->iova))
return -EBUSY;
spin_lock(>lock);
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus
On 14/04/2022 23:09, Doug Anderson wrote: Hi, On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-04-14 12:16:14) I think it's too verbose and a bit incorrect. Not sure which part you're asserting is incorrect, but shorter is OK w/ me too. I was referring to the "If we don't implement the ops..." part. For some reason I thought that panel implements detect() callback (and thus the DRM will not care because the next bridge takes precedence). However I was mistaken, please excuse me. Your description was correct and I was wrong. The panel bridge doesn't implement callback. Most probably I mixed it with the display_connector bridge. So... your description is more correct. This is a bit saner: /* * These ops do not make sense for eDP, since they are provided * by the panel-bridge corresponding to the attached eDP panel. */ My question was whether we really need to disable them for eDP since for eDP the detect and and get_modes will be overridden anyway. Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work? It's definitely worth confirming but from my reading of the code it _probably_ wouldn't hurt. One thing someone would want to confirm would be what would happen if we move this code and the panel code to implement DRM_BRIDGE_OP_EDID properly. It looks as if both actually ought to be implementing that instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a future day. Could we get into trouble if one moved before the other? Then the panel would no longer override the eDP controller and the eDP controller would try to read from a possibly unpowered panel? That would depend on the way the get_edid would be implemented in DP driver. Currently the edid is cached via the dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain. With this patchset, the dp_hpd_plug_handle() -> dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be called too late for the get_modes/get_edid (from dp_bridge's enable() op). There is another issue. drm_panel has only get_modes() callback, so panel_bridge can not implement get_edid() unless we extend the panel interface (which might be a good idea). So I guess in the end my preference would be that we know that driving the EDID read from the controller isn't a great idea for eDP (since we have no way to ensure that the panel is powered) so why risk it and set the bit saying we can do it? Yep. For hotplug/detect I'm even less confident that setting the bits would be harmless. I haven't sat down and traced everything, but from what I can see the panel _doesn't_ set these bits, does it? I believe that the rule is that when every bridge in the chain _doesn't_ implement detect/hotplug that the panel is always present. The moment someone says "hey, I can detect" then it suddenly becomes _not_ always present. Yes, I guess we could have the panel implement "detect" and return true, but I'm not convinced that's actually better... I think it makes sense to implement OP_DETECT in panel bridge (that always returns connector_status_connected) at least to override the possible detect ops in previous bridges. And to go further, I'd expect that a bridge should expose the functionality that it supports, regardless of what is connected down the chain. Otherwise we won't be able to mix and match bridges because the code is brittle, making assumptions about what is connected. From my point of view the bridge simply doesn't support any of the three things when we're in eDP mode. Yes, it's the same driver. Yes, eDP and DP share nearly the same signalling on the wire. Yes, it's easily possible to make a single controller that supports DP and eDP. ...but the rules around detection and power sequencing are simply different for the two cases. I just hope that during refactoring this can be expressed in a more natural way. The controller simply _cannot_ detect whether the panel is connected in the eDP case and it _must_ assume that the panel is always connected. Yes, it has an HPD pin. No, that HPD pin doesn't tell when the panel is present. The panel is always present. The panel is always present. Yep, I remember regarding the HPD pin. And I still think that panel-edp (and panel bridge) should provide an overriding OP_DETECT. So, IMO, it is _incorrect_ to say we can support HPD and DETECT if we know we're in eDP mode. I see your point. Let's do it this way. Maybe (hopefully) it will become more logical during refactoring. Or maybe I'll just tune myself into the DP/eDP logic :D -- With best wishes Dmitry
[Freedreno] [PATCH] drm/msm/dp: tear down main link at unplug handle immediately
Two stages are required to setup up main link to be ready to transmit
video stream.
Stage 1: dp_hpd_plug_handle() perform link training to set up main link
stage 2: user space framework (msm_dp_display_enable()) to enable pixel
clock and transfer main link to video ready state.
At current implementation, when dongle unplugged dp_hdp_unplug_handle()
has to wait until stage 2 completed before it can send link down uevent
to user space framework to disable pixel clock followed by tearing down
main link. This introduce unnecessary latency if dongle unplugged happen
after stage 1 and before stage 2. It also has possibility leave main link
stay at ready state after dongle unplugged if framework does not response
to link down uevent notification. This will prevent next dongle plug in
from working. This scenario could possibly happen when dongle unplug while
system in the middle of suspending.
This patch allow unplug handle to tear down main link and notify
framework link down immediately if dongle unplugged happen after
stage 1 and before stage 2. With this approach, dp driver is much
more resilient to any different scenarios. Also redundant both
dp_connect_pending_timeout() and dp_disconnect_pending_timeout()
are removed to reduce logic complexity.
Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_ctrl.c| 29 +++
drivers/gpu/drm/msm/dp/dp_ctrl.h| 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 101 +++-
3 files changed, 59 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 1a13e50..c2d060d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1891,6 +1891,35 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
return ret;
}
+int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+ struct dp_io *dp_io;
+ struct phy *phy;
+ int ret;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+ dp_io = >parser->io;
+ phy = dp_io->phy;
+
+ dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
+
+ ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
+ if (ret) {
+ DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
+ }
+
+ DRM_DEBUG_DP("Before, phy=%p init_count=%d power_on=%d\n",
+ phy, phy->init_count, phy->power_count);
+
+ phy_power_off(phy);
+
+ DRM_DEBUG_DP("After, phy=%p init_count=%d power_on=%d\n",
+ phy, phy->init_count, phy->power_count);
+
+ return ret;
+}
+
int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
{
struct dp_ctrl_private *ctrl;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2433edb..ffafe17 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -22,6 +22,7 @@ struct dp_ctrl {
int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
+int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..f5bd8f5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -57,14 +57,11 @@ enum {
EV_IRQ_HPD_INT,
EV_HPD_UNPLUG_INT,
EV_USER_NOTIFICATION,
- EV_CONNECT_PENDING_TIMEOUT,
- EV_DISCONNECT_PENDING_TIMEOUT,
};
#define EVENT_TIMEOUT (HZ/10) /* 100ms */
#define DP_EVENT_Q_MAX 8
-#define DP_TIMEOUT_5_SECOND(5000/EVENT_TIMEOUT)
#define DP_TIMEOUT_NONE0
#define WAIT_FOR_RESUME_TIMEOUT_JIFFIES (HZ / 2)
@@ -430,6 +427,11 @@ static int dp_display_usbpd_configure_cb(struct device
*dev)
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
+ return 0;
+}
+
+static int dp_display_notify_disconnect(struct device *dev)
+{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
@@ -512,7 +514,6 @@ static int dp_hpd_plug_handle(struct dp_display_private
*dp, u32 data)
{
struct dp_usbpd *hpd = dp->usbpd;
u32 state;
- u32 tout = DP_TIMEOUT_5_SECOND;
int ret;
if (!hpd)
@@ -539,14 +540,11 @@ static int dp_hpd_plug_handle(struct dp_display_private
*dp, u32 data)
return 0;
}
- dp->hpd_state = ST_CONNECT_PENDING;
-
ret = dp_display_usbpd_configure_cb(>pdev->dev);
if (ret) { /* link train failed */
dp->hpd_state = ST_DISCONNECTED;
} else {
- /* start sentinel checking in case of
Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus
On 14/04/2022 23:19, Abhinav Kumar wrote: On 4/14/2022 1:03 PM, Dmitry Baryshkov wrote: On 14/04/2022 23:00, Abhinav Kumar wrote: Hi Dmitry On 4/14/2022 12:43 PM, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-04-14 12:20:31) On 14/04/2022 19:40, Doug Anderson wrote: Hi, On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti wrote: This series adds support for generic eDP panel over aux_bus. These changes are dependent on the following series: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654=* You're basically depending on the last two patches of that series. What's the plan there? In patchwork they're marked as "Not Applicable". If they're good to go, maybe we should land them? If not, maybe you should include them (with Dmitry as the author, of course) at the beginning of your series? No, please do not resend patches. The patches in question are marked as 'Not applicable' as they are really not applicable to Bjorn's tree. It would be better to point to the correct patchwork: https://patchwork.freedesktop.org/series/98585/ Note those patches still lack the R-B tag. I can include them anyway, basing on Sankeerth's Tested-by tag, but the formal R-B would also be good. Can you resend those as not RFC? Yes, please resend these, I can ack them. Previously I held off my ack, as kuogee ran into some issues testing them which was later concluded to be a mismatch in QC internal trees due to different versions of the changes.( another reason why we should get these landed ). Now, that Sankeerth has tested these, if you can remove RFC and post them, I can ack the. Well, you can ack those patches without them being resent. You have already added your Reviewed-by to first three patches (which were merged during last window). I thought you might have to rebase them :) that way you could have resent the rebased patch with the RFC tag removed. If you dont, you now have my R-b. Thank you! -- With best wishes Dmitry
Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus
On 4/14/2022 1:03 PM, Dmitry Baryshkov wrote: On 14/04/2022 23:00, Abhinav Kumar wrote: Hi Dmitry On 4/14/2022 12:43 PM, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-04-14 12:20:31) On 14/04/2022 19:40, Doug Anderson wrote: Hi, On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti wrote: This series adds support for generic eDP panel over aux_bus. These changes are dependent on the following series: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654=* You're basically depending on the last two patches of that series. What's the plan there? In patchwork they're marked as "Not Applicable". If they're good to go, maybe we should land them? If not, maybe you should include them (with Dmitry as the author, of course) at the beginning of your series? No, please do not resend patches. The patches in question are marked as 'Not applicable' as they are really not applicable to Bjorn's tree. It would be better to point to the correct patchwork: https://patchwork.freedesktop.org/series/98585/ Note those patches still lack the R-B tag. I can include them anyway, basing on Sankeerth's Tested-by tag, but the formal R-B would also be good. Can you resend those as not RFC? Yes, please resend these, I can ack them. Previously I held off my ack, as kuogee ran into some issues testing them which was later concluded to be a mismatch in QC internal trees due to different versions of the changes.( another reason why we should get these landed ). Now, that Sankeerth has tested these, if you can remove RFC and post them, I can ack the. Well, you can ack those patches without them being resent. You have already added your Reviewed-by to first three patches (which were merged during last window). I thought you might have to rebase them :) that way you could have resent the rebased patch with the RFC tag removed. If you dont, you now have my R-b. Thanks Abhinav
Re: [Freedreno] [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On 3/16/2022 9:45 AM, Sankeerth Billakanti (QUIC) wrote:
Subject: Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with
drm_bridge_connector
Date: Wed, 23 Feb 2022 16:40:56 -0800
From: Kuogee Hsieh
To: Stephen Boyd , Dmitry Baryshkov
CC: Abhinav Kumar , Bjorn Andersson
, Rob Clark , Sean Paul
, David Airlie , Daniel Vetter
, linux-arm-...@vger.kernel.org, dri-
de...@lists.freedesktop.org, freedreno@lists.freedesktop.org
On 2/23/2022 1:33 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-02-23 10:27:26)
On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote:
On 23/02/2022 20:21, Kuogee Hsieh wrote:
In the panel device node.
Can you please share it too?
{
edp_power_supply: edp_power {
compatible = "regulator-fixed";
regulator-name = "edp_backlight_power";
regulator-always-on;
regulator-boot-on;
};
edp_backlight: edp_backlight {
compatible = "pwm-backlight";
pwms = <_pwm 3 65535>;
power-supply = <_power_supply>;
enable-gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
pinctrl-0 = <_pwm_default>;
};
edp_panel: edp_panel {
compatible = "sharp_lq140m1jw46";
Is that supposed to be sharp,lq140m1jw46 with a comma instead of an
underscore?
Stephen,
This is our internal branch which does not have patches up to date yet.
I will cherry-pick newer edp related patches which are under review now to
re test it.
Tested-by: Sankeerth Billakanti
Reviewed-by: Abhinav Kumar
Detect returned eDP not connected because the panel power was not on and
mode_valid was failing because the DP mode_valid is different from eDP
Re: [Freedreno] [RFC PATCH v2 5/5] drm/msm/dp: remove extra wrappers and public functions
On 3/16/2022 9:31 AM, Sankeerth Billakanti (QUIC) wrote:
Subject: [RFC PATCH v2 5/5] drm/msm/dp: remove extra wrappers and
public functions
Date: Sat, 12 Feb 2022 01:40:06 +0300
From: Dmitry Baryshkov
To: Bjorn Andersson , Rob Clark
, Sean Paul , Abhinav Kumar
, Kuogee Hsieh
CC: Stephen Boyd , David Airlie ,
Daniel Vetter , linux-arm-...@vger.kernel.org, dri-
de...@lists.freedesktop.org, freedreno@lists.freedesktop.org
dp_bridge's functions are thin wrappers around the msm_dp_display_*
family. Squash dp_bridge callbacks into respective msm_dp_display
functions, removing the latter functions from public space.
Signed-off-by: Dmitry Baryshkov
Tested-by: Sankeerth Billakanti
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_display.c | 54 +++---
drivers/gpu/drm/msm/dp/dp_display.h | 1 -
drivers/gpu/drm/msm/dp/dp_drm.c | 72 ++---
drivers/gpu/drm/msm/dp/dp_drm.h | 22 -
drivers/gpu/drm/msm/msm_drv.h | 31 -
5 files changed, 60 insertions(+), 120 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 59e5e5b8e5b4..a9b641a68bff 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,7 +10,6 @@
#include
#include
#include
-#include
#include "msm_drv.h"
#include "msm_kms.h"
@@ -945,18 +944,36 @@ int dp_display_set_plugged_cb(struct msm_dp
*dp_display,
return 0;
}
-int dp_display_validate_mode(struct msm_dp *dp, u32 mode_pclk_khz)
+/**
+ * dp_bridge_mode_valid - callback to determine if specified mode is
+valid
+ * @bridge: Pointer to drm bridge structure
+ * @info: display info
+ * @mode: Pointer to drm mode structure
+ * Returns: Validity status for specified mode */ enum drm_mode_status
+dp_bridge_mode_valid(struct drm_bridge *bridge,
+ const struct drm_display_info *info,
+ const struct drm_display_mode
*mode)
{
const u32 num_components = 3, default_bpp = 24;
struct dp_display_private *dp_display;
struct dp_link_info *link_info;
u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
+ struct msm_dp *dp;
+ int mode_pclk_khz = mode->clock;
+
+ dp = to_dp_bridge(bridge)->dp_display;
if (!dp || !mode_pclk_khz || !dp->connector) {
DRM_ERROR("invalid params\n");
return -EINVAL;
}
+ if ((dp->max_pclk_khz <= 0) ||
+ (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
+ (mode->clock > dp->max_pclk_khz))
+ return MODE_BAD;
+
dp_display = container_of(dp, struct dp_display_private,
dp_display);
link_info = _display->panel->link_info;
@@ -1501,7 +1518,7 @@ int msm_dp_modeset_init(struct msm_dp
*dp_display, struct drm_device *dev,
dp_display->encoder = encoder;
- dp_display->bridge = msm_dp_bridge_init(dp_display, dev,
encoder);
+ dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
if (IS_ERR(dp_display->bridge)) {
ret = PTR_ERR(dp_display->bridge);
DRM_DEV_ERROR(dev->dev,
@@ -1528,8 +1545,10 @@ int msm_dp_modeset_init(struct msm_dp
*dp_display, struct drm_device *dev,
return 0;
}
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder
*encoder)
+void dp_bridge_enable(struct drm_bridge *drm_bridge)
{
+ struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
+ struct msm_dp *dp = dp_bridge->dp_display;
int rc = 0;
struct dp_display_private *dp_display;
u32 state;
@@ -1537,7 +1556,7 @@ int msm_dp_display_enable(struct msm_dp *dp,
struct drm_encoder *encoder)
dp_display = container_of(dp, struct dp_display_private,
dp_display);
if (!dp_display->dp_mode.drm_mode.clock) {
DRM_ERROR("invalid params\n");
- return -EINVAL;
+ return;
}
mutex_lock(_display->event_mutex);
@@ -1549,14 +1568,14 @@ int msm_dp_display_enable(struct msm_dp *dp,
struct drm_encoder *encoder)
if (rc) {
DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
mutex_unlock(_display->event_mutex);
- return rc;
+ return;
}
rc = dp_display_prepare(dp);
if (rc) {
DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
mutex_unlock(_display->event_mutex);
- return rc;
+ return;
}
state = dp_display->hpd_state;
@@ -1581,23 +1600,23 @@ int msm_dp_display_enable(struct msm_dp *dp,
struct drm_encoder *encoder)
dp_display->hpd_state = ST_CONNECTED;
mutex_unlock(_display->event_mutex);
-
- return rc;
}
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder
*encoder)
+void dp_bridge_disable(struct drm_bridge *drm_bridge)
{
+
Re: [Freedreno] [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus
Hi, On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2022-04-14 12:16:14) > > > > I think it's too verbose and a bit incorrect. Not sure which part you're asserting is incorrect, but shorter is OK w/ me too. > > This is a bit saner: > > /* > > * These ops do not make sense for eDP, since they are provided > > * by the panel-bridge corresponding to the attached eDP panel. > > */ > > > > My question was whether we really need to disable them for eDP since for > > eDP the detect and and get_modes will be overridden anyway. Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work? It's definitely worth confirming but from my reading of the code it _probably_ wouldn't hurt. One thing someone would want to confirm would be what would happen if we move this code and the panel code to implement DRM_BRIDGE_OP_EDID properly. It looks as if both actually ought to be implementing that instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a future day. Could we get into trouble if one moved before the other? Then the panel would no longer override the eDP controller and the eDP controller would try to read from a possibly unpowered panel? So I guess in the end my preference would be that we know that driving the EDID read from the controller isn't a great idea for eDP (since we have no way to ensure that the panel is powered) so why risk it and set the bit saying we can do it? For hotplug/detect I'm even less confident that setting the bits would be harmless. I haven't sat down and traced everything, but from what I can see the panel _doesn't_ set these bits, does it? I believe that the rule is that when every bridge in the chain _doesn't_ implement detect/hotplug that the panel is always present. The moment someone says "hey, I can detect" then it suddenly becomes _not_ always present. Yes, I guess we could have the panel implement "detect" and return true, but I'm not convinced that's actually better... > And to go further, I'd expect that a bridge should expose the > functionality that it supports, regardless of what is connected down the > chain. Otherwise we won't be able to mix and match bridges because the > code is brittle, making assumptions about what is connected. >From my point of view the bridge simply doesn't support any of the three things when we're in eDP mode. Yes, it's the same driver. Yes, eDP and DP share nearly the same signalling on the wire. Yes, it's easily possible to make a single controller that supports DP and eDP. ...but the rules around detection and power sequencing are simply different for the two cases. The controller simply _cannot_ detect whether the panel is connected in the eDP case and it _must_ assume that the panel is always connected. Yes, it has an HPD pin. No, that HPD pin doesn't tell when the panel is present. The panel is always present. The panel is always present. So, IMO, it is _incorrect_ to say we can support HPD and DETECT if we know we're in eDP mode. -Doug
Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus
On 14/04/2022 23:00, Abhinav Kumar wrote: Hi Dmitry On 4/14/2022 12:43 PM, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-04-14 12:20:31) On 14/04/2022 19:40, Doug Anderson wrote: Hi, On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti wrote: This series adds support for generic eDP panel over aux_bus. These changes are dependent on the following series: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654=* You're basically depending on the last two patches of that series. What's the plan there? In patchwork they're marked as "Not Applicable". If they're good to go, maybe we should land them? If not, maybe you should include them (with Dmitry as the author, of course) at the beginning of your series? No, please do not resend patches. The patches in question are marked as 'Not applicable' as they are really not applicable to Bjorn's tree. It would be better to point to the correct patchwork: https://patchwork.freedesktop.org/series/98585/ Note those patches still lack the R-B tag. I can include them anyway, basing on Sankeerth's Tested-by tag, but the formal R-B would also be good. Can you resend those as not RFC? Yes, please resend these, I can ack them. Previously I held off my ack, as kuogee ran into some issues testing them which was later concluded to be a mismatch in QC internal trees due to different versions of the changes.( another reason why we should get these landed ). Now, that Sankeerth has tested these, if you can remove RFC and post them, I can ack the. Well, you can ack those patches without them being resent. You have already added your Reviewed-by to first three patches (which were merged during last window). -- With best wishes Dmitry
Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus
Hi Dmitry On 4/14/2022 12:43 PM, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-04-14 12:20:31) On 14/04/2022 19:40, Doug Anderson wrote: Hi, On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti wrote: This series adds support for generic eDP panel over aux_bus. These changes are dependent on the following series: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654=* You're basically depending on the last two patches of that series. What's the plan there? In patchwork they're marked as "Not Applicable". If they're good to go, maybe we should land them? If not, maybe you should include them (with Dmitry as the author, of course) at the beginning of your series? No, please do not resend patches. The patches in question are marked as 'Not applicable' as they are really not applicable to Bjorn's tree. It would be better to point to the correct patchwork: https://patchwork.freedesktop.org/series/98585/ Note those patches still lack the R-B tag. I can include them anyway, basing on Sankeerth's Tested-by tag, but the formal R-B would also be good. Can you resend those as not RFC? Yes, please resend these, I can ack them. Previously I held off my ack, as kuogee ran into some issues testing them which was later concluded to be a mismatch in QC internal trees due to different versions of the changes.( another reason why we should get these landed ). Now, that Sankeerth has tested these, if you can remove RFC and post them, I can ack the. Thanks Abhinav
Re: [Freedreno] [PATCH v3] drm/msm/dp: stop event kernel thread when DP unbind
On 14/04/2022 20:25, Kuogee Hsieh wrote:
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().
Changes in v2:
-- start event thread at dp_display_bind()
Changes in v3:
-- disable all HDP interrupts at unbind
-- replace dp_hpd_event_setup() with dp_hpd_event_thread_start()
-- replace dp_hpd_event_stop() with dp_hpd_event_thread_stop()
-- move init_waitqueue_head(>event_q) to probe()
-- move spin_lock_init(>event_lock) to probe()
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
Reported-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 42 -
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..0b9a96f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
u32 hpd_state;
u32 event_pndx;
u32 event_gndx;
+ struct task_struct *ev_tsk;
struct dp_event event_list[DP_EVENT_Q_MAX];
spinlock_t event_lock;
@@ -230,6 +231,29 @@ void dp_display_signal_audio_complete(struct msm_dp *dp_display)
complete_all(>audio_comp);
}
+static int hpd_event_thread(void *data);
+
+static int dp_hpd_event_thread_start(struct dp_display_private *dp_priv)
+{
+ int err = 0;
+
+ dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv,
"dp_hpd_handler");
+ if (IS_ERR(dp_priv->ev_tsk)) {
+ DRM_ERROR("failed to create DP event thread\n");
+ err = PTR_ERR(dp_priv->ev_tsk);
+ }
+ return err;
Generally the preference is for the following style:
if (error) {
return error;
}
return 0;
It makes it clear that at the end the function succeeds.
+}
+
+static void dp_hpd_event_thread_stop(struct dp_display_private *dp_priv)
+{
+ kthread_stop(dp_priv->ev_tsk);
+
+ /* reset event q to empty */
+ dp_priv->event_gndx = 0;
+ dp_priv->event_pndx = 0;
+}
+
static int dp_display_bind(struct device *dev, struct device *master,
void *data)
{
@@ -269,6 +293,7 @@ static int dp_display_bind(struct device *dev, struct
device *master,
if (rc)
DRM_ERROR("Audio registration Dp failed\n");
Isn't 'goto end' missing here?
That's why it's suggested not to mix error and success paths.
+ rc = dp_hpd_event_thread_start(dp);
end:
return rc;
}
@@ -280,6 +305,9 @@ static void dp_display_unbind(struct device *dev, struct
device *master,
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
+ /* disable all HPD interrupts */
+ dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
Generic nit: if we had _enable and _disable functions, one wouldn't need
the comment here. It would be obvious from the code itself.
+ dp_hpd_event_thread_stop(dp);
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp[dp->id] = NULL;
@@ -1054,7 +1082,7 @@ static int hpd_event_thread(void *data)
dp_priv = (struct dp_display_private *)data;
- while (1) {
+ while (!kthread_should_stop()) {
if (timeout_mode) {
wait_event_timeout(dp_priv->event_q,
(dp_priv->event_pndx == dp_priv->event_gndx),
@@ -1132,13 +1160,6 @@ static int hpd_event_thread(void *data)
return 0;
}
-static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
-{
- init_waitqueue_head(_priv->event_q);
- spin_lock_init(_priv->event_lock);
-
- kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
-}
static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
{
@@ -1266,7 +1287,10 @@ static int dp_display_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
}
+ /* setup event q */
mutex_init(>event_mutex);
+ init_waitqueue_head(>event_q);
+ spin_lock_init(>event_lock);
/* Store DP audio handle inside DP display */
dp->dp_display.dp_audio = dp->audio;
@@ -1441,8 +1465,6 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp = container_of(dp_display, struct dp_display_private, dp_display);
- dp_hpd_event_setup(dp);
-
dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
}
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v3] drm/msm/dp: stop event kernel thread when DP unbind
Quoting Kuogee Hsieh (2022-04-14 10:25:37)
> Current DP driver implementation, event thread is kept running
> after DP display is unbind. This patch fix this problem by disabling
> DP irq and stop event thread to exit gracefully at dp_display_unbind().
>
> Changes in v2:
> -- start event thread at dp_display_bind()
>
> Changes in v3:
> -- disable all HDP interrupts at unbind
> -- replace dp_hpd_event_setup() with dp_hpd_event_thread_start()
> -- replace dp_hpd_event_stop() with dp_hpd_event_thread_stop()
> -- move init_waitqueue_head(>event_q) to probe()
> -- move spin_lock_init(>event_lock) to probe()
>
> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
> Chipsets")
> Signed-off-by: Kuogee Hsieh
> Reported-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 42
> -
> 1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 01453db..0b9a96f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -230,6 +231,29 @@ void dp_display_signal_audio_complete(struct msm_dp
> *dp_display)
> complete_all(>audio_comp);
> }
>
> +static int hpd_event_thread(void *data);
> +
> +static int dp_hpd_event_thread_start(struct dp_display_private *dp_priv)
> +{
> + int err = 0;
Drop local.
> +
> + dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv,
> "dp_hpd_handler");
> + if (IS_ERR(dp_priv->ev_tsk)) {
> + DRM_ERROR("failed to create DP event thread\n");
> + err = PTR_ERR(dp_priv->ev_tsk);
return PTR_ERR(dp_priv->ev_tsk);
> + }
Newline
> + return err;
return 0;
> +}
> +
> +static void dp_hpd_event_thread_stop(struct dp_display_private *dp_priv)
> +{
> + kthread_stop(dp_priv->ev_tsk);
> +
> + /* reset event q to empty */
> + dp_priv->event_gndx = 0;
> + dp_priv->event_pndx = 0;
> +}
> +
> static int dp_display_bind(struct device *dev, struct device *master,
>void *data)
> {
> @@ -269,6 +293,7 @@ static int dp_display_bind(struct device *dev, struct
> device *master,
> if (rc)
> DRM_ERROR("Audio registration Dp failed\n");
>
> + rc = dp_hpd_event_thread_start(dp);
Forward declare this new function?
> end:
> return rc;
> }
> @@ -280,6 +305,9 @@ static void dp_display_unbind(struct device *dev, struct
> device *master,
> struct drm_device *drm = dev_get_drvdata(master);
> struct msm_drm_private *priv = drm->dev_private;
>
> + /* disable all HPD interrupts */
> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
> + dp_hpd_event_thread_stop(dp);
> dp_power_client_deinit(dp->power);
> dp_aux_unregister(dp->aux);
> priv->dp[dp->id] = NULL;
> @@ -1054,7 +1082,7 @@ static int hpd_event_thread(void *data)
>
> dp_priv = (struct dp_display_private *)data;
>
> - while (1) {
> + while (!kthread_should_stop()) {
> if (timeout_mode) {
> wait_event_timeout(dp_priv->event_q,
> (dp_priv->event_pndx == dp_priv->event_gndx),
> @@ -1132,13 +1160,6 @@ static int hpd_event_thread(void *data)
> return 0;
> }
>
> -static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
> -{
> - init_waitqueue_head(_priv->event_q);
> - spin_lock_init(_priv->event_lock);
> -
> - kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
> -}
And then dp_hpd_event_thread_start() be defined here? The bind/unbind
functions should be moved to the bottom of this file because they're
probe/remove basically. Please do that in a followup patch.
>
> static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
> {
Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus
Quoting Dmitry Baryshkov (2022-04-14 12:20:31) > On 14/04/2022 19:40, Doug Anderson wrote: > > Hi, > > > > On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti > > wrote: > >> > >> This series adds support for generic eDP panel over aux_bus. > >> > >> These changes are dependent on the following series: > >> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654=* > > > > You're basically depending on the last two patches of that series. > > What's the plan there? In patchwork they're marked as "Not > > Applicable". If they're good to go, maybe we should land them? If not, > > maybe you should include them (with Dmitry as the author, of course) > > at the beginning of your series? > > No, please do not resend patches. The patches in question are marked as > 'Not applicable' as they are really not applicable to Bjorn's tree. > It would be better to point to the correct patchwork: > > https://patchwork.freedesktop.org/series/98585/ > > Note those patches still lack the R-B tag. I can include them anyway, > basing on Sankeerth's Tested-by tag, but the formal R-B would also be good. > Can you resend those as not RFC?
Re: [Freedreno] [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus
Quoting Dmitry Baryshkov (2022-04-14 12:16:14) > > I think it's too verbose and a bit incorrect. > This is a bit saner: > /* > * These ops do not make sense for eDP, since they are provided > * by the panel-bridge corresponding to the attached eDP panel. > */ > > My question was whether we really need to disable them for eDP since for > eDP the detect and and get_modes will be overridden anyway. And to go further, I'd expect that a bridge should expose the functionality that it supports, regardless of what is connected down the chain. Otherwise we won't be able to mix and match bridges because the code is brittle, making assumptions about what is connected.
Re: [Freedreno] [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
Quoting Kuogee Hsieh (2022-04-14 09:34:55) > > On 4/13/2022 5:02 PM, Stephen Boyd wrote: > > The subject is still misleading. It is fixing something. It may be > > enhancing it as well but it is clearly fixing it first. > > [...] > > I'd prefer this part to be a different patch. It can come after the fix > > to ease backporting. > > > > Also, is there any response to Dmitry's question yet? I haven't seen > > anything. > > Sorry, since our internal review does not like this approach. The internal review shouldn't prevent you from responding to code review on the mailing list.
Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus
On 14/04/2022 19:40, Doug Anderson wrote: Hi, On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti wrote: This series adds support for generic eDP panel over aux_bus. These changes are dependent on the following series: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654=* You're basically depending on the last two patches of that series. What's the plan there? In patchwork they're marked as "Not Applicable". If they're good to go, maybe we should land them? If not, maybe you should include them (with Dmitry as the author, of course) at the beginning of your series? No, please do not resend patches. The patches in question are marked as 'Not applicable' as they are really not applicable to Bjorn's tree. It would be better to point to the correct patchwork: https://patchwork.freedesktop.org/series/98585/ Note those patches still lack the R-B tag. I can include them anyway, basing on Sankeerth's Tested-by tag, but the formal R-B would also be good. Sankeerth Billakanti (4): drm/msm/dp: Add eDP support via aux_bus drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP drm/msm/dp: wait for hpd high before aux transaction Support the eDP modes given by panel One of these things is not like the others. One of these things just doesn't belong. Can you spot which patch is missing the prefix by looking at the subject line of all 4 patches? ;-) :-) -- With best wishes Dmitry
Re: [Freedreno] [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus
On 14/04/2022 19:39, Doug Anderson wrote:
Hi,
On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
wrote:
@@ -1530,6 +1532,60 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display,
struct drm_minor *minor)
}
}
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+ int rc;
+ struct dp_display_private *dp_priv;
+ struct device_node *aux_bus;
+ struct device *dev;
+
+ dp_priv = container_of(dp, struct dp_display_private, dp_display);
+ dev = _priv->pdev->dev;
+ aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+ if (aux_bus && dp->is_edp) {
+ dp_display_host_init(dp_priv);
+ dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+ dp_display_host_phy_init(dp_priv);
+ enable_irq(dp_priv->irq);
+
+ rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+ of_node_put(aux_bus);
+ if (rc) {
+ disable_irq(dp_priv->irq);
+ dp_display_host_phy_exit(dp_priv);
+ dp_display_host_deinit(dp_priv);
+ return rc;
+ }
+ } else if (dp->is_edp) {
+ DRM_ERROR("eDP aux_bus not found\n");
+ return -ENODEV;
+ }
+
+ /*
+* External bridges are mandatory for eDP interfaces: one has to
+* provide at least an eDP panel (which gets wrapped into panel-bridge).
+*
+* For DisplayPort interfaces external bridges are optional, so
+* silently ignore an error if one is not present (-ENODEV).
+*/
+ rc = dp_parser_find_next_bridge(dp_priv->parser);
This gets into the same problem that Dmitry pointed out that ps8640
has that's addressed by my recent series [1]. Namely it's not
guaranteed that the panel will have finished probing by the time
devm_of_dp_aux_populate_ep_devices() finishes probing. I don't think
it's going to be really solvable without the bigger rewrite that we've
been discussing, though. ...it's probably OK to land something like
what you have here, but it might at least deserve a comment in the
code?
[1] https://lore.kernel.org/r/20220409023628.2104952-1-diand...@chromium.org
We agreed that rework would follow up in a timely manner if these
patches are applied. However a comment would be still a good thing.
+ if (rc == -ENODEV) {
+ if (dp->is_edp) {
+ DRM_ERROR("eDP: next bridge is not present\n");
+ return rc;
+ }
+ } else if (rc) {
+ if (rc != -EPROBE_DEFER)
+ DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
+ return rc;
In both of your two error returns here isn't it a problem that you don't do:
disable_irq(dp_priv->irq);
dp_display_host_phy_exit(dp_priv);
dp_display_host_deinit(dp_priv);
Should probably at least fix that clear error before landing, unless
I'm misunderstanding and there's some reason not to do that?
As discussed previously, I'm not convinced that we've covered every
corner case for properly doing and undoing the above things. I'm
hoping that once we do the cleanup and move to pm_runtime() management
that it will be cleaned up?
@@ -114,10 +114,12 @@ struct drm_bridge *dp_bridge_init(struct msm_dp
*dp_display, struct drm_device *
bridge->funcs = _bridge_ops;
bridge->type = dp_display->connector_type;
- bridge->ops =
- DRM_BRIDGE_OP_DETECT |
- DRM_BRIDGE_OP_HPD |
- DRM_BRIDGE_OP_MODES;
+ if (!dp_display->is_edp) {
+ bridge->ops =
+ DRM_BRIDGE_OP_DETECT |
+ DRM_BRIDGE_OP_HPD |
+ DRM_BRIDGE_OP_MODES;
Given that Dmitry had questions about why eDP has different ops in his
previous review of this code, the above probably deserves an inline
code comment. If you want to use my wording, you could paste this into
your code:
/*
* Many ops only make sense for DP. Why?
* - Detect/HPD are used by DRM to know if a display is _physically_
* there, not whether the display is powered on / finished initting.
* On eDP we assume the display is always there because you can't
* know until power is applied. If we don't implement the ops DRM will
* assume our display is always there.
* - Currently eDP mode reading is driven by the panel driver. This
* allows the panel driver to properly power itself on to read the
* modes.
*/
I think it's too verbose and a bit incorrect.
This is a bit saner:
/*
* These ops do not make sense for eDP, since they are provided
* by the panel-bridge corresponding to the attached eDP panel.
*/
My question was whether we really need to disable them for eDP since for
eDP the detect and and get_modes will be overridden anyway.
Overall: as discussed, I think
[Freedreno] [PATCH v2 2/3] drm/msm/a6xx: Add speedbin support for A619 GPU
There are various SKUs of A619, ranging from 565 MHz to 850 MHz, depending
on the bin. Add support for distinguishing them, so that proper frequency
ranges can be applied, depending on the HW.
Signed-off-by: Konrad Dybcio
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 +-
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 19 +++
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index e8d4cca6cd46..2cd632fdd890 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1540,7 +1540,7 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
SZ_16M - SZ_16K, 0x04000, "icache");
if (ret)
goto err_memory;
- } else {
+ } else if (adreno_is_a640_family(adreno_gpu)) {
ret = a6xx_gmu_memory_alloc(gmu, >icache,
SZ_256K - SZ_16K, 0x04000, "icache");
if (ret)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index ddeb04a77662..57d07ae86b2a 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1836,6 +1836,22 @@ static u32 a618_get_speed_bin(u32 fuse)
return UINT_MAX;
}
+static u32 a619_get_speed_bin(u32 fuse)
+{
+ if (fuse == 0)
+ return 0;
+ else if (fuse == 120)
+ return 4;
+ else if (fuse == 138)
+ return 3;
+ else if (fuse == 169)
+ return 2;
+ else if (fuse == 180)
+ return 1;
+
+ return UINT_MAX;
+}
+
static u32 adreno_7c3_get_speed_bin(u32 fuse)
{
if (fuse == 0)
@@ -1855,6 +1871,9 @@ static u32 fuse_to_supp_hw(struct device *dev, struct
adreno_rev rev, u32 fuse)
if (adreno_cmp_rev(ADRENO_REV(6, 1, 8, ANY_ID), rev))
val = a618_get_speed_bin(fuse);
+ if (adreno_cmp_rev(ADRENO_REV(6, 1, 9, ANY_ID), rev))
+ val = a619_get_speed_bin(fuse);
+
if (adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), rev))
val = adreno_7c3_get_speed_bin(fuse);
--
2.35.2
[Freedreno] [PATCH v2 1/3] drm/msm/adreno: Add A619 support
Add support for the Adreno 619 GPU, as found in Snapdragon 690 (SM6350),
480 (SM4350) and 750G (SM7225).
Signed-off-by: Konrad Dybcio
---
Changes in v2:
- Don't reserve icache/dcache regions on legacy GMUs, as that
is apparently not necessary and simply a downstream leftover.
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 11 ++--
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 70 +-
drivers/gpu/drm/msm/adreno/a6xx_hfi.c | 66 +++-
drivers/gpu/drm/msm/adreno/adreno_device.c | 14 +
drivers/gpu/drm/msm/adreno/adreno_gpu.h| 13 +++-
5 files changed, 166 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 3e325e2a2b1b..e8d4cca6cd46 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -527,6 +527,8 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
pdc_in_aop = true;
else if (adreno_is_a618(adreno_gpu) ||
adreno_is_a640_family(adreno_gpu))
pdc_address_offset = 0x30090;
+ else if (adreno_is_a619(adreno_gpu))
+ pdc_address_offset = 0x300a0;
else
pdc_address_offset = 0x30080;
@@ -601,7 +603,8 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_MSGID + 4, 0x10108);
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_ADDR + 4, 0x3);
- if (adreno_is_a618(adreno_gpu) || adreno_is_a650_family(adreno_gpu))
+ if (adreno_is_a618(adreno_gpu) || adreno_is_a619(adreno_gpu) ||
+ adreno_is_a650_family(adreno_gpu))
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 4, 0x2);
else
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 4, 0x3);
@@ -1537,7 +1540,7 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
SZ_16M - SZ_16K, 0x04000, "icache");
if (ret)
goto err_memory;
- } else if (adreno_is_a640_family(adreno_gpu)) {
+ } else {
ret = a6xx_gmu_memory_alloc(gmu, >icache,
SZ_256K - SZ_16K, 0x04000, "icache");
if (ret)
@@ -1547,9 +1550,9 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
SZ_256K - SZ_16K, 0x44000, "dcache");
if (ret)
goto err_memory;
- } else {
- BUG_ON(adreno_is_a660_family(adreno_gpu));
+ }
+ if (adreno_is_a630(adreno_gpu) || adreno_is_a615_family(adreno_gpu)) {
/* HFI v1, has sptprac */
gmu->legacy = true;
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 83c31b2ad865..ddeb04a77662 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -252,6 +252,74 @@ static void a6xx_submit(struct msm_gpu *gpu, struct
msm_gem_submit *submit)
a6xx_flush(gpu, ring);
}
+/* For a615 family (a615, a616, a618 and a619) */
+const struct adreno_reglist a615_hwcg[] = {
+ {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x0222},
+ {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x0220},
+ {REG_A6XX_RBBM_CLOCK_DELAY_SP0, 0x0080},
+ {REG_A6XX_RBBM_CLOCK_HYST_SP0, 0xF3CF},
+ {REG_A6XX_RBBM_CLOCK_CNTL_TP0, 0x0222},
+ {REG_A6XX_RBBM_CLOCK_CNTL_TP1, 0x0222},
+ {REG_A6XX_RBBM_CLOCK_CNTL2_TP0, 0x},
+ {REG_A6XX_RBBM_CLOCK_CNTL2_TP1, 0x},
+ {REG_A6XX_RBBM_CLOCK_CNTL3_TP0, 0x},
+ {REG_A6XX_RBBM_CLOCK_CNTL3_TP1, 0x},
+ {REG_A6XX_RBBM_CLOCK_CNTL4_TP0, 0x0002},
+ {REG_A6XX_RBBM_CLOCK_CNTL4_TP1, 0x0002},
+ {REG_A6XX_RBBM_CLOCK_HYST_TP0, 0x},
+ {REG_A6XX_RBBM_CLOCK_HYST_TP1, 0x},
+ {REG_A6XX_RBBM_CLOCK_HYST2_TP0, 0x},
+ {REG_A6XX_RBBM_CLOCK_HYST2_TP1, 0x},
+ {REG_A6XX_RBBM_CLOCK_HYST3_TP0, 0x},
+ {REG_A6XX_RBBM_CLOCK_HYST3_TP1, 0x},
+ {REG_A6XX_RBBM_CLOCK_HYST4_TP0, 0x0007},
+ {REG_A6XX_RBBM_CLOCK_HYST4_TP1, 0x0007},
+ {REG_A6XX_RBBM_CLOCK_DELAY_TP0, 0x},
+ {REG_A6XX_RBBM_CLOCK_DELAY_TP1, 0x},
+ {REG_A6XX_RBBM_CLOCK_DELAY2_TP0, 0x},
+ {REG_A6XX_RBBM_CLOCK_DELAY2_TP1, 0x},
+ {REG_A6XX_RBBM_CLOCK_DELAY3_TP0, 0x},
+ {REG_A6XX_RBBM_CLOCK_DELAY3_TP1, 0x},
+ {REG_A6XX_RBBM_CLOCK_DELAY4_TP0, 0x0001},
+ {REG_A6XX_RBBM_CLOCK_DELAY4_TP1, 0x0001},
+ {REG_A6XX_RBBM_CLOCK_CNTL_UCHE, 0x},
+ {REG_A6XX_RBBM_CLOCK_CNTL2_UCHE, 0x},
+ {REG_A6XX_RBBM_CLOCK_CNTL3_UCHE, 0x},
+ {REG_A6XX_RBBM_CLOCK_CNTL4_UCHE, 0x0022},
+ {REG_A6XX_RBBM_CLOCK_HYST_UCHE, 0x0004},
+
[Freedreno] [PATCH v2 3/3] drm/msm/adreno: Fix up formatting
Leading spaces are not something checkpatch likes, and it says so when
they are present. Use tabs consistently to indent function body and
unwrap a 83-char-long line, as 100 is cool nowadays.
Signed-off-by: Konrad Dybcio
---
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index a13a3e5a294b..f73f7b5dfd10 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -199,7 +199,7 @@ static inline int adreno_is_a420(struct adreno_gpu *gpu)
static inline int adreno_is_a430(struct adreno_gpu *gpu)
{
- return gpu->revn == 430;
+ return gpu->revn == 430;
}
static inline int adreno_is_a506(struct adreno_gpu *gpu)
@@ -239,7 +239,7 @@ static inline int adreno_is_a540(struct adreno_gpu *gpu)
static inline int adreno_is_a618(struct adreno_gpu *gpu)
{
- return gpu->revn == 618;
+ return gpu->revn == 618;
}
static inline int adreno_is_a619(struct adreno_gpu *gpu)
@@ -249,7 +249,7 @@ static inline int adreno_is_a619(struct adreno_gpu *gpu)
static inline int adreno_is_a630(struct adreno_gpu *gpu)
{
- return gpu->revn == 630;
+ return gpu->revn == 630;
}
static inline int adreno_is_a640_family(struct adreno_gpu *gpu)
@@ -259,18 +259,18 @@ static inline int adreno_is_a640_family(struct adreno_gpu
*gpu)
static inline int adreno_is_a650(struct adreno_gpu *gpu)
{
- return gpu->revn == 650;
+ return gpu->revn == 650;
}
static inline int adreno_is_7c3(struct adreno_gpu *gpu)
{
/* The order of args is important here to handle ANY_ID correctly */
- return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
+ return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
}
static inline int adreno_is_a660(struct adreno_gpu *gpu)
{
- return gpu->revn == 660;
+ return gpu->revn == 660;
}
/* check for a615, a616, a618, a619 or any derivatives */
@@ -281,14 +281,13 @@ static inline int adreno_is_a615_family(struct adreno_gpu
*gpu)
static inline int adreno_is_a660_family(struct adreno_gpu *gpu)
{
- return adreno_is_a660(gpu) || adreno_is_7c3(gpu);
+ return adreno_is_a660(gpu) || adreno_is_7c3(gpu);
}
/* check for a650, a660, or any derivatives */
static inline int adreno_is_a650_family(struct adreno_gpu *gpu)
{
- return gpu->revn == 650 || gpu->revn == 620 ||
- adreno_is_a660_family(gpu);
+ return gpu->revn == 650 || gpu->revn == 620 ||
adreno_is_a660_family(gpu);
}
int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
--
2.35.2
[Freedreno] [PATCH v3] drm/msm/dp: stop event kernel thread when DP unbind
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().
Changes in v2:
-- start event thread at dp_display_bind()
Changes in v3:
-- disable all HDP interrupts at unbind
-- replace dp_hpd_event_setup() with dp_hpd_event_thread_start()
-- replace dp_hpd_event_stop() with dp_hpd_event_thread_stop()
-- move init_waitqueue_head(>event_q) to probe()
-- move spin_lock_init(>event_lock) to probe()
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
Reported-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 42 -
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..0b9a96f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
u32 hpd_state;
u32 event_pndx;
u32 event_gndx;
+ struct task_struct *ev_tsk;
struct dp_event event_list[DP_EVENT_Q_MAX];
spinlock_t event_lock;
@@ -230,6 +231,29 @@ void dp_display_signal_audio_complete(struct msm_dp
*dp_display)
complete_all(>audio_comp);
}
+static int hpd_event_thread(void *data);
+
+static int dp_hpd_event_thread_start(struct dp_display_private *dp_priv)
+{
+ int err = 0;
+
+ dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv,
"dp_hpd_handler");
+ if (IS_ERR(dp_priv->ev_tsk)) {
+ DRM_ERROR("failed to create DP event thread\n");
+ err = PTR_ERR(dp_priv->ev_tsk);
+ }
+ return err;
+}
+
+static void dp_hpd_event_thread_stop(struct dp_display_private *dp_priv)
+{
+ kthread_stop(dp_priv->ev_tsk);
+
+ /* reset event q to empty */
+ dp_priv->event_gndx = 0;
+ dp_priv->event_pndx = 0;
+}
+
static int dp_display_bind(struct device *dev, struct device *master,
void *data)
{
@@ -269,6 +293,7 @@ static int dp_display_bind(struct device *dev, struct
device *master,
if (rc)
DRM_ERROR("Audio registration Dp failed\n");
+ rc = dp_hpd_event_thread_start(dp);
end:
return rc;
}
@@ -280,6 +305,9 @@ static void dp_display_unbind(struct device *dev, struct
device *master,
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
+ /* disable all HPD interrupts */
+ dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
+ dp_hpd_event_thread_stop(dp);
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp[dp->id] = NULL;
@@ -1054,7 +1082,7 @@ static int hpd_event_thread(void *data)
dp_priv = (struct dp_display_private *)data;
- while (1) {
+ while (!kthread_should_stop()) {
if (timeout_mode) {
wait_event_timeout(dp_priv->event_q,
(dp_priv->event_pndx == dp_priv->event_gndx),
@@ -1132,13 +1160,6 @@ static int hpd_event_thread(void *data)
return 0;
}
-static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
-{
- init_waitqueue_head(_priv->event_q);
- spin_lock_init(_priv->event_lock);
-
- kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
-}
static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
{
@@ -1266,7 +1287,10 @@ static int dp_display_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
}
+ /* setup event q */
mutex_init(>event_mutex);
+ init_waitqueue_head(>event_q);
+ spin_lock_init(>event_lock);
/* Store DP audio handle inside DP display */
dp->dp_display.dp_audio = dp->audio;
@@ -1441,8 +1465,6 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp = container_of(dp_display, struct dp_display_private, dp_display);
- dp_hpd_event_setup(dp);
-
dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [Freedreno] [PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind
On 4/13/2022 4:19 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-04-13 14:04:25)
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().
Changes in v2:
-- start event thread at dp_display_bind()
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
Reported-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 40 +++--
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..943e4f1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
u32 hpd_state;
u32 event_pndx;
u32 event_gndx;
+ struct task_struct *ev_tsk;
struct dp_event event_list[DP_EVENT_Q_MAX];
spinlock_t event_lock;
@@ -230,6 +231,31 @@ void dp_display_signal_audio_complete(struct msm_dp
*dp_display)
complete_all(>audio_comp);
}
+static int hpd_event_thread(void *data);
Is there a reason why this is needed vs. defining the kthread start
function after hpd_event_thread()?
too many code need to be relocated.
+
+static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
Maybe dp_hpd_event_thread_start()?
+{
+ init_waitqueue_head(_priv->event_q);
+ spin_lock_init(_priv->event_lock);
+
+ dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv,
"dp_hpd_handler");
+
+ if (IS_ERR(dp_priv->ev_tsk))
+ DRM_ERROR("failed to create DP event thread\n");
Can we return an error from this function?
+}
+
+static void dp_hpd_event_stop(struct dp_display_private *dp_priv)
Maybe dp_hpd_event_thread_stop()?
+{
+ if (IS_ERR(dp_priv->ev_tsk))
+ return;
If we handled the error then this check becomes impossible.
+
+ kthread_stop(dp_priv->ev_tsk);
+
+ /* reset event q to empty */
+ dp_priv->event_gndx = 0;
+ dp_priv->event_pndx = 0;
+}
+
static int dp_display_bind(struct device *dev, struct device *master,
void *data)
{
@@ -269,6 +295,7 @@ static int dp_display_bind(struct device *dev, struct
device *master,
if (rc)
DRM_ERROR("Audio registration Dp failed\n");
+ dp_hpd_event_setup(dp); /* start event thread */
The comment is useless, please remove.
end:
return rc;
}
@@ -280,6 +307,8 @@ static void dp_display_unbind(struct device *dev, struct
device *master,
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
+ disable_irq(dp->irq);
Is the disable_irq() necessary? It would be nicer to silence the
hardware and remove the disable_irq() so that we can reason about the
code assuming the irq is always enabled after it is requested.
+ dp_hpd_event_stop(dp); /* stop event thread */
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp[dp->id] = NULL;
Re: [Freedreno] [PATCH v7 4/4] Support the eDP modes given by panel
Hi, On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti wrote: > > The eDP controller does not have a reliable way keep panel > powered on to read the sink capabilities. So, the controller > driver cannot validate if a mode can be supported by the > source. We will rely on the panel driver to populate only > the supported modes for now. > > Signed-off-by: Sankeerth Billakanti > --- > drivers/gpu/drm/msm/dp/dp_display.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index c7277f0..0f18a16 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -998,6 +998,14 @@ enum drm_mode_status dp_bridge_mode_valid(struct > drm_bridge *bridge, > return -EINVAL; > } > > + /* > +* The eDP controller currently does not have a reliable way of > +* enabling panel power to read sink capabilities. So, we rely > +* on the panel driver to populate only supported modes for now. > +*/ > + if (dp->is_edp) > + return MODE_OK; As discussed out-of-band, I agree that this is the right thing for now and making this assumption won't break anything. In general the set of eDP panels is known ahead of time it's fairly unlikely someone would set things up so that a panel couldn't use the mode it was reporting. Longer term we should figure out a way to solve this but it doesn't have to be today. To properly implement mode_valid() we've got to combine knowledge from the panel (mostly rates supported and number of lanes supported) with the controller (rates supported, number of lanes supported/hooked up on this board). In any case: Reviewed-by: Douglas Anderson
Re: [Freedreno] [PATCH v7 3/4] drm/msm/dp: wait for hpd high before aux transaction
Hi,
On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
wrote:
>
> The source device should ensure the sink is ready before proceeding to
> read the sink capability or perform any aux transactions. The sink
> will indicate its readiness by asserting the HPD line. The controller
> driver needs to wait for the hpd line to be asserted by the sink before
> it performs any aux transactions.
>
> The eDP sink is assumed to be always connected. It needs power from the
> source and its HPD line will be asserted only after the panel is powered
> on. The panel power will be enabled from the panel-edp driver and only
> after that, the hpd line will be asserted.
>
> Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
> line gets asserted to indicate the sink is connected and ready. Hence
> there is no need to wait for the hpd line to be asserted for a DP sink.
>
> Signed-off-by: Sankeerth Billakanti
> ---
It might be worth mentioning "after the cut" that we may eventually
end up changing the rules if people like my proposal [1]. However,
what your code is doing here for eDP is correct as things are
currently intended to work and it would make sense to land it while we
debate about whether we want to add the is_hpd_asserted() callback
like my patch does.
[1]
https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69eebf803e22ebc1@changeid/
> Changes in v7:
> - add a comment to say why the wait si done for eDP
> - correct the commit text
>
> Changes in v6:
> - Wait for hpd high only for eDP
> - Split into smaller patches
>
> drivers/gpu/drm/msm/dp/dp_aux.c | 21 -
> drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++-
> drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +
> drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
> drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
> 5 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 6d36f63..cf0739f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -36,6 +36,7 @@ struct dp_aux_private {
> bool initted;
> u32 offset;
> u32 segment;
> + bool is_edp;
Kinda nitty, but can you put it next to the other booleans? This will
help with structure packing.
> struct drm_dp_aux dp_aux;
> };
> @@ -337,6 +338,22 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> goto exit;
> }
>
> + /*
> +* For eDP it's important to give a reasonably long wait here for HPD
> +* to be asserted. This is because the panel driver may have _just_
> +* turned on the panel and then tried to do an AUX transfer. The panel
> +* driver has no way of knowing when the panel is ready, so it's up
> +* to us to wait. For DP we never get into this situation so let's
> +* avoid ever doing the extra long wait for DP.
> +*/
> + if (aux->is_edp) {
> + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> + if (ret) {
> + DRM_DEBUG_DP("Panel not ready for aux
> transactions\n");
> + goto exit;
> + }
> + }
> +
> dp_aux_update_offset_and_segment(aux, msg);
> dp_aux_transfer_helper(aux, msg, true);
>
> @@ -491,7 +508,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
> drm_dp_aux_unregister(dp_aux);
> }
>
> -struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
> +struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
> + bool is_edp)
nit: I think indentation rules for this file are that the type of the
argument for the 2nd line should line up right under the 1st. Thus you
should delete one tab character and insert 6 spaces before the "bool".
Similar in other places, like your header file.
Stuff above is all nits and this looks right to me. I'm happy with:
Reviewed-by: Douglas Anderson
Re: [Freedreno] [PATCH v7 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
Hi,
On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and enable.
Technically the panel-edp powers on the panel in pre_enable()
> The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti
> ---
>
> Changes in v7:
> - reordered the patch in the series
> - modified the return statement for isr
> - connector check modified to just check for eDP
>
> drivers/gpu/drm/msm/dp/dp_catalog.c | 9 +++--
> drivers/gpu/drm/msm/dp/dp_display.c | 22 +-
> 2 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index fac815f..07f2389 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> *dp_catalog)
>
> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>
> - /* enable HPD plug and unplug interrupts */
> - dp_catalog_hpd_config_intr(dp_catalog,
> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
> -
> /* Configure REFTIMER and enable it */
> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> @@ -599,13 +595,14 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog
> *dp_catalog)
> {
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
> - int isr = 0;
> + int isr, mask;
>
> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> (isr & DP_DP_HPD_INT_MASK));
> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>
> - return isr;
> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
Please add a comment above this explaining what the goal of the above
statement is. I guess it's something like this, though you might want
to modify it to remove snark and insert the real reason unless you
like being snarky:
/*
* Report the raw status of all interrupts (AKA we still report the
* interrupt as asserted even if it's masked) _except_ for HPD-related.
* interrupts. We only report HPD-related interrupts if they're
* unmasked. We do it this way because we thought it would be extra
* confusing for readers of this code and we were bribed by Mordac to
* confuse you. OK, maybe that's not true. We actually do it this way
* because of .
*/
Along the same lines as my comments in patch #1, I don't have a great
feel for exactly when the various HPD bits are enabled / disabled and
it feels like it need to be made super obvious / well documented. That
being said, I'd be OK w/ that happening in the proposed cleanup.
-Doug
Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus
Hi, On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti wrote: > > This series adds support for generic eDP panel over aux_bus. > > These changes are dependent on the following series: > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654=* You're basically depending on the last two patches of that series. What's the plan there? In patchwork they're marked as "Not Applicable". If they're good to go, maybe we should land them? If not, maybe you should include them (with Dmitry as the author, of course) at the beginning of your series? > Sankeerth Billakanti (4): > drm/msm/dp: Add eDP support via aux_bus > drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP > drm/msm/dp: wait for hpd high before aux transaction > Support the eDP modes given by panel One of these things is not like the others. One of these things just doesn't belong. Can you spot which patch is missing the prefix by looking at the subject line of all 4 patches? ;-)
Re: [Freedreno] [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus
Hi,
On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
wrote:
>
> @@ -1530,6 +1532,60 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display,
> struct drm_minor *minor)
> }
> }
>
> +static int dp_display_get_next_bridge(struct msm_dp *dp)
> +{
> + int rc;
> + struct dp_display_private *dp_priv;
> + struct device_node *aux_bus;
> + struct device *dev;
> +
> + dp_priv = container_of(dp, struct dp_display_private, dp_display);
> + dev = _priv->pdev->dev;
> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +
> + if (aux_bus && dp->is_edp) {
> + dp_display_host_init(dp_priv);
> + dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> + dp_display_host_phy_init(dp_priv);
> + enable_irq(dp_priv->irq);
> +
> + rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> + of_node_put(aux_bus);
> + if (rc) {
> + disable_irq(dp_priv->irq);
> + dp_display_host_phy_exit(dp_priv);
> + dp_display_host_deinit(dp_priv);
> + return rc;
> + }
> + } else if (dp->is_edp) {
> + DRM_ERROR("eDP aux_bus not found\n");
> + return -ENODEV;
> + }
> +
> + /*
> +* External bridges are mandatory for eDP interfaces: one has to
> +* provide at least an eDP panel (which gets wrapped into
> panel-bridge).
> +*
> +* For DisplayPort interfaces external bridges are optional, so
> +* silently ignore an error if one is not present (-ENODEV).
> +*/
> + rc = dp_parser_find_next_bridge(dp_priv->parser);
This gets into the same problem that Dmitry pointed out that ps8640
has that's addressed by my recent series [1]. Namely it's not
guaranteed that the panel will have finished probing by the time
devm_of_dp_aux_populate_ep_devices() finishes probing. I don't think
it's going to be really solvable without the bigger rewrite that we've
been discussing, though. ...it's probably OK to land something like
what you have here, but it might at least deserve a comment in the
code?
[1] https://lore.kernel.org/r/20220409023628.2104952-1-diand...@chromium.org
> + if (rc == -ENODEV) {
> + if (dp->is_edp) {
> + DRM_ERROR("eDP: next bridge is not present\n");
> + return rc;
> + }
> + } else if (rc) {
> + if (rc != -EPROBE_DEFER)
> + DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
> + return rc;
In both of your two error returns here isn't it a problem that you don't do:
disable_irq(dp_priv->irq);
dp_display_host_phy_exit(dp_priv);
dp_display_host_deinit(dp_priv);
Should probably at least fix that clear error before landing, unless
I'm misunderstanding and there's some reason not to do that?
As discussed previously, I'm not convinced that we've covered every
corner case for properly doing and undoing the above things. I'm
hoping that once we do the cleanup and move to pm_runtime() management
that it will be cleaned up?
> @@ -114,10 +114,12 @@ struct drm_bridge *dp_bridge_init(struct msm_dp
> *dp_display, struct drm_device *
> bridge->funcs = _bridge_ops;
> bridge->type = dp_display->connector_type;
>
> - bridge->ops =
> - DRM_BRIDGE_OP_DETECT |
> - DRM_BRIDGE_OP_HPD |
> - DRM_BRIDGE_OP_MODES;
> + if (!dp_display->is_edp) {
> + bridge->ops =
> + DRM_BRIDGE_OP_DETECT |
> + DRM_BRIDGE_OP_HPD |
> + DRM_BRIDGE_OP_MODES;
Given that Dmitry had questions about why eDP has different ops in his
previous review of this code, the above probably deserves an inline
code comment. If you want to use my wording, you could paste this into
your code:
/*
* Many ops only make sense for DP. Why?
* - Detect/HPD are used by DRM to know if a display is _physically_
* there, not whether the display is powered on / finished initting.
* On eDP we assume the display is always there because you can't
* know until power is applied. If we don't implement the ops DRM will
* assume our display is always there.
* - Currently eDP mode reading is driven by the panel driver. This
* allows the panel driver to properly power itself on to read the
* modes.
*/
Overall: as discussed, I think that the current implementation is a
bit fragile and might have some wrong corner cases since it's hard for
me to reason about exactly when we init/de-init things. Even if it
works great, the fact that it's hard to reason about isn't wonderful.
That being said, I honestly believe that would benefit upstream to get
this landed and iterate on it. I don't think this should be causing
any existing behavior to be
Re: [Freedreno] [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
On 4/13/2022 5:02 PM, Stephen Boyd wrote:
The subject is still misleading. It is fixing something. It may be
enhancing it as well but it is clearly fixing it first.
Quoting Kuogee Hsieh (2022-04-06 14:28:13)
dp_hpd_plug_handle() is responsible for setting up main link and send
uevent to notify user space framework to start video stream. Similarly,
dp_hdp_unplug_handle is responsible to send uevent to notify user space
framework to stop video stream and then tear down main link.
However there are rare cases, such as in the middle of system suspending,
that uevent could not be delivered to user space framework. Therefore
some kind of recover mechanism armed by timer need to be in place in the
case of user space framework does not respond to uevent.
This patch have both dp_conenct_pending_timeout and
dp_disconnect_pending_timeout are used to stop video stream and tear down
main link and eventually restore DP driver state to known default
DISCONNECTED state in the case of timer fired due to framework does not
respond to uevent so that DP driver can recover itself gracefully at next
dongle unplug followed by plugin event.
Changes in v2:
-- replace dp_display_usbpd_disconnect_cb with dp_display_notify_disconnect
I'd prefer this part to be a different patch. It can come after the fix
to ease backporting.
Also, is there any response to Dmitry's question yet? I haven't seen
anything.
Sorry, since our internal review does not like this approach.
I will upload new patch for review soon.
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2433edb..ffafe17 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -22,6 +22,7 @@ struct dp_ctrl {
int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
+int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 178b774..a6200a5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -451,11 +451,14 @@ static int dp_display_usbpd_configure_cb(struct device
*dev)
static int dp_display_usbpd_disconnect_cb(struct device *dev)
We shouldn't need to keep around an empty function.
{
+ return 0;
+}
+
+static void dp_display_notify_disconnect(struct device *dev)
+{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
-
- return 0;
}
static void dp_display_handle_video_request(struct dp_display_private *dp)
@@ -593,10 +596,16 @@ static int dp_connect_pending_timeout(struct
dp_display_private *dp, u32 data)
mutex_lock(>event_mutex);
+ /*
+* main link had been setup but video is not ready yet
+* only tear down main link
+*/
state = dp->hpd_state;
if (state == ST_CONNECT_PENDING) {
- dp->hpd_state = ST_CONNECTED;
DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
+ dp_ctrl_off_link(dp->ctrl);
+ dp_display_host_phy_exit(dp);
+ dp->hpd_state = ST_DISCONNECTED;
}
mutex_unlock(>event_mutex);
@@ -645,6 +654,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private
*dp, u32 data)
if (dp->link->sink_count == 0) {
dp_display_host_phy_exit(dp);
}
+ dp_display_notify_disconnect(>pdev->dev);
mutex_unlock(>event_mutex);
return 0;
}
@@ -661,19 +671,22 @@ static int dp_hpd_unplug_handle(struct dp_display_private
*dp, u32 data)
return 0;
}
- dp->hpd_state = ST_DISCONNECT_PENDING;
-
/* disable HPD plug interrupts */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK,
false);
/*
* We don't need separate work for disconnect as
* connect/attention interrupts are disabled
-*/
- dp_display_usbpd_disconnect_cb(>pdev->dev);
+ */
This comment end is wrong. It should be unchanged.
+ dp_display_notify_disconnect(>pdev->dev);
- /* start sentinel checking in case of missing uevent */
- dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
+ if (state == ST_DISPLAY_OFF) {
+ dp->hpd_state = ST_DISCONNECTED;
+ } else {
+ /* start sentinel checking in case of missing uevent */
+ dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0,
DP_TIMEOUT_5_SECOND);
+ dp->hpd_state = ST_DISCONNECT_PENDING;
+ }
/* signal the disconnect event early to ensure proper teardown */
Re: [Freedreno] [PATCH v5 00/10] drm/hdcp: Pull HDCP auth/exchange/check into helpers
On Tue, Apr 12, 2022 at 09:41:35AM -0400, Rodrigo Vivi wrote: > On Mon, Apr 11, 2022 at 08:47:29PM +, Sean Paul wrote: > > From: Sean Paul > > > > Rebased set from November. Fixed a nit from Stephen in the msm patch and > > moved hdcp registers into the trogdor dtsi file to avoid differences > > with sc7180-based windows devices. The set is 4 patches lighter since > > some of the changes were accepted into msm. > > > > I'm still waiting for Intel review of the first 7 patches. Rodrigo/Jani, > > would you please provide your input so we can move forward with this > > set? > > I'm a bit concerned with patches 4 and 7. It is hard to map the removals > and additions and there are some changes that looks like changing behaviors, > but end up not being clear in the big patch. Also with big patch it is prune > to the rebasing and backport conflicts. I had the same concerns when I was writing this. I originally had it split up, but it seemed really cluttered with 2 sets of helpers (intel-internal + drm) that worked slightly differently. I'll try again now that some time has passed, perhaps a fresh look will help. Sean > > Would be possible to split some work in moving individual functions from i915 > to drm little by little with smaller patches? > > But thank you for this great work. It is also good to align our drm drivers. > > Thanks, > Rodrigo. > > > > > Thanks, > > > > Sean > > > > Link: https://patchwork.freedesktop.org/series/94623/ #v1 > > Link: https://patchwork.freedesktop.org/series/94713/ #v2 > > Link: https://patchwork.freedesktop.org/series/94712/ #v3 > > Link: https://patchwork.freedesktop.org/series/94712/ #v4 > > > > Sean Paul (10): > > drm/hdcp: Add drm_hdcp_atomic_check() > > drm/hdcp: Avoid changing crtc state in hdcp atomic check > > drm/hdcp: Update property value on content type and user changes > > drm/hdcp: Expand HDCP helper library for enable/disable/check > > drm/i915/hdcp: Consolidate HDCP setup/state cache > > drm/i915/hdcp: Retain hdcp_capable return codes > > drm/i915/hdcp: Use HDCP helpers for i915 > > dt-bindings: msm/dp: Add bindings for HDCP registers > > arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller > > drm/msm: Implement HDCP 1.x using the new drm HDCP helpers > > > > .../bindings/display/msm/dp-controller.yaml |7 +- > > arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi |8 + > > arch/arm64/boot/dts/qcom/sc7180.dtsi |6 +- > > drivers/gpu/drm/drm_hdcp.c| 1197 - > > drivers/gpu/drm/i915/display/intel_atomic.c |7 +- > > drivers/gpu/drm/i915/display/intel_ddi.c | 29 +- > > .../drm/i915/display/intel_display_debugfs.c | 11 +- > > .../drm/i915/display/intel_display_types.h| 58 +- > > drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 345 ++--- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 17 +- > > drivers/gpu/drm/i915/display/intel_hdcp.c | 1011 +++--- > > drivers/gpu/drm/i915/display/intel_hdcp.h | 36 +- > > drivers/gpu/drm/i915/display/intel_hdmi.c | 256 ++-- > > drivers/gpu/drm/msm/Makefile |1 + > > drivers/gpu/drm/msm/dp/dp_debug.c | 46 +- > > drivers/gpu/drm/msm/dp/dp_debug.h |6 +- > > drivers/gpu/drm/msm/dp/dp_display.c | 46 +- > > drivers/gpu/drm/msm/dp/dp_display.h |5 + > > drivers/gpu/drm/msm/dp/dp_drm.c | 68 +- > > drivers/gpu/drm/msm/dp/dp_drm.h |5 + > > drivers/gpu/drm/msm/dp/dp_hdcp.c | 453 +++ > > drivers/gpu/drm/msm/dp/dp_hdcp.h | 27 + > > drivers/gpu/drm/msm/dp/dp_parser.c| 20 +- > > drivers/gpu/drm/msm/dp/dp_parser.h|4 + > > drivers/gpu/drm/msm/dp/dp_reg.h | 32 +- > > drivers/gpu/drm/msm/msm_atomic.c | 15 + > > include/drm/drm_hdcp.h| 194 +++ > > 27 files changed, 2582 insertions(+), 1328 deletions(-) > > create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.c > > create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.h > > > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > > -- Sean Paul, Software Engineer, Google / Chromium OS
Re: [Freedreno] [PATCH v5 03/10] drm/hdcp: Update property value on content type and user changes
On Tue, Apr 12, 2022 at 09:25:59AM -0400, Rodrigo Vivi wrote:
> On Mon, Apr 11, 2022 at 08:47:32PM +, Sean Paul wrote:
> > From: Sean Paul
> >
> > This patch updates the connector's property value in 2 cases which were
> > previously missed:
> >
> > 1- Content type changes. The value should revert back to DESIRED from
> >ENABLED in case the driver must re-authenticate the link due to the
> >new content type.
> >
> > 2- Userspace sets value to DESIRED while ENABLED. In this case, the
> >value should be reset immediately to ENABLED since the link is
> >actively being encrypted.
> >
> > To accommodate these changes, I've split up the conditionals to make
> > things a bit more clear (as much as one can with this mess of state).
> >
> > Acked-by: Jani Nikula
> > Reviewed-by: Abhinav Kumar
> > Signed-off-by: Sean Paul
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-4-s...@poorly.run
> > #v1
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-4-s...@poorly.run
> > #v2
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-4-s...@poorly.run
> > #v3
> > Link:
> > https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-4-s...@poorly.run
> > #v4
> >
> > Changes in v2:
> > -None
> > Changes in v3:
> > -Fixed indentation issue identified by 0-day
> > Changes in v4:
> > -None
> > Changes in v5:
> > -None
> > ---
> > drivers/gpu/drm/drm_hdcp.c | 26 +-
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > index dd8fa91c51d6..8c851d40cd45 100644
> > --- a/drivers/gpu/drm/drm_hdcp.c
> > +++ b/drivers/gpu/drm/drm_hdcp.c
> > @@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector
> > *connector,
> > return true;
> >
> > /*
> > -* Nothing to do if content type is unchanged and one of:
> > -* - state didn't change
> > +* Content type changes require an HDCP disable/enable cycle.
> > +*/
> > + if (new_conn_state->hdcp_content_type !=
> > old_conn_state->hdcp_content_type) {
>
> shouldn't we add some && ( old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
> here?
Thanks for your reviews Rodrigo.
I don't think so since the content type is changing the current state of old
content protection is immaterial (ie: if we need to enable HDCP 2.x, the state
of HDCP 1.x doesn't really matter), we need to re-evaluate whether the current
level of HDCP is sufficient.
Hopefully that makes sense, but I could be missing something :-)
Sean
>
> > + new_conn_state->content_protection =
> > + DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > + return true;
> > + }
> > +
> > + /*
> > +* Ignore meaningless state changes:
> > * - HDCP was activated since the last commit
> > -* - attempting to set to desired while already enabled
> > +* - Attempting to set to desired while already enabled
> > */
> > - if (old_hdcp == new_hdcp ||
> > - (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > + if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
> > new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
> > (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
> > new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
> > - if (old_conn_state->hdcp_content_type ==
> > - new_conn_state->hdcp_content_type)
> > - return false;
> > + new_conn_state->content_protection =
> > + DRM_MODE_CONTENT_PROTECTION_ENABLED;
> > + return false;
> > }
> >
> > - return true;
> > + /* Finally, if state changes, we need action */
> > + return old_hdcp != new_hdcp;
> > }
> > EXPORT_SYMBOL(drm_hdcp_atomic_check);
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> >
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v7 4/4] Support the eDP modes given by panel
The eDP controller does not have a reliable way keep panel powered on to read the sink capabilities. So, the controller driver cannot validate if a mode can be supported by the source. We will rely on the panel driver to populate only the supported modes for now. Signed-off-by: Sankeerth Billakanti --- drivers/gpu/drm/msm/dp/dp_display.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index c7277f0..0f18a16 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -998,6 +998,14 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, return -EINVAL; } + /* +* The eDP controller currently does not have a reliable way of +* enabling panel power to read sink capabilities. So, we rely +* on the panel driver to populate only supported modes for now. +*/ + if (dp->is_edp) + return MODE_OK; + if ((dp->max_pclk_khz <= 0) || (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || (mode->clock > dp->max_pclk_khz)) -- 2.7.4
[Freedreno] [PATCH v7 3/4] drm/msm/dp: wait for hpd high before aux transaction
The source device should ensure the sink is ready before proceeding to
read the sink capability or perform any aux transactions. The sink
will indicate its readiness by asserting the HPD line. The controller
driver needs to wait for the hpd line to be asserted by the sink before
it performs any aux transactions.
The eDP sink is assumed to be always connected. It needs power from the
source and its HPD line will be asserted only after the panel is powered
on. The panel power will be enabled from the panel-edp driver and only
after that, the hpd line will be asserted.
Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
line gets asserted to indicate the sink is connected and ready. Hence
there is no need to wait for the hpd line to be asserted for a DP sink.
Signed-off-by: Sankeerth Billakanti
---
Changes in v7:
- add a comment to say why the wait si done for eDP
- correct the commit text
Changes in v6:
- Wait for hpd high only for eDP
- Split into smaller patches
drivers/gpu/drm/msm/dp/dp_aux.c | 21 -
drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++-
drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
5 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 6d36f63..cf0739f 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -36,6 +36,7 @@ struct dp_aux_private {
bool initted;
u32 offset;
u32 segment;
+ bool is_edp;
struct drm_dp_aux dp_aux;
};
@@ -337,6 +338,22 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
goto exit;
}
+ /*
+* For eDP it's important to give a reasonably long wait here for HPD
+* to be asserted. This is because the panel driver may have _just_
+* turned on the panel and then tried to do an AUX transfer. The panel
+* driver has no way of knowing when the panel is ready, so it's up
+* to us to wait. For DP we never get into this situation so let's
+* avoid ever doing the extra long wait for DP.
+*/
+ if (aux->is_edp) {
+ ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+ if (ret) {
+ DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+ goto exit;
+ }
+ }
+
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);
@@ -491,7 +508,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
drm_dp_aux_unregister(dp_aux);
}
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp)
{
struct dp_aux_private *aux;
@@ -506,6 +524,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct
dp_catalog *catalog)
init_completion(>comp);
aux->cmd_busy = false;
+ aux->is_edp = is_edp;
mutex_init(>mutex);
aux->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 82afc8d..c99aeec 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
void dp_aux_deinit(struct drm_dp_aux *dp_aux);
void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog);
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp);
void dp_aux_put(struct drm_dp_aux *aux);
#endif /*__DP_AUX_H_*/
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 07f2389..4282a98 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -242,6 +242,19 @@ void dp_catalog_aux_update_cfg(struct dp_catalog
*dp_catalog)
phy_calibrate(phy);
}
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
+{
+ u32 state;
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+
+ /* poll for hpd connected status every 2ms and timeout after 500ms */
+ return readl_poll_timeout(catalog->io->dp_controller.aux.base +
+ REG_DP_DP_HPD_INT_STATUS,
+ state, state & DP_DP_HPD_STATE_STATUS_CONNECTED,
+ 2000, 50);
+}
+
static void dump_regs(void __iomem *base, int len)
{
int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 7dea101..45140a3 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++
[Freedreno] [PATCH v7 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
The panel-edp enables the eDP panel power during probe, get_modes
and enable. The eDP connect and disconnect interrupts for the eDP/DP
controller are directly dependent on panel power. As eDP display can be
assumed as always connected, the controller driver can skip the eDP
connect and disconnect interrupts. Any disruption in the link status
will be indicated via the IRQ_HPD interrupts.
So, the eDP controller driver can just enable the IRQ_HPD and replug
interrupts. The DP controller driver still needs to enable all the
interrupts.
Signed-off-by: Sankeerth Billakanti
---
Changes in v7:
- reordered the patch in the series
- modified the return statement for isr
- connector check modified to just check for eDP
drivers/gpu/drm/msm/dp/dp_catalog.c | 9 +++--
drivers/gpu/drm/msm/dp/dp_display.c | 22 +-
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index fac815f..07f2389 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
*dp_catalog)
u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
- /* enable HPD plug and unplug interrupts */
- dp_catalog_hpd_config_intr(dp_catalog,
- DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
-
/* Configure REFTIMER and enable it */
reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
@@ -599,13 +595,14 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog
*dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
- int isr = 0;
+ int isr, mask;
isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
(isr & DP_DP_HPD_INT_MASK));
+ mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
- return isr;
+ return isr & (mask | ~DP_DP_HPD_INT_MASK);
}
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 43c59cb..1339c45 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private
*dp, u32 data)
dp_display_handle_plugged_change(>dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
- dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog,
DP_DP_HPD_PLUG_INT_MASK, true);
DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
@@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
dp_display_private *dp)
dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
+ /* Enable plug and unplug interrupts only for external DisplayPort */
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
+
/* Enable interrupt first time
* we are leaving dp clocks on during disconnect
* and never disable interrupt
@@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
dp_catalog_ctrl_hpd_config(dp->catalog);
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
+
if (dp_catalog_link_is_connected(dp->catalog)) {
/*
* set sink to normal operation mode -- D0
@@ -1654,6 +1668,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
return;
}
+ if (dp->is_edp)
+ dp_hpd_plug_handle(dp_display, 0);
+
mutex_lock(_display->event_mutex);
/* stop sentinel checking */
@@ -1718,6 +1735,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
dp_display = container_of(dp, struct dp_display_private, dp_display);
+ if (dp->is_edp)
+ dp_hpd_unplug_handle(dp_display, 0);
+
mutex_lock(_display->event_mutex);
/* stop sentinel checking */
--
2.7.4
[Freedreno] [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus
This patch adds support for generic eDP sink through aux_bus. The eDP/DP
controller driver should support aux transactions originating from the
panel-edp driver and hence should be initialized and ready.
The panel bridge supporting the panel should be ready before the bridge
connector is initialized. The generic panel probe needs the controller
resources to be enabled to support the aux transactions originating from
the panel probe.
Signed-off-by: Sankeerth Billakanti
---
Changes in v7:
- aux_bus is mandatory for eDP
- connector type check modified to just check for eDP
Changes in v6:
- Remove initialization
- Fix aux_bus node leak
- Split the patches
drivers/gpu/drm/msm/dp/dp_display.c | 68 ++---
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 10 +++---
drivers/gpu/drm/msm/dp/dp_parser.c | 23 ++---
drivers/gpu/drm/msm/dp/dp_parser.h | 13 ++-
5 files changed, 85 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d7a19d6..43c59cb 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include "msm_drv.h"
#include "msm_kms.h"
@@ -259,14 +260,12 @@ static int dp_display_bind(struct device *dev, struct
device *master,
dp->dp_display.drm_dev = drm;
priv->dp[dp->id] = >dp_display;
- rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
+ rc = dp->parser->parse(dp->parser);
if (rc) {
DRM_ERROR("device tree parsing failed\n");
goto end;
}
- dp->dp_display.next_bridge = dp->parser->next_bridge;
-
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
if (rc) {
@@ -1319,6 +1318,8 @@ static int dp_display_probe(struct platform_device *pdev)
dp->pdev = pdev;
dp->name = "drm_dp";
dp->dp_display.connector_type = desc->connector_type;
+ dp->dp_display.is_edp =
+ (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
rc = dp_init_sub_modules(dp);
if (rc) {
@@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp_hpd_event_setup(dp);
- dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
+ if (!dp_display->is_edp)
+ dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
}
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
@@ -1530,6 +1532,60 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display,
struct drm_minor *minor)
}
}
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+ int rc;
+ struct dp_display_private *dp_priv;
+ struct device_node *aux_bus;
+ struct device *dev;
+
+ dp_priv = container_of(dp, struct dp_display_private, dp_display);
+ dev = _priv->pdev->dev;
+ aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+ if (aux_bus && dp->is_edp) {
+ dp_display_host_init(dp_priv);
+ dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+ dp_display_host_phy_init(dp_priv);
+ enable_irq(dp_priv->irq);
+
+ rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+ of_node_put(aux_bus);
+ if (rc) {
+ disable_irq(dp_priv->irq);
+ dp_display_host_phy_exit(dp_priv);
+ dp_display_host_deinit(dp_priv);
+ return rc;
+ }
+ } else if (dp->is_edp) {
+ DRM_ERROR("eDP aux_bus not found\n");
+ return -ENODEV;
+ }
+
+ /*
+* External bridges are mandatory for eDP interfaces: one has to
+* provide at least an eDP panel (which gets wrapped into panel-bridge).
+*
+* For DisplayPort interfaces external bridges are optional, so
+* silently ignore an error if one is not present (-ENODEV).
+*/
+ rc = dp_parser_find_next_bridge(dp_priv->parser);
+ if (rc == -ENODEV) {
+ if (dp->is_edp) {
+ DRM_ERROR("eDP: next bridge is not present\n");
+ return rc;
+ }
+ } else if (rc) {
+ if (rc != -EPROBE_DEFER)
+ DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
+ return rc;
+ }
+
+ dp->next_bridge = dp_priv->parser->next_bridge;
+
+ return 0;
+}
+
int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder)
{
@@ -1553,6 +1609,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
dp_display->encoder = encoder;
+ ret = dp_display_get_next_bridge(dp_display);
+ if (ret)
+ return ret;
[Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus
This series adds support for generic eDP panel over aux_bus. These changes are dependent on the following series: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654=* Sankeerth Billakanti (4): drm/msm/dp: Add eDP support via aux_bus drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP drm/msm/dp: wait for hpd high before aux transaction Support the eDP modes given by panel drivers/gpu/drm/msm/dp/dp_aux.c | 21 +++- drivers/gpu/drm/msm/dp/dp_aux.h | 3 +- drivers/gpu/drm/msm/dp/dp_catalog.c | 22 +--- drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 100 +--- drivers/gpu/drm/msm/dp/dp_display.h | 1 + drivers/gpu/drm/msm/dp/dp_drm.c | 10 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 23 + drivers/gpu/drm/msm/dp/dp_parser.h | 13 - 9 files changed, 154 insertions(+), 40 deletions(-) -- 2.7.4
