Re: [Freedreno] [PATCH v4 08/11] arm64: dts: qcom: sm8350: Use 2 interconnect cells
On Fri, 30 Dec 2022 at 17:12, Krzysztof Kozlowski wrote: > > On 30/12/2022 16:35, Robert Foss wrote: > > Use two interconnect cells in order to optionally > > support a path tag. > > > > Signed-off-by: Robert Foss > > Reviewed-by: Konrad Dybcio > > --- > > arch/arm64/boot/dts/qcom/sm8350.dtsi | 28 ++-- > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > I think you need to rebase to include: > https://lore.kernel.org/all/167233461761.1099840.5517525898039031248.b4...@kernel.org/ Ah, I see. Functionally I seemed to do fine without those commits. > > On which tree/revision did you base this? msm/drm-msm-display-for-6.2 > > Best regards, > Krzysztof >
Re: [Freedreno] [PATCH] drm/msm/a2xx: support loading legacy (iMX) firmware
On 02/01/2023 03:39, Rob Clark wrote:
On Sun, Jan 1, 2023 at 7:57 AM Dmitry Baryshkov
wrote:
Support loading A200 firmware generated from the iMX firmware header
files. The firmware lacks protection support, however it allows GPU to
function properly while using the firmware files with clear license
which allows redistribution.
Cc: Jonathan Marek
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 27 +++
drivers/gpu/drm/msm/adreno/a2xx_gpu.h | 1 +
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
index 6c9a747eb4ad..c67089a7ebc1 100644
--- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
@@ -53,6 +53,8 @@ static void a2xx_submit(struct msm_gpu *gpu, struct
msm_gem_submit *submit)
static bool a2xx_me_init(struct msm_gpu *gpu)
{
+ struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+ struct a2xx_gpu *a2xx_gpu = to_a2xx_gpu(adreno_gpu);
struct msm_ringbuffer *ring = gpu->rb[0];
OUT_PKT3(ring, CP_ME_INIT, 18);
@@ -84,15 +86,20 @@ static bool a2xx_me_init(struct msm_gpu *gpu)
/* NQ and External Memory Swap */
OUT_RING(ring, 0x);
/* protected mode error checking (0x1f2 is REG_AXXX_CP_INT_CNTL) */
- OUT_RING(ring, 0x21f2);
+ if (a2xx_gpu->protection_disabled)
+ OUT_RING(ring, 0x);
+ else
+ OUT_RING(ring, 0x21f2);
/* Disable header dumping and Header dump address */
OUT_RING(ring, 0x);
/* Header dump size */
OUT_RING(ring, 0x);
- /* enable protected mode */
- OUT_PKT3(ring, CP_SET_PROTECTED_MODE, 1);
- OUT_RING(ring, 1);
+ if (!a2xx_gpu->protection_disabled) {
+ /* enable protected mode */
+ OUT_PKT3(ring, CP_SET_PROTECTED_MODE, 1);
+ OUT_RING(ring, 1);
+ }
adreno_flush(gpu, ring, REG_AXXX_CP_RB_WPTR);
return a2xx_idle(gpu);
@@ -101,6 +108,7 @@ static bool a2xx_me_init(struct msm_gpu *gpu)
static int a2xx_hw_init(struct msm_gpu *gpu)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+ struct a2xx_gpu *a2xx_gpu = to_a2xx_gpu(adreno_gpu);
dma_addr_t pt_base, tran_error;
uint32_t *ptr, len;
int i, ret;
@@ -221,6 +229,17 @@ static int a2xx_hw_init(struct msm_gpu *gpu)
len = adreno_gpu->fw[ADRENO_FW_PM4]->size / 4;
DBG("loading PM4 ucode version: %x", ptr[1]);
+ /*
+* New firmware files seem to have GPU and firmware version in this
+* word (0x20 for A200, 0x220xxx for A220, 0x225xxx for A225).
+* Older firmware files, which lack protection support, have 0 instead.
+*/
+ if (ptr[1] == 0) {
I don't really have a good enough picture about all the possible fw
versions floating around out there, esp back to the pre-qc days, to
know if this is a good enough check. But I guess we can go with it,
and in the worst case later add an allowlist table of fw checksums (or
similar) if this doesn't turn out to be sufficient, so the overall
approach isn't painting us into a corner.
Reviewed-by: Rob Clark
For the reference. I have pushed existing redistributable firmware files
to https://github.com/lumag/yamato-firmware.git (I can move the repo to
some other location, e.g. to gitlab.fdo.org/msm if that's a better place).
I've also sent a patch to linux-firmware, so at some point we should be
able to use a200 with the default setup.
+ dev_warn(gpu->dev->dev,
+"Legacy firmware detected, disabling protection
support\n");
+ a2xx_gpu->protection_disabled = true;
+ }
+
gpu_write(gpu, REG_AXXX_CP_DEBUG,
AXXX_CP_DEBUG_MIU_128BIT_WRITE_ENABLE);
gpu_write(gpu, REG_AXXX_CP_ME_RAM_WADDR, 0);
diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.h
b/drivers/gpu/drm/msm/adreno/a2xx_gpu.h
index 02fba2cb8932..161a075f94af 100644
--- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.h
@@ -15,6 +15,7 @@
struct a2xx_gpu {
struct adreno_gpu base;
bool pm_enabled;
+ bool protection_disabled;
};
#define to_a2xx_gpu(x) container_of(x, struct a2xx_gpu, base)
--
2.39.0
--
With best wishes
Dmitry
Re: [Freedreno] [RFC PATCH 6/7] drm/msm/dpu: Implement tearcheck support on INTF block
On 02/01/2023 13:06, Marijn Suijten wrote:
On 2023-01-01 15:32:11, Dmitry Baryshkov wrote:
On 31/12/2022 23:50, Marijn Suijten wrote:
Since DPU 5.0.0 the TEARCHECK registers and interrupts moved out of the
PINGPONG block and into the INTF. Implement the necessary callbacks in
the INTF block, and use these callbacks together with the INTF_TEAR
interrupts
Signed-off-by: Marijn Suijten
Generally I have the same question as for the patch 2. Can we have some
higher level functions in the hw_pp and hw_intf files?
That is mostly because patch 2 only cleaned up non-optional handling of
hw_pp callbacks in dpu_encoder_phys_cmd_prepare_commit, which utilizes
hw_intf's autorefresh callbacks since this patch. I don't think there's
any logic in the encoder currently that is unique to either PP or INTF?
I think it would be better to duplicate the logic rather than having a
spaghetti of hw_pp and hw_intf calls.
There are quite a few functions that check for NULL hw_pp only, while -
especially after this patch - should also check hw_intf to raise
"invalid encoder". Should I extend those checks as well?
I think so. However in most of cases these checks should be void, since
cmd encoder can not be instantiated without intf.
Moreover, as I
review your patch I have the feeling that it would make sense to have to
two sets of encoder callbacks, one for the hw_pp tearing handling and
another set for hw_intf-based one.
Do you mean to duplicate most phy_cmd functions and switch them based on
has_intf_te in dpu_encoder_phys_cmd_init_ops? Or introduce an entirely
new set of callbacks that simply hide / abstract away the check on
has_intf_te? The former would duplicate a bunch of code unless that is
abstracted away into other functions, mainly in
dpu_encoder_phys_cmd_tearcheck_config and
dpu_encoder_phys_cmd_prepare_commit.
For the dpu_encoder_phys_cmd_tearcheck_config() it seems logical to fill
in the tc_cfg and then to call either the single hw_pp callback or a
single hw_intf callback.
I'd move most of the code from prepare_commit to dpu_hw_pp and then
duplicate it to dpu_hw_intf. This seems like the lesser evil.
This function really stands out, since if you inline
_dpu_encoder_phys_cmd_connect_te() and
dpu_encoder_phys_cmd_is_ongoing_pptx() it becomes obvious that the whole
function is a mixture of linked calls to hw_pp ops. And judging from
your patch it doesn't make sense to check them one by one. Either we
have all of them, or none.
Alternatively, could we find a way where these PP and INTF ops share the
same struct and function signature? That might be tricky for passing in
the hw_pp or hw_intf struct without leaking those details to the
callback and/or have the switching logic in there.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 10 +-
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 113 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 206 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 29 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 +
6 files changed, 340 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b5a194..8b9070220ab2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -673,6 +673,7 @@ static void _dpu_encoder_update_vsync_source(struct
dpu_encoder_virt *dpu_enc,
struct dpu_kms *dpu_kms;
struct dpu_hw_mdp *hw_mdptop;
struct drm_encoder *drm_enc;
+ struct dpu_encoder_phys *phys_enc;
int i;
if (!dpu_enc || !disp_info) {
@@ -703,12 +704,22 @@ static void _dpu_encoder_update_vsync_source(struct
dpu_encoder_virt *dpu_enc,
vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx;
vsync_cfg.pp_count = dpu_enc->num_phys_encs;
+ vsync_cfg.frame_rate =
drm_mode_vrefresh(_enc->base.crtc->state->adjusted_mode);
+
if (disp_info->is_te_using_watchdog_timer)
vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
else
vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
hw_mdptop->ops.setup_vsync_source(hw_mdptop, _cfg);
+
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ phys_enc = dpu_enc->phys_encs[i];
+
+ if (phys_enc->has_intf_te &&
phys_enc->hw_intf->ops.vsync_sel)
+
phys_enc->hw_intf->ops.vsync_sel(phys_enc->hw_intf,
+ vsync_cfg.vsync_source);
+ }
}
}
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 f2af07d87f56..47e79401032c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++
[Freedreno] [PATCH 2/2] drm/msm/dpu: remove CRTC frame event callback registration
The frame event callback is always set to dpu_crtc_frame_event_cb() (or
to NULL) and the data is always either the CRTC itself or NULL
(correpondingly). Thus drop the event callback registration, call the
dpu_crtc_frame_event_cb() directly and gate on the dpu_enc->crtc
assigned using dpu_encoder_assign_crtc().
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 17 +
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 14 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 -
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 4 --
5 files changed, 21 insertions(+), 65 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..64cd2ec8a0c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -640,18 +640,8 @@ static void dpu_crtc_frame_event_work(struct kthread_work
*work)
DPU_ATRACE_END("crtc_frame_event");
}
-/*
- * dpu_crtc_frame_event_cb - crtc frame event callback API. CRTC module
- * registers this API to encoder for all frame event callbacks like
- * frame_error, frame_done, idle_timeout, etc. Encoder may call different
events
- * from different context - IRQ, user thread, commit_thread, etc. Each event
- * should be carefully reviewed and should be processed in proper task context
- * to avoid schedulin delay or properly manage the irq context's bottom half
- * processing.
- */
-static void dpu_crtc_frame_event_cb(void *data, u32 event)
+void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event)
{
- struct drm_crtc *crtc = (struct drm_crtc *)data;
struct dpu_crtc *dpu_crtc;
struct msm_drm_private *priv;
struct dpu_crtc_frame_event *fevent;
@@ -1051,9 +1041,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
dpu_core_perf_crtc_update(crtc, 0, true);
- drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
- dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
-
memset(cstate->mixers, 0, sizeof(cstate->mixers));
cstate->num_mixers = 0;
@@ -1089,8 +1076,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
*/
if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
request_bandwidth = true;
- dpu_encoder_register_frame_event_callback(encoder,
- dpu_crtc_frame_event_cb, (void *)crtc);
}
if (request_bandwidth)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b1626a..3aa536d95721 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -300,4 +300,18 @@ static inline enum dpu_crtc_client_type
dpu_crtc_get_client_type(
return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
}
+/**
+ * dpu_crtc_frame_event_cb - crtc frame event callback API
+ * @crtc: Pointer to crtc
+ * @event: Event to process
+ *
+ * CRTC module registers this API to encoder for all frame event callbacks like
+ * frame_error, frame_done, idle_timeout, etc. Encoder may call different
events
+ * from different context - IRQ, user thread, commit_thread, etc. Each event
+ * should be carefully reviewed and should be processed in proper task context
+ * to avoid schedulin delay or properly manage the irq context's bottom half
+ * processing.
+ */
+void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event);
+
#endif /* _DPU_CRTC_H_ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 84f8c8a1b049..bf0af5af4306 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -147,8 +147,6 @@ enum dpu_enc_rc_states {
* @frame_busy_mask: Bitmask tracking which phys_enc we are still
* busy processing current command.
* Bit0 = phys_encs[0] etc.
- * @crtc_frame_event_cb: callback handler for frame event
- * @crtc_frame_event_cb_data: callback handler private data
* @frame_done_timeout_ms: frame done timeout in ms
* @frame_done_timer: watchdog timer for frame done event
* @vsync_event_timer: vsync timer
@@ -187,8 +185,6 @@ struct dpu_encoder_virt {
struct dentry *debugfs_root;
struct mutex enc_lock;
DECLARE_BITMAP(frame_busy_mask, MAX_PHYS_ENCODERS_PER_VIRTUAL);
- void (*crtc_frame_event_cb)(void *, u32 event);
- void *crtc_frame_event_cb_data;
atomic_t frame_done_timeout_ms;
struct timer_list frame_done_timer;
@@ -1358,28 +1354,6 @@ void dpu_encoder_toggle_vblank_for_crtc(struct
drm_encoder *drm_enc,
}
}
-void dpu_encoder_register_frame_event_callback(struct drm_encoder
[Freedreno] [PATCH 1/2] drm/msm/dpu: remove dpu_encoder_virt_ops
Struct dpu_encoder_virt_ops is used to provide several callbacks to the
phys_enc backends. However these ops are static and are not supposed to
change in the foreseeble future. Drop the indirection and call
corresponding functions directly.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 ++-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 47 ++-
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 17 ++-
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 11 ++---
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 13 ++---
5 files changed, 40 insertions(+), 65 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b5a194..84f8c8a1b049 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -340,9 +340,7 @@ void dpu_encoder_helper_report_irq_timeout(struct
dpu_encoder_phys *phys_enc,
phys_enc->intf_idx - INTF_0, phys_enc->wb_idx - WB_0,
phys_enc->hw_pp->idx - PINGPONG_0, intr_idx);
- if (phys_enc->parent_ops->handle_frame_done)
- phys_enc->parent_ops->handle_frame_done(
- phys_enc->parent, phys_enc,
+ dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc,
DPU_ENCODER_FRAME_EVENT_ERROR);
}
@@ -1284,7 +1282,7 @@ static enum dpu_wb dpu_encoder_get_wb(const struct
dpu_mdss_cfg *catalog,
return WB_MAX;
}
-static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
+void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
{
struct dpu_encoder_virt *dpu_enc = NULL;
@@ -1306,7 +1304,7 @@ static void dpu_encoder_vblank_callback(struct
drm_encoder *drm_enc,
DPU_ATRACE_END("encoder_vblank_callback");
}
-static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
+void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
{
if (!phy_enc)
@@ -1382,7 +1380,7 @@ void dpu_encoder_register_frame_event_callback(struct
drm_encoder *drm_enc,
spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags);
}
-static void dpu_encoder_frame_done_callback(
+void dpu_encoder_frame_done_callback(
struct drm_encoder *drm_enc,
struct dpu_encoder_phys *ready_phys, u32 event)
{
@@ -2233,12 +2231,6 @@ static int dpu_encoder_virt_add_phys_encs(
return 0;
}
-static const struct dpu_encoder_virt_ops dpu_encoder_parent_ops = {
- .handle_vblank_virt = dpu_encoder_vblank_callback,
- .handle_underrun_virt = dpu_encoder_underrun_callback,
- .handle_frame_done = dpu_encoder_frame_done_callback,
-};
-
static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
struct dpu_kms *dpu_kms,
struct msm_display_info *disp_info)
@@ -2258,7 +2250,6 @@ static int dpu_encoder_setup_display(struct
dpu_encoder_virt *dpu_enc,
memset(_params, 0, sizeof(phys_params));
phys_params.dpu_kms = dpu_kms;
phys_params.parent = _enc->base;
- phys_params.parent_ops = _encoder_parent_ops;
phys_params.enc_spinlock = _enc->enc_spinlock;
switch (disp_info->intf_type) {
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 f2af07d87f56..1d434b22180d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -60,25 +60,6 @@ enum dpu_enc_enable_state {
struct dpu_encoder_phys;
-/**
- * struct dpu_encoder_virt_ops - Interface the containing virtual encoder
- * provides for the physical encoders to use to callback.
- * @handle_vblank_virt:Notify virtual encoder of vblank IRQ reception
- * Note: This is called from IRQ handler context.
- * @handle_underrun_virt: Notify virtual encoder of underrun IRQ reception
- * Note: This is called from IRQ handler context.
- * @handle_frame_done: Notify virtual encoder that this phys encoder
- * completes last request frame.
- */
-struct dpu_encoder_virt_ops {
- void (*handle_vblank_virt)(struct drm_encoder *,
- struct dpu_encoder_phys *phys);
- void (*handle_underrun_virt)(struct drm_encoder *,
- struct dpu_encoder_phys *phys);
- void (*handle_frame_done)(struct drm_encoder *,
- struct dpu_encoder_phys *phys, u32 event);
-};
-
/**
* struct dpu_encoder_phys_ops - Interface the physical encoders provide to
* the containing virtual encoder.
@@ -199,7 +180,6 @@ enum dpu_intr_idx {
struct dpu_encoder_phys {
struct drm_encoder *parent;
struct
Re: [Freedreno] [PATCH v5 4/5] drm/msm/a6xx: Remove cx gdsc polling using 'reset'
On Mon, Jan 02, 2023 at 04:18:30PM +0530, Akhil P Oommen wrote:
> Remove the unused 'reset' interface which was supposed to help to ensure
> that cx gdsc has collapsed during gpu recovery. This is was not enabled
> so far due to missing gpucc driver support. Similar functionality using
> genpd framework will be implemented in the upcoming patch.
>
> This effectively reverts commit 1f6cca404918
> ("drm/msm/a6xx: Ensure CX collapse during gpu recovery").
>
> Signed-off-by: Akhil P Oommen
> Reviewed-by: Ulf Hansson
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [Freedreno] [RFC PATCH 2/7] drm/msm/dpu: Disable pingpong TE on DPU 5.0.0 and above
On 02/01/2023 12:25, Marijn Suijten wrote:
On 2023-01-01 06:28:23, Dmitry Baryshkov wrote:
On 31/12/2022 23:50, Marijn Suijten wrote:
Since hardware revision 5.0.0 the TE configuration moved out of the
PINGPONG block into the INTF block. Writing these registers has no
effect, and is omitted downstream via the DPU/SDE_PINGPONG_TE feature
flag. This flag is only added to PINGPONG blocks used by hardware prior
to 5.0.0.
The code that writes to these registers in the INTF block will follow in
subsequent patches.
Signed-off-by: Marijn Suijten
---
.../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +-
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 53 +++
.../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 18 ---
3 files changed, 44 insertions(+), 32 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 ae28b2b93e69..7e5ba52197cd 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
@@ -582,7 +582,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
{
struct dpu_hw_pp_vsync_info info;
- if (!phys_enc)
+ if (!phys_enc || !phys_enc->hw_pp->ops.get_vsync_info)
return false;
phys_enc->hw_pp->ops.get_vsync_info(phys_enc->hw_pp, );
@@ -607,6 +607,9 @@ static void dpu_encoder_phys_cmd_prepare_commit(
This function works only with the hw_pp and if I'm not mistaken it
becomes void for newer platforms. Please consider moving completely to
the dpu_hw_pp.c Then we'd have a single optional callback instead of
having a pile of them.
It also works for hw_intf, which I'm introducing in a later patch. This
change is just cleaning up the fact that these are the only callbacks
(on hw_pp->ops) that weren't considered optional yet.
Even though removing these writes should not have any effect, perhaps it
is more clear to insert this patch /after/ introducing INTF TE? Then
that patch will likely already include the change that makes this error
checking consistent for both variants, as it currently has:
/* If autorefresh is already disabled, we have nothing to do */
if (phys_enc->has_intf_te) {
if (!phys_enc->hw_intf ||
!phys_enc->hw_intf->ops.get_autorefresh ||
!phys_enc->hw_intf->ops.setup_autorefresh)
return;
if (!phys_enc->hw_intf->ops.get_autorefresh(phys_enc->hw_intf,
NULL))
return;
} else {
if (!phys_enc->hw_pp || !phys_enc->hw_pp->ops.get_autorefresh ||
!phys_enc->hw_pp->ops.setup_autorefresh)
return;
if (!phys_enc->hw_pp->ops.get_autorefresh(phys_enc->hw_pp,
NULL))
return;
}
This is what I'd like to stay away from.
The following code looks better from my point of view:
if (!phys_enc)
return;
if (!dpu_encoder_phys_cmd_is_master(phys_enc))
return;
/* I'd use WARN_ON here, but existing code doesn't have these warnings. */
if (phys_enc->has_intf_te) {
if (!phys_enc->hw_intf)
return;
if (!phys_enc->hw_intf->enable_tearing)
return;
phys_enc->hw_intf->ops.enable_tearing(phys_enc->hw_intf);
}
if (!phys_enc->hw_pp)
return;
if (!phys_enc->hw_pp->enable_tearing)
return;
phys_enc->hw_pp->ops.enable_tearing(phys_enc->hw_pp);
- Marijn
if (!dpu_encoder_phys_cmd_is_master(phys_enc))
return;
+ if (!phys_enc->hw_pp->ops.get_autorefresh || !phys_enc->hw_pp->ops.setup_autorefresh)
+ return;
+
/* If autorefresh is already disabled, we have nothing to do */
if (!phys_enc->hw_pp->ops.get_autorefresh(phys_enc->hw_pp, NULL))
return;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 9814ad52cc04..39d4b293710c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -59,11 +59,18 @@
#define MIXER_SC7180_MASK \
(BIT(DPU_DIM_LAYER) | BIT(DPU_MIXER_COMBINED_ALPHA))
-#define PINGPONG_SDM845_MASK BIT(DPU_PINGPONG_DITHER)
+#define PINGPONG_SDM845_MASK \
+ (BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_TE))
-#define PINGPONG_SDM845_SPLIT_MASK \
+#define PINGPONG_SDM845_TE2_MASK \
(PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
+#define PINGPONG_SM8150_MASK \
+ (BIT(DPU_PINGPONG_DITHER))
+
+#define PINGPONG_SM8150_TE2_MASK \
+ (PINGPONG_SM8150_MASK | BIT(DPU_PINGPONG_TE2))
+
#define CTL_SC7280_MASK \
(BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE) |
BIT(DPU_CTL_VM_CFG))
@@ -1156,21 +1163,21 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
.len = 0x20, .version = 0x2},
};
-#define PP_BLK_TE(_name, _id,
Re: [Freedreno] [RFC PATCH 6/7] drm/msm/dpu: Implement tearcheck support on INTF block
On 2023-01-01 15:32:11, Dmitry Baryshkov wrote:
> On 31/12/2022 23:50, Marijn Suijten wrote:
> > Since DPU 5.0.0 the TEARCHECK registers and interrupts moved out of the
> > PINGPONG block and into the INTF. Implement the necessary callbacks in
> > the INTF block, and use these callbacks together with the INTF_TEAR
> > interrupts
> >
> > Signed-off-by: Marijn Suijten
>
> Generally I have the same question as for the patch 2. Can we have some
> higher level functions in the hw_pp and hw_intf files?
That is mostly because patch 2 only cleaned up non-optional handling of
hw_pp callbacks in dpu_encoder_phys_cmd_prepare_commit, which utilizes
hw_intf's autorefresh callbacks since this patch. I don't think there's
any logic in the encoder currently that is unique to either PP or INTF?
There are quite a few functions that check for NULL hw_pp only, while -
especially after this patch - should also check hw_intf to raise
"invalid encoder". Should I extend those checks as well?
> Moreover, as I
> review your patch I have the feeling that it would make sense to have to
> two sets of encoder callbacks, one for the hw_pp tearing handling and
> another set for hw_intf-based one.
Do you mean to duplicate most phy_cmd functions and switch them based on
has_intf_te in dpu_encoder_phys_cmd_init_ops? Or introduce an entirely
new set of callbacks that simply hide / abstract away the check on
has_intf_te? The former would duplicate a bunch of code unless that is
abstracted away into other functions, mainly in
dpu_encoder_phys_cmd_tearcheck_config and
dpu_encoder_phys_cmd_prepare_commit.
Alternatively, could we find a way where these PP and INTF ops share the
same struct and function signature? That might be tricky for passing in
the hw_pp or hw_intf struct without leaking those details to the
callback and/or have the switching logic in there.
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +
> > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 10 +-
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 113 +++---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 206 ++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 29 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 +
> > 6 files changed, 340 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 9c6817b5a194..8b9070220ab2 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -673,6 +673,7 @@ static void _dpu_encoder_update_vsync_source(struct
> > dpu_encoder_virt *dpu_enc,
> > struct dpu_kms *dpu_kms;
> > struct dpu_hw_mdp *hw_mdptop;
> > struct drm_encoder *drm_enc;
> > + struct dpu_encoder_phys *phys_enc;
> > int i;
> >
> > if (!dpu_enc || !disp_info) {
> > @@ -703,12 +704,22 @@ static void _dpu_encoder_update_vsync_source(struct
> > dpu_encoder_virt *dpu_enc,
> > vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx;
> >
> > vsync_cfg.pp_count = dpu_enc->num_phys_encs;
> > + vsync_cfg.frame_rate =
> > drm_mode_vrefresh(_enc->base.crtc->state->adjusted_mode);
> > +
> > if (disp_info->is_te_using_watchdog_timer)
> > vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
> > else
> > vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
> >
> > hw_mdptop->ops.setup_vsync_source(hw_mdptop, _cfg);
> > +
> > + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > + phys_enc = dpu_enc->phys_encs[i];
> > +
> > + if (phys_enc->has_intf_te &&
> > phys_enc->hw_intf->ops.vsync_sel)
> > +
> > phys_enc->hw_intf->ops.vsync_sel(phys_enc->hw_intf,
> > + vsync_cfg.vsync_source);
> > + }
> > }
> > }
> >
> > 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 f2af07d87f56..47e79401032c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > @@ -148,10 +148,10 @@ struct dpu_encoder_phys_ops {
> > /**
> >* enum dpu_intr_idx - dpu encoder interrupt index
> >* @INTR_IDX_VSYNC:Vsync interrupt for video mode panel
> > - * @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
> > + * @INTR_IDX_PINGPONG: Pingpong done interrupt for cmd mode panel
> > + * @INTR_IDX_UNDERRUN: Underrun interrupt for video and cmd mode panel
> > + * @INTR_IDX_RDPTR:Readpointer done interrupt for cmd mode panel
Re: [Freedreno] [PATCH v2] drm/msm/adreno: Make adreno quirks not overwrite each other
On 1/2/2023 3:32 PM, Konrad Dybcio wrote:
> So far the adreno quirks have all been assigned with an OR operator,
> which is problematic, because they were assigned consecutive integer
> values, which makes checking them with an AND operator kind of no bueno..
>
> Switch to using BIT(n) so that only the quirks that the programmer chose
> are taken into account when evaluating info->quirks & ADRENO_QUIRK_...
>
> Fixes: 370063ee427a ("drm/msm/adreno: Add A540 support")
> Reviewed-by: Dmitry Baryshkov
> Reviewed-by: Marijn Suijten
> Reviewed-by: Rob Clark
> Signed-off-by: Konrad Dybcio
> ---
> v1 -> v2:
> - pick up tags
> - correct the Fixes: tag
>
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index c85857c0a228..5eb254c9832a 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -29,11 +29,9 @@ enum {
> ADRENO_FW_MAX,
> };
>
> -enum adreno_quirks {
> - ADRENO_QUIRK_TWO_PASS_USE_WFI = 1,
> - ADRENO_QUIRK_FAULT_DETECT_MASK = 2,
> - ADRENO_QUIRK_LMLOADKILL_DISABLE = 3,
> -};
> +#define ADRENO_QUIRK_TWO_PASS_USE_WFIBIT(0)
> +#define ADRENO_QUIRK_FAULT_DETECT_MASK BIT(1)
> +#define ADRENO_QUIRK_LMLOADKILL_DISABLE BIT(2)
>
> struct adreno_rev {
> uint8_t core;
> @@ -65,7 +63,7 @@ struct adreno_info {
> const char *name;
> const char *fw[ADRENO_FW_MAX];
> uint32_t gmem;
> - enum adreno_quirks quirks;
> + u64 quirks;
> struct msm_gpu *(*init)(struct drm_device *dev);
> const char *zapfw;
> u32 inactive_period;
Reviewed-by: Akhil P Oommen
-Akhil.
[Freedreno] [PATCH v5 5/5] drm/msm/a6xx: Use genpd notifier to ensure cx-gdsc collapse
As per the recommended recovery sequence of adreno gpu, cx gdsc should
collapse at hardware before it is turned back ON. This helps to clear
out the stale states in hardware before it is reinitialized. Use the
genpd notifier along with the newly introduced
dev_pm_genpd_synced_poweroff() api to ensure that cx gdsc has collapsed
before we turn it back ON.
Signed-off-by: Akhil P Oommen
Reviewed-by: Ulf Hansson
---
(no changes since v2)
Changes in v2:
- Select PM_GENERIC_DOMAINS from Kconfig
drivers/gpu/drm/msm/Kconfig | 1 +
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 15 +++
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 6 ++
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++
4 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 3c9dfdb0b328..74f5916f5ca5 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -28,6 +28,7 @@ config DRM_MSM
select SYNC_FILE
select PM_OPP
select NVMEM
+ select PM_GENERIC_DOMAINS
help
DRM/KMS driver for MSM/snapdragon.
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 1580d0090f35..c03830957c26 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1507,6 +1507,17 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
gmu->initialized = false;
}
+static int cxpd_notifier_cb(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct a6xx_gmu *gmu = container_of(nb, struct a6xx_gmu, pd_nb);
+
+ if (action == GENPD_NOTIFY_OFF)
+ complete_all(>pd_gate);
+
+ return 0;
+}
+
int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
{
struct adreno_gpu *adreno_gpu = _gpu->base;
@@ -1640,6 +1651,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
goto detach_cxpd;
}
+ init_completion(>pd_gate);
+ complete_all(>pd_gate);
+ gmu->pd_nb.notifier_call = cxpd_notifier_cb;
+
/*
* Get a link to the GX power domain to reset the GPU in case of GMU
* crash
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 5a42dd4dd31f..0bc3eb443fec 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -4,8 +4,10 @@
#ifndef _A6XX_GMU_H_
#define _A6XX_GMU_H_
+#include
#include
#include
+#include
#include "msm_drv.h"
#include "a6xx_hfi.h"
@@ -90,6 +92,10 @@ struct a6xx_gmu {
bool initialized;
bool hung;
bool legacy; /* a618 or a630 */
+
+ /* For power domain callback */
+ struct notifier_block pd_nb;
+ struct completion pd_gate;
};
static inline u32 gmu_read(struct a6xx_gmu *gmu, u32 offset)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 4b16e75dfa50..dd618b099110 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -10,6 +10,7 @@
#include
#include
+#include
#include
#define GPU_PAS_ID 13
@@ -1258,6 +1259,7 @@ static void a6xx_recover(struct msm_gpu *gpu)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+ struct a6xx_gmu *gmu = _gpu->gmu;
int i, active_submits;
adreno_dump_info(gpu);
@@ -1290,6 +1292,10 @@ static void a6xx_recover(struct msm_gpu *gpu)
*/
gpu->active_submits = 0;
+ reinit_completion(>pd_gate);
+ dev_pm_genpd_add_notifier(gmu->cxpd, >pd_nb);
+ dev_pm_genpd_synced_poweroff(gmu->cxpd);
+
/* Drop the rpm refcount from active submits */
if (active_submits)
pm_runtime_put(>pdev->dev);
@@ -1297,6 +1303,11 @@ static void a6xx_recover(struct msm_gpu *gpu)
/* And the final one from recover worker */
pm_runtime_put_sync(>pdev->dev);
+ if (!wait_for_completion_timeout(>pd_gate, msecs_to_jiffies(1000)))
+ DRM_DEV_ERROR(>pdev->dev, "cx gdsc didn't collapse\n");
+
+ dev_pm_genpd_remove_notifier(gmu->cxpd);
+
pm_runtime_use_autosuspend(>pdev->dev);
if (active_submits)
--
2.7.4
[Freedreno] [PATCH v5 4/5] drm/msm/a6xx: Remove cx gdsc polling using 'reset'
Remove the unused 'reset' interface which was supposed to help to ensure
that cx gdsc has collapsed during gpu recovery. This is was not enabled
so far due to missing gpucc driver support. Similar functionality using
genpd framework will be implemented in the upcoming patch.
This effectively reverts commit 1f6cca404918
("drm/msm/a6xx: Ensure CX collapse during gpu recovery").
Signed-off-by: Akhil P Oommen
Reviewed-by: Ulf Hansson
---
(no changes since v3)
Changes in v3:
- Updated commit msg (Philipp)
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4
drivers/gpu/drm/msm/msm_gpu.c | 4
drivers/gpu/drm/msm/msm_gpu.h | 4
3 files changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 36c8fb699b56..4b16e75dfa50 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -10,7 +10,6 @@
#include
#include
-#include
#include
#define GPU_PAS_ID 13
@@ -1298,9 +1297,6 @@ static void a6xx_recover(struct msm_gpu *gpu)
/* And the final one from recover worker */
pm_runtime_put_sync(>pdev->dev);
- /* Call into gpucc driver to poll for cx gdsc collapse */
- reset_control_reset(gpu->cx_collapse);
-
pm_runtime_use_autosuspend(>pdev->dev);
if (active_submits)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 30ed45af76ad..97e1319d4577 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -16,7 +16,6 @@
#include
#include
#include
-#include
#include
/*
@@ -933,9 +932,6 @@ int msm_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
if (IS_ERR(gpu->gpu_cx))
gpu->gpu_cx = NULL;
- gpu->cx_collapse = devm_reset_control_get_optional_exclusive(>dev,
- "cx_collapse");
-
gpu->pdev = pdev;
platform_set_drvdata(pdev, >adreno_smmu);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 651786bc55e5..fa9e34d02c91 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -13,7 +13,6 @@
#include
#include
#include
-#include
#include "msm_drv.h"
#include "msm_fence.h"
@@ -282,9 +281,6 @@ struct msm_gpu {
bool hw_apriv;
struct thermal_cooling_device *cooling;
-
- /* To poll for cx gdsc collapse during gpu recovery */
- struct reset_control *cx_collapse;
};
static inline struct msm_gpu *dev_to_gpu(struct device *dev)
--
2.7.4
[Freedreno] [PATCH v5 3/5] drm/msm/a6xx: Vote for cx gdsc from gpu driver
When a device has multiple power domains, dev->power_domain is left
empty during probe. That didn't cause any issue so far because we are
freeloading on smmu driver's vote on cx gdsc. Instead of that, create
a device_link between cx genpd device and gmu device to keep a vote from
gpu driver.
Before this patch:
localhost ~ # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
gx_gdsc on 0
/devices/genpd:1:3d6a000.gmuactive 0
cx_gdsc on 0
/devices/platform/soc@0/3da.iommu active 0
After this patch:
localhost ~ # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
gx_gdsc on 0
/devices/genpd:1:3d6a000.gmuactive 0
cx_gdsc on 0
/devices/platform/soc@0/3da.iommu active 0
/devices/genpd:0:3d6a000.gmuactive 0
Signed-off-by: Akhil P Oommen
Reviewed-by: Ulf Hansson
---
(no changes since v1)
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 31 +++
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 1 +
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 6484b97c5344..1580d0090f35 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1479,6 +1479,12 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
pm_runtime_force_suspend(gmu->dev);
+ /*
+* Since cxpd is a virt device, the devlink with gmu-dev will be removed
+* automatically when we do detach
+*/
+ dev_pm_domain_detach(gmu->cxpd, false);
+
if (!IS_ERR_OR_NULL(gmu->gxpd)) {
pm_runtime_disable(gmu->gxpd);
dev_pm_domain_detach(gmu->gxpd, false);
@@ -1605,8 +1611,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
if (adreno_is_a650_family(adreno_gpu)) {
gmu->rscc = a6xx_gmu_get_mmio(pdev, "rscc");
- if (IS_ERR(gmu->rscc))
+ if (IS_ERR(gmu->rscc)) {
+ ret = -ENODEV;
goto err_mmio;
+ }
} else {
gmu->rscc = gmu->mmio + 0x23000;
}
@@ -1615,8 +1623,22 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
gmu->hfi_irq = a6xx_gmu_get_irq(gmu, pdev, "hfi", a6xx_hfi_irq);
gmu->gmu_irq = a6xx_gmu_get_irq(gmu, pdev, "gmu", a6xx_gmu_irq);
- if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0)
+ if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0) {
+ ret = -ENODEV;
+ goto err_mmio;
+ }
+
+ gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx");
+ if (IS_ERR(gmu->cxpd)) {
+ ret = PTR_ERR(gmu->cxpd);
goto err_mmio;
+ }
+
+ if (!device_link_add(gmu->dev, gmu->cxpd,
+ DL_FLAG_PM_RUNTIME)) {
+ ret = -ENODEV;
+ goto detach_cxpd;
+ }
/*
* Get a link to the GX power domain to reset the GPU in case of GMU
@@ -1634,6 +1656,9 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
return 0;
+detach_cxpd:
+ dev_pm_domain_detach(gmu->cxpd, false);
+
err_mmio:
iounmap(gmu->mmio);
if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "rscc"))
@@ -1641,8 +1666,6 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
free_irq(gmu->gmu_irq, gmu);
free_irq(gmu->hfi_irq, gmu);
- ret = -ENODEV;
-
err_memory:
a6xx_gmu_memory_free(gmu);
err_put_device:
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index e034935b3986..5a42dd4dd31f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -56,6 +56,7 @@ struct a6xx_gmu {
int gmu_irq;
struct device *gxpd;
+ struct device *cxpd;
int idle_level;
--
2.7.4
[Freedreno] [PATCH v5 2/5] clk: qcom: gdsc: Support 'synced_poweroff' genpd flag
Add support for the newly added 'synced_poweroff' genpd flag. This allows
some clients (like adreno gpu driver) to request gdsc driver to ensure
a votable gdsc (like gpucc cx gdsc) has collapsed at hardware.
Signed-off-by: Akhil P Oommen
Reviewed-by: Ulf Hansson
---
(no changes since v3)
Changes in v3:
- Rename the var 'force_sync' to 'wait (Stephen)
drivers/clk/qcom/gdsc.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 9e4d6ce891aa..5358e28122ab 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -136,7 +136,8 @@ static int gdsc_update_collapse_bit(struct gdsc *sc, bool
val)
return 0;
}
-static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
+static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
+ bool wait)
{
int ret;
@@ -149,7 +150,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum
gdsc_status status)
ret = gdsc_update_collapse_bit(sc, status == GDSC_OFF);
/* If disabling votable gdscs, don't poll on status */
- if ((sc->flags & VOTABLE) && status == GDSC_OFF) {
+ if ((sc->flags & VOTABLE) && status == GDSC_OFF && !wait) {
/*
* Add a short delay here to ensure that an enable
* right after it was disabled does not put it in an
@@ -275,7 +276,7 @@ static int gdsc_enable(struct generic_pm_domain *domain)
gdsc_deassert_clamp_io(sc);
}
- ret = gdsc_toggle_logic(sc, GDSC_ON);
+ ret = gdsc_toggle_logic(sc, GDSC_ON, false);
if (ret)
return ret;
@@ -352,7 +353,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
if (sc->pwrsts == PWRSTS_RET_ON)
return 0;
- ret = gdsc_toggle_logic(sc, GDSC_OFF);
+ ret = gdsc_toggle_logic(sc, GDSC_OFF, domain->synced_poweroff);
if (ret)
return ret;
@@ -392,7 +393,7 @@ static int gdsc_init(struct gdsc *sc)
/* Force gdsc ON if only ON state is supported */
if (sc->pwrsts == PWRSTS_ON) {
- ret = gdsc_toggle_logic(sc, GDSC_ON);
+ ret = gdsc_toggle_logic(sc, GDSC_ON, false);
if (ret)
return ret;
}
--
2.7.4
[Freedreno] [PATCH v5 1/5] PM: domains: Allow a genpd consumer to require a synced power off
From: Ulf Hansson
Some genpd providers doesn't ensure that it has turned off at hardware.
This is fine until the consumer really requires during some special
scenarios that the power domain collapse at hardware before it is
turned ON again.
An example is the reset sequence of Adreno GPU which requires that the
'gpucc cx gdsc' power domain should move to OFF state in hardware at
least once before turning in ON again to clear the internal state.
Signed-off-by: Ulf Hansson
Signed-off-by: Akhil P Oommen
Reviewed-by: Bjorn Andersson
---
(no changes since v4)
Changes in v4:
- Update genpd function documentation (Ulf)
Changes in v2:
- Minor formatting fix
drivers/base/power/domain.c | 26 ++
include/linux/pm_domain.h | 5 +
2 files changed, 31 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 967bcf9d415e..84662d338188 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -519,6 +519,31 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_genpd_get_next_hrtimer);
+/*
+ * dev_pm_genpd_synced_poweroff - Next power off should be synchronous
+ *
+ * @dev: A device that is attached to the genpd.
+ *
+ * Allows a consumer of the genpd to notify the provider that the next power
off
+ * should be synchronous.
+ *
+ * It is assumed that the users guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ */
+void dev_pm_genpd_synced_poweroff(struct device *dev)
+{
+ struct generic_pm_domain *genpd;
+
+ genpd = dev_to_genpd_safe(dev);
+ if (!genpd)
+ return;
+
+ genpd_lock(genpd);
+ genpd->synced_poweroff = true;
+ genpd_unlock(genpd);
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
+
static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
{
unsigned int state_idx = genpd->state_idx;
@@ -562,6 +587,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd,
bool timed)
out:
raw_notifier_call_chain(>power_notifiers, GENPD_NOTIFY_ON, NULL);
+ genpd->synced_poweroff = false;
return 0;
err:
raw_notifier_call_chain(>power_notifiers, GENPD_NOTIFY_OFF,
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 1cd41bdf73cf..f776fb93eaa0 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -136,6 +136,7 @@ struct generic_pm_domain {
unsigned int prepared_count;/* Suspend counter of prepared devices
*/
unsigned int performance_state; /* Aggregated max performance state */
cpumask_var_t cpus; /* A cpumask of the attached CPUs */
+ bool synced_poweroff; /* A consumer needs a synced poweroff */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
struct raw_notifier_head power_notifiers; /* Power on/off notifiers */
@@ -235,6 +236,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct
notifier_block *nb);
int dev_pm_genpd_remove_notifier(struct device *dev);
void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
+void dev_pm_genpd_synced_poweroff(struct device *dev);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
@@ -300,6 +302,9 @@ static inline ktime_t dev_pm_genpd_get_next_hrtimer(struct
device *dev)
{
return KTIME_MAX;
}
+static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
+{ }
+
#define simple_qos_governor(*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov(*(struct dev_power_governor
*)(NULL))
#endif
--
2.7.4
[Freedreno] [PATCH v5 0/5] Improve GPU reset sequence for Adreno GPU
This is a rework of [1] using genpd instead of 'reset' framework. As per the recommended reset sequence of Adreno gpu, we should ensure that gpucc-cx-gdsc has collapsed at hardware to reset gpu's internal hardware states. Because this gdsc is implemented as 'votable', gdsc driver doesn't poll and wait until its hw status says OFF. So use the newly introduced genpd api (dev_pm_genpd_synced_poweroff()) to provide a hint to the gdsc driver to poll for the hw status and use genpd notifier to wait from adreno gpu driver until gdsc is turned OFF. This series is rebased on top of linux-next (20221215) since the changes span multiple drivers. [1] https://patchwork.freedesktop.org/series/107507/ Changes in v5: - Capture all Reviewed-by tags Changes in v4: - Update genpd function documentation (Ulf) Changes in v3: - Rename the var 'force_sync' to 'wait (Stephen) Changes in v2: - Minor formatting fix - Select PM_GENERIC_DOMAINS from Kconfig Akhil P Oommen (4): clk: qcom: gdsc: Support 'synced_poweroff' genpd flag drm/msm/a6xx: Vote for cx gdsc from gpu driver drm/msm/a6xx: Remove cx gdsc polling using 'reset' drm/msm/a6xx: Use genpd notifier to ensure cx-gdsc collapse Ulf Hansson (1): PM: domains: Allow a genpd consumer to require a synced power off drivers/base/power/domain.c | 26 drivers/clk/qcom/gdsc.c | 11 + drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 46 --- drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 7 ++ drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 13 +++--- drivers/gpu/drm/msm/msm_gpu.c | 4 --- drivers/gpu/drm/msm/msm_gpu.h | 4 --- include/linux/pm_domain.h | 5 9 files changed, 97 insertions(+), 20 deletions(-) -- 2.7.4
Re: [Freedreno] [RFC PATCH 5/7] drm/msm/dpu: Document and enable TEAR interrupts on DSI interfaces
On 2023-01-01 15:12:35, Dmitry Baryshkov wrote:
> On 31/12/2022 23:50, Marijn Suijten wrote:
> >
> > -#define INTF_BLK(_name, _id, _base, _type, _ctrl_id, _progfetch,
> > _features, _reg, _underrun_bit, _vsync_bit) \
> > +#define INTF_BLK(_name, _id, _base, _len, _type, _ctrl_id, _progfetch,
> > _features, _reg, _underrun_bit, _vsync_bit, _tear_reg, _tear_rd_ptr_bit) \
> > {\
> > .name = _name, .id = _id, \
> > - .base = _base, .len = 0x280, \
> > + .base = _base, .len = _len, \
>
> Please move .len setting to a separate patch, it is not direclty related
> to tear interrupt addition.
It is directly related in that the TE registers reside in the extra
space beyond 0x280, but I can surely make that explicit in a separate
patch.
> > .features = _features, \
> > .type = _type, \
> > .controller_id = _ctrl_id, \
> > .prog_fetch_lines_worst_case = _progfetch, \
> > .intr_underrun = DPU_IRQ_IDX(_reg, _underrun_bit), \
> > .intr_vsync = DPU_IRQ_IDX(_reg, _vsync_bit), \
> > + .intr_tear_rd_ptr = DPU_IRQ_IDX(_tear_reg, _tear_rd_ptr_bit), \
>
> Initially I added separate _reg and _bit settings because reg was common
> to both interrupts. However now as tear interrups use different reg it
> might be better to first move DPU_IRQ_IDX out of this macro () and then
> to add your tear_rd_ptr_intr as a single intr_idx.
I assumed as much; then we do get the duplication of _reg but I guess
it's not too bad if the lines are nicely wrapped like in _pp[]. I'll do
so in a separate patch.
- Marijn
Re: [Freedreno] [RFC PATCH 2/7] drm/msm/dpu: Disable pingpong TE on DPU 5.0.0 and above
On 2023-01-01 06:28:23, Dmitry Baryshkov wrote:
> On 31/12/2022 23:50, Marijn Suijten wrote:
> > Since hardware revision 5.0.0 the TE configuration moved out of the
> > PINGPONG block into the INTF block. Writing these registers has no
> > effect, and is omitted downstream via the DPU/SDE_PINGPONG_TE feature
> > flag. This flag is only added to PINGPONG blocks used by hardware prior
> > to 5.0.0.
> >
> > The code that writes to these registers in the INTF block will follow in
> > subsequent patches.
> >
> > Signed-off-by: Marijn Suijten
> > ---
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +-
> > .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 53 +++
> > .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 18 ---
> > 3 files changed, 44 insertions(+), 32 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 ae28b2b93e69..7e5ba52197cd 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
> > @@ -582,7 +582,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
> > {
> > struct dpu_hw_pp_vsync_info info;
> >
> > - if (!phys_enc)
> > + if (!phys_enc || !phys_enc->hw_pp->ops.get_vsync_info)
> > return false;
> >
> > phys_enc->hw_pp->ops.get_vsync_info(phys_enc->hw_pp, );
> > @@ -607,6 +607,9 @@ static void dpu_encoder_phys_cmd_prepare_commit(
>
> This function works only with the hw_pp and if I'm not mistaken it
> becomes void for newer platforms. Please consider moving completely to
> the dpu_hw_pp.c Then we'd have a single optional callback instead of
> having a pile of them.
It also works for hw_intf, which I'm introducing in a later patch. This
change is just cleaning up the fact that these are the only callbacks
(on hw_pp->ops) that weren't considered optional yet.
Even though removing these writes should not have any effect, perhaps it
is more clear to insert this patch /after/ introducing INTF TE? Then
that patch will likely already include the change that makes this error
checking consistent for both variants, as it currently has:
/* If autorefresh is already disabled, we have nothing to do */
if (phys_enc->has_intf_te) {
if (!phys_enc->hw_intf ||
!phys_enc->hw_intf->ops.get_autorefresh ||
!phys_enc->hw_intf->ops.setup_autorefresh)
return;
if (!phys_enc->hw_intf->ops.get_autorefresh(phys_enc->hw_intf,
NULL))
return;
} else {
if (!phys_enc->hw_pp || !phys_enc->hw_pp->ops.get_autorefresh ||
!phys_enc->hw_pp->ops.setup_autorefresh)
return;
if (!phys_enc->hw_pp->ops.get_autorefresh(phys_enc->hw_pp,
NULL))
return;
}
- Marijn
> > if (!dpu_encoder_phys_cmd_is_master(phys_enc))
> > return;
> >
> > + if (!phys_enc->hw_pp->ops.get_autorefresh ||
> > !phys_enc->hw_pp->ops.setup_autorefresh)
> > + return;
> > +
> > /* If autorefresh is already disabled, we have nothing to do */
> > if (!phys_enc->hw_pp->ops.get_autorefresh(phys_enc->hw_pp, NULL))
> > return;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > index 9814ad52cc04..39d4b293710c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > @@ -59,11 +59,18 @@
> > #define MIXER_SC7180_MASK \
> > (BIT(DPU_DIM_LAYER) | BIT(DPU_MIXER_COMBINED_ALPHA))
> >
> > -#define PINGPONG_SDM845_MASK BIT(DPU_PINGPONG_DITHER)
> > +#define PINGPONG_SDM845_MASK \
> > + (BIT(DPU_PINGPONG_DITHER) | BIT(DPU_PINGPONG_TE))
> >
> > -#define PINGPONG_SDM845_SPLIT_MASK \
> > +#define PINGPONG_SDM845_TE2_MASK \
> > (PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
> >
> > +#define PINGPONG_SM8150_MASK \
> > + (BIT(DPU_PINGPONG_DITHER))
> > +
> > +#define PINGPONG_SM8150_TE2_MASK \
> > + (PINGPONG_SM8150_MASK | BIT(DPU_PINGPONG_TE2))
> > +
> > #define CTL_SC7280_MASK \
> > (BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE) |
> > BIT(DPU_CTL_VM_CFG))
> >
> > @@ -1156,21 +1163,21 @@ static const struct dpu_pingpong_sub_blks
> > sc7280_pp_sblk = {
> > .len = 0x20, .version = 0x2},
> > };
> >
> > -#define PP_BLK_TE(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
> > +#define PP_BLK_TE(_name, _id, _base, _features, _merge_3d, _sblk, _done,
> > _rdptr) \
> > {\
> > .name = _name, .id = _id, \
> > .base = _base, .len = 0xd4, \
> > - .features = PINGPONG_SDM845_SPLIT_MASK, \
> > + .features = _features, \
> > .merge_3d = _merge_3d, \
> > .sblk = &_sblk, \
> > .intr_done = _done, \
> > .intr_rdptr = _rdptr, \
Re: [Freedreno] [RFC PATCH 3/7] drm/msm/dpu: Disable MDP vsync source selection on DPU 5.0.0 and above
On 2.01.2023 11:18, Marijn Suijten wrote:
> On 2023-01-02 10:30:58, Konrad Dybcio wrote:
>>
>>
>> On 31.12.2022 22:52, Marijn Suijten wrote:
>>> On 2022-12-31 22:50:02, Marijn Suijten wrote:
Since hardware revision 5.0.0 the TE configuration moved out of the
PINGPONG block into the INTF block, including vsync source selection
that was previously part of MDP top. Writing to the MDP_VSYNC_SEL
register has no effect anymore and is omitted downstream via the
DPU/SDE_MDP_VSYNC_SEL feature flag. This flag is only added to INTF
blocks used by hardware prior to 5.0.0.
The code that writes to these registers in the INTF block will follow in
subsequent patches.
Signed-off-by: Marijn Suijten
---
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 33 ++--
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c| 52 +--
3 files changed, 66 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 39d4b293710c..1cfe94494135 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -407,7 +407,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = {
{
.name = "top_0", .id = MDP_TOP,
.base = 0x0, .len = 0x458,
- .features = 0,
+ .features = BIT(DPU_MDP_VSYNC_SEL),
.highest_bank_bit = 0x2,
.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
.reg_off = 0x2AC, .bit_off = 0},
@@ -436,7 +436,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = {
{
.name = "top_0", .id = MDP_TOP,
.base = 0x0, .len = 0x45C,
- .features = BIT(DPU_MDP_AUDIO_SELECT),
+ .features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL),
.highest_bank_bit = 0x2,
.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
.reg_off = 0x2AC, .bit_off = 0},
@@ -512,6 +512,31 @@ static const struct dpu_mdp_cfg sm6115_mdp[] = {
},
};
+static const struct dpu_mdp_cfg sdm8150_mdp[] = {
>>>
>>> Sometimes it is only possible to spot such things _after_ sending,
>>> probably the thing that makes us human :)
>>>
>>> sm8150_mdp*, not sdm.
>>>
>>> - Marijn
>>>
+ {
+ .name = "top_0", .id = MDP_TOP,
+ .base = 0x0, .len = 0x45C,
+ .features = BIT(DPU_MDP_AUDIO_SELECT),
+ .highest_bank_bit = 0x2,
+ .clk_ctrls[DPU_CLK_CTRL_VIG0] = {
+ .reg_off = 0x2AC, .bit_off = 0},
>> Keeping the hex values lowercase would be nice.
>
> Ack, this was copied verbatim from sdm845_mdp but will cleanup as we go.
> Looks like this file has a mixed approach towards lower and uppercase,
> when does the normalization patch land?
Rob was against changing everything in one commit, as that would mess
with git blame, so we settled on preventing adding new uppercase hex
for now (outside of #defines ofc).
Konrad
>
> - Marijn
Re: [Freedreno] [RFC PATCH 3/7] drm/msm/dpu: Disable MDP vsync source selection on DPU 5.0.0 and above
On 2023-01-02 10:30:58, Konrad Dybcio wrote:
>
>
> On 31.12.2022 22:52, Marijn Suijten wrote:
> > On 2022-12-31 22:50:02, Marijn Suijten wrote:
> >> Since hardware revision 5.0.0 the TE configuration moved out of the
> >> PINGPONG block into the INTF block, including vsync source selection
> >> that was previously part of MDP top. Writing to the MDP_VSYNC_SEL
> >> register has no effect anymore and is omitted downstream via the
> >> DPU/SDE_MDP_VSYNC_SEL feature flag. This flag is only added to INTF
> >> blocks used by hardware prior to 5.0.0.
> >>
> >> The code that writes to these registers in the INTF block will follow in
> >> subsequent patches.
> >>
> >> Signed-off-by: Marijn Suijten
> >> ---
> >> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 33 ++--
> >> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 1 +
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c| 52 +--
> >> 3 files changed, 66 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> index 39d4b293710c..1cfe94494135 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> @@ -407,7 +407,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = {
> >>{
> >>.name = "top_0", .id = MDP_TOP,
> >>.base = 0x0, .len = 0x458,
> >> - .features = 0,
> >> + .features = BIT(DPU_MDP_VSYNC_SEL),
> >>.highest_bank_bit = 0x2,
> >>.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> >>.reg_off = 0x2AC, .bit_off = 0},
> >> @@ -436,7 +436,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = {
> >>{
> >>.name = "top_0", .id = MDP_TOP,
> >>.base = 0x0, .len = 0x45C,
> >> - .features = BIT(DPU_MDP_AUDIO_SELECT),
> >> + .features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL),
> >>.highest_bank_bit = 0x2,
> >>.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> >>.reg_off = 0x2AC, .bit_off = 0},
> >> @@ -512,6 +512,31 @@ static const struct dpu_mdp_cfg sm6115_mdp[] = {
> >>},
> >> };
> >>
> >> +static const struct dpu_mdp_cfg sdm8150_mdp[] = {
> >
> > Sometimes it is only possible to spot such things _after_ sending,
> > probably the thing that makes us human :)
> >
> > sm8150_mdp*, not sdm.
> >
> > - Marijn
> >
> >> + {
> >> + .name = "top_0", .id = MDP_TOP,
> >> + .base = 0x0, .len = 0x45C,
> >> + .features = BIT(DPU_MDP_AUDIO_SELECT),
> >> + .highest_bank_bit = 0x2,
> >> + .clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> >> + .reg_off = 0x2AC, .bit_off = 0},
> Keeping the hex values lowercase would be nice.
Ack, this was copied verbatim from sdm845_mdp but will cleanup as we go.
Looks like this file has a mixed approach towards lower and uppercase,
when does the normalization patch land?
- Marijn
Re: [Freedreno] [RFC PATCH 1/7] drm/msm/dpu: Remove unused INTF0 interrupt mask from sm6115/qcm2290
On 2023-01-02 10:29:03, Konrad Dybcio wrote: > > > On 31.12.2022 22:50, Marijn Suijten wrote: > > Neither of these SoCs has INTF0, they only have a DSI interface on index > > 1. Stop enabling an interrupt that can't fire. > Double space. In case you hadn't noticed I'm employing this habit for quite some time now: after ending a sentence with a period, use a double space. The likes of GKH do it too. It may look a bit off though with 1. at the beginning of the sentence resembling the start of an ordered list. - Marijn > Reviewed-by: Konrad Dybcio > Konrad
[Freedreno] [PATCH v2] drm/msm/adreno: Make adreno quirks not overwrite each other
So far the adreno quirks have all been assigned with an OR operator,
which is problematic, because they were assigned consecutive integer
values, which makes checking them with an AND operator kind of no bueno..
Switch to using BIT(n) so that only the quirks that the programmer chose
are taken into account when evaluating info->quirks & ADRENO_QUIRK_...
Fixes: 370063ee427a ("drm/msm/adreno: Add A540 support")
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Marijn Suijten
Reviewed-by: Rob Clark
Signed-off-by: Konrad Dybcio
---
v1 -> v2:
- pick up tags
- correct the Fixes: tag
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index c85857c0a228..5eb254c9832a 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -29,11 +29,9 @@ enum {
ADRENO_FW_MAX,
};
-enum adreno_quirks {
- ADRENO_QUIRK_TWO_PASS_USE_WFI = 1,
- ADRENO_QUIRK_FAULT_DETECT_MASK = 2,
- ADRENO_QUIRK_LMLOADKILL_DISABLE = 3,
-};
+#define ADRENO_QUIRK_TWO_PASS_USE_WFI BIT(0)
+#define ADRENO_QUIRK_FAULT_DETECT_MASK BIT(1)
+#define ADRENO_QUIRK_LMLOADKILL_DISABLEBIT(2)
struct adreno_rev {
uint8_t core;
@@ -65,7 +63,7 @@ struct adreno_info {
const char *name;
const char *fw[ADRENO_FW_MAX];
uint32_t gmem;
- enum adreno_quirks quirks;
+ u64 quirks;
struct msm_gpu *(*init)(struct drm_device *dev);
const char *zapfw;
u32 inactive_period;
--
2.39.0
Re: [Freedreno] [PATCH v4 1/5] PM: domains: Allow a genpd consumer to require a synced power off
On 12/29/2022 12:13 AM, Bjorn Andersson wrote:
> On Wed, Dec 21, 2022 at 10:43:59PM +0530, Akhil P Oommen wrote:
>> From: Ulf Hansson
>>
>> Some genpd providers doesn't ensure that it has turned off at hardware.
>> This is fine until the consumer really requires during some special
>> scenarios that the power domain collapse at hardware before it is
>> turned ON again.
>>
>> An example is the reset sequence of Adreno GPU which requires that the
>> 'gpucc cx gdsc' power domain should move to OFF state in hardware at
>> least once before turning in ON again to clear the internal state.
>>
>> Signed-off-by: Ulf Hansson
>> Signed-off-by: Akhil P Oommen
> Reviewed-by: Bjorn Andersson
>
> @Rafael, would you be willing to share an immutable branch with this
> change? Or would you be okay with me doing so from the qcom clock tree?
>
> Regards,
> Bjorn
Rafael, gentle ping. Could you please check Bjorn's question here?
-Akhil.
>
>> ---
>>
>> Changes in v4:
>> - Update genpd function documentation (Ulf)
>>
>> Changes in v2:
>> - Minor formatting fix
>>
>> drivers/base/power/domain.c | 26 ++
>> include/linux/pm_domain.h | 5 +
>> 2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 967bcf9d415e..84662d338188 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -519,6 +519,31 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device
>> *dev)
>> }
>> EXPORT_SYMBOL_GPL(dev_pm_genpd_get_next_hrtimer);
>>
>> +/*
>> + * dev_pm_genpd_synced_poweroff - Next power off should be synchronous
>> + *
>> + * @dev: A device that is attached to the genpd.
>> + *
>> + * Allows a consumer of the genpd to notify the provider that the next
>> power off
>> + * should be synchronous.
>> + *
>> + * It is assumed that the users guarantee that the genpd wouldn't be
>> detached
>> + * while this routine is getting called.
>> + */
>> +void dev_pm_genpd_synced_poweroff(struct device *dev)
>> +{
>> +struct generic_pm_domain *genpd;
>> +
>> +genpd = dev_to_genpd_safe(dev);
>> +if (!genpd)
>> +return;
>> +
>> +genpd_lock(genpd);
>> +genpd->synced_poweroff = true;
>> +genpd_unlock(genpd);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
>> +
>> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>> {
>> unsigned int state_idx = genpd->state_idx;
>> @@ -562,6 +587,7 @@ static int _genpd_power_on(struct generic_pm_domain
>> *genpd, bool timed)
>>
>> out:
>> raw_notifier_call_chain(>power_notifiers, GENPD_NOTIFY_ON, NULL);
>> +genpd->synced_poweroff = false;
>> return 0;
>> err:
>> raw_notifier_call_chain(>power_notifiers, GENPD_NOTIFY_OFF,
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 1cd41bdf73cf..f776fb93eaa0 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -136,6 +136,7 @@ struct generic_pm_domain {
>> unsigned int prepared_count;/* Suspend counter of prepared devices
>> */
>> unsigned int performance_state; /* Aggregated max performance state */
>> cpumask_var_t cpus; /* A cpumask of the attached CPUs */
>> +bool synced_poweroff; /* A consumer needs a synced poweroff */
>> int (*power_off)(struct generic_pm_domain *domain);
>> int (*power_on)(struct generic_pm_domain *domain);
>> struct raw_notifier_head power_notifiers; /* Power on/off notifiers */
>> @@ -235,6 +236,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct
>> notifier_block *nb);
>> int dev_pm_genpd_remove_notifier(struct device *dev);
>> void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
>> ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
>> +void dev_pm_genpd_synced_poweroff(struct device *dev);
>>
>> extern struct dev_power_governor simple_qos_governor;
>> extern struct dev_power_governor pm_domain_always_on_gov;
>> @@ -300,6 +302,9 @@ static inline ktime_t
>> dev_pm_genpd_get_next_hrtimer(struct device *dev)
>> {
>> return KTIME_MAX;
>> }
>> +static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
>> +{ }
>> +
>> #define simple_qos_governor (*(struct dev_power_governor *)(NULL))
>> #define pm_domain_always_on_gov (*(struct dev_power_governor
>> *)(NULL))
>> #endif
>> --
>> 2.7.4
>>
Re: [Freedreno] [RFC PATCH 3/7] drm/msm/dpu: Disable MDP vsync source selection on DPU 5.0.0 and above
On 31.12.2022 22:52, Marijn Suijten wrote:
> On 2022-12-31 22:50:02, Marijn Suijten wrote:
>> Since hardware revision 5.0.0 the TE configuration moved out of the
>> PINGPONG block into the INTF block, including vsync source selection
>> that was previously part of MDP top. Writing to the MDP_VSYNC_SEL
>> register has no effect anymore and is omitted downstream via the
>> DPU/SDE_MDP_VSYNC_SEL feature flag. This flag is only added to INTF
>> blocks used by hardware prior to 5.0.0.
>>
>> The code that writes to these registers in the INTF block will follow in
>> subsequent patches.
>>
>> Signed-off-by: Marijn Suijten
>> ---
>> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 33 ++--
>> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 1 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c| 52 +--
>> 3 files changed, 66 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> index 39d4b293710c..1cfe94494135 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -407,7 +407,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = {
>> {
>> .name = "top_0", .id = MDP_TOP,
>> .base = 0x0, .len = 0x458,
>> -.features = 0,
>> +.features = BIT(DPU_MDP_VSYNC_SEL),
>> .highest_bank_bit = 0x2,
>> .clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>> .reg_off = 0x2AC, .bit_off = 0},
>> @@ -436,7 +436,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = {
>> {
>> .name = "top_0", .id = MDP_TOP,
>> .base = 0x0, .len = 0x45C,
>> -.features = BIT(DPU_MDP_AUDIO_SELECT),
>> +.features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL),
>> .highest_bank_bit = 0x2,
>> .clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>> .reg_off = 0x2AC, .bit_off = 0},
>> @@ -512,6 +512,31 @@ static const struct dpu_mdp_cfg sm6115_mdp[] = {
>> },
>> };
>>
>> +static const struct dpu_mdp_cfg sdm8150_mdp[] = {
>
> Sometimes it is only possible to spot such things _after_ sending,
> probably the thing that makes us human :)
>
> sm8150_mdp*, not sdm.
>
> - Marijn
>
>> +{
>> +.name = "top_0", .id = MDP_TOP,
>> +.base = 0x0, .len = 0x45C,
>> +.features = BIT(DPU_MDP_AUDIO_SELECT),
>> +.highest_bank_bit = 0x2,
>> +.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>> +.reg_off = 0x2AC, .bit_off = 0},
Keeping the hex values lowercase would be nice.
Konrad
>> +.clk_ctrls[DPU_CLK_CTRL_VIG1] = {
>> +.reg_off = 0x2B4, .bit_off = 0},
>> +.clk_ctrls[DPU_CLK_CTRL_VIG2] = {
>> +.reg_off = 0x2BC, .bit_off = 0},
>> +.clk_ctrls[DPU_CLK_CTRL_VIG3] = {
>> +.reg_off = 0x2C4, .bit_off = 0},
>> +.clk_ctrls[DPU_CLK_CTRL_DMA0] = {
>> +.reg_off = 0x2AC, .bit_off = 8},
>> +.clk_ctrls[DPU_CLK_CTRL_DMA1] = {
>> +.reg_off = 0x2B4, .bit_off = 8},
>> +.clk_ctrls[DPU_CLK_CTRL_CURSOR0] = {
>> +.reg_off = 0x2BC, .bit_off = 8},
>> +.clk_ctrls[DPU_CLK_CTRL_CURSOR1] = {
>> +.reg_off = 0x2C4, .bit_off = 8},
>> +},
>> +};
>> +
>> static const struct dpu_mdp_cfg sm8250_mdp[] = {
>> {
>> .name = "top_0", .id = MDP_TOP,
>> @@ -1901,8 +1926,8 @@ static const struct dpu_mdss_cfg sm6115_dpu_cfg = {
>>
>> static const struct dpu_mdss_cfg sm8150_dpu_cfg = {
>> .caps = _dpu_caps,
>> -.mdp_count = ARRAY_SIZE(sdm845_mdp),
>> -.mdp = sdm845_mdp,
>> +.mdp_count = ARRAY_SIZE(sdm8150_mdp),
>> +.mdp = sdm8150_mdp,
>> .ctl_count = ARRAY_SIZE(sm8150_ctl),
>> .ctl = sm8150_ctl,
>> .sspp_count = ARRAY_SIZE(sdm845_sspp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index 3b645d5aa9aa..e0e153889ab7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -93,6 +93,7 @@ enum {
>> DPU_MDP_UBWC_1_0,
>> DPU_MDP_UBWC_1_5,
>> DPU_MDP_AUDIO_SELECT,
>> +DPU_MDP_VSYNC_SEL,
>> DPU_MDP_MAX
>> };
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> index c3110a25a30d..2e699c9ad13c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> @@ -151,28 +151,16 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp
>> *mdp,
>> status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3;
>> }
>>
>> -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
>> +static void dpu_hw_setup_vsync_source_v1(struct dpu_hw_mdp *mdp,
>> struct dpu_vsync_source_cfg *cfg)
>> {
>> struct dpu_hw_blk_reg_map *c;
>> -u32 reg, wd_load_value, wd_ctl, wd_ctl2, i;
>> -static const u32 pp_offset[PINGPONG_MAX]
Re: [Freedreno] [RFC PATCH 1/7] drm/msm/dpu: Remove unused INTF0 interrupt mask from sm6115/qcm2290
On 31.12.2022 22:50, Marijn Suijten wrote:
> Neither of these SoCs has INTF0, they only have a DSI interface on index
> 1. Stop enabling an interrupt that can't fire.
Double space.
Reviewed-by: Konrad Dybcio
Konrad
>
> Fixes: 3581b7062cec ("drm/msm/disp/dpu1: add support for display on SM6115")
> Fixes: 5334087ee743 ("drm/msm: add support for QCM2290 MDSS")
> Signed-off-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index 2196e205efa5..9814ad52cc04 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -90,6 +90,11 @@
>BIT(MDP_AD4_0_INTR) | \
>BIT(MDP_AD4_1_INTR))
>
> +#define IRQ_QCM2290_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
> + BIT(MDP_SSPP_TOP0_INTR2) | \
> + BIT(MDP_SSPP_TOP0_HIST_INTR) | \
> + BIT(MDP_INTF1_INTR))
> +
> #define IRQ_SC7180_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
>BIT(MDP_SSPP_TOP0_INTR2) | \
>BIT(MDP_SSPP_TOP0_HIST_INTR) | \
> @@ -1884,7 +1889,7 @@ static const struct dpu_mdss_cfg sm6115_dpu_cfg = {
> .vbif_count = ARRAY_SIZE(sdm845_vbif),
> .vbif = sdm845_vbif,
> .perf = _perf_data,
> - .mdss_irqs = IRQ_SC7180_MASK,
> + .mdss_irqs = IRQ_QCM2290_MASK,
> };
>
> static const struct dpu_mdss_cfg sm8150_dpu_cfg = {
> @@ -2008,7 +2013,7 @@ static const struct dpu_mdss_cfg qcm2290_dpu_cfg = {
> .reg_dma_count = 1,
> .dma_cfg = _regdma,
> .perf = _perf_data,
> - .mdss_irqs = IRQ_SC7180_MASK,
> + .mdss_irqs = IRQ_QCM2290_MASK,
> };
>
> static const struct dpu_mdss_hw_cfg_handler cfg_handler[] = {
