date:20230712

The msm_gem_get_vaddr() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.

Fixes: 6a8bd08d0465 ("drm/msm: add sudo flag to submit ioctl")
Signed-off-by: Gaosheng Cui 
Reviewed-by: Abhinav Kumar 
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index a99310b68793..a499e3b350fc 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -89,7 +89,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct 
msm_gem_submit *submit
 * since we've already mapped it once in
 * submit_reloc()
 */
-   if (WARN_ON(!ptr))
+   if (WARN_ON(IS_ERR(ptr)))
return;
 
for (i = 0; i < dwords; i++) {
-- 
2.25.1

[Freedreno] [PATCH v2 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()

The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.

Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for CORE")
Signed-off-by: Gaosheng Cui 
---
 drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 3276a3e82c62..e9c92439398d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct komeda_component 
*c,
u32 avail_scalers;
 
pipe_st = komeda_pipeline_get_state(c->pipeline, state);
-   if (!pipe_st)
+   if (IS_ERR(pipe_st))
return NULL;
 
avail_scalers = (pipe_st->active_comps & KOMEDA_PIPELINE_SCALERS) ^
-- 
2.25.1

[Freedreno] [PATCH v2 0/3] Fix IS_ERR() vs NULL check for drm

v2:
- I'm sorry I missed some emails, these patches were submitted last year,
  now let me resend it with the following changes:
  1. remove the patch: "drm/msm: Fix IS_ERR_OR_NULL() vs NULL check in 
msm_icc_get()"
  2. remove the patch: "drm/vc4: kms: Fix IS_ERR() vs NULL check for vc4_kms"
  3. add "Reviewed-by: Abhinav Kumar " to the second 
patch.

  Thanks!

v1:
- This series contains a few fixup patches, to fix IS_ERR() vs NULL check
  for drm, and avoid a potential null-ptr-defer issue, too. Thanks!

Gaosheng Cui (3):
  drm/panel: Fix IS_ERR() vs NULL check in nt35950_probe()
  drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb()
  drm/komeda: Fix IS_ERR() vs NULL check in
komeda_component_get_avail_scaler()

 drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c  | 2 +-
 drivers/gpu/drm/panel/panel-novatek-nt35950.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.25.1

[Freedreno] [PATCH v2 1/3] drm/panel: Fix IS_ERR() vs NULL check in nt35950_probe()

The mipi_dsi_device_register_full() returns an ERR_PTR() on failure,
we should use IS_ERR() to check the return value.

Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC 
panels")
Signed-off-by: Gaosheng Cui 
---
 drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c 
b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
index 8b108ac80b55..4903bbf1df55 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
@@ -571,7 +571,7 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
}
 
nt->dsi[1] = mipi_dsi_device_register_full(dsi_r_host, info);
-   if (!nt->dsi[1]) {
+   if (IS_ERR(nt->dsi[1])) {
dev_err(dev, "Cannot get secondary DSI node\n");
return -ENODEV;
}
-- 
2.25.1

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Luben Tuikov

On 2023-07-12 09:53, Christian König wrote:
> Am 12.07.23 um 15:38 schrieb Uwe Kleine-König:
>> Hello Maxime,
>>
>> On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:
>>> On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:
> Background is that this makes merge conflicts easier to handle and detect.
 Really?
>>> FWIW, I agree with Christian here.
>>>
 Each file (apart from include/drm/drm_crtc.h) is only touched once. So
 unless I'm missing something you don't get less or easier conflicts by
 doing it all in a single patch. But you gain the freedom to drop a
 patch for one driver without having to drop the rest with it.
>>> Not really, because the last patch removed the union anyway. So you have
>>> to revert both the last patch, plus that driver one. And then you need
>>> to add a TODO to remove that union eventually.
>> Yes, with a single patch you have only one revert (but 194 files changed,
>> 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
>> file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
>> bigger). (And maybe you get away with just reverting the last patch.)
>>
>> With a single patch the TODO after a revert is "redo it all again (and
>> prepare for a different set of conflicts)" while with the split series
>> it's only "fix that one driver that was forgotten/borked" + reapply that
>> 10 line patch.
> 
> Yeah, but for a maintainer the size of the patches doesn't matter. 
> That's only interesting if you need to manually review the patch, which 
> you hopefully doesn't do in case of something auto-generated.
> 
> In other words if the patch is auto-generated re-applying it completely 
> is less work than fixing things up individually.
> 
>>   As the one who gets that TODO, I prefer the latter.
> 
> Yeah, but your personal preferences are not a technical relevant 
> argument to a maintainer.
> 
> At the end of the day Dave or Daniel need to decide, because they need 
> to live with it.
> 
> Regards,
> Christian.
> 
>>
>> So in sum: If your metric is "small count of reverted commits", you're
>> right. If however your metric is: Better get 95% of this series' change
>> in than maybe 0%, the split series is the way to do it.
>>
>> With me having spend ~3h on this series' changes, it's maybe
>> understandable that I did it the way I did.
>>
>> FTR: This series was created on top of v6.5-rc1. If you apply it to
>> drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
>> be the responsible maintainer who applies this series, I like being able
>> to just do git am --skip then.
>>
>> FTR#2: In drm-misc-next is a new driver
>> (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
>> now might indeed be a good idea.
>>
 So I still like the split version better, but I'm open to a more
 verbose reasoning from your side.
>>> You're doing only one thing here, really: you change the name of a
>>> structure field. If it was shared between multiple maintainers, then
>>> sure, splitting that up is easier for everyone, but this will go through
>>> drm-misc, so I can't see the benefit it brings.
>> I see your argument, but I think mine weights more.

I'm with Maxime and Christian on this--a single action necessitates a single 
patch.
One single movement. As Maxime said "either 0 or 100."

As to the name, perhaps "drm_dev" is more descriptive than just "drm".
What is "drm"? Ah it's a "dev", as in "drm dev"... Then why not rename it
to "drm_dev"? You are renaming it from "dev" to something more descriptive
after all. "dev" --> "drm" is no better, but "dev" --> "drm_dev" is just
right.
-- 
Regards,
Luben

Re: [Freedreno] [PATCH 2/5] drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb()





On 11/10/2022 1:44 AM, Gaosheng Cui wrote:

The msm_gem_get_vaddr() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.

Fixes: 6a8bd08d0465 ("drm/msm: add sudo flag to submit ioctl")
Signed-off-by: Gaosheng Cui 
---
  drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Abhinav Kumar

[Freedreno] [PATCH] drm/msm: Fix hw_fence error path cleanup

2023-07-12 Thread Rob Clark

From: Rob Clark 

In an error path where the submit is free'd without the job being run,
the hw_fence pointer is simply a kzalloc'd block of memory.  In this
case we should just kfree() it, rather than trying to decrement it's
reference count.  Fortunately we can tell that this is the case by
checking for a zero refcount, since if the job was run, the submit would
be holding a reference to the hw_fence.

Fixes: f94e6a51e17c ("drm/msm: Pre-allocate hw_fence")
Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/msm_fence.c  |  6 ++
 drivers/gpu/drm/msm/msm_gem_submit.c | 14 +-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
index 96599ec3eb78..1a5d4f1c8b42 100644
--- a/drivers/gpu/drm/msm/msm_fence.c
+++ b/drivers/gpu/drm/msm/msm_fence.c
@@ -191,6 +191,12 @@ msm_fence_init(struct dma_fence *fence, struct 
msm_fence_context *fctx)
 
f->fctx = fctx;
 
+   /*
+* Until this point, the fence was just some pre-allocated memory,
+* no-one should have taken a reference to it yet.
+*/
+   WARN_ON(kref_read(>refcount));
+
dma_fence_init(>base, _fence_ops, >spinlock,
   fctx->context, ++fctx->last_fence);
 }
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c 
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3f1aa4de3b87..9d66498cdc04 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -86,7 +86,19 @@ void __msm_gem_submit_destroy(struct kref *kref)
}
 
dma_fence_put(submit->user_fence);
-   dma_fence_put(submit->hw_fence);
+
+   /*
+* If the submit is freed before msm_job_run(), then hw_fence is
+* just some pre-allocated memory, not a reference counted fence.
+* Once the job runs and the hw_fence is initialized, it will
+* have a refcount of at least one, since the submit holds a ref
+* to the hw_fence.
+*/
+   if (kref_read(>hw_fence->refcount) == 0) {
+   kfree(submit->hw_fence);
+   } else {
+   dma_fence_put(submit->hw_fence);
+   }
 
put_pid(submit->pid);
msm_submitqueue_put(submit->queue);
-- 
2.41.0

Re: [Freedreno] [PATCH v5 3/5] drm/msm/dpu: rename all hw_intf structs to have dpu_hw prefix


On 12/07/2023 04:20, Abhinav Kumar wrote:

dpu_hw_intf has a few instances of structs which do not have
the dpu_hw prefix. Lets fix this by renaming those structs
and updating the usage of those accordingly.

Signed-off-by: Abhinav Kumar 
---
  .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 18 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c|  6 +++---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 12 ++--
  3 files changed, 18 insertions(+), 18 deletions(-)


Reviewed-by: Dmitry Baryshkov 

--
With best wishes
Dmitry

Re: [Freedreno] [PATCH v5 4/5] drm/msm/dpu: rename enable_compression() to program_intf_cmd_cfg()


On 12/07/2023 04:20, Abhinav Kumar wrote:

Rename the intf's enable_compression() op to program_intf_cmd_cfg()
and allow it to accept a struct intf_cmd_mode_cfg to program
all the bits at once. This can be re-used by widebus later on as
well as it touches the same register.

changes in v5:
- rename struct intf_cmd_mode_cfg to dpu_hw_intf_cmd_mode_cfg
- remove couple of comments

Signed-off-by: Abhinav Kumar 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 ++--
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c  | 8 +---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h  | 9 +++--
  3 files changed, 18 insertions(+), 7 deletions(-)


Reviewed-by: Dmitry Baryshkov 

--
With best wishes
Dmitry

[Freedreno] [PATCH v4 11/11] drm/msm/dpu: drop dpu_core_perf_destroy()

This function does nothing, just clears one struct field. Drop it now.

Acked-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 10 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  1 -
 3 files changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 16a4d6c67f4d..31813a322afd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -486,16 +486,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, 
struct dentry *parent)
 }
 #endif
 
-void dpu_core_perf_destroy(struct dpu_core_perf *perf)
-{
-   if (!perf) {
-   DPU_ERROR("invalid parameters\n");
-   return;
-   }
-
-   perf->max_core_clk_rate = 0;
-}
-
 int dpu_core_perf_init(struct dpu_core_perf *perf,
const struct dpu_perf_cfg *perf_cfg,
unsigned long max_core_clk_rate)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 8cc55752db5e..4186977390bd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -78,12 +78,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
  */
 void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
 
-/**
- * dpu_core_perf_destroy - destroy the given core performance context
- * @perf: Pointer to core performance context
- */
-void dpu_core_perf_destroy(struct dpu_core_perf *perf);
-
 /**
  * dpu_core_perf_init - initialize the given core performance context
  * @perf: Pointer to core performance context
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 5bfea4868e43..76ba86d3e436 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1212,7 +1212,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
return 0;
 
 drm_obj_init_err:
-   dpu_core_perf_destroy(_kms->perf);
 hw_intr_init_err:
 perf_err:
 power_error:
-- 
2.39.2

[Freedreno] [PATCH v4 09/11] drm/msm/dpu: remove extra clk_round_rate() call

The dev_pm_opp_set_rate() already contains a call for clk_round_rate for
the passed value. Stop calling it manually from
_dpu_core_perf_get_core_clk_rate(). It is slightly incorrect to call it
this way, as we should round the final calculated clock rate rather than
rounding all the intermediate values.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index d8c88ce5190d..896e87b13dbe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -299,8 +299,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms 
*kms)
dpu_cstate = to_dpu_crtc_state(crtc->state);
clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
clk_rate);
-   clk_rate = clk_round_rate(kms->perf.core_clk,
-   clk_rate);
}
}
 
-- 
2.39.2

[Freedreno] [PATCH v4 08/11] drm/msm/dpu: remove unused fields from struct dpu_core_perf

Remove dpu_core_perf::dev and dpu_core_perf::debugfs_root fields, they
are not used by the code.

Reviewed-by: Konrad Dybcio 
Reviewed-by: Abhinav Kumar 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 3 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 6 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 2 +-
 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 27a0312bd140..d8c88ce5190d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -497,15 +497,12 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
 
perf->max_core_clk_rate = 0;
perf->core_clk = NULL;
-   perf->dev = NULL;
 }
 
 int dpu_core_perf_init(struct dpu_core_perf *perf,
-   struct drm_device *dev,
const struct dpu_perf_cfg *perf_cfg,
struct clk *core_clk)
 {
-   perf->dev = dev;
perf->perf_cfg = perf_cfg;
perf->core_clk = core_clk;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index f4b84e67138c..e718d523ff30 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -36,8 +36,6 @@ struct dpu_core_perf_tune {
 
 /**
  * struct dpu_core_perf - definition of core performance context
- * @dev: Pointer to drm device
- * @debugfs_root: top level debug folder
  * @perf_cfg: Platform-specific performance configuration
  * @core_clk: Pointer to the core clock
  * @core_clk_rate: current core clock rate
@@ -49,8 +47,6 @@ struct dpu_core_perf_tune {
  * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2
  */
 struct dpu_core_perf {
-   struct drm_device *dev;
-   struct dentry *debugfs_root;
const struct dpu_perf_cfg *perf_cfg;
struct clk *core_clk;
u64 core_clk_rate;
@@ -95,12 +91,10 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf);
 /**
  * dpu_core_perf_init - initialize the given core performance context
  * @perf: Pointer to core performance context
- * @dev: Pointer to drm device
  * @perf_cfg: Pointer to platform performance configuration
  * @core_clk: pointer to core clock
  */
 int dpu_core_perf_init(struct dpu_core_perf *perf,
-   struct drm_device *dev,
const struct dpu_perf_cfg *perf_cfg,
struct clk *core_clk);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 074c032cd24e..80e08302680c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1156,7 +1156,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dpu_kms->hw_vbif[vbif->id] = hw;
}
 
