Re: [Freedreno] [PATCH] drm/msm/a6xx: fix for kernels without CONFIG_NVMEM
On 4/2/2021 3:19 AM, Rob Clark wrote:
On Thu, Apr 1, 2021 at 2:03 PM Dmitry Baryshkov
wrote:
On Thu, 1 Apr 2021 at 23:09, Rob Clark wrote:
On Mon, Feb 22, 2021 at 8:06 AM Rob Clark wrote:
On Mon, Feb 22, 2021 at 7:45 AM Akhil P Oommen wrote:
On 2/19/2021 9:30 PM, Rob Clark wrote:
On Fri, Feb 19, 2021 at 2:44 AM Akhil P Oommen wrote:
On 2/18/2021 9:41 PM, Rob Clark wrote:
On Thu, Feb 18, 2021 at 4:28 AM Akhil P Oommen wrote:
On 2/18/2021 2:05 AM, Jonathan Marek wrote:
On 2/17/21 3:18 PM, Rob Clark wrote:
On Wed, Feb 17, 2021 at 11:08 AM Jordan Crouse
wrote:
On Wed, Feb 17, 2021 at 07:14:16PM +0530, Akhil P Oommen wrote:
On 2/17/2021 8:36 AM, Rob Clark wrote:
On Tue, Feb 16, 2021 at 12:10 PM Jonathan Marek
wrote:
Ignore nvmem_cell_get() EOPNOTSUPP error in the same way as a
ENOENT error,
to fix the case where the kernel was compiled without CONFIG_NVMEM.
Fixes: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu")
Signed-off-by: Jonathan Marek
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index ba8e9d3cf0fe..7fe5d97606aa 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1356,10 +1356,10 @@ static int a6xx_set_supported_hw(struct
device *dev, struct a6xx_gpu *a6xx_gpu,
cell = nvmem_cell_get(dev, "speed_bin");
/*
-* -ENOENT means that the platform doesn't support
speedbin which is
-* fine
+* -ENOENT means no speed bin in device tree,
+* -EOPNOTSUPP means kernel was built without CONFIG_NVMEM
very minor nit, it would be nice to at least preserve the gist of the
"which is fine" (ie. some variation of "this is an optional thing and
things won't catch fire without it" ;-))
(which is, I believe, is true, hopefully Akhil could confirm.. if not
we should have a harder dependency on CONFIG_NVMEM..)
IIRC, if the gpu opp table in the DT uses the 'opp-supported-hw'
property,
we will see some error during boot up if we don't call
dev_pm_opp_set_supported_hw(). So calling "nvmem_cell_get(dev,
"speed_bin")"
is a way to test this.
If there is no other harm, we can put a hard dependency on
CONFIG_NVMEM.
I'm not sure if we want to go this far given the squishiness about
module
dependencies. As far as I know we are the only driver that uses this
seriously
on QCOM SoCs and this is only needed for certain targets. I don't
know if we
want to force every target to build NVMEM and QFPROM on our behalf.
But maybe
I'm just saying that because Kconfig dependencies tend to break my
brain (and
then Arnd has to send a patch to fix it).
Hmm, good point.. looks like CONFIG_NVMEM itself doesn't have any
other dependencies, so I suppose it wouldn't be the end of the world
to select that.. but I guess we don't want to require QFPROM
I guess at the end of the day, what is the failure mode if you have a
speed-bin device, but your kernel config misses QFPROM (and possibly
NVMEM)? If the result is just not having the highest clk rate(s)
Atleast on sc7180's gpu, using an unsupported FMAX breaks gmu. It won't
be very obvious what went wrong when this happens!
Ugg, ok..
I suppose we could select NVMEM, but not QFPROM, and then the case
where QFPROM is not enabled on platforms that have the speed-bin field
in DT will fail gracefully and all other platforms would continue on
happily?
BR,
-R
Sounds good to me.
You probably should do a quick test with NVMEM enabled but QFPROM
disabled to confirm my theory, but I *think* that should work
BR,
-R
I tried it on an sc7180 device. The suggested combo (CONFIG_NVMEM + no
CONFIG_QCOM_QFPROM) makes the gpu probe fail with error "failed to read
speed-bin. Some OPPs may not be supported by hardware". This is good
enough clue for the developer that he should fix the broken speedbin
detection.
Ok, great.. then sounds like selecting NVMEM is a good approach
btw, did anyone ever send a patch to select NVMEM? I'm not seeing one
but I could be overlooking something
I thought Jonathan would send it as the discussion was going on in his
patch. No problem, I will send it out. :)
-Akhil.
Judging by the amount of issues surrounding speed-bin, I might have a
bold suggestion to revert these patches for now and get them once all
the issues are sorted, so that we'd have a single working commit
instead of scattered patch series breaking git bisect, having bad
side-effects on non-sc7180 platforms, etc.
We do really need some pre-merge CI like we have on the mesa side of
things (and we at least have 845 devices in our CI farm, but it would
be useful to add more generations).. but other than the config issue,
I *think* this fixes the last of the speedbin fallout?
https://patchwork.freedesktop.org/patch/426538/?series=88558=1
Planning to include that in a -fixes pull req in the next day or
Re: [Freedreno] [PATCH v2] drm/msm: Drop mm_lock in scan loop
Hi, On Fri, Apr 2, 2021 at 2:08 PM Rob Clark wrote: > > From: Rob Clark > > lock_stat + mmm_donut[1] say that this reduces contention on mm_lock > significantly (~350x lower waittime-max, and ~100x lower waittime-avg) > > [1] > https://chromium.googlesource.com/chromiumos/platform/microbenchmarks/+/refs/heads/main/mmm_donut.py > > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/msm/msm_drv.h | 3 +- > drivers/gpu/drm/msm/msm_gem.c | 2 +- > drivers/gpu/drm/msm/msm_gem_shrinker.c | 48 ++ > 3 files changed, 45 insertions(+), 8 deletions(-) Looks good to me now! Reviewed-by: Douglas Anderson ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v2] drm/msm: Drop mm_lock in scan loop
From: Rob Clark
lock_stat + mmm_donut[1] say that this reduces contention on mm_lock
significantly (~350x lower waittime-max, and ~100x lower waittime-avg)
[1]
https://chromium.googlesource.com/chromiumos/platform/microbenchmarks/+/refs/heads/main/mmm_donut.py
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_drv.h | 3 +-
drivers/gpu/drm/msm/msm_gem.c | 2 +-
drivers/gpu/drm/msm/msm_gem_shrinker.c | 48 ++
3 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index c84e6f84cb6d..d8d64d34e6e3 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -184,7 +184,8 @@ struct msm_drm_private {
/**
* Lists of inactive GEM objects. Every bo is either in one of the
* inactive lists (depending on whether or not it is shrinkable) or
-* gpu->active_list (for the gpu it is active on[1])
+* gpu->active_list (for the gpu it is active on[1]), or transiently
+* on a temporary list as the shrinker is running.
*
* These lists are protected by mm_lock (which should be acquired
* before per GEM object lock). One should *not* hold mm_lock in
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 2ecf7f1cef25..75cea5b801da 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -719,7 +719,7 @@ void msm_gem_purge(struct drm_gem_object *obj)
put_iova_vmas(obj);
msm_obj->madv = __MSM_MADV_PURGED;
- mark_unpurgable(msm_obj);
+ update_inactive(msm_obj);
drm_vma_node_unmap(>vma_node, dev->anon_inode->i_mapping);
drm_gem_free_mmap_offset(obj);
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index f3e948af01c5..33a49641ef30 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -22,26 +22,62 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
shrink_control *sc)
{
struct msm_drm_private *priv =
container_of(shrinker, struct msm_drm_private, shrinker);
- struct msm_gem_object *msm_obj;
+ struct list_head still_in_list;
unsigned long freed = 0;
+ INIT_LIST_HEAD(_in_list);
+
mutex_lock(>mm_lock);
- list_for_each_entry(msm_obj, >inactive_dontneed, mm_list) {
- if (freed >= sc->nr_to_scan)
+ while (freed < sc->nr_to_scan) {
+ struct msm_gem_object *msm_obj = list_first_entry_or_null(
+ >inactive_dontneed, typeof(*msm_obj),
mm_list);
+
+ if (!msm_obj)
break;
- /* Use trylock, because we cannot block on a obj that
-* might be trying to acquire mm_lock
+
+ list_move_tail(_obj->mm_list, _in_list);
+
+ /*
+* If it is in the process of being freed, msm_gem_free_object
+* can be blocked on mm_lock waiting to remove it. So just
+* skip it.
*/
- if (!msm_gem_trylock(_obj->base))
+ if (!kref_get_unless_zero(_obj->base.refcount))
continue;
+
+ /*
+* Now that we own a reference, we can drop mm_lock for the
+* rest of the loop body, to reduce contention with the
+* retire_submit path (which could make more objects purgable)
+*/
+
+ mutex_unlock(>mm_lock);
+
+ /*
+* Note that this still needs to be trylock, since we can
+* hit shrinker in response to trying to get backing pages
+* for this obj (ie. while it's lock is already held)
+*/
+ if (!msm_gem_trylock(_obj->base))
+ goto tail;
+
if (is_purgeable(msm_obj)) {
+ /*
+* This will move the obj out of still_in_list to
+* the purged list
+*/
msm_gem_purge(_obj->base);
freed += msm_obj->base.size >> PAGE_SHIFT;
}
msm_gem_unlock(_obj->base);
+
+tail:
+ drm_gem_object_put(_obj->base);
+ mutex_lock(>mm_lock);
}
+ list_splice_tail(_in_list, >inactive_dontneed);
mutex_unlock(>mm_lock);
if (freed > 0) {
--
2.30.2
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [pull] drm/msm: drm-msm-fixes-2021-04-02 for v5.12-rc6
Hi Dave & Daniel, A couple more small fixes for v5.12 The following changes since commit 627dc55c273dab308303a5217bd3e767d7083ddb: drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume (2021-03-22 18:52:34 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git drm-msm-fixes-2021-04-02 for you to fetch changes up to 12aca1ce9ee33af3751aec5e55a5900747cbdd4b: drm/msm/disp/dpu1: program 3d_merge only if block is attached (2021-04-02 08:23:41 -0700) Dmitry Baryshkov (1): drm/msm: a6xx: fix version check for the A650 SQE microcode John Stultz (1): drm/msm: Fix removal of valid error case when checking speed_bin Kalyan Thota (1): drm/msm/disp/dpu1: program 3d_merge only if block is attached Rob Clark (1): drm/msm: Fix a5xx/a6xx timestamps Stephen Boyd (1): drm/msm: Set drvdata to NULL when msm_drm_init() fails drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++-- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 18 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 4 +++- drivers/gpu/drm/msm/msm_drv.c | 1 + 4 files changed, 18 insertions(+), 9 deletions(-) ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v1] drm/msm/disp/dpu1: program 3d_merge only if block is attached
On Fri, Apr 2, 2021 at 4:55 AM Kalyan Thota wrote:
>
> Update the 3d merge as active in the data path only if
> the hw block is selected in the configuration.
>
> Reported-by: Stephen Boyd
Thanks, I've added:
Fixes: 73bfb790ac78 ("msm:disp:dpu1: setup display datapath for SC7180 target")
> Signed-off-by: Kalyan Thota
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> 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 8981cfa..92e6f1b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -496,7 +496,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
>
> DPU_REG_WRITE(c, CTL_TOP, mode_sel);
> DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
> - DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, BIT(cfg->merge_3d -
> MERGE_3D_0));
> + if (cfg->merge_3d)
> + DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
> + BIT(cfg->merge_3d - MERGE_3D_0));
> }
>
> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
> --
> 2.7.4
>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/2] drm/msm: Add param for userspace to query suspend count
On Wed, Mar 24, 2021 at 06:23:53PM -0700, Rob Clark wrote:
> From: Rob Clark
>
> Performance counts, and ALWAYS_ON counters used for capturing GPU
> timestamps, lose their state across suspend/resume cycles. Userspace
> tooling for performance monitoring needs to be aware of this. For
> example, after a suspend userspace needs to recalibrate it's offset
> between CPU and GPU time.
>
Acked-by: Jordan Crouse
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 3 +++
> drivers/gpu/drm/msm/msm_drv.c | 1 +
> drivers/gpu/drm/msm/msm_gpu.c | 2 ++
> drivers/gpu/drm/msm/msm_gpu.h | 2 ++
> include/uapi/drm/msm_drm.h | 1 +
> 5 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index f09175698827..e473b7c9ff7f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -280,6 +280,9 @@ int adreno_get_param(struct msm_gpu *gpu, uint32_t param,
> uint64_t *value)
> case MSM_PARAM_FAULTS:
> *value = gpu->global_faults;
> return 0;
> + case MSM_PARAM_SUSPENDS:
> + *value = gpu->suspend_count;
> + return 0;
> default:
> DBG("%s: invalid param: %u", gpu->name, param);
> return -EINVAL;
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index b29e439eb299..4f9fa0189a07 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -39,6 +39,7 @@
> * GEM object's debug name
> * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> * - 1.6.0 - Syncobj support
> + * - 1.7.0 - Add MSM_PARAM_SUSPENDS to access suspend count
> */
> #define MSM_VERSION_MAJOR1
> #define MSM_VERSION_MINOR6
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 7bdb01f202f4..ab888d83b887 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -256,6 +256,8 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
> if (ret)
> return ret;
>
> + gpu->suspend_count++;
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index d7cd02cd2109..18baf935e143 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -152,6 +152,8 @@ struct msm_gpu {
> ktime_t time;
> } devfreq;
>
> + uint32_t suspend_count;
> +
> struct msm_gpu_state *crashstate;
> /* True if the hardware supports expanded apriv (a650 and newer) */
> bool hw_apriv;
> diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
> index a6c1f3eb2623..5596d7c37f9e 100644
> --- a/include/uapi/drm/msm_drm.h
> +++ b/include/uapi/drm/msm_drm.h
> @@ -76,6 +76,7 @@ struct drm_msm_timespec {
> #define MSM_PARAM_NR_RINGS 0x07
> #define MSM_PARAM_PP_PGTABLE 0x08 /* => 1 for per-process pagetables, else
> 0 */
> #define MSM_PARAM_FAULTS 0x09
> +#define MSM_PARAM_SUSPENDS 0x0a
>
> struct drm_msm_param {
> __u32 pipe; /* in, MSM_PIPE_x */
> --
> 2.29.2
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/2] drm/msm: Fix a5xx/a6xx timestamps
On Wed, Mar 24, 2021 at 06:23:52PM -0700, Rob Clark wrote:
> From: Rob Clark
>
> They were reading a counter that was configured to ALWAYS_COUNT (ie.
> cycles that the GPU is doing something) rather than ALWAYS_ON. This
> isn't the thing that userspace is looking for.
Acked-by: Jordan Crouse
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++--
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index a5af223eaf50..bb82fcd9df81 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1241,8 +1241,8 @@ static int a5xx_pm_suspend(struct msm_gpu *gpu)
>
> static int a5xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
> {
> - *value = gpu_read64(gpu, REG_A5XX_RBBM_PERFCTR_CP_0_LO,
> - REG_A5XX_RBBM_PERFCTR_CP_0_HI);
> + *value = gpu_read64(gpu, REG_A5XX_RBBM_ALWAYSON_COUNTER_LO,
> + REG_A5XX_RBBM_ALWAYSON_COUNTER_HI);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 130661898546..59718c304488 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1173,8 +1173,8 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
> uint64_t *value)
> /* Force the GPU power on so we can read this register */
> a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_GPU_SET);
>
> - *value = gpu_read64(gpu, REG_A6XX_RBBM_PERFCTR_CP_0_LO,
> - REG_A6XX_RBBM_PERFCTR_CP_0_HI);
> + *value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER_LO,
> + REG_A6XX_CP_ALWAYS_ON_COUNTER_HI);
>
> a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_GPU_SET);
> return 0;
> --
> 2.29.2
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [v1] drm/msm/disp/dpu1: program 3d_merge only if block is attached
Update the 3d merge as active in the data path only if the hw block is selected in the configuration. Reported-by: Stephen Boyd Signed-off-by: Kalyan Thota --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 8981cfa..92e6f1b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c @@ -496,7 +496,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, DPU_REG_WRITE(c, CTL_TOP, mode_sel); DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active); - DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, BIT(cfg->merge_3d - MERGE_3D_0)); + if (cfg->merge_3d) + DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, + BIT(cfg->merge_3d - MERGE_3D_0)); } static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, -- 2.7.4 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
On 2021-04-01 19:01, Dmitry Baryshkov wrote:
On Thu, 1 Apr 2021 at 16:19, wrote:
On 2021-04-01 07:37, Dmitry Baryshkov wrote:
> On 01/04/2021 01:47, Rob Clark wrote:
>> On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov
>> wrote:
>>>
>>> On 31/03/2021 14:27, Kalyan Thota wrote:
WARN_ON was introduced by the below commit to catch runtime resumes
that are getting triggered before icc path was set.
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime
resume"
For the targets where the bw scaling is not enabled, this WARN_ON is
a false alarm. Fix the WARN condition appropriately.
>>>
>>> Should we change all DPU targets to use bw scaling to the mdp from
>>> the
>>> mdss nodes? The limitation to sc7180 looks artificial.
>>
>> yes, we should, this keeps biting us on 845
>
> Done,
>
https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.barysh...@linaro.org/
Hi Dmitry,
https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.barysh...@linaro.org/
you need to add clk_inefficiency_factor, bw_inefficiency_factor in the
catalogue for the new
targets where bw scaling is being enabled. please reuse sc7180 values.
Done in patch 1 in that series.
got it.
secondly, the AXI clock needs to be moved from mdss to mdp device like
as in sc7180 dt if its not done already.
Is this enough:
sm8250 has < GCC_DISP_HF_AXI_CLK> both in mdss and mdp nodes
sdm845 has < GCC_DISP_AXI_CLK> in mdss node and <
DISP_CC_MDSS_AXI_CLK> in the mdp node.
Since there is no BW vote in mdss, we need to move the AXI clock ON to
mdp node.
for sm8250
remove GCC_DISP_HF_AXI_CLK from mdss device and add it in mdp device.
for sdm845
remove GCC_DISP_AXI_CLK from mdss device and add it in mdp device before
we turn on DISP_CC_MDSS_AXI_CLK, i.e as first item.
lastly, if you are planning to remove the static votes from dpu_mdss,
do
you also want to move the
interconnect paths from mdss device to mdp device in the dt ?
I have no strong opinion on this. So far I did not change dt to be
compatible with the current device trees.
Thanks,
Kalyan
>
>>
Reported-by: Steev Klimaszewski
>>
>> Please add Fixes: tag as well
Adding Fixes tag above my sign-off, should i push another version or
can
it be picked from here ?
Fixes: 627dc55c273d ("drm/msm/disp/dpu1: icc path needs to be set
before
dpu runtime resume")
>>
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 +---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 9 +
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++-
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index cab387f..0071a4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -294,6 +294,9 @@ static int
dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
struct icc_path *path1;
struct drm_device *dev = dpu_kms->dev;
+ if (!dpu_supports_bw_scaling(dev))
+ return 0;
+
path0 = of_icc_get(dev->dev, "mdp0-mem");
path1 = of_icc_get(dev->dev, "mdp1-mem");
@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
DPU_DEBUG("REG_DMA is not defined");
}
- if (of_device_is_compatible(dev->dev->of_node,
"qcom,sc7180-mdss"))
- dpu_kms_parse_data_bus_icc_path(dpu_kms);
+ dpu_kms_parse_data_bus_icc_path(dpu_kms);
pm_runtime_get_sync(_kms->pdev->dev);
@@ -1198,7 +1200,7 @@ static int __maybe_unused
dpu_runtime_resume(struct device *dev)
ddev = dpu_kms->dev;
- WARN_ON(!(dpu_kms->num_paths));
+ WARN_ON((dpu_supports_bw_scaling(ddev) &&
!dpu_kms->num_paths));
/* Min vote of BW is required before turning on AXI clk */
for (i = 0; i < dpu_kms->num_paths; i++)
icc_set_bw(dpu_kms->path[i], 0,
Bps_to_icc(MIN_IB_BW));
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index d6717d6..f7bcc0a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -154,6 +154,15 @@ struct vsync_info {
#define to_dpu_global_state(x) container_of(x, struct
dpu_global_state, base)
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support
bw scaling.
+ * @dev: Pointer to drm_device structure
+ */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+ return of_device_is_compatible(dev->dev->of_node,
"qcom,sc7180-mdss");
+}
+
/* Global private object state for tracking
