Re: [PATCH] drm/msm/mdss: specify cfg bandwidth for SDM670
On Fri, Dec 15, 2023 at 03:32:22AM +0200, Dmitry Baryshkov wrote:
> Lower the requested CFG bus bandwidth for the SDM670 platform. The
> default value is 153600 kBps, which is twice as big as required by the
> platform according to the vendor kernel.
>
> Fixes: a55c8ff252d3 ("drm/msm/mdss: Handle the reg bus ICC path")
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/msm_mdss.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 455b2e3a0cdd..35423d10aafa 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -562,6 +562,7 @@ static const struct msm_mdss_data sdm670_data = {
> .ubwc_enc_version = UBWC_2_0,
> .ubwc_dec_version = UBWC_2_0,
> .highest_bank_bit = 1,
> + .reg_bus_bw = 76800,
This seems to be the bandwidth applied to the "cpu-cfg" path, but it is
not in the device tree yet and is not allowed by schema (for no
particular reason). In sdm670.dtsi, it would be defined as:
<_noc MASTER_AMPSS_M0 0 _noc SLAVE_DISPLAY_CFG 0>
Furthermore, I have not yet emailed the patches that I use to test the
display on SDM670, namely, the panel driver and device tree changes for
the Pixel 3a. Nevertheless, this does not break anything, even with the
interconnect path and everything needed to test.
Tested-by: Richard Acayan
> };
>
> static const struct msm_mdss_data sdm845_data = {
> --
> 2.39.2
>
Re: [PATCH v1] drm/msm/dp: correct configure Colorimetry Indicator Field at MISC0
On 1/10/2024 3:38 PM, Dmitry Baryshkov wrote:
On Wed, 10 Jan 2024 at 22:18, Kuogee Hsieh wrote:
MSA MISC0 bit 1 to 7 contains Colorimetry Indicator Field. At current
implementation, Colorimetry Indicator Field of MISC0 is not configured
correctly. This patch add support of RGB formats Colorimetry.
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
Also the commit message doesn't provide any details or what was incorrect.
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 5 ++--
drivers/gpu/drm/msm/dp/dp_link.c | 26 -
drivers/gpu/drm/msm/dp/dp_panel.c | 48 +++
drivers/gpu/drm/msm/dp/dp_panel.h | 2 ++
4 files changed, 73 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 77a8d93..2ef89fb 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2023, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights
reserved
*/
#define pr_fmt(fmt)"[drm-dp] %s: " fmt, __func__
@@ -172,7 +173,7 @@ static void dp_ctrl_configure_source_params(struct
dp_ctrl_private *ctrl)
tb = dp_link_get_test_bits_depth(ctrl->link,
ctrl->panel->dp_mode.bpp);
- cc = dp_link_get_colorimetry_config(ctrl->link);
+ cc = dp_panel_get_misc_colorimetry_val(ctrl->panel);
dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
dp_panel_timing_cfg(ctrl->panel);
}
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index 98427d4..21fa1a2 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
* Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights
reserved
*/
#define pr_fmt(fmt)"[drm-dp] %s: " fmt, __func__
@@ -12,6 +13,11 @@
#define DP_TEST_REQUEST_MASK 0x7F
+enum dynamic_range {
+ DP_DYNAMIC_RANGE_RGB_VESA,
+ DP_DYNAMIC_RANGE_RGB_CEA,
+};
+
enum audio_sample_rate {
AUDIO_SAMPLE_RATE_32_KHZ= 0x00,
AUDIO_SAMPLE_RATE_44_1_KHZ = 0x01,
@@ -1083,6 +1089,7 @@ int dp_link_process_request(struct dp_link *dp_link)
int dp_link_get_colorimetry_config(struct dp_link *dp_link)
{
u32 cc;
+ enum dynamic_range dr;
struct dp_link_private *link;
if (!dp_link) {
@@ -1092,14 +1099,21 @@ int dp_link_get_colorimetry_config(struct dp_link
*dp_link)
link = container_of(dp_link, struct dp_link_private, dp_link);
- /*
-* Unless a video pattern CTS test is ongoing, use RGB_VESA
-* Only RGB_VESA and RGB_CEA supported for now
-*/
+ /* unless a video pattern CTS test is ongoing, use CEA_VESA */
if (dp_link_is_video_pattern_requested(link))
- cc = link->dp_link.test_video.test_dyn_range;
+ dr = link->dp_link.test_video.test_dyn_range;
test_dyn_range has the value of (dpcd[DP_TEST_MISC0] &
DP_TEST_DYNAMIC_RANGE_CEA), so it can not be assigned to dr.
I don't feel like this has been tested.
yes, you are correct.
This code derived from down stream code.
I will fix it.
else
- cc = DP_TEST_DYNAMIC_RANGE_VESA;
+ dr = DP_DYNAMIC_RANGE_RGB_VESA;
+
+ /* Only RGB_VESA and RGB_CEA supported for now */
+ switch (dr) {
+ case DP_DYNAMIC_RANGE_RGB_CEA:
+ cc = BIT(2);
No undefined magic, please.
+ break;
+ case DP_DYNAMIC_RANGE_RGB_VESA:
+ default:
+ cc = 0;
+ }
return cc;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 127f6af..785bb59 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
* Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights
reserved
*/
#include "dp_panel.h"
@@ -386,6 +387,53 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel)
return 0;
}
+/*
+ * Mapper function which outputs colorimetry to be used for a
+ * given colorspace value when misc field of MSA is used to
+ * change the colorimetry. Currently only RGB formats have been
+ * added. This API will be extended to YUV once it's supported on DP.
+ */
+u8 dp_panel_get_misc_colorimetry_val(struct dp_panel *dp_panel)
+{
+ u8 colorimetry;
+ u32 colorspace;
+ u32
Re: [PATCH v1] drm/msm/dp: remove mdss_dp_test_bit_depth_to_bpc()
On 1/11/2024 9:14 AM, Kuogee Hsieh wrote: mdss_dp_test_bit_depth_to_bpc() can be replace by mdss_dp_test_bit_depth_to_bpp() / 3. Hence remove it. Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_debug.c | 2 +- drivers/gpu/drm/msm/dp/dp_link.h | 23 --- 2 files changed, 1 insertion(+), 24 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH v1] drm/msm/dp: remove mdss_dp_test_bit_depth_to_bpc()
On Thu, 11 Jan 2024 at 19:14, Kuogee Hsieh wrote: > > mdss_dp_test_bit_depth_to_bpc() can be replace by > mdss_dp_test_bit_depth_to_bpp() / 3. Hence remove it. > > Signed-off-by: Kuogee Hsieh > --- > drivers/gpu/drm/msm/dp/dp_debug.c | 2 +- > drivers/gpu/drm/msm/dp/dp_link.h | 23 --- > 2 files changed, 1 insertion(+), 24 deletions(-) Thank you! Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
[PATCH v1] drm/msm/dp: remove mdss_dp_test_bit_depth_to_bpc()
mdss_dp_test_bit_depth_to_bpc() can be replace by
mdss_dp_test_bit_depth_to_bpp() / 3. Hence remove it.
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_debug.c | 2 +-
drivers/gpu/drm/msm/dp/dp_link.h | 23 ---
2 files changed, 1 insertion(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
b/drivers/gpu/drm/msm/dp/dp_debug.c
index 3bba901..534079e 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -105,7 +105,7 @@ static int dp_test_data_show(struct seq_file *m, void *data)
seq_printf(m, "vdisplay: %d\n",
debug->link->test_video.test_v_height);
seq_printf(m, "bpc: %u\n",
- dp_link_bit_depth_to_bpc(bpc));
+ dp_link_bit_depth_to_bpp(bpc) / 3);
} else {
seq_puts(m, "0");
}
diff --git a/drivers/gpu/drm/msm/dp/dp_link.h b/drivers/gpu/drm/msm/dp/dp_link.h
index 9dd4dd9..83da170 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.h
+++ b/drivers/gpu/drm/msm/dp/dp_link.h
@@ -112,29 +112,6 @@ static inline u32 dp_link_bit_depth_to_bpp(u32 tbd)
}
}
-/**
- * dp_test_bit_depth_to_bpc() - convert test bit depth to bpc
- * @tbd: test bit depth
- *
- * Returns the bits per comp (bpc) to be used corresponding to the
- * bit depth value. This function assumes that bit depth has
- * already been validated.
- */
-static inline u32 dp_link_bit_depth_to_bpc(u32 tbd)
-{
- switch (tbd) {
- case DP_TEST_BIT_DEPTH_6:
- return 6;
- case DP_TEST_BIT_DEPTH_8:
- return 8;
- case DP_TEST_BIT_DEPTH_10:
- return 10;
- case DP_TEST_BIT_DEPTH_UNKNOWN:
- default:
- return 0;
- }
-}
-
void dp_link_reset_phy_params_vx_px(struct dp_link *dp_link);
u32 dp_link_get_test_bits_depth(struct dp_link *dp_link, u32 bpp);
int dp_link_process_request(struct dp_link *dp_link);
--
2.7.4
Re: [PATCH] Revert "drm/msm/gpu: Push gpu lock down past runpm"
On Wed, Jan 10, 2024 at 06:54:53AM -0800, Rob Clark wrote:
> On Wed, Jan 10, 2024 at 2:50 AM Daniel Vetter wrote:
> >
> > On Tue, Jan 09, 2024 at 10:22:17AM -0800, Rob Clark wrote:
> > > From: Rob Clark
> > >
> > > This reverts commit abe2023b4cea192ab266b351fd38dc9dbd846df0.
> > >
> > > Changing the locking order means that scheduler/msm_job_run() can race
> > > with the recovery kthread worker, with the result that the GPU gets an
> > > extra runpm get when we are trying to power it off. Leaving the GPU in
> > > an unrecovered state.
> >
> > The recovery kthread is supposed to stop all the relevant schedulers,
> > which should remove any possible race conditions. So unless there's more
> > going on, or you have your own recovery kthread (don't, reuse the one from
> > the scheduler with your own work items, that's why you can provide that)
> > this looks like an incomplete/incorrect explanation ... ?
> >
> > Slightly confused
>
> msm still uses it's own recovery, which pre-dates the scheduler
> conversion. At one point (a yr or two back?) I started looking at
> integrating recovery w/ scheduler.. at the time I think you talked me
> out of it, but I don't remember the reason
hm ... most scheduler discussions I remember was around the "allocate your
own workqueue and hand that to scheduler to avoid races/deadlocks". Which
iirc Boris implemented a while ago. Once you have that workqueue you can
then also process any other error condition on there with the exact same
locking design (like hw error or page faults or whatever), not just
drm/sched tdr.
I don't remember anything else that ever came up at least at a fundamental
level ...
So if that discussion was older than 78efe21b6f8e ("drm/sched: Allow using
a dedicated workqueue for the timeout/fault tdr") you should be covered.
Fingers crossed :-)
Meanwhile if you do not use drm/sched tdr at all then doing the exact same
design but just on your own workqueue should also work. The critical thing
is really only:
- have one single-thread workqueue for all gpu recover
- bracket each handler in there with drm_sched_stop/start for all affected
engines
No more races!
Cheers, Sima
>
> BR,
> -R
>
> > -Sima
> >
> > >
> > > I'll need to come up with a different scheme for appeasing lockdep.
> > >
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/gpu/drm/msm/msm_gpu.c| 11 +--
> > > drivers/gpu/drm/msm/msm_ringbuffer.c | 7 +--
> > > 2 files changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > > index 095390774f22..655002b21b0d 100644
> > > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > > @@ -751,12 +751,14 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct
> > > msm_gem_submit *submit)
> > > struct msm_ringbuffer *ring = submit->ring;
> > > unsigned long flags;
> > >
> > > - pm_runtime_get_sync(>pdev->dev);
> > > + WARN_ON(!mutex_is_locked(>lock));
> > >
> > > - mutex_lock(>lock);
> > > + pm_runtime_get_sync(>pdev->dev);
> > >
> > > msm_gpu_hw_init(gpu);
> > >
> > > + submit->seqno = submit->hw_fence->seqno;
> > > +
> > > update_sw_cntrs(gpu);
> > >
> > > /*
> > > @@ -781,11 +783,8 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct
> > > msm_gem_submit *submit)
> > > gpu->funcs->submit(gpu, submit);
> > > gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
> > >
> > > - hangcheck_timer_reset(gpu);
> > > -
> > > - mutex_unlock(>lock);
> > > -
> > > pm_runtime_put(>pdev->dev);
> > > + hangcheck_timer_reset(gpu);
> > > }
> > >
> > > /*
> > > diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c
> > > b/drivers/gpu/drm/msm/msm_ringbuffer.c
> > > index e0ed27739449..548f5266a7d3 100644
> > > --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
> > > +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
> > > @@ -21,8 +21,6 @@ static struct dma_fence *msm_job_run(struct
> > > drm_sched_job *job)
> > >
> > > msm_fence_init(submit->hw_fence, fctx);
> > >
> > > - submit->seqno = submit->hw_fence->seqno;
> > > -
> > > mutex_lock(>lru.lock);
> > >
> > > for (i = 0; i < submit->nr_bos; i++) {
> > > @@ -35,8 +33,13 @@ static struct dma_fence *msm_job_run(struct
> > > drm_sched_job *job)
> > >
> > > mutex_unlock(>lru.lock);
> > >
> > > + /* TODO move submit path over to using a per-ring lock.. */
> > > + mutex_lock(>lock);
> > > +
> > > msm_gpu_submit(gpu, submit);
> > >
> > > + mutex_unlock(>lock);
> > > +
> > > return dma_fence_get(submit->hw_fence);
> > > }
> > >
> > > --
> > > 2.43.0
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