-   rc = dpu_core_perf_init(_kms->perf, dev, dpu_kms->catalog->perf,
+   rc = dpu_core_perf_init(_kms->perf, dpu_kms->catalog->perf,
msm_clk_bulk_get_clock(dpu_kms->clocks, 
dpu_kms->num_clocks, "core"));
if (rc) {
DPU_ERROR("failed to init perf %d\n", rc);
-- 
2.39.2

[Freedreno] [PATCH v4 05/11] drm/msm/dpu: rework indentation in dpu_core_perf

dpu_core_perf.c contains several multi-line conditions which are hard to
comprehent because of the indentation. Rework the identation of these
conditions to make it easier to understand them.

Reviewed-by: Abhinav Kumar 
Acked-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index f9f44cfcfbf2..841e1fc0c6a7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -173,8 +173,8 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 
drm_for_each_crtc(tmp_crtc, crtc->dev) {
if (tmp_crtc->enabled &&
-   (dpu_crtc_get_client_type(tmp_crtc) ==
-   curr_client_type) && (tmp_crtc != crtc)) {
+   dpu_crtc_get_client_type(tmp_crtc) == curr_client_type &&
+   tmp_crtc != crtc) {
struct dpu_crtc_state *tmp_cstate =
to_dpu_crtc_state(tmp_crtc->state);
 
@@ -365,10 +365,8 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
update_bus = true;
}
 
-   if ((params_changed &&
-   (new->core_clk_rate > old->core_clk_rate)) ||
-   (!params_changed &&
-   (new->core_clk_rate < old->core_clk_rate))) {
+   if ((params_changed && new->core_clk_rate > old->core_clk_rate) 
||
+   (!params_changed && new->core_clk_rate < 
old->core_clk_rate)) {
old->core_clk_rate = new->core_clk_rate;
update_clk = true;
}
-- 
2.39.2

[Freedreno] [PATCH v4 10/11] drm/msm/dpu: move max clock decision to dpu_kms.

dpu_core_perf should not make decisions on the maximum possible core
clock rate. Pass the value from dpu_kms_hw_init().

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 11 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 13 +++--
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 896e87b13dbe..16a4d6c67f4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -494,21 +494,14 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
}
 
perf->max_core_clk_rate = 0;
-   perf->core_clk = NULL;
 }
 
 int dpu_core_perf_init(struct dpu_core_perf *perf,
const struct dpu_perf_cfg *perf_cfg,
-   struct clk *core_clk)
+   unsigned long max_core_clk_rate)
 {
perf->perf_cfg = perf_cfg;
-   perf->core_clk = core_clk;
-
-   perf->max_core_clk_rate = clk_get_rate(core_clk);
-   if (!perf->max_core_clk_rate) {
-   DPU_DEBUG("optional max core clk rate, use default\n");
-   perf->max_core_clk_rate = DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE;
-   }
+   perf->max_core_clk_rate = max_core_clk_rate;
 
return 0;
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index e718d523ff30..8cc55752db5e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -12,8 +12,6 @@
 
 #include "dpu_hw_catalog.h"
 
-#defineDPU_PERF_DEFAULT_MAX_CORE_CLK_RATE  41250
-
 /**
  * struct dpu_core_perf_params - definition of performance parameters
  * @max_per_pipe_ib: maximum instantaneous bandwidth request
@@ -37,7 +35,6 @@ struct dpu_core_perf_tune {
 /**
  * struct dpu_core_perf - definition of core performance context
  * @perf_cfg: Platform-specific performance configuration
- * @core_clk: Pointer to the core clock
  * @core_clk_rate: current core clock rate
  * @max_core_clk_rate: maximum allowable core clock rate
  * @perf_tune: debug control for performance tuning
@@ -48,7 +45,6 @@ struct dpu_core_perf_tune {
  */
 struct dpu_core_perf {
const struct dpu_perf_cfg *perf_cfg;
-   struct clk *core_clk;
u64 core_clk_rate;
u64 max_core_clk_rate;
struct dpu_core_perf_tune perf_tune;
@@ -92,11 +88,11 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf);
  * dpu_core_perf_init - initialize the given core performance context
  * @perf: Pointer to core performance context
  * @perf_cfg: Pointer to platform performance configuration
- * @core_clk: pointer to core clock
+ * @max_core_clk_rate: Maximum core clock rate
  */
 int dpu_core_perf_init(struct dpu_core_perf *perf,
const struct dpu_perf_cfg *perf_cfg,
-   struct clk *core_clk);
+   unsigned long max_core_clk_rate);
 
 struct dpu_kms;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 80e08302680c..5bfea4868e43 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1051,11 +1051,14 @@ unsigned long dpu_kms_get_clk_rate(struct dpu_kms 
*dpu_kms, char *clock_name)
return clk_get_rate(clk);
 }
 
+#defineDPU_PERF_DEFAULT_MAX_CORE_CLK_RATE  41250
+
 static int dpu_kms_hw_init(struct msm_kms *kms)
 {
struct dpu_kms *dpu_kms;
struct drm_device *dev;
int i, rc = -EINVAL;
+   unsigned long max_core_clk_rate;
u32 core_rev;
 
if (!kms) {
@@ -1156,8 +1159,14 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dpu_kms->hw_vbif[vbif->id] = hw;
}
 
-   rc = dpu_core_perf_init(_kms->perf, dpu_kms->catalog->perf,
-   msm_clk_bulk_get_clock(dpu_kms->clocks, 
dpu_kms->num_clocks, "core"));
+   /* TODO: use the same max_freq as in dpu_kms_hw_init */
+   max_core_clk_rate = dpu_kms_get_clk_rate(dpu_kms, "core");
+   if (!max_core_clk_rate) {
+   DPU_DEBUG("max core clk rate not determined, using default\n");
+   max_core_clk_rate = DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE;
+   }
+
+   rc = dpu_core_perf_init(_kms->perf, dpu_kms->catalog->perf, 
max_core_clk_rate);
if (rc) {
DPU_ERROR("failed to init perf %d\n", rc);
goto perf_err;
-- 
2.39.2

[Freedreno] [PATCH v4 07/11] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code

Simplify dpu_core_perf code by using only dpu_perf_cfg instead of using
full-featured catalog data.

Acked-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 73 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  2 +-
 3 files changed, 35 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 3b3c2659297d..27a0312bd140 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -33,11 +33,11 @@ enum dpu_perf_mode {
 
 /**
  * _dpu_core_perf_calc_bw() - to calculate BW per crtc
- * @kms:  pointer to the dpu_kms
+ * @perf_cfg: performance configuration
  * @crtc: pointer to a crtc
  * Return: returns aggregated BW for all planes in crtc.
  */
-static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
+static u64 _dpu_core_perf_calc_bw(const struct dpu_perf_cfg *perf_cfg,
struct drm_crtc *crtc)
 {
struct drm_plane *plane;
@@ -53,7 +53,7 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
crtc_plane_bw += pstate->plane_fetch_bw;
}
 
-   bw_factor = kms->catalog->perf->bw_inefficiency_factor;
+   bw_factor = perf_cfg->bw_inefficiency_factor;
if (bw_factor) {
crtc_plane_bw *= bw_factor;
do_div(crtc_plane_bw, 100);
@@ -64,12 +64,12 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
 
 /**
  * _dpu_core_perf_calc_clk() - to calculate clock per crtc
- * @kms:  pointer to the dpu_kms
+ * @perf_cfg: performance configuration
  * @crtc: pointer to a crtc
  * @state: pointer to a crtc state
  * Return: returns max clk for all planes in crtc.
  */
-static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
+static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg,
struct drm_crtc *crtc, struct drm_crtc_state *state)
 {
struct drm_plane *plane;
@@ -90,7 +90,7 @@ static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
crtc_clk = max(pstate->plane_clk, crtc_clk);
}
 
-   clk_factor = kms->catalog->perf->clk_inefficiency_factor;
+   clk_factor = perf_cfg->clk_inefficiency_factor;
if (clk_factor) {
crtc_clk *= clk_factor;
do_div(crtc_clk, 100);
@@ -106,30 +106,32 @@ static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc 
*crtc)
return to_dpu_kms(priv->kms);
 }
 
-static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
-   struct drm_crtc *crtc,
-   struct drm_crtc_state *state,
-   struct dpu_core_perf_params *perf)
+static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
+struct drm_crtc *crtc,
+struct drm_crtc_state *state,
+struct dpu_core_perf_params *perf)
 {
-   if (!kms || !kms->catalog || !crtc || !state || !perf) {
+   const struct dpu_perf_cfg *perf_cfg = core_perf->perf_cfg;
+
+   if (!perf_cfg || !crtc || !state || !perf) {
DPU_ERROR("invalid parameters\n");
return;
}
 
memset(perf, 0, sizeof(struct dpu_core_perf_params));
 
-   if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
+   if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
perf->bw_ctl = 0;
perf->max_per_pipe_ib = 0;
perf->core_clk_rate = 0;
-   } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
-   perf->bw_ctl = kms->perf.fix_core_ab_vote;
-   perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote;
-   perf->core_clk_rate = kms->perf.fix_core_clk_rate;
+   } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
+   perf->bw_ctl = core_perf->fix_core_ab_vote;
+   perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
+   perf->core_clk_rate = core_perf->fix_core_clk_rate;
} else {
-   perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
-   perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
-   perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
+   perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
+   perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
+   perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, 
state);
}
 
DRM_DEBUG_ATOMIC(
@@ -154,10 +156,6 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
}
 
kms = _dpu_crtc_get_kms(crtc);
-   if (!kms->catalog) {
-   DPU_ERROR("invalid parameters\n");
-   return 0;
-   }
 
/* we only need bandwidth check on real-time clients (interfaces) */
if

[Freedreno] [PATCH v4 06/11] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

The stop_req is true only in the dpu_crtc_disable() case, when
crtc->enable has already been set to false. This renders the stop_req
argument useless. Remove it completely.

Reviewed-by: Abhinav Kumar 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  6 +++---
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 841e1fc0c6a7..3b3c2659297d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -315,7 +315,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms 
*kms)
 }
 
 int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
-   int params_changed, bool stop_req)
+ int params_changed)
 {
struct dpu_core_perf_params *new, *old;
bool update_bus = false, update_clk = false;
@@ -339,13 +339,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
dpu_crtc = to_dpu_crtc(crtc);
dpu_cstate = to_dpu_crtc_state(crtc->state);
 
-   DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
-   crtc->base.id, stop_req, kms->perf.core_clk_rate);
+   DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
+   crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
 
old = _crtc->cur_perf;
new = _cstate->new_perf;
 
-   if (crtc->enabled && !stop_req) {
+   if (crtc->enabled) {
/*
 * cases for bus bandwidth update.
 * 1. new bandwidth vote - "ab or ib vote" is higher
@@ -378,7 +378,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
}
 
trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
-   new->core_clk_rate, stop_req, update_bus, update_clk);
+   new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
 
if (update_bus) {
ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
@@ -398,7 +398,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 
DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
 
-   trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
+   trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, 
clk_rate);
 
clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
ret = dev_pm_opp_set_rate(>pdev->dev, clk_rate);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index c965dfbc3007..c0097b67f9dd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -75,11 +75,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
  * dpu_core_perf_crtc_update - update performance of the given crtc
  * @crtc: Pointer to crtc
  * @params_changed: true if crtc parameters are modified
- * @stop_req: true if this is a stop request
  * return: zero if success, or error code otherwise
  */
 int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
-   int params_changed, bool stop_req);
+ int params_changed);
 
 /**
  * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 1edf2b6b0a26..8ce7586e2ddf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
 void dpu_crtc_complete_commit(struct drm_crtc *crtc)
 {
trace_dpu_crtc_complete_commit(DRMID(crtc));
-   dpu_core_perf_crtc_update(crtc, 0, false);
+   dpu_core_perf_crtc_update(crtc, 0);
_dpu_crtc_complete_flip(crtc);
 }
 
@@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
return;
 
/* update performance setting before crtc kickoff */
-   dpu_core_perf_crtc_update(crtc, 1, false);
+   dpu_core_perf_crtc_update(crtc, 1);
 
/*
 * Final plane updates: Give each plane a chance to complete all
@@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
atomic_set(_crtc->frame_pending, 0);
}
 
-   dpu_core_perf_crtc_update(crtc, 0, true);
+   dpu_core_perf_crtc_update(crtc, 0);
 
drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
-- 
2.39.2

[Freedreno] [PATCH v4 03/11] drm/msm/dpu: core_perf: bail earlier if there are no ICC paths

Skip bandwidth aggregation and return early if there are no interconnect
paths defined for the DPU device.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 333dcfe57800..05d340aa18c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -237,11 +237,11 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms 
*kms,
int i, ret = 0;
u64 avg_bw;
 
-   dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), 
);
-
if (!kms->num_paths)
return 0;
 
+   dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), 
);
+
avg_bw = perf.bw_ctl;
do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
 
-- 
2.39.2

[Freedreno] [PATCH v4 04/11] drm/msm/dpu: drop separate dpu_core_perf_tune overrides

The values in struct dpu_core_perf_tune are fixed per the core perf
mode. Drop the 'tune' values and substitute them with known values when
performing perf management.

Note: min_bus_vote was not used at all, so it is just silently dropped.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 27 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  4 ---
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 05d340aa18c5..f9f44cfcfbf2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -291,10 +291,16 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
 
 static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
 {
-   u64 clk_rate = kms->perf.perf_tune.min_core_clk;
+   u64 clk_rate;
struct drm_crtc *crtc;
struct dpu_crtc_state *dpu_cstate;
 
+   if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
+   return kms->perf.fix_core_clk_rate;
+
+   if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
+   return kms->perf.max_core_clk_rate;
+
drm_for_each_crtc(crtc, kms->dev) {
if (crtc->enabled) {
dpu_cstate = to_dpu_crtc_state(crtc->state);
@@ -305,11 +311,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms 
*kms)
}
}
 
-   if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
-   clk_rate = kms->perf.fix_core_clk_rate;
-
-   DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
-
return clk_rate;
 }
 
@@ -397,6 +398,8 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
if (update_clk) {
clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
 
+   DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
+
trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
 
clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
@@ -418,7 +421,6 @@ static ssize_t _dpu_core_perf_mode_write(struct file *file,
const char __user *user_buf, size_t count, loff_t *ppos)
 {
struct dpu_core_perf *perf = file->private_data;
-   const struct dpu_perf_cfg *cfg = perf->catalog->perf;
u32 perf_mode = 0;
int ret;
 
@@ -433,14 +435,9 @@ static ssize_t _dpu_core_perf_mode_write(struct file *file,
DRM_INFO("fix performance mode\n");
} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
/* run the driver with max clk and BW vote */
-   perf->perf_tune.min_core_clk = perf->max_core_clk_rate;
-   perf->perf_tune.min_bus_vote =
-   (u64) cfg->max_bw_high * 1000;
DRM_INFO("minimum performance mode\n");
} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
/* reset the perf tune params to 0 */
-   perf->perf_tune.min_core_clk = 0;
-   perf->perf_tune.min_bus_vote = 0;
DRM_INFO("normal performance mode\n");
}
perf->perf_tune.mode = perf_mode;
@@ -456,10 +453,8 @@ static ssize_t _dpu_core_perf_mode_read(struct file *file,
char buf[128];
 
len = scnprintf(buf, sizeof(buf),
-   "mode %d min_mdp_clk %llu min_bus_vote %llu\n",
-   perf->perf_tune.mode,
-   perf->perf_tune.min_core_clk,
-   perf->perf_tune.min_bus_vote);
+   "mode %d\n",
+   perf->perf_tune.mode);
 
return simple_read_from_buffer(buff, count, ppos, buf, len);
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 29bb8ee2bc26..c965dfbc3007 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -29,13 +29,9 @@ struct dpu_core_perf_params {
 /**
  * struct dpu_core_perf_tune - definition of performance tuning control
  * @mode: performance mode
- * @min_core_clk: minimum core clock
- * @min_bus_vote: minimum bus vote
  */
 struct dpu_core_perf_tune {
u32 mode;
-   u64 min_core_clk;
-   u64 min_bus_vote;
 };
 
 /**
-- 
2.39.2

[Freedreno] [PATCH v4 01/11] drm/msm/dpu: drop enum dpu_core_perf_data_bus_id

Drop the leftover of bus-client -> interconnect conversion, the enum
dpu_core_perf_data_bus_id.

Fixes: cb88482e2570 ("drm/msm/dpu: clean up references of DPU custom bus 
scaling")
Reviewed-by: Konrad Dybcio 
Reviewed-by: Abhinav Kumar 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 13 -
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index e3795995e145..29bb8ee2bc26 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -14,19 +14,6 @@
 
 #defineDPU_PERF_DEFAULT_MAX_CORE_CLK_RATE  41250
 
-/**
- * enum dpu_core_perf_data_bus_id - data bus identifier
- * @DPU_CORE_PERF_DATA_BUS_ID_MNOC: DPU/MNOC data bus
- * @DPU_CORE_PERF_DATA_BUS_ID_LLCC: MNOC/LLCC data bus
- * @DPU_CORE_PERF_DATA_BUS_ID_EBI: LLCC/EBI data bus
- */
-enum dpu_core_perf_data_bus_id {
-   DPU_CORE_PERF_DATA_BUS_ID_MNOC,
-   DPU_CORE_PERF_DATA_BUS_ID_LLCC,
-   DPU_CORE_PERF_DATA_BUS_ID_EBI,
-   DPU_CORE_PERF_DATA_BUS_ID_MAX,
-};
-
 /**
  * struct dpu_core_perf_params - definition of performance parameters
  * @max_per_pipe_ib: maximum instantaneous bandwidth request
-- 
2.39.2

[Freedreno] [PATCH v4 02/11] drm/msm/dpu: core_perf: extract bandwidth aggregation function

In preparation to refactoring the dpu_core_perf debugfs interface,
extract the bandwidth aggregation function from
_dpu_core_perf_crtc_update_bus().

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 39 +++
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 1d9d83d7b99e..333dcfe57800 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -206,33 +206,38 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
return 0;
 }
 
-static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
-   struct drm_crtc *crtc)
+static void dpu_core_perf_aggregate(struct drm_device *ddev,
+   enum dpu_crtc_client_type curr_client_type,
+   struct dpu_core_perf_params *perf)
 {
-   struct dpu_core_perf_params perf = { 0 };
-   enum dpu_crtc_client_type curr_client_type
-   = dpu_crtc_get_client_type(crtc);
-   struct drm_crtc *tmp_crtc;
struct dpu_crtc_state *dpu_cstate;
-   int i, ret = 0;
-   u64 avg_bw;
+   struct drm_crtc *tmp_crtc;
 
-   drm_for_each_crtc(tmp_crtc, crtc->dev) {
+   drm_for_each_crtc(tmp_crtc, ddev) {
if (tmp_crtc->enabled &&
-   curr_client_type ==
-   dpu_crtc_get_client_type(tmp_crtc)) {
+   curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
 
-   perf.max_per_pipe_ib = max(perf.max_per_pipe_ib,
-   dpu_cstate->new_perf.max_per_pipe_ib);
+   perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
+   
dpu_cstate->new_perf.max_per_pipe_ib);
 
-   perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
+   perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
 
-   DRM_DEBUG_ATOMIC("crtc=%d bw=%llu paths:%d\n",
- tmp_crtc->base.id,
- dpu_cstate->new_perf.bw_ctl, kms->num_paths);
+   DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
+tmp_crtc->base.id,
+dpu_cstate->new_perf.bw_ctl);
}
}
+}
+
+static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
+   struct drm_crtc *crtc)
+{
+   struct dpu_core_perf_params perf = { 0 };
+   int i, ret = 0;
+   u64 avg_bw;
+
+   dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), 
);
 
if (!kms->num_paths)
return 0;
-- 
2.39.2

[Freedreno] [PATCH v4 00/11] drm/msm/dpu: cleanup dpu_core_perf module

Apply several cleanups to the DPU's core_perf module.

Changes since v3:
- Dropped avg_bw type change (Abhinav)
- Removed core_perf from the commit cubject (Abhinav)

Changes since v2:
- Dropped perf tuning patches for now (Abhinav)
- Restored kms variable assignment in dpu_core_perf_crtc_release_bw
  (LKP)
- Fixed description for the last patch (Abhinav)

Changes since v1:
- Reworked overrides for the perf parameters instead of completely
  dropping them. Abhinav described why these overrides are useful.
- Moved max clock rate determination to dpu_kms.c

Dmitry Baryshkov (11):
  drm/msm/dpu: drop enum dpu_core_perf_data_bus_id
  drm/msm/dpu: core_perf: extract bandwidth aggregation function
  drm/msm/dpu: core_perf: bail earlier if there are no ICC paths
  drm/msm/dpu: drop separate dpu_core_perf_tune overrides
  drm/msm/dpu: rework indentation in dpu_core_perf
  drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param
  drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code
  drm/msm/dpu: remove unused fields from struct dpu_core_perf
  drm/msm/dpu: remove extra clk_round_rate() call
  drm/msm/dpu: move max clock decision to dpu_kms.
  drm/msm/dpu: drop dpu_core_perf_destroy()

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 187 +++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  48 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  14 +-
 4 files changed, 96 insertions(+), 159 deletions(-)

-- 
2.39.2

Re: [Freedreno] [PATCH v2 2/8] drm/msm/mdss: correct UBWC programming for SM8550


On 13/07/2023 01:02, Abhinav Kumar wrote:



On 7/12/2023 5:11 AM, Dmitry Baryshkov wrote:

The SM8550 platform employs newer UBWC decoder, which requires slightly
different programming.

Fixes: a2f33995c19d ("drm/msm: mdss: add support for SM8550")
Signed-off-by: Dmitry Baryshkov 


Reviewed-by: Abhinav Kumar 

Do we also need another fixes tag

Fixes: d68db6069a8e ("drm/msm/mdss: convert UBWC setup to use match data")


No. We do not need to fix the conversion. Only the sm8550 setup.



Also, this was previously part of 
https://patchwork.freedesktop.org/series/118074/ .


In this one its after the bindings change.

For easier picking into -fixes, will you be moving this ahead of the 
bindings change and OR do you want to keep this part of the old series 
as it seems better suited there.


I think even if I pick this for -fixes, rest of this series can be 
rebased without issues. But let me know what you would prefer.



I had rejects with the original series (and 0 reviews), so it was easier 
to push that as a part of this series too.


I'm fine with you pulling either of the patches into -fixes.

--
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2 2/8] drm/msm/mdss: correct UBWC programming for SM8550





On 7/12/2023 5:11 AM, Dmitry Baryshkov wrote:

The SM8550 platform employs newer UBWC decoder, which requires slightly
different programming.

Fixes: a2f33995c19d ("drm/msm: mdss: add support for SM8550")
Signed-off-by: Dmitry Baryshkov 


Reviewed-by: Abhinav Kumar 

Do we also need another fixes tag

Fixes: d68db6069a8e ("drm/msm/mdss: convert UBWC setup to use match data")

Also, this was previously part of 
https://patchwork.freedesktop.org/series/118074/ .


In this one its after the bindings change.

For easier picking into -fixes, will you be moving this ahead of the 
bindings change and OR do you want to keep this part of the old series 
as it seems better suited there.


I think even if I pick this for -fixes, rest of this series can be 
rebased without issues. But let me know what you would prefer.

Re: [Freedreno] [PATCH v2 01/15] drm/msm/dsi: Drop unused regulators from QCM2290 14nm DSI PHY config





On 6/27/2023 1:14 PM, Marijn Suijten wrote:

The regulator setup was likely copied from other SoCs by mistake.  Just
like SM6125 the DSI PHY on this platform is not getting power from a
regulator but from the MX power domain.

Fixes: 572e9fd6d14a ("drm/msm/dsi: Add phy configuration for QCM2290")
Signed-off-by: Marijn Suijten 
---
  drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 2 --
  1 file changed, 2 deletions(-)



Reviewed-by: Abhinav Kumar

Re: [Freedreno] [PATCH v3 09/11] drm/msm/dpu: core_perf: remove extra clk_round_rate() call





On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote:

The dev_pm_opp_set_rate() already contains a call for clk_round_rate for
the passed value. Stop calling it manually from
_dpu_core_perf_get_core_clk_rate(). It is slightly incorrect to call it
this way, as we should round the final calculated clock rate rather than
rounding all the intermediate values.



Change is alright but do we really need a core_perf tag on the subject line?


Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index afd75750380c..a570810c9254 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -299,8 +299,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms 
*kms)
dpu_cstate = to_dpu_crtc_state(crtc->state);
clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
clk_rate);
-   clk_rate = clk_round_rate(kms->perf.core_clk,
-   clk_rate);
}
}

Re: [Freedreno] [PATCH v3 08/11] drm/msm/dpu: remove unused fields from struct dpu_core_perf





On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote:

Remove dpu_core_perf::dev and dpu_core_perf::debugfs_root fields, they
are not used by the code.

Reviewed-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 3 ---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 6 --
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 2 +-
  3 files changed, 1 insertion(+), 10 deletions(-)



Reviewed-by: Abhinav Kumar

Re: [Freedreno] [PATCH v3 04/11] drm/msm/dpu: drop separate dpu_core_perf_tune overrides





On 7/10/2023 7:34 PM, Dmitry Baryshkov wrote:

On 11/07/2023 05:31, Abhinav Kumar wrote:



On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote:

The values in struct dpu_core_perf_tune are fixed per the core perf
mode. Drop the 'tune' values and substitute them with known values when
performing perf management.

Note: min_bus_vote was not used at all, so it is just silently dropped.



Interesting . should bring this back properly. Will take it up.


Ack, thanks.




Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 ---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  4 ---
  2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c

index 05d340aa18c5..348550ac7e51 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -235,7 +235,7 @@ static int _dpu_core_perf_crtc_update_bus(struct 
dpu_kms *kms,

  {
  struct dpu_core_perf_params perf = { 0 };
  int i, ret = 0;
-    u64 avg_bw;
+    u32 avg_bw;


avg_bw seems unused in this patch, so unrelated change?


  if (!kms->num_paths)
  return 0;
@@ -291,10 +291,16 @@ void dpu_core_perf_crtc_release_bw(struct 
drm_crtc *crtc)

  static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
  {
-    u64 clk_rate = kms->perf.perf_tune.min_core_clk;
+    u64 clk_rate;
  struct drm_crtc *crtc;
  struct dpu_crtc_state *dpu_cstate;
+    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
+    return kms->perf.fix_core_clk_rate;
+
+    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
+    return kms->perf.max_core_clk_rate;
+
  drm_for_each_crtc(crtc, kms->dev) {
  if (crtc->enabled) {
  dpu_cstate = to_dpu_crtc_state(crtc->state);
@@ -305,11 +311,6 @@ static u64 
_dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)

  }
  }
-    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
-    clk_rate = kms->perf.fix_core_clk_rate;
-
-    DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
-


Why dont you move both FIXED and MINIMUM handling below instead of above.

So that they will just override the clk_rate and you can keep this 
useful log here and it matches where the function is.


I can keep the log in the next version. The logic was quite simple: 
there is no need to loop over CRTCs if we know that we are overriding 
the value.




Yes I understood that part. I wanted to keep the log and the function 
together that way we are logging whats the value its going to return.


This patch is logging it at the caller. Thats the only difference.

I am fine either way. Once the avg_bw change is removed, I can ack this.



This chunk looks better that way.

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

Hello Jani,

On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> On Wed, 12 Jul 2023, Uwe Kleine-König  wrote:
> > Hello,
> >
> > while I debugged an issue in the imx-lcdc driver I was constantly
> > irritated about struct drm_device pointer variables being named "dev"
> > because with that name I usually expect a struct device pointer.
> >
> > I think there is a big benefit when these are all renamed to "drm_dev".
> > I have no strong preference here though, so "drmdev" or "drm" are fine
> > for me, too. Let the bikesheding begin!
> >
> > Some statistics:
> >
> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq 
> > -c | sort -n
> >   1 struct drm_device *adev_to_drm
> >   1 struct drm_device *drm_
> >   1 struct drm_device  *drm_dev
> >   1 struct drm_device*drm_dev
> >   1 struct drm_device *pdev
> >   1 struct drm_device *rdev
> >   1 struct drm_device *vdev
> >   2 struct drm_device *dcss_drv_dev_to_drm
> >   2 struct drm_device **ddev
> >   2 struct drm_device *drm_dev_alloc
> >   2 struct drm_device *mock
> >   2 struct drm_device *p_ddev
> >   5 struct drm_device *device
> >   9 struct drm_device * dev
> >  25 struct drm_device *d
> >  95 struct drm_device *
> > 216 struct drm_device *ddev
> > 234 struct drm_device *drm_dev
> > 611 struct drm_device *drm
> >4190 struct drm_device *dev
> >
> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > it's not only me and others like the result of this effort it should be
> > followed up by adapting the other structs and the individual usages in
> > the different drivers.
> 
> I think this is an unnecessary change. In drm, a dev is usually a drm
> device, i.e. struct drm_device *.

Well, unless it's not. Prominently there is

struct drm_device {
...
struct device *dev;
...
};

which yields quite a few code locations using dev->dev which is
IMHO unnecessary irritating:

$ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
1633

Also the functions that deal with both a struct device and a struct
drm_device often use "dev" for the struct device and then "ddev" for
the drm_device (see for example amdgpu_device_get_pcie_replay_count()).

> If folks insist on following through with this anyway, I'm firmly in the
> camp the name should be "drm" and nothing else.

Up to now positive feedback is in the majority.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Krzysztof Kozlowski

On 12/07/2023 20:31, Sean Paul wrote:
>>> 216 struct drm_device *ddev
>>> 234 struct drm_device *drm_dev
>>> 611 struct drm_device *drm
>>>4190 struct drm_device *dev
>>>
>>> This series starts with renaming struct drm_crtc::dev to drm_dev. If
>>> it's not only me and others like the result of this effort it should be
>>> followed up by adapting the other structs and the individual usages in
>>> the different drivers.
>>
>> I think this is an unnecessary change. In drm, a dev is usually a drm
>> device, i.e. struct drm_device *. As shown by the numbers above.
>>
> 
> I'd really prefer this patch (series or single) is not accepted. This
> will cause problems for everyone cherry-picking patches to a
> downstream kernel (LTS or distro tree). I usually wouldn't expect
> sympathy here, but the questionable benefit does not outweigh the cost
> IM[biased]O.

You know, every code cleanup and style adjustment is interfering with
backporting. The only argument for a fast-pacing kernel should be
whether the developers of this code find it more readable with such cleanup.

Best regards,
Krzysztof

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Sean Paul

On Wed, Jul 12, 2023 at 10:52 AM Jani Nikula  wrote:
>
> On Wed, 12 Jul 2023, Uwe Kleine-König  wrote:
> > Hello,
> >
> > while I debugged an issue in the imx-lcdc driver I was constantly
> > irritated about struct drm_device pointer variables being named "dev"
> > because with that name I usually expect a struct device pointer.
> >
> > I think there is a big benefit when these are all renamed to "drm_dev".
> > I have no strong preference here though, so "drmdev" or "drm" are fine
> > for me, too. Let the bikesheding begin!
> >
> > Some statistics:
> >
> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq 
> > -c | sort -n
> >   1 struct drm_device *adev_to_drm
> >   1 struct drm_device *drm_
> >   1 struct drm_device  *drm_dev
> >   1 struct drm_device*drm_dev
> >   1 struct drm_device *pdev
> >   1 struct drm_device *rdev
> >   1 struct drm_device *vdev
> >   2 struct drm_device *dcss_drv_dev_to_drm
> >   2 struct drm_device **ddev
> >   2 struct drm_device *drm_dev_alloc
> >   2 struct drm_device *mock
> >   2 struct drm_device *p_ddev
> >   5 struct drm_device *device
> >   9 struct drm_device * dev
> >  25 struct drm_device *d
> >  95 struct drm_device *
> > 216 struct drm_device *ddev
> > 234 struct drm_device *drm_dev
> > 611 struct drm_device *drm
> >4190 struct drm_device *dev
> >
> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > it's not only me and others like the result of this effort it should be
> > followed up by adapting the other structs and the individual usages in
> > the different drivers.
>
> I think this is an unnecessary change. In drm, a dev is usually a drm
> device, i.e. struct drm_device *. As shown by the numbers above.
>

I'd really prefer this patch (series or single) is not accepted. This
will cause problems for everyone cherry-picking patches to a
downstream kernel (LTS or distro tree). I usually wouldn't expect
sympathy here, but the questionable benefit does not outweigh the cost
IM[biased]O.

Sean

> If folks insist on following through with this anyway, I'm firmly in the
> camp the name should be "drm" and nothing else.
>
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Graphics Center

Re: [Freedreno] [PATCH v2 1/8] dt-bindings: display/msm: Add reg bus and rotator interconnects

2023-07-12 Thread Krzysztof Kozlowski

On 12/07/2023 14:11, Dmitry Baryshkov wrote:
> From: Konrad Dybcio 
> 
> Apart from the already handled data bus (MAS_MDP_Pn<->DDR), there are
> other connection paths:
> - a path that connects rotator block to the DDR.
> - a path that needs to be handled to ensure MDSS register access
>   functions properly, namely the "reg bus", a.k.a the CPU-MDSS CFG
>   interconnect.
> 
> Describe these paths bindings to allow using them in device trees and in
> the driver
> 
> Signed-off-by: Konrad Dybcio 
> Signed-off-by: Dmitry Baryshkov 


Acked-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

Re: [Freedreno] [PATCH 2/2] drm/bridge: lt9611: Do not generate HFP/HBP/HSA and EOT packet

2023-07-12 Thread Marek Vasut

On 7/9/23 03:03, Abhinav Kumar wrote:

On 7/7/2023 1:47 AM, Neil Armstrong wrote:

On 07/07/2023 09:18, Neil Armstrong wrote:

Hi,

On 06/07/2023 11:20, Amit Pundir wrote:

On Wed, 5 Jul 2023 at 11:09, Dmitry Baryshkov
wrote:

[Adding freedreno@ to cc list]

On Wed, 5 Jul 2023 at 08:31, Jagan Teki
wrote:

Hi Amit,

On Wed, Jul 5, 2023 at 10:15 AM Amit Pundir
wrote:

Hi Marek,

On Wed, 5 Jul 2023 at 01:48, Marek Vasut wrote:

Do not generate the HS front and back porch gaps, the HSA gap and
EOT packet, as these packets are not required. This makes the
bridge

work with Samsung DSIM on i.MX8MM and i.MX8MP.

This patch broke display on Dragonboard 845c (SDM845) devboard
running

AOSP. This is what I see
https://people.linaro.org/~amit.pundir/db845c-userdebug/v6.5-broken-display/PXL_20230704_150156326.jpg.
Reverting this patch fixes this regression for me.

Might be msm dsi host require proper handling on these updated
mode_flags? did they?

The msm DSI host supports those flags. Also, I'd like to point out
that the patch didn't change the rest of the driver code. So even if
drm/msm ignored some of the flags, it should not have caused the
issue. Most likely the issue is on the lt9611 side. I's suspect that
additional programming is required to make it work with these flags.

I spent some time today on smoke testing these flags (individually and
in limited combination) on DB845c, to narrow down this breakage to one
or more flag(s) triggering it. Here are my observations in limited
testing done so far.

There is no regression with MIPI_DSI_MODE_NO_EOT_PACKET when enabled
alone and system boots to UI as usual.

MIPI_DSI_MODE_VIDEO_NO_HFP always trigger the broken display as in the
screenshot[1] shared earlier as well.

Adding either of MIPI_DSI_MODE_VIDEO_NO_HSA and
MIPI_DSI_MODE_VIDEO_NO_HBP always result in no display, unless paired
with MIPI_DSI_MODE_VIDEO_NO_HFP and in that case we get the broken
display as reported.

In short other than MIPI_DSI_MODE_NO_EOT_PACKET flag, all other flags
added in this commit break the display on DB845c one way or another.

I think the investigation would be to understand why samsung-dsim
requires
such flags and/or what are the difference in behavior between MSM DSI
and samsung DSIM

for those flags ?

If someone has access to the lt9611 datasheet, so it requires
HSA/HFP/HBP to be

skipped ? and does MSM DSI and samsung DSIM skip them in the same way ?

I think there's a mismatch, where on one side this flags sets the link
in LP-11 while
in HSA/HFP/HPB while on the other it completely removes those blanking
packets.

The name MIPI_DSI_MODE_VIDEO_NO_HBP suggests removal of HPB, not LP-11
while HPB.

the registers used in both controllers are different:
- samsung-dsim: DSIM_HBP_DISABLE_MODE
- msm dsi: DSI_VID_CFG0_HBP_POWER_STOP

The first one suggest removing the packet, while the second one
suggests powering

off the line while in the blanking packet period.

@Abhinav, can you comment on that ?

I dont get what it means by completely removes blanking packets in DSIM.

MIPI_DSI_MODE_VIDEO_NO_HFP means the HBP period is just skipped by DSIM.

Maybe there is a need for new set of flags which differentiate between
HBP skipped (i.e. NO HBP) and HBP LP11 ?

It should be replacing those periods with LP11 too.

The traffic mode being used on this bridge is
MIPI_DSI_MODE_VIDEO_SYNC_PULSE which is "Non-Burst Mode with Sync Pulses".

As per this traffic mode in the DSI spec,

"Normally, periods shown as HSA (Horizontal Sync Active), HBP
(Horizontal Back Porch) and HFP (Horizontal Front Porch) are filled by
Blanking Packets, with lengths (including packet overhead) calculated
to match the period specified by the peripheral’s data sheet.
Alternatively, if there is sufficient time to transition from HS to LP
mode and back again, a timed interval in LP mode may substitute for a
Blanking Packet, thus saving power. During HSA, HBP and HFP periods, the
bus should stay in the LP-11 state."

So we can either send the blanking packets or transition to LP state and
those 3 flags are controlling exactly that during those periods for MSM
driver.

If you stop sending the blanking packets, you need to replace that gap
with LP.

I don't think that's what MIPI_DSI_MODE_VIDEO_NO_HBP means, the way I
understand MIPI_DSI_MODE_VIDEO_NO_HBP is that it skips the HBP
completely. So if you want HBP, then do not set
MIPI_DSI_MODE_VIDEO_NO_HBP . And if you want LP11 during HBP, that is I
think up to the controller (or maybe another new flag?).

One reason I can think of why this could break with MSM is perhaps we do
not have sufficient time in those periods for the LP-HS transition like
the spec has written.

1) What is the resolution which is getting broken on db845c with this? I
would like to know the full drm_display_mode for it to see how much time
we have in those periods. Is any resolution working or all are broken.

2) I also do not completely

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Jani Nikula

On Wed, 12 Jul 2023, Uwe Kleine-König  wrote:
> Hello,
>
> while I debugged an issue in the imx-lcdc driver I was constantly
> irritated about struct drm_device pointer variables being named "dev"
> because with that name I usually expect a struct device pointer.
>
> I think there is a big benefit when these are all renamed to "drm_dev".
> I have no strong preference here though, so "drmdev" or "drm" are fine
> for me, too. Let the bikesheding begin!
>
> Some statistics:
>
> $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c 
> | sort -n
>   1 struct drm_device *adev_to_drm
>   1 struct drm_device *drm_
>   1 struct drm_device  *drm_dev
>   1 struct drm_device*drm_dev
>   1 struct drm_device *pdev
>   1 struct drm_device *rdev
>   1 struct drm_device *vdev
>   2 struct drm_device *dcss_drv_dev_to_drm
>   2 struct drm_device **ddev
>   2 struct drm_device *drm_dev_alloc
>   2 struct drm_device *mock
>   2 struct drm_device *p_ddev
>   5 struct drm_device *device
>   9 struct drm_device * dev
>  25 struct drm_device *d
>  95 struct drm_device *
> 216 struct drm_device *ddev
> 234 struct drm_device *drm_dev
> 611 struct drm_device *drm
>4190 struct drm_device *dev
>
> This series starts with renaming struct drm_crtc::dev to drm_dev. If
> it's not only me and others like the result of this effort it should be
> followed up by adapting the other structs and the individual usages in
> the different drivers.

I think this is an unnecessary change. In drm, a dev is usually a drm
device, i.e. struct drm_device *. As shown by the numbers above.

If folks insist on following through with this anyway, I'm firmly in the
camp the name should be "drm" and nothing else.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Freedreno] [PATCH 1/2] dt-bindings: display/msm: qcom, sdm845-mdss: add memory-region property

2023-07-12 Thread Krzysztof Kozlowski

On 12/07/2023 15:02, Amit Pundir wrote:
> Add and document the reserved memory region property
> in the qcom,sdm845-mdss schema.
> 
> Signed-off-by: Amit Pundir 

Please keep consistent versioning, so this is new patch in v4.

> ---
>  .../devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml| 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml 
> b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
> index 6ecb00920d7f..3ea1dbd7e317 100644
> --- a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
> @@ -39,6 +39,11 @@ properties:
>interconnect-names:
>  maxItems: 2
>  
> +  memory-region:
> +maxItems: 1
> +description:
> +  Phandle to a node describing a reserved memory region.

Your description says nothing new. It's entirely redundant/obvious.
Instead please describe what reserved memory is expected to be here.


Best regards,
Krzysztof

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Christian König


Am 12.07.23 um 15:38 schrieb Uwe Kleine-König:

Hello Maxime,

On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:

On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:

Background is that this makes merge conflicts easier to handle and detect.

Really?

FWIW, I agree with Christian here.


Each file (apart from include/drm/drm_crtc.h) is only touched once. So
unless I'm missing something you don't get less or easier conflicts by
doing it all in a single patch. But you gain the freedom to drop a
patch for one driver without having to drop the rest with it.

Not really, because the last patch removed the union anyway. So you have
to revert both the last patch, plus that driver one. And then you need
to add a TODO to remove that union eventually.

Yes, with a single patch you have only one revert (but 194 files changed,
1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
bigger). (And maybe you get away with just reverting the last patch.)

With a single patch the TODO after a revert is "redo it all again (and
prepare for a different set of conflicts)" while with the split series
it's only "fix that one driver that was forgotten/borked" + reapply that
10 line patch.


Yeah, but for a maintainer the size of the patches doesn't matter. 
That's only interesting if you need to manually review the patch, which 
you hopefully doesn't do in case of something auto-generated.


In other words if the patch is auto-generated re-applying it completely 
is less work than fixing things up individually.



  As the one who gets that TODO, I prefer the latter.


Yeah, but your personal preferences are not a technical relevant 
argument to a maintainer.


At the end of the day Dave or Daniel need to decide, because they need 
to live with it.


Regards,
Christian.



So in sum: If your metric is "small count of reverted commits", you're
right. If however your metric is: Better get 95% of this series' change
in than maybe 0%, the split series is the way to do it.

With me having spend ~3h on this series' changes, it's maybe
understandable that I did it the way I did.

FTR: This series was created on top of v6.5-rc1. If you apply it to
drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
be the responsible maintainer who applies this series, I like being able
to just do git am --skip then.

FTR#2: In drm-misc-next is a new driver
(drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
now might indeed be a good idea.


So I still like the split version better, but I'm open to a more
verbose reasoning from your side.

You're doing only one thing here, really: you change the name of a
structure field. If it was shared between multiple maintainers, then
sure, splitting that up is easier for everyone, but this will go through
drm-misc, so I can't see the benefit it brings.

I see your argument, but I think mine weights more.

Best regards
Uwe

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Maxime Ripard

On Wed, Jul 12, 2023 at 03:38:03PM +0200, Uwe Kleine-König wrote:
> Hello Maxime,
> 
> On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:
> > On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:
> > > > Background is that this makes merge conflicts easier to handle and 
> > > > detect.
> > > 
> > > Really?
> > 
> > FWIW, I agree with Christian here.
> > 
> > > Each file (apart from include/drm/drm_crtc.h) is only touched once. So
> > > unless I'm missing something you don't get less or easier conflicts by
> > > doing it all in a single patch. But you gain the freedom to drop a
> > > patch for one driver without having to drop the rest with it.
> > 
> > Not really, because the last patch removed the union anyway. So you have
> > to revert both the last patch, plus that driver one. And then you need
> > to add a TODO to remove that union eventually.
> 
> Yes, with a single patch you have only one revert (but 194 files changed,
> 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
> file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
> bigger). (And maybe you get away with just reverting the last patch.)
> 
> With a single patch the TODO after a revert is "redo it all again (and
> prepare for a different set of conflicts)" while with the split series
> it's only "fix that one driver that was forgotten/borked" + reapply that
> 10 line patch. As the one who gets that TODO, I prefer the latter.
> 
> So in sum: If your metric is "small count of reverted commits", you're
> right. If however your metric is: Better get 95% of this series' change
> in than maybe 0%, the split series is the way to do it.

I guess that's where we disagree: I don't see the point of having 95% of
it, either 0 or 100.

> With me having spend ~3h on this series' changes, it's maybe
> understandable that I did it the way I did.

I'm sorry, but that's never been an argument? I'm sure you and I both
have had series that took much longer dropped because it wasn't the
right approach.

> FTR: This series was created on top of v6.5-rc1. If you apply it to
> drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
> be the responsible maintainer who applies this series, I like being able
> to just do git am --skip then. 

Or we can ask that the driver is based on drm-misc-next ...

> FTR#2: In drm-misc-next is a new driver
> (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
> now might indeed be a good idea.

... which is going to fix that one too.

Maxime


signature.asc
Description: PGP signature

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

Hello Maxime,

On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:
> On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:
> > > Background is that this makes merge conflicts easier to handle and detect.
> > 
> > Really?
> 
> FWIW, I agree with Christian here.
> 
> > Each file (apart from include/drm/drm_crtc.h) is only touched once. So
> > unless I'm missing something you don't get less or easier conflicts by
> > doing it all in a single patch. But you gain the freedom to drop a
> > patch for one driver without having to drop the rest with it.
> 
> Not really, because the last patch removed the union anyway. So you have
> to revert both the last patch, plus that driver one. And then you need
> to add a TODO to remove that union eventually.

Yes, with a single patch you have only one revert (but 194 files changed,
1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
bigger). (And maybe you get away with just reverting the last patch.)

With a single patch the TODO after a revert is "redo it all again (and
prepare for a different set of conflicts)" while with the split series
it's only "fix that one driver that was forgotten/borked" + reapply that
10 line patch. As the one who gets that TODO, I prefer the latter.

So in sum: If your metric is "small count of reverted commits", you're
right. If however your metric is: Better get 95% of this series' change
in than maybe 0%, the split series is the way to do it.

With me having spend ~3h on this series' changes, it's maybe
understandable that I did it the way I did.

FTR: This series was created on top of v6.5-rc1. If you apply it to
drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
be the responsible maintainer who applies this series, I like being able
to just do git am --skip then. 

FTR#2: In drm-misc-next is a new driver
(drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
now might indeed be a good idea.

> > So I still like the split version better, but I'm open to a more
> > verbose reasoning from your side.
> 
> You're doing only one thing here, really: you change the name of a
> structure field. If it was shared between multiple maintainers, then
> sure, splitting that up is easier for everyone, but this will go through
> drm-misc, so I can't see the benefit it brings.

I see your argument, but I think mine weights more.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |

signature.asc
Description: PGP signature

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Maxime Ripard

On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:
> > Background is that this makes merge conflicts easier to handle and detect.
> 
> Really?

FWIW, I agree with Christian here.

> Each file (apart from include/drm/drm_crtc.h) is only touched once. So
> unless I'm missing something you don't get less or easier conflicts by
> doing it all in a single patch. But you gain the freedom to drop a
> patch for one driver without having to drop the rest with it.

Not really, because the last patch removed the union anyway. So you have
to revert both the last patch, plus that driver one. And then you need
to add a TODO to remove that union eventually.

> So I still like the split version better, but I'm open to a more
> verbose reasoning from your side.

You're doing only one thing here, really: you change the name of a
structure field. If it was shared between multiple maintainers, then
sure, splitting that up is easier for everyone, but this will go through
drm-misc, so I can't see the benefit it brings.

Maxime

signature.asc
Description: PGP signature

Re: [Freedreno] [PATCH 1/2] dt-bindings: display/msm: qcom, sdm845-mdss: add memory-region property


On 12/07/2023 16:02, Amit Pundir wrote:

Add and document the reserved memory region property
in the qcom,sdm845-mdss schema.

Signed-off-by: Amit Pundir 
---
  .../devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml| 5 +
  1 file changed, 5 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml 
b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
index 6ecb00920d7f..3ea1dbd7e317 100644
--- a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
+++ b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
@@ -39,6 +39,11 @@ properties:
interconnect-names:
  maxItems: 2
  
+  memory-region:

+maxItems: 1
+description:
+  Phandle to a node describing a reserved memory region.
+


Please add it to mdss-common.yaml instead


  patternProperties:
"^display-controller@[0-9a-f]+$":
  type: object


--
With best wishes
Dmitry

[Freedreno] [PATCH 2/2][v4] arm64: dts: qcom: sdm845-db845c: Mark cont splash memory region as reserved

2023-07-12 Thread Amit Pundir

Adding a reserved memory region for the framebuffer memory
(the splash memory region set up by the bootloader).

Signed-off-by: Amit Pundir 
---
v4: Re-sending this along with a new dt-bindings patch to
document memory-region property in qcom,sdm845-mdss
schema and keep dtbs_check happy.

v3: Point this reserved region to MDSS.

v2: Updated commit message.

There was some dicussion on v1 but it didn't go anywhere,
https://lore.kernel.org/linux-kernel/20230124182857.1524912-1-amit.pun...@linaro.org/T/#u.
The general consensus is that this memory should be freed and be
made resuable but that (releasing this piece of memory) has been
tried before and it is not trivial to return the reserved memory
node to the system RAM pool in this case.

 arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts 
b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index d6b464cb61d6..f546f6f57c1e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -101,6 +101,14 @@ hdmi_con: endpoint {
};
};
 
+   reserved-memory {
+   /* Cont splash region set up by the bootloader */
+   cont_splash_mem: framebuffer@9d40 {
+   reg = <0x0 0x9d40 0x0 0x240>;
+   no-map;
+   };
+   };
+
lt9611_1v8: lt9611-vdd18-regulator {
compatible = "regulator-fixed";
regulator-name = "LT9611_1V8";
@@ -506,6 +514,7 @@  {
 };
 
  {
+   memory-region = <_splash_mem>;
status = "okay";
 };
 
-- 
2.25.1

[Freedreno] [PATCH 1/2] dt-bindings: display/msm: qcom, sdm845-mdss: add memory-region property

2023-07-12 Thread Amit Pundir

Add and document the reserved memory region property
in the qcom,sdm845-mdss schema.

Signed-off-by: Amit Pundir 
---
 .../devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml| 5 +
 1 file changed, 5 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml 
b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
index 6ecb00920d7f..3ea1dbd7e317 100644
--- a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
+++ b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml
@@ -39,6 +39,11 @@ properties:
   interconnect-names:
 maxItems: 2
 
+  memory-region:
+maxItems: 1
+description:
+  Phandle to a node describing a reserved memory region.
+
 patternProperties:
   "^display-controller@[0-9a-f]+$":
 type: object
-- 
2.25.1

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Christian König


Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:

Hello,

while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.

I think there is a big benefit when these are all renamed to "drm_dev".
I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!

Some statistics:

$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | 
sort -n
   1 struct drm_device *adev_to_drm
   1 struct drm_device *drm_
   1 struct drm_device  *drm_dev
   1 struct drm_device*drm_dev
   1 struct drm_device *pdev
   1 struct drm_device *rdev
   1 struct drm_device *vdev
   2 struct drm_device *dcss_drv_dev_to_drm
   2 struct drm_device **ddev
   2 struct drm_device *drm_dev_alloc
   2 struct drm_device *mock
   2 struct drm_device *p_ddev
   5 struct drm_device *device
   9 struct drm_device * dev
  25 struct drm_device *d
  95 struct drm_device *
 216 struct drm_device *ddev
 234 struct drm_device *drm_dev
 611 struct drm_device *drm
4190 struct drm_device *dev

This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.

To make this series a bit easier handleable, I first added an alias for
drm_crtc::dev, then converted the drivers one after another and the last
patch drops the "dev" name. This has the advantage of being easier to
review, and if I should have missed an instance only the last patch must
be dropped/reverted. Also this series might conflict with other patches,
in this case the remaining patches can still go in (apart from the last
one of course). Maybe it also makes sense to delay applying the last
patch by one development cycle?


When you automatically generate the patch (with cocci for example) I 
usually prefer a single patch instead.


Background is that this makes merge conflicts easier to handle and detect.

When you have multiple patches and a merge conflict because of some 
added lines using the old field the build breaks only on the last patch 
which removes the old field.


In such cases reviewing the patch just means automatically re-generating 
it and double checking that you don't see anything funky.


Apart from that I honestly absolutely don't care what the name is.

Cheers,
Christian.



The series was compile tested for arm, arm64, powerpc and amd64 using an
allmodconfig (though I only build drivers/gpu/).

Best regards
Uwe

Uwe Kleine-König (52):
   drm/crtc: Start renaming struct drm_crtc::dev to drm_dev
   drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/armada: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/aspeed: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/exynos: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/gma500: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/hyperv: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/ingenic: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/logicvc: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/mediatek: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/meson: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/mgag200: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/nouveau: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/pl111: Use struct

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Andrzej Hajda





On 12.07.2023 13:07, Julia Lawall wrote:


On Wed, 12 Jul 2023, Uwe Kleine-König wrote:


On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote:

Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:

Hello,

while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.

I think there is a big benefit when these are all renamed to "drm_dev".
I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!

Some statistics:

$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | 
sort -n
1 struct drm_device *adev_to_drm
1 struct drm_device *drm_
1 struct drm_device  *drm_dev
1 struct drm_device*drm_dev
1 struct drm_device *pdev
1 struct drm_device *rdev
1 struct drm_device *vdev
2 struct drm_device *dcss_drv_dev_to_drm
2 struct drm_device **ddev
2 struct drm_device *drm_dev_alloc
2 struct drm_device *mock
2 struct drm_device *p_ddev
5 struct drm_device *device
9 struct drm_device * dev
   25 struct drm_device *d
   95 struct drm_device *
  216 struct drm_device *ddev
  234 struct drm_device *drm_dev
  611 struct drm_device *drm
 4190 struct drm_device *dev

This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.

To make this series a bit easier handleable, I first added an alias for
drm_crtc::dev, then converted the drivers one after another and the last
patch drops the "dev" name. This has the advantage of being easier to
review, and if I should have missed an instance only the last patch must
be dropped/reverted. Also this series might conflict with other patches,
in this case the remaining patches can still go in (apart from the last
one of course). Maybe it also makes sense to delay applying the last
patch by one development cycle?

When you automatically generate the patch (with cocci for example) I usually
prefer a single patch instead.

Maybe I'm too stupid, but only parts of this patch were created by
coccinelle. I failed to convert code like

-   spin_lock_irq(>dev->event_lock);
+   spin_lock_irq(>drm_dev->event_lock);

Added Julia to Cc, maybe she has a hint?!

A priori, I see no reason why the rule below should not apply to the above
code.  Is there a parsing problem in the containing function?  You can run

spatch --parse-c file.c

If there is a paring problem, please let me know and i will try to fix it
so the while thing can be done automatically.


I guess some clever macros can fool spatch, at least I observe such 
things in i915 which often uses custom iterators.


Regards
Andrzej



julia


(Up to now it's only

@@
struct drm_crtc *crtc;
@@
-crtc->dev
+crtc->drm_dev

)


Background is that this makes merge conflicts easier to handle and detect.

Really? Each file (apart from include/drm/drm_crtc.h) is only touched
once. So unless I'm missing something you don't get less or easier
conflicts by doing it all in a single patch. But you gain the freedom to
drop a patch for one driver without having to drop the rest with it. So
I still like the split version better, but I'm open to a more verbose
reasoning from your side.


When you have multiple patches and a merge conflict because of some added
lines using the old field the build breaks only on the last patch which
removes the old field.

Then you can revert/drop the last patch without having to respin the
whole single patch and thus caring for still more conflicts that arise
until the new version is sent.

Best regards
Uwe

--
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |

>

[Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

Hello,

while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.

I think there is a big benefit when these are all renamed to "drm_dev".
I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!

Some statistics:

$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | 
sort -n
  1 struct drm_device *adev_to_drm
  1 struct drm_device *drm_
  1 struct drm_device  *drm_dev
  1 struct drm_device*drm_dev
  1 struct drm_device *pdev
  1 struct drm_device *rdev
  1 struct drm_device *vdev
  2 struct drm_device *dcss_drv_dev_to_drm
  2 struct drm_device **ddev
  2 struct drm_device *drm_dev_alloc
  2 struct drm_device *mock
  2 struct drm_device *p_ddev
  5 struct drm_device *device
  9 struct drm_device * dev
 25 struct drm_device *d
 95 struct drm_device *
216 struct drm_device *ddev
234 struct drm_device *drm_dev
611 struct drm_device *drm
   4190 struct drm_device *dev

This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.

To make this series a bit easier handleable, I first added an alias for
drm_crtc::dev, then converted the drivers one after another and the last
patch drops the "dev" name. This has the advantage of being easier to
review, and if I should have missed an instance only the last patch must
be dropped/reverted. Also this series might conflict with other patches,
in this case the remaining patches can still go in (apart from the last
one of course). Maybe it also makes sense to delay applying the last
patch by one development cycle?

The series was compile tested for arm, arm64, powerpc and amd64 using an
allmodconfig (though I only build drivers/gpu/).

Best regards
Uwe

Uwe Kleine-König (52):
  drm/crtc: Start renaming struct drm_crtc::dev to drm_dev
  drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/armada: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/aspeed: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/exynos: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/gma500: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/hyperv: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/ingenic: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/logicvc: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/mediatek: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/meson: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/mgag200: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/nouveau: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/pl111: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/qxl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/radeon: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/renesas: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/rockchip: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/solomon: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/sprd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/sti: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/stm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/sun4i: Use struct

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Paul Kocialkowski

Hi Uwe,

On Wed 12 Jul 23, 11:46, Uwe Kleine-König wrote:
> Hello,
> 
> while I debugged an issue in the imx-lcdc driver I was constantly
> irritated about struct drm_device pointer variables being named "dev"
> because with that name I usually expect a struct device pointer.

Well personally I usually expect that the "dev" member of a subsystem-specific
struct refers to a device of that subsystem, so for me having "dev" refer to
a drm_device for e.g. drm_crtc makes good sense.

I would only expect dev to refer to a struct device in the subsystem-specific
device structure (drm_device). I don't think it makes much sense to carry
the struct device in any other subsystem-specific structure anyway.

So IMO things are fine as-is but this is not a very strong opinion either.

> I think there is a big benefit when these are all renamed to "drm_dev".
> I have no strong preference here though, so "drmdev" or "drm" are fine
> for me, too. Let the bikesheding begin!

I would definitely prefer "drm_dev" over "drmdev" (hard to read, feels like
aborted camelcase, pretty ugly) or "drm" (too vague).

Cheers,

Paul

> Some statistics:
> 
> $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c 
> | sort -n
>   1 struct drm_device *adev_to_drm
>   1 struct drm_device *drm_
>   1 struct drm_device  *drm_dev
>   1 struct drm_device*drm_dev
>   1 struct drm_device *pdev
>   1 struct drm_device *rdev
>   1 struct drm_device *vdev
>   2 struct drm_device *dcss_drv_dev_to_drm
>   2 struct drm_device **ddev
>   2 struct drm_device *drm_dev_alloc
>   2 struct drm_device *mock
>   2 struct drm_device *p_ddev
>   5 struct drm_device *device
>   9 struct drm_device * dev
>  25 struct drm_device *d
>  95 struct drm_device *
> 216 struct drm_device *ddev
> 234 struct drm_device *drm_dev
> 611 struct drm_device *drm
>4190 struct drm_device *dev
> 
> This series starts with renaming struct drm_crtc::dev to drm_dev. If
> it's not only me and others like the result of this effort it should be
> followed up by adapting the other structs and the individual usages in
> the different drivers.
> 
> To make this series a bit easier handleable, I first added an alias for
> drm_crtc::dev, then converted the drivers one after another and the last
> patch drops the "dev" name. This has the advantage of being easier to
> review, and if I should have missed an instance only the last patch must
> be dropped/reverted. Also this series might conflict with other patches,
> in this case the remaining patches can still go in (apart from the last
> one of course). Maybe it also makes sense to delay applying the last
> patch by one development cycle?
> 
> The series was compile tested for arm, arm64, powerpc and amd64 using an
> allmodconfig (though I only build drivers/gpu/).
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (52):
>   drm/crtc: Start renaming struct drm_crtc::dev to drm_dev
>   drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/armada: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/aspeed: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/exynos: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/gma500: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/hyperv: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/ingenic: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/logicvc: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/mediatek: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/meson: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/mgag200: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/nouveau: Use struct drm_crtc::drm_dev instead of struct
>

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Julia Lawall



On Wed, 12 Jul 2023, Uwe Kleine-König wrote:

> On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote:
> > Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:
> > > Hello,
> > >
> > > while I debugged an issue in the imx-lcdc driver I was constantly
> > > irritated about struct drm_device pointer variables being named "dev"
> > > because with that name I usually expect a struct device pointer.
> > >
> > > I think there is a big benefit when these are all renamed to "drm_dev".
> > > I have no strong preference here though, so "drmdev" or "drm" are fine
> > > for me, too. Let the bikesheding begin!
> > >
> > > Some statistics:
> > >
> > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq 
> > > -c | sort -n
> > >1 struct drm_device *adev_to_drm
> > >1 struct drm_device *drm_
> > >1 struct drm_device  *drm_dev
> > >1 struct drm_device*drm_dev
> > >1 struct drm_device *pdev
> > >1 struct drm_device *rdev
> > >1 struct drm_device *vdev
> > >2 struct drm_device *dcss_drv_dev_to_drm
> > >2 struct drm_device **ddev
> > >2 struct drm_device *drm_dev_alloc
> > >2 struct drm_device *mock
> > >2 struct drm_device *p_ddev
> > >5 struct drm_device *device
> > >9 struct drm_device * dev
> > >   25 struct drm_device *d
> > >   95 struct drm_device *
> > >  216 struct drm_device *ddev
> > >  234 struct drm_device *drm_dev
> > >  611 struct drm_device *drm
> > > 4190 struct drm_device *dev
> > >
> > > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > > it's not only me and others like the result of this effort it should be
> > > followed up by adapting the other structs and the individual usages in
> > > the different drivers.
> > >
> > > To make this series a bit easier handleable, I first added an alias for
> > > drm_crtc::dev, then converted the drivers one after another and the last
> > > patch drops the "dev" name. This has the advantage of being easier to
> > > review, and if I should have missed an instance only the last patch must
> > > be dropped/reverted. Also this series might conflict with other patches,
> > > in this case the remaining patches can still go in (apart from the last
> > > one of course). Maybe it also makes sense to delay applying the last
> > > patch by one development cycle?
> >
> > When you automatically generate the patch (with cocci for example) I usually
> > prefer a single patch instead.
>
> Maybe I'm too stupid, but only parts of this patch were created by
> coccinelle. I failed to convert code like
>
> -   spin_lock_irq(>dev->event_lock);
> +   spin_lock_irq(>drm_dev->event_lock);
>
> Added Julia to Cc, maybe she has a hint?!

A priori, I see no reason why the rule below should not apply to the above
code.  Is there a parsing problem in the containing function?  You can run

spatch --parse-c file.c

If there is a paring problem, please let me know and i will try to fix it
so the while thing can be done automatically.

julia

>
> (Up to now it's only
>
> @@
> struct drm_crtc *crtc;
> @@
> -crtc->dev
> +crtc->drm_dev
>
> )
>
> > Background is that this makes merge conflicts easier to handle and detect.
>
> Really? Each file (apart from include/drm/drm_crtc.h) is only touched
> once. So unless I'm missing something you don't get less or easier
> conflicts by doing it all in a single patch. But you gain the freedom to
> drop a patch for one driver without having to drop the rest with it. So
> I still like the split version better, but I'm open to a more verbose
> reasoning from your side.
>
> > When you have multiple patches and a merge conflict because of some added
> > lines using the old field the build breaks only on the last patch which
> > removes the old field.
>
> Then you can revert/drop the last patch without having to respin the
> whole single patch and thus caring for still more conflicts that arise
> until the new version is sent.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.   | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |
>

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

Hello Thomas,

On Wed, Jul 12, 2023 at 12:19:37PM +0200, Thomas Zimmermann wrote:
> Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:
> > Hello,
> > 
> > while I debugged an issue in the imx-lcdc driver I was constantly
> > irritated about struct drm_device pointer variables being named "dev"
> > because with that name I usually expect a struct device pointer.
> > 
> > I think there is a big benefit when these are all renamed to "drm_dev".
> 
> If you rename drm_crtc.dev, you should also address *all* other data
> structures.

Yes. Changing drm_crtc::dev was some effort, so I thought to send that
one out before doing the same to

drm_dp_mst_topology_mgr
drm_atomic_state
drm_master
drm_bridge
drm_client_dev
drm_connector
drm_debugfs_entry
drm_encoder
drm_fb_helper
drm_minor
drm_framebuffer
drm_gem_object
drm_plane
drm_property
drm_property_blob
drm_vblank_crtc

when in the end the intention isn't welcome.

> > I have no strong preference here though, so "drmdev" or "drm" are fine
> > for me, too. Let the bikesheding begin!
> 
> We've discussed this to death. IIRC 'drm' would be the prefered choice.

"drm" at least has the advantage to be the 2nd most common name. With
Paul Kocialkowski prefering "drm_dev" there is no clear favourite yet.
Maybe all the other people with strong opinions are dead if this was
"discussed to death" before? :-)

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote:
> Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:
> > Hello,
> > 
> > while I debugged an issue in the imx-lcdc driver I was constantly
> > irritated about struct drm_device pointer variables being named "dev"
> > because with that name I usually expect a struct device pointer.
> > 
> > I think there is a big benefit when these are all renamed to "drm_dev".
> > I have no strong preference here though, so "drmdev" or "drm" are fine
> > for me, too. Let the bikesheding begin!
> > 
> > Some statistics:
> > 
> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq 
> > -c | sort -n
> >1 struct drm_device *adev_to_drm
> >1 struct drm_device *drm_
> >1 struct drm_device  *drm_dev
> >1 struct drm_device*drm_dev
> >1 struct drm_device *pdev
> >1 struct drm_device *rdev
> >1 struct drm_device *vdev
> >2 struct drm_device *dcss_drv_dev_to_drm
> >2 struct drm_device **ddev
> >2 struct drm_device *drm_dev_alloc
> >2 struct drm_device *mock
> >2 struct drm_device *p_ddev
> >5 struct drm_device *device
> >9 struct drm_device * dev
> >   25 struct drm_device *d
> >   95 struct drm_device *
> >  216 struct drm_device *ddev
> >  234 struct drm_device *drm_dev
> >  611 struct drm_device *drm
> > 4190 struct drm_device *dev
> > 
> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > it's not only me and others like the result of this effort it should be
> > followed up by adapting the other structs and the individual usages in
> > the different drivers.
> > 
> > To make this series a bit easier handleable, I first added an alias for
> > drm_crtc::dev, then converted the drivers one after another and the last
> > patch drops the "dev" name. This has the advantage of being easier to
> > review, and if I should have missed an instance only the last patch must
> > be dropped/reverted. Also this series might conflict with other patches,
> > in this case the remaining patches can still go in (apart from the last
> > one of course). Maybe it also makes sense to delay applying the last
> > patch by one development cycle?
> 
> When you automatically generate the patch (with cocci for example) I usually
> prefer a single patch instead.

Maybe I'm too stupid, but only parts of this patch were created by
coccinelle. I failed to convert code like

-   spin_lock_irq(>dev->event_lock);
+   spin_lock_irq(>drm_dev->event_lock);

Added Julia to Cc, maybe she has a hint?!

(Up to now it's only 

@@
struct drm_crtc *crtc;
@@
-crtc->dev
+crtc->drm_dev

)

> Background is that this makes merge conflicts easier to handle and detect.

Really? Each file (apart from include/drm/drm_crtc.h) is only touched
once. So unless I'm missing something you don't get less or easier
conflicts by doing it all in a single patch. But you gain the freedom to
drop a patch for one driver without having to drop the rest with it. So
I still like the split version better, but I'm open to a more verbose
reasoning from your side.

> When you have multiple patches and a merge conflict because of some added
> lines using the old field the build breaks only on the last patch which
> removes the old field.

Then you can revert/drop the last patch without having to respin the
whole single patch and thus caring for still more conflicts that arise
until the new version is sent.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Thomas Zimmermann


Hi

Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:

Hello,

while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.

I think there is a big benefit when these are all renamed to "drm_dev".


If you rename drm_crtc.dev, you should also address *all* other data 
structures.



I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!


We've discussed this to death. IIRC 'drm' would be the prefered choice.

Best regards
Thomas



Some statistics:

$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | 
sort -n
   1 struct drm_device *adev_to_drm
   1 struct drm_device *drm_
   1 struct drm_device  *drm_dev
   1 struct drm_device*drm_dev
   1 struct drm_device *pdev
   1 struct drm_device *rdev
   1 struct drm_device *vdev
   2 struct drm_device *dcss_drv_dev_to_drm
   2 struct drm_device **ddev
   2 struct drm_device *drm_dev_alloc
   2 struct drm_device *mock
   2 struct drm_device *p_ddev
   5 struct drm_device *device
   9 struct drm_device * dev
  25 struct drm_device *d
  95 struct drm_device *
 216 struct drm_device *ddev
 234 struct drm_device *drm_dev
 611 struct drm_device *drm
4190 struct drm_device *dev

This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.

To make this series a bit easier handleable, I first added an alias for
drm_crtc::dev, then converted the drivers one after another and the last
patch drops the "dev" name. This has the advantage of being easier to
review, and if I should have missed an instance only the last patch must
be dropped/reverted. Also this series might conflict with other patches,
in this case the remaining patches can still go in (apart from the last
one of course). Maybe it also makes sense to delay applying the last
patch by one development cycle?

The series was compile tested for arm, arm64, powerpc and amd64 using an
allmodconfig (though I only build drivers/gpu/).

Best regards
Uwe

Uwe Kleine-König (52):
   drm/crtc: Start renaming struct drm_crtc::dev to drm_dev
   drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/armada: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/aspeed: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/exynos: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/gma500: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/hyperv: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/ingenic: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/logicvc: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/mediatek: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/meson: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/mgag200: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/nouveau: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/pl111: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/qxl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/radeon: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/renesas: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/rockchip: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/solomon: Use

[Freedreno] [PATCH v2 8/8] arm64: dts: qcom: sm8450: provide MDSS cfg interconnect

Add support for the MDSS cfg-cpu bus vote on the SM8450 platform.

Signed-off-by: Dmitry Baryshkov 
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi 
b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 595533aeafc4..0b01f3027ee3 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -2672,8 +2673,12 @@ mdss: display-subsystem@ae0 {
 
/* same path used twice */
interconnects = <_noc MASTER_MDP_DISP 0 _virt 
SLAVE_EBI1_DISP 0>,
-   <_noc MASTER_MDP_DISP 0 _virt 
SLAVE_EBI1_DISP 0>;
-   interconnect-names = "mdp0-mem", "mdp1-mem";
+   <_noc MASTER_MDP_DISP 0 _virt 
SLAVE_EBI1_DISP 0>,
+   <_noc MASTER_APPSS_PROC 
QCOM_ICC_TAG_ACTIVE_ONLY
+_noc SLAVE_DISPLAY_CFG 
QCOM_ICC_TAG_ACTIVE_ONLY>;
+   interconnect-names = "mdp0-mem",
+"mdp1-mem",
+"cpu-cfg";
 
resets = < DISP_CC_MDSS_CORE_BCR>;
 
-- 
2.40.1

[Freedreno] [PATCH v2 7/8] drm/msm/mdss: Handle the reg bus ICC path

Apart from the already handled data bus (MAS_MDP_Pn<->DDR), there's
another path that needs to be handled to ensure MDSS functions properly,
namely the "reg bus", a.k.a the CPU-MDSS interconnect.

Gating that path may have a variety of effects, from none to otherwise
inexplicable DSI timeouts.

Provide a way for MDSS driver to vote on this bus.

A note regarding vote values. Newer platforms have corresponding
bandwidth values in the vendor DT files. For the older platforms there
was a static vote in the mdss_mdp and rotator drivers. I choose to be
conservative here and choose this value as a default.

Co-developed-by: Konrad Dybcio 
Signed-off-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/msm_mdss.c | 51 +++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index b7765e63d549..ee31a9ab88d4 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -26,6 +26,8 @@
 
 #define MIN_IB_BW  4UL /* Min ib vote 400MB */
 
+#define DEFAULT_REG_BW 15360UL /* Used in mdss fbdev driver */
+
 struct msm_mdss_data {
u32 ubwc_version;
/* can be read from register 0x58 */
@@ -34,6 +36,8 @@ struct msm_mdss_data {
u32 ubwc_static;
u32 highest_bank_bit;
u32 macrotile_mode;
+
+   unsigned long reg_bus_bw;
 };
 
 struct msm_mdss {
@@ -50,6 +54,7 @@ struct msm_mdss {
const struct msm_mdss_data *mdss_data;
struct icc_path *mdp_path[2];
u32 num_mdp_paths;
+   struct icc_path *reg_bus_path;
 };
 
 static int msm_mdss_parse_data_bus_icc_path(struct device *dev,
@@ -57,6 +62,7 @@ static int msm_mdss_parse_data_bus_icc_path(struct device 
*dev,
 {
struct icc_path *path0;
struct icc_path *path1;
+   struct icc_path *reg_bus_path;
 
path0 = devm_of_icc_get(dev, "mdp0-mem");
if (IS_ERR_OR_NULL(path0))
@@ -71,6 +77,10 @@ static int msm_mdss_parse_data_bus_icc_path(struct device 
*dev,
msm_mdss->num_mdp_paths++;
}
 
+   reg_bus_path = of_icc_get(dev, "cpu-cfg");
+   if (!IS_ERR_OR_NULL(reg_bus_path))
+   msm_mdss->reg_bus_path = reg_bus_path;
+
return 0;
 }
 
@@ -231,6 +241,13 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
for (i = 0; i < msm_mdss->num_mdp_paths; i++)
icc_set_bw(msm_mdss->mdp_path[i], 0, Bps_to_icc(MIN_IB_BW));
 
+   if (msm_mdss->mdss_data && msm_mdss->mdss_data->reg_bus_bw)
+   icc_set_bw(msm_mdss->reg_bus_path, 0,
+  Bps_to_icc(msm_mdss->mdss_data->reg_bus_bw));
+   else
+   icc_set_bw(msm_mdss->reg_bus_path, 0,
+  Bps_to_icc(DEFAULT_REG_BW));
+
ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
if (ret) {
dev_err(msm_mdss->dev, "clock enable failed, ret:%d\n", ret);
@@ -288,6 +305,9 @@ static int msm_mdss_disable(struct msm_mdss *msm_mdss)
for (i = 0; i < msm_mdss->num_mdp_paths; i++)
icc_set_bw(msm_mdss->mdp_path[i], 0, 0);
 
+   if (msm_mdss->reg_bus_path)
+   icc_set_bw(msm_mdss->reg_bus_path, 0, 0);
+
return 0;
 }
 
@@ -374,6 +394,8 @@ static struct msm_mdss *msm_mdss_init(struct 
platform_device *pdev, bool is_mdp5
if (!msm_mdss)
return ERR_PTR(-ENOMEM);
 
+   msm_mdss->mdss_data = of_device_get_match_data(>dev);
+
msm_mdss->mmio = devm_platform_ioremap_resource_byname(pdev, is_mdp5 ? 
"mdss_phys" : "mdss");
if (IS_ERR(msm_mdss->mmio))
return ERR_CAST(msm_mdss->mmio);
@@ -464,8 +486,6 @@ static int mdss_probe(struct platform_device *pdev)
if (IS_ERR(mdss))
return PTR_ERR(mdss);
 
-   mdss->mdss_data = of_device_get_match_data(>dev);
-
platform_set_drvdata(pdev, mdss);
 
/*
@@ -499,11 +519,13 @@ static const struct msm_mdss_data msm8998_data = {
.ubwc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_1_0,
.highest_bank_bit = 1,
+   .reg_bus_bw = 76800 * 1000,
 };
 
 static const struct msm_mdss_data qcm2290_data = {
/* no UBWC */
.highest_bank_bit = 0x2,
+   .reg_bus_bw = 76800 * 1000,
 };
 
 static const struct msm_mdss_data sc7180_data = {
@@ -511,6 +533,7 @@ static const struct msm_mdss_data sc7180_data = {
.ubwc_dec_version = UBWC_2_0,
.ubwc_static = 0x1e,
.highest_bank_bit = 0x3,
+   .reg_bus_bw = 76800 * 1000,
 };
 
 static const struct msm_mdss_data sc7280_data = {
@@ -520,6 +543,7 @@ static const struct msm_mdss_data sc7280_data = {
.ubwc_static = 1,
.highest_bank_bit = 1,
.macrotile_mode = 1,
+   .reg_bus_bw = 74000 * 1000,
 };
 
 static const struct msm_mdss_data sc8180x_data = {
@@ -527,6 +551,7 @@ static const struct msm_mdss_data sc8180x_data = {

[Freedreno] [PATCH v2 6/8] drm/msm/mdss: populate missing data

As we are going to use MDSS data for DPU programming, populate missing
MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
programming, so skip them.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/msm_mdss.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index eed96976e260..b7765e63d549 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -252,6 +252,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 * UBWC_n and the rest of params comes from hw data.
 */
switch (msm_mdss->mdss_data->ubwc_dec_version) {
+   case 0: /* no UBWC */
+   case UBWC_1_0:
+   /* do nothing */
+   break;
case UBWC_2_0:
msm_mdss_setup_ubwc_dec_20(msm_mdss);
break;
@@ -491,10 +495,22 @@ static int mdss_remove(struct platform_device *pdev)
return 0;
 }
 
+static const struct msm_mdss_data msm8998_data = {
+   .ubwc_version = UBWC_1_0,
+   .ubwc_dec_version = UBWC_1_0,
+   .highest_bank_bit = 1,
+};
+
+static const struct msm_mdss_data qcm2290_data = {
+   /* no UBWC */
+   .highest_bank_bit = 0x2,
+};
+
 static const struct msm_mdss_data sc7180_data = {
.ubwc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_static = 0x1e,
+   .highest_bank_bit = 0x3,
 };
 
 static const struct msm_mdss_data sc7280_data = {
@@ -547,6 +563,7 @@ static const struct msm_mdss_data sm6115_data = {
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = 7,
.ubwc_static = 0x11f,
+   .highest_bank_bit = 0x1,
 };
 
 static const struct msm_mdss_data sm8250_data = {
@@ -571,8 +588,8 @@ static const struct msm_mdss_data sm8550_data = {
 
 static const struct of_device_id mdss_dt_match[] = {
{ .compatible = "qcom,mdss" },
-   { .compatible = "qcom,msm8998-mdss" },
-   { .compatible = "qcom,qcm2290-mdss" },
+   { .compatible = "qcom,msm8998-mdss", .data = _data },
+   { .compatible = "qcom,qcm2290-mdss", .data = _data },
{ .compatible = "qcom,sdm845-mdss", .data = _data },
{ .compatible = "qcom,sc7180-mdss", .data = _data },
{ .compatible = "qcom,sc7280-mdss", .data = _data },
-- 
2.40.1

[Freedreno] [PATCH v2 1/8] dt-bindings: display/msm: Add reg bus and rotator interconnects

From: Konrad Dybcio 

Apart from the already handled data bus (MAS_MDP_Pn<->DDR), there are
other connection paths:
- a path that connects rotator block to the DDR.
- a path that needs to be handled to ensure MDSS register access
  functions properly, namely the "reg bus", a.k.a the CPU-MDSS CFG
  interconnect.

Describe these paths bindings to allow using them in device trees and in
the driver

Signed-off-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
 Documentation/devicetree/bindings/display/msm/mdss-common.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml 
b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
index ccd7d6417523..30a8aed4289a 100644
--- a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
+++ b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
@@ -66,12 +66,14 @@ properties:
 items:
   - description: Interconnect path from mdp0 (or a single mdp) port to the 
data bus
   - description: Interconnect path from mdp1 port to the data bus
+  - description: Interconnect path from CPU to the reg bus
 
   interconnect-names:
 minItems: 1
 items:
   - const: mdp0-mem
   - const: mdp1-mem
+  - const: cpu-cfg
 
   resets:
 items:
-- 
2.40.1

[Freedreno] [PATCH v2 4/8] drm/msm/mdss: Rename path references to mdp_path

From: Konrad Dybcio 

The DPU1 driver needs to handle all MDPn<->DDR paths, as well as
CPU<->SLAVE_DISPLAY_CFG. The former ones share how their values are
calculated, but the latter one has static predefines spanning all SoCs.

In preparation for supporting the CPU<->SLAVE_DISPLAY_CFG path, rename
the path-related struct members to include "mdp_".

Signed-off-by: Konrad Dybcio 
Reviewed-by: Dmitry Baryshkov 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/msm_mdss.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 304a6509e1fb..809c93b22c9c 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -48,8 +48,8 @@ struct msm_mdss {
struct irq_domain *domain;
} irq_controller;
const struct msm_mdss_data *mdss_data;
-   struct icc_path *path[2];
-   u32 num_paths;
+   struct icc_path *mdp_path[2];
+   u32 num_mdp_paths;
 };
 
 static int msm_mdss_parse_data_bus_icc_path(struct device *dev,
@@ -62,13 +62,13 @@ static int msm_mdss_parse_data_bus_icc_path(struct device 
*dev,
if (IS_ERR_OR_NULL(path0))
return PTR_ERR_OR_ZERO(path0);
 
-   msm_mdss->path[0] = path0;
-   msm_mdss->num_paths = 1;
+   msm_mdss->mdp_path[0] = path0;
+   msm_mdss->num_mdp_paths = 1;
 
path1 = devm_of_icc_get(dev, "mdp1-mem");
if (!IS_ERR_OR_NULL(path1)) {
-   msm_mdss->path[1] = path1;
-   msm_mdss->num_paths++;
+   msm_mdss->mdp_path[1] = path1;
+   msm_mdss->num_mdp_paths++;
}
 
return 0;
@@ -78,8 +78,8 @@ static void msm_mdss_icc_request_bw(struct msm_mdss 
*msm_mdss, unsigned long bw)
 {
int i;
 
-   for (i = 0; i < msm_mdss->num_paths; i++)
-   icc_set_bw(msm_mdss->path[i], 0, Bps_to_icc(bw));
+   for (i = 0; i < msm_mdss->num_mdp_paths; i++)
+   icc_set_bw(msm_mdss->mdp_path[i], 0, Bps_to_icc(bw));
 }
 
 static void msm_mdss_irq(struct irq_desc *desc)
-- 
2.40.1

[Freedreno] [PATCH v2 5/8] drm/msm/mdss: inline msm_mdss_icc_request_bw()

There are just two places where we set the bandwidth: in the resume and
in the suspend paths. Drop the wrapping function
msm_mdss_icc_request_bw() and call icc_set_bw() directly.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/msm_mdss.c | 19 ---
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 809c93b22c9c..eed96976e260 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -74,14 +74,6 @@ static int msm_mdss_parse_data_bus_icc_path(struct device 
*dev,
return 0;
 }
 
-static void msm_mdss_icc_request_bw(struct msm_mdss *msm_mdss, unsigned long 
bw)
-{
-   int i;
-
-   for (i = 0; i < msm_mdss->num_mdp_paths; i++)
-   icc_set_bw(msm_mdss->mdp_path[i], 0, Bps_to_icc(bw));
-}
-
 static void msm_mdss_irq(struct irq_desc *desc)
 {
struct msm_mdss *msm_mdss = irq_desc_get_handler_data(desc);
@@ -229,14 +221,15 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss 
*msm_mdss)
 
 static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 {
-   int ret;
+   int ret, i;
 
/*
 * Several components have AXI clocks that can only be turned on if
 * the interconnect is enabled (non-zero bandwidth). Let's make sure
 * that the interconnects are at least at a minimum amount.
 */
-   msm_mdss_icc_request_bw(msm_mdss, MIN_IB_BW);
+   for (i = 0; i < msm_mdss->num_mdp_paths; i++)
+   icc_set_bw(msm_mdss->mdp_path[i], 0, Bps_to_icc(MIN_IB_BW));
 
ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
if (ret) {
@@ -284,8 +277,12 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 
 static int msm_mdss_disable(struct msm_mdss *msm_mdss)
 {
+   int i;
+
clk_bulk_disable_unprepare(msm_mdss->num_clocks, msm_mdss->clocks);
-   msm_mdss_icc_request_bw(msm_mdss, 0);
+
+   for (i = 0; i < msm_mdss->num_mdp_paths; i++)
+   icc_set_bw(msm_mdss->mdp_path[i], 0, 0);
 
return 0;
 }
-- 
2.40.1

[Freedreno] [PATCH v2 3/8] drm/msm/mdss: switch mdss to use devm_of_icc_get()

Stop using hand-written reset function for ICC release, use
devm_of_icc_get() instead.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/msm_mdss.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 798bd4f3b662..304a6509e1fb 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -58,14 +58,14 @@ static int msm_mdss_parse_data_bus_icc_path(struct device 
*dev,
struct icc_path *path0;
struct icc_path *path1;
 
-   path0 = of_icc_get(dev, "mdp0-mem");
+   path0 = devm_of_icc_get(dev, "mdp0-mem");
if (IS_ERR_OR_NULL(path0))
return PTR_ERR_OR_ZERO(path0);
 
msm_mdss->path[0] = path0;
msm_mdss->num_paths = 1;
 
-   path1 = of_icc_get(dev, "mdp1-mem");
+   path1 = devm_of_icc_get(dev, "mdp1-mem");
if (!IS_ERR_OR_NULL(path1)) {
msm_mdss->path[1] = path1;
msm_mdss->num_paths++;
@@ -74,15 +74,6 @@ static int msm_mdss_parse_data_bus_icc_path(struct device 
*dev,
return 0;
 }
 
-static void msm_mdss_put_icc_path(void *data)
-{
-   struct msm_mdss *msm_mdss = data;
-   int i;
-
-   for (i = 0; i < msm_mdss->num_paths; i++)
-   icc_put(msm_mdss->path[i]);
-}
-
 static void msm_mdss_icc_request_bw(struct msm_mdss *msm_mdss, unsigned long 
bw)
 {
int i;
@@ -389,9 +380,6 @@ static struct msm_mdss *msm_mdss_init(struct 
platform_device *pdev, bool is_mdp5
dev_dbg(>dev, "mapped mdss address space @%pK\n", msm_mdss->mmio);
 
ret = msm_mdss_parse_data_bus_icc_path(>dev, msm_mdss);
-   if (ret)
-   return ERR_PTR(ret);
-   ret = devm_add_action_or_reset(>dev, msm_mdss_put_icc_path, 
msm_mdss);
if (ret)
return ERR_PTR(ret);
 
-- 
2.40.1

[Freedreno] [PATCH v2 2/8] drm/msm/mdss: correct UBWC programming for SM8550

The SM8550 platform employs newer UBWC decoder, which requires slightly
different programming.

Fixes: a2f33995c19d ("drm/msm: mdss: add support for SM8550")
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/msm_mdss.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 05648c910c68..798bd4f3b662 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -189,6 +189,7 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss 
*msm_mdss)
 #define UBWC_2_0 0x2000
 #define UBWC_3_0 0x3000
 #define UBWC_4_0 0x4000
+#define UBWC_4_3 0x4003
 
 static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
 {
@@ -227,7 +228,10 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss 
*msm_mdss)
writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
} else {
-   writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
+   if (data->ubwc_dec_version == UBWC_4_3)
+   writel_relaxed(3, msm_mdss->mmio + UBWC_CTRL_2);
+   else
+   writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
}
 }
@@ -271,6 +275,7 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
msm_mdss_setup_ubwc_dec_30(msm_mdss);
break;
case UBWC_4_0:
+   case UBWC_4_3:
msm_mdss_setup_ubwc_dec_40(msm_mdss);
break;
default:
@@ -569,6 +574,16 @@ static const struct msm_mdss_data sm8250_data = {
.macrotile_mode = 1,
 };
 
+static const struct msm_mdss_data sm8550_data = {
+   .ubwc_version = UBWC_4_0,
+   .ubwc_dec_version = UBWC_4_3,
+   .ubwc_swizzle = 6,
+   .ubwc_static = 1,
+   /* TODO: highest_bank_bit = 2 for LP_DDR4 */
+   .highest_bank_bit = 3,
+   .macrotile_mode = 1,
+};
+
 static const struct of_device_id mdss_dt_match[] = {
{ .compatible = "qcom,mdss" },
{ .compatible = "qcom,msm8998-mdss" },
@@ -585,7 +600,7 @@ static const struct of_device_id mdss_dt_match[] = {
{ .compatible = "qcom,sm8250-mdss", .data = _data },
{ .compatible = "qcom,sm8350-mdss", .data = _data },
{ .compatible = "qcom,sm8450-mdss", .data = _data },
-   { .compatible = "qcom,sm8550-mdss", .data = _data },
+   { .compatible = "qcom,sm8550-mdss", .data = _data },
{}
 };
 MODULE_DEVICE_TABLE(of, mdss_dt_match);
-- 
2.40.1

[Freedreno] [PATCH v2 0/8] MDSS reg bus interconnect

Per agreement with Konrad, picked up this patch series.

Apart from the already handled data bus (MAS_MDP_Pn<->DDR), there's
another path that needs to be handled to ensure MDSS functions properly,
namely the "reg bus", a.k.a the CPU-MDSS interconnect.

Gating that path may have a variety of effects. from none to otherwise
inexplicable DSI timeouts.

This series tries to address the lack of that.

Changes since v1:
- Dropped the DPU part, the MDSS vote seems to be enough
- Reworked MDSS voting patch. Replaced static bw value with the
  per-platform confgurable values.
- Added sm8450 DT patch.

Dmitry Baryshkov (6):
  drm/msm/mdss: correct UBWC programming for SM8550
  drm/msm/mdss: switch mdss to use devm_of_icc_get()
  drm/msm/mdss: inline msm_mdss_icc_request_bw()
  drm/msm/mdss: populate missing data
  drm/msm/mdss: Handle the reg bus ICC path
  arm64: dts: qcom: sm8450: provide MDSS cfg interconnect

Konrad Dybcio (2):
  dt-bindings: display/msm: Add reg bus and rotator interconnects
  drm/msm/mdss: Rename path references to mdp_path

 .../bindings/display/msm/mdss-common.yaml |   2 +
 arch/arm64/boot/dts/qcom/sm8450.dtsi  |   9 +-
 drivers/gpu/drm/msm/msm_mdss.c| 138 +-
 3 files changed, 108 insertions(+), 41 deletions(-)

-- 
2.40.1

Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Javier Martinez Canillas

Uwe Kleine-König  writes:

[dropping some recipients since my SMTP server was complaining about the size]

> Hello Thomas,
>
> On Wed, Jul 12, 2023 at 12:19:37PM +0200, Thomas Zimmermann wrote:
>> Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:
>> > Hello,
>> > 
>> > while I debugged an issue in the imx-lcdc driver I was constantly
>> > irritated about struct drm_device pointer variables being named "dev"
>> > because with that name I usually expect a struct device pointer.
>> > 
>> > I think there is a big benefit when these are all renamed to "drm_dev".
>> 
>> If you rename drm_crtc.dev, you should also address *all* other data
>> structures.
>
> Yes. Changing drm_crtc::dev was some effort, so I thought to send that
> one out before doing the same to
>
>   drm_dp_mst_topology_mgr
>   drm_atomic_state
>   drm_master
>   drm_bridge
>   drm_client_dev
>   drm_connector
>   drm_debugfs_entry
>   drm_encoder
>   drm_fb_helper
>   drm_minor
>   drm_framebuffer
>   drm_gem_object
>   drm_plane
>   drm_property
>   drm_property_blob
>   drm_vblank_crtc
>
> when in the end the intention isn't welcome.
>
>> > I have no strong preference here though, so "drmdev" or "drm" are fine
>> > for me, too. Let the bikesheding begin!
>> 
>> We've discussed this to death. IIRC 'drm' would be the prefered choice.
>
> "drm" at least has the advantage to be the 2nd most common name. With
> Paul Kocialkowski prefering "drm_dev" there is no clear favourite yet.

I think that either "drm" or "drm_dev" would be more clear than "dev",
which I also found it confusing and thinking about a "struct device".

Probably leaning to "drm", since as you said is the second most used name
in drivers that assign crtc->dev to a local variable.

> Maybe all the other people with strong opinions are dead if this was
> "discussed to death" before? :-)
>
> Best regards
> Uwe
>
> -- 
> Pengutronix e.K.   | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [Freedreno] (subset) [PATCH v3 0/4] Qualcomm REFGEN regulator

2023-07-12 Thread Mark Brown

On Mon, 03 Jul 2023 20:15:53 +0200, Konrad Dybcio wrote:
> Recent Qualcomm SoCs have a REFGEN (reference voltage generator) regulator
> responsible for providing a reference voltage to some on-SoC IPs (like DSI
> or PHYs). It can be turned off when unused to save power.
> 
> This series introduces the driver for it and lets the DSI driver
> consume it.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 
for-next

Thanks!

[1/4] dt-bindings: regulator: Describe Qualcomm REFGEN regulator
  commit: d16db38c2a66060ee25c6b86ee7b6d66d40fc8e0
[2/4] regulator: Introduce Qualcomm REFGEN regulator driver
  commit: 7cbfbe23796086fdb72b681e2c182b02acd36a04

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

[Freedreno] [PATCH RFC v1 24/52] drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev