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

[Dmitry Baryshkov]

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.




This chunk looks better that way.




--
With best wishes
Dmitry

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

[Abhinav Kumar]

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.


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.


This chunk looks better that way.

Re: [Freedreno] [PATCH v2] drm/msm/dsi: Enable BURST_MODE for command mode for DSI 6G v1.3+

[Dmitry Baryshkov]

On 27/06/2023 23:31, Jessica Zhang wrote:

During a frame transfer in command mode, there could be frequent
LP11 <-> HS transitions when multiple DCS commands are sent mid-frame or
if the DSI controller is running on slow clock and is throttled. To
minimize frame latency due to these transitions, it is recommended to
send the frame in a single burst.

This feature is supported for DSI 6G 1.3 and above, thus enable burst
mode if supported.

Signed-off-by: Jessica Zhang 
---
Changes in v2:
- Moved MDP_CTRL2 register setting to dsi_ctrl_config() (Dmitry/Marijn)
- Read previous value of MDP_CTRL2 register before writing to it
   (Dmitry)
- Link to v1: 
https://lore.kernel.org/r/20230608-b4-add-burst-mode-v1-1-55dfbcfad...@quicinc.com
---
  drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +++
  1 file changed, 7 insertions(+)


Reviewed-by: Dmitry Baryshkov 



diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 3f6dfb4f9d5a..cdb404885f3c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -752,6 +752,13 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
/* Always insert DCS command */
data |= DSI_CMD_CFG1_INSERT_DCS_COMMAND;
dsi_write(msm_host, REG_DSI_CMD_CFG1, data);
+
+   if (msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
+   msm_host->cfg_hnd->minor >= 
MSM_DSI_6G_VER_MINOR_V1_3) {


Nit: please intent in future to the same level (vim: "set cino=(0").


+   data = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
+   data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
+   dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, data);
+   }
}
  
  	dsi_write(msm_host, REG_DSI_CMD_DMA_CTRL,


---
base-commit: a0364260213c96f6817f7e85cdce293cb743460f
change-id: 20230608-b4-add-burst-mode-a5bb144069fa

Best regards,


--
With best wishes
Dmitry

Re: [Freedreno] [PATCH] drm/msm/a6xx: Fix misleading comment

[Dmitry Baryshkov]

On 30/06/2023 19:20, Rob Clark wrote:

From: Rob Clark 

The range is actually len+1.

Signed-off-by: Rob Clark 
---
  drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Dmitry Baryshkov 

--
With best wishes
Dmitry

Re: [Freedreno] [PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property

[Jessica Zhang]

On 6/30/2023 1:27 AM, Pekka Paalanen wrote:

On Thu, 29 Jun 2023 17:25:00 -0700
Jessica Zhang  wrote:


Document and add support for solid_fill property to drm_plane. In
addition, add support for setting and getting the values for solid_fill.

To enable solid fill planes, userspace must assign a property blob to
the "solid_fill" plane property containing the following information:

struct drm_solid_fill_info {
u8 version;
u32 r, g, b;
};

Signed-off-by: Jessica Zhang 


Hi Jessica,

I've left some general UAPI related comments here.


---
  drivers/gpu/drm/drm_atomic_state_helper.c |  9 +
  drivers/gpu/drm/drm_atomic_uapi.c | 55 +++
  drivers/gpu/drm/drm_blend.c   | 33 +++
  include/drm/drm_blend.h   |  1 +
  include/drm/drm_plane.h   | 43 
  5 files changed, 141 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a42..fe14be2bd2b2 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -253,6 +253,11 @@ void __drm_atomic_helper_plane_state_reset(struct 
drm_plane_state *plane_state,
plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
  
+	if (plane_state->solid_fill_blob) {

+   drm_property_blob_put(plane_state->solid_fill_blob);
+   plane_state->solid_fill_blob = NULL;
+   }
+
if (plane->color_encoding_property) {
if (!drm_object_property_get_default_value(>base,
   
plane->color_encoding_property,
@@ -335,6 +340,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,
if (state->fb)
drm_framebuffer_get(state->fb);
  
+	if (state->solid_fill_blob)

+   drm_property_blob_get(state->solid_fill_blob);
+
state->fence = NULL;
state->commit = NULL;
state->fb_damage_clips = NULL;
@@ -384,6 +392,7 @@ void __drm_atomic_helper_plane_destroy_state(struct 
drm_plane_state *state)
drm_crtc_commit_put(state->commit);
  
  	drm_property_blob_put(state->fb_damage_clips);

+   drm_property_blob_put(state->solid_fill_blob);
  }
  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
  
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c

index d867e7f9f2cd..a28b4ee79444 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct 
drm_connector_state *conn_state,
  }
  EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
  
+static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,

+   struct drm_property_blob *blob)
+{
+   int ret = 0;
+   int blob_version;
+
+   if (blob == state->solid_fill_blob)
+   return 0;
+
+   drm_property_blob_put(state->solid_fill_blob);
+   state->solid_fill_blob = NULL;


Is it ok to destroy old state before it is guaranteed that the new
state is accepted?


Hi Pekka,

Good point. I'll change this behavior so that an error case will keep 
the old solid_fill_blob value.





+
+   memset(>solid_fill, 0, sizeof(state->solid_fill));
+
+   if (blob) {
+   struct drm_solid_fill_info *user_info = (struct 
drm_solid_fill_info *)blob->data;
+
+   if (blob->length != sizeof(struct drm_solid_fill_info)) {
+   drm_dbg_atomic(state->plane->dev,
+  "[PLANE:%d:%s] bad solid fill blob length: 
%zu\n",
+  state->plane->base.id, 
state->plane->name,
+  blob->length);
+   return -EINVAL;
+   }
+
+   blob_version = user_info->version;
+
+   /* Add more versions if necessary */
+   if (blob_version == 1) {
+   state->solid_fill.r = user_info->r;
+   state->solid_fill.g = user_info->g;
+   state->solid_fill.b = user_info->b;
+   } else {
+   drm_dbg_atomic(state->plane->dev,
+  "[PLANE:%d:%s] failed to set solid fill 
(ret=%d)\n",
+  state->plane->base.id, 
state->plane->name,
+  ret);
+   return -EINVAL;


Btw. how does the atomic check machinery work here?

I expect that a TEST_ONLY atomic commit will do all the above checks
and return failure if anything is not right. Right?


Correct, drm_atomic_set_property() will still be called for a TEST_ONLY 
commit, so these checks will still happen and return an error (or set 
the property) when appropriate.





+   

Re: [Freedreno] [PATCH] drm/client: Send hotplug event after registering a client

[Dmitry Baryshkov]

On 10/07/2023 12:10, Thomas Zimmermann wrote:

Generate a hotplug event after registering a client to allow the
client to configure its display. Remove the hotplug calls from the
existing clients for fbdev emulation. This change fixes a concurrency
bug between registering a client and receiving events from the DRM
core. The bug is present in the fbdev emulation of all drivers.

The fbdev emulation currently generates a hotplug event before
registering the client to the device. For each new output, the DRM
core sends an additional hotplug event to each registered client.

If the DRM core detects first output between sending the artificial
hotplug and registering the device, the output's hotplug event gets
lost. If this is the first output, the fbdev console display remains
dark. This has been observed with amdgpu and fbdev-generic.

Fix this by adding hotplug generation directly to the client's
register helper drm_client_register(). Registering the client and
receiving events are serialized by struct drm_device.clientlist_mutex.
So an output is either configured by the initial hotplug event, or
the client has already been registered.

The bug was originally added in commit 6e3f17ee73f7 ("drm/fb-helper:
generic: Call drm_client_add() after setup is done"), in which adding
a client and receiving a hotplug event switched order. It was hidden,
as most hardware and drivers have at least on static output configured.
Other drivers didn't use the internal DRM client or still had struct
drm_mode_config_funcs.output_poll_changed set. That callback handled
hotplug events as well. After not setting the callback in amdgpu in
commit 0e3172bac3f4 ("drm/amdgpu: Don't set struct
drm_driver.output_poll_changed"), amdgpu did not show a framebuffer
console if output events got lost. The bug got copy-pasted from
fbdev-generic into the other fbdev emulation.

Reported-by: Moritz Duge 
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2649
Fixes: 6e3f17ee73f7 ("drm/fb-helper: generic: Call drm_client_add() after setup is 
done")
Fixes: 8ab59da26bc0 ("drm/fb-helper: Move generic fbdev emulation into separate 
source file")
Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA 
helpers")
Fixes: 63c381552f69 ("drm/armada: Implement fbdev emulation as in-kernel 
client")
Fixes: 49953b70e7d3 ("drm/exynos: Implement fbdev emulation as in-kernel 
client")
Fixes: 8f1aaccb04b7 ("drm/gma500: Implement client-based fbdev emulation")
Fixes: 940b869c2f2f ("drm/msm: Implement fbdev emulation as in-kernel client")
Fixes: 9e69bcd88e45 ("drm/omapdrm: Implement fbdev emulation as in-kernel 
client")
Fixes: e317a69fe891 ("drm/radeon: Implement client-based fbdev emulation")
Fixes: 71ec16f45ef8 ("drm/tegra: Implement fbdev emulation as in-kernel client")
Signed-off-by: Thomas Zimmermann 
Tested-by: Moritz Duge 
Tested-by: Torsten Krah 
Tested-by: Paul Schyska 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Noralf Trønnes 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Javier Martinez Canillas 
Cc: Russell King 
Cc: Inki Dae 
Cc: Seung-Woo Kim 
Cc: Kyungmin Park 
Cc: Krzysztof Kozlowski 
Cc: Patrik Jakobsson 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Tomi Valkeinen 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: Thierry Reding 
Cc: Mikko Perttunen 
Cc: dri-de...@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Cc: amd-...@lists.freedesktop.org
Cc: linux-te...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc:  # v5.2+
---
  drivers/gpu/drm/armada/armada_fbdev.c |  4 
  drivers/gpu/drm/drm_client.c  | 21 +
  drivers/gpu/drm/drm_fbdev_dma.c   |  4 
  drivers/gpu/drm/drm_fbdev_generic.c   |  4 
  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |  4 
  drivers/gpu/drm/gma500/fbdev.c|  4 
  drivers/gpu/drm/msm/msm_fbdev.c   |  4 


Reviewed-by: Dmitry Baryshkov  # msm


  drivers/gpu/drm/omapdrm/omap_fbdev.c  |  4 
  drivers/gpu/drm/radeon/radeon_fbdev.c |  4 
  drivers/gpu/drm/tegra/fbdev.c |  4 
  10 files changed, 21 insertions(+), 36 deletions(-)


BTW: As you have been clearing this area. I see that significant amount 
of DRM drivers use exactly the same code for msm_fbdev_client_funcs and 
for the significant part of foo_fbdev_setup(). Do you have any plans for 
moving that into a library / generic code? If not, I can take a look at 
crafting the patch.


--
With best wishes
Dmitry

Re: [Freedreno] [PATCH 07/12] drm/msm/adreno: Move speedbin mapping to device table

[Dmitry Baryshkov]

On 10/07/2023 22:56, Rob Clark wrote:

On Thu, Jul 6, 2023 at 7:54 PM Dmitry Baryshkov
 wrote:


On 07/07/2023 00:10, Rob Clark wrote:

From: Rob Clark 

This simplifies the code.

Signed-off-by: Rob Clark 
---
   drivers/gpu/drm/msm/adreno/a6xx_gpu.c  | 171 ++---
   drivers/gpu/drm/msm/adreno/adreno_device.c |  51 ++
   drivers/gpu/drm/msm/adreno/adreno_gpu.h|  25 +++
   3 files changed, 92 insertions(+), 155 deletions(-)



Interesting hack, I'd say.

Reviewed-by: Dmitry Baryshkov 

Minor nit below.






diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index d5335b99c64c..994ac26ce731 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -72,8 +72,33 @@ struct adreno_info {
   u32 inactive_period;
   const struct adreno_reglist *hwcg;
   u64 address_space_size;
+ /**
+  * @speedbins: Optional table of fuse to speedbin mappings
+  *
+  * Consists of pairs of fuse, index mappings, terminated with
+  * UINT_MAX sentinal.
+  */
+ uint32_t *speedbins;


Would it be better to explicitly list this as pairs of uint32_t? And
then use braces in ADRENO_SPEEDBIN initialisation.


It would mean the sentinel would take 8 bytes instead of 4.. maybe
that is over-thinking it, but it was the reason I just stuck with a
flat table


Guessed so. But we are wasting so much memory already... I think that 
the paired structure would better reflect the data - it's not a flat 
list, but a list of nvmem <-> speedbin pairs.




BR,
-R


   };

+/*
+ * Helper to build a speedbin table, ie. the table:
+ *  fuse | speedbin
+ *  -+-
+ *0  |   0
+ *   169 |   1
+ *   174 |   2
+ *
+ * would be declared as:
+ *
+ * .speedbins = ADRENO_SPEEDBINS(
+ *  0,   0,
+ *  169, 1,
+ *  174, 2
+ *  ),
+ */
+#define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
+
   const struct adreno_info *adreno_info(struct adreno_rev rev);

   struct adreno_gpu {


--
With best wishes
Dmitry



--
With best wishes
Dmitry

Re: [Freedreno] [PATCH 10/12] drm/msm/adreno: Add helper for formating chip-id

[Rob Clark]

On Thu, Jul 6, 2023 at 4:36 PM Konrad Dybcio  wrote:
>
> On 6.07.2023 23:10, Rob Clark wrote:
> > From: Rob Clark 
> >
> > This is used in a few places, including one that is parsed by userspace
> > tools.  So let's standardize it a bit better.
> >
> > Signed-off-by: Rob Clark 
> > ---
> Userspace parsed this weird string instead of the hex-based chipid?
>
> weird^2

AFAICT it is just crashdec (the creatively named tool for parsing gpu
devcore dumps) which parses using "%u.%u.%u.%u"..  I suppose one
_could_ make the argument that, since userspace doesn't yet support
any device where "%x.%x.%x.%x" parsing would be different, we could
get away with switching to hex without it being an ABI break..

BR,
-R

> Konrad
> >  drivers/gpu/drm/msm/adreno/adreno_device.c |  8 +++-
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.c| 19 ---
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.h|  6 ++
> >  3 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index dcd6363ac7b0..fd2e183bce60 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -661,14 +661,12 @@ static int adreno_bind(struct device *dev, struct 
> > device *master, void *data)
> >   info = adreno_info(config.rev);
> >
> >   if (!info) {
> > - dev_warn(drm->dev, "Unknown GPU revision: %u.%u.%u.%u\n",
> > - config.rev.core, config.rev.major,
> > - config.rev.minor, config.rev.patchid);
> > + dev_warn(drm->dev, "Unknown GPU revision: 
> > %"ADRENO_CHIPID_FMT"\n",
> > + ADRENO_CHIPID_ARGS(config.rev));
> >   return -ENXIO;
> >   }
> >
> > - DBG("Found GPU: %u.%u.%u.%u", config.rev.core, config.rev.major,
> > - config.rev.minor, config.rev.patchid);
> > + DBG("Found GPU: %"ADRENO_CHIPID_FMT, ADRENO_CHIPID_ARGS(config.rev));
> >
> >   priv->is_a2xx = info->family < ADRENO_3XX;
> >   priv->has_cached_coherent =
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index 75ff7fb46099..1a982a926f21 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -847,10 +847,9 @@ void adreno_show(struct msm_gpu *gpu, struct 
> > msm_gpu_state *state,
> >   if (IS_ERR_OR_NULL(state))
> >   return;
> >
> > - drm_printf(p, "revision: %d (%d.%d.%d.%d)\n",
> > - adreno_gpu->info->revn, adreno_gpu->rev.core,
> > - adreno_gpu->rev.major, adreno_gpu->rev.minor,
> > - adreno_gpu->rev.patchid);
> > + drm_printf(p, "revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > + adreno_gpu->info->revn,
> > + ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> >   /*
> >* If this is state collected due to iova fault, so fault related info
> >*
> > @@ -921,10 +920,9 @@ void adreno_dump_info(struct msm_gpu *gpu)
> >   struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >   int i;
> >
> > - printk("revision: %d (%d.%d.%d.%d)\n",
> > - adreno_gpu->info->revn, adreno_gpu->rev.core,
> > - adreno_gpu->rev.major, adreno_gpu->rev.minor,
> > - adreno_gpu->rev.patchid);
> > + printk("revision: %u (%"ADRENO_CHIPID_FMT")\n",
> > + adreno_gpu->info->revn,
> > + ADRENO_CHIPID_ARGS(adreno_gpu->rev));
> >
> >   for (i = 0; i < gpu->nr_rings; i++) {
> >   struct msm_ringbuffer *ring = gpu->rb[i];
> > @@ -1105,9 +1103,8 @@ int adreno_gpu_init(struct drm_device *drm, struct 
> > platform_device *pdev,
> >   speedbin = 0x;
> >   adreno_gpu->speedbin = (uint16_t) (0x & speedbin);
> >
> > - gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%d.%d.%d.%d",
> > - rev->core, rev->major, rev->minor,
> > - rev->patchid);
> > + gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> > + ADRENO_CHIPID_ARGS(config->rev));
> >   if (!gpu_name)
> >   return -ENOMEM;
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index 2fa14dcd4e40..73e7155f164c 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -66,6 +66,12 @@ struct adreno_rev {
> >  #define ADRENO_REV(core, major, minor, patchid) \
> >   ((struct adreno_rev){ core, major, minor, patchid })
> >
> > +/* Helper for formating the chip_id in the way that userspace tools like
> > + * crashdec expect.
> > + */
> > +#define ADRENO_CHIPID_FMT "u.%u.%u.%u"
> > +#define ADRENO_CHIPID_ARGS(_r) (_r).core, (_r).major, (_r).minor, 
> > (_r).patchid
> > +
> > 

Re: [Freedreno] [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property

[Dmitry Baryshkov]

On 10/07/2023 22:51, Jessica Zhang wrote:



On 6/30/2023 1:27 AM, Pekka Paalanen wrote:

On Fri, 30 Jun 2023 03:42:28 +0300
Dmitry Baryshkov  wrote:


On 30/06/2023 03:25, Jessica Zhang wrote:

Add support for pixel_source property to drm_plane and related
documentation.

This enum property will allow user to specify a pixel source for the
plane. Possible pixel sources will be defined in the
drm_plane_pixel_source enum.

The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.


I think, this should come before the solid fill property addition. First
you tell that there is a possibility to define other pixel sources, then
additional sources are defined.


Hi,

that would be logical indeed.


Hi Dmitry and Pekka,

Sorry for the delay in response, was out of office last week.

Acked.





Signed-off-by: Jessica Zhang 
---
   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
   drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
   drivers/gpu/drm/drm_blend.c   | 81 
+++

   include/drm/drm_blend.h   |  2 +
   include/drm/drm_plane.h   | 21 
   5 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
b/drivers/gpu/drm/drm_atomic_state_helper.c

index fe14be2bd2b2..86fb876efbe6 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -252,6 +252,7 @@ void 
__drm_atomic_helper_plane_state_reset(struct drm_plane_state 
*plane_state,

   plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
   plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
+    plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
   if (plane_state->solid_fill_blob) {
   drm_property_blob_put(plane_state->solid_fill_blob);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c

index a28b4ee79444..6e59c21af66b 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct 
drm_plane *plane,

   drm_property_blob_put(solid_fill);
   return ret;
+    } else if (property == plane->pixel_source_property) {
+    state->pixel_source = val;
   } else if (property == plane->alpha_property) {
   state->alpha = val;
   } else if (property == plane->blend_mode_property) {


I think, it was pointed out in the discussion that drm_mode_setplane()
(a pre-atomic IOCTL to turn the plane on and off) should also reset
pixel_source to FB.


I don't remember drm_mode_setplane() being mentioned in the pixel_source 
discussion... can you share where it was mentioned?


https://lore.kernel.org/dri-devel/20230627105849.004050b3@eldfell/

Let me quote it here:
"Legacy non-atomic UAPI wrappers can do whatever they want, and program
any (new) properties they want in order to implement the legacy
expectations, so that does not seem to be a problem."




I'd prefer to avoid having driver change the pixel_source directly as it 
could cause some unexpected side effects. In general, I would like 
userspace to assign the value of pixel_source without driver doing 
anything "under the hood".


s/driver/drm core/

We have to remain compatible with old userspace, especially with the 
non-atomic one. If the userspace calls ioctl(DRM_IOCTL_MODE_SETPLANE), 
we have to display the specified FB, no matter what was the value of 
PIXEL_SOURCE before this ioctl.







@@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane 
*plane,

   } else if (property == plane->solid_fill_property) {
   *val = state->solid_fill_blob ?
   state->solid_fill_blob->base.id : 0;
+    } else if (property == plane->pixel_source_property) {
+    *val = state->pixel_source;
   } else if (property == plane->alpha_property) {
   *val = state->alpha;
   } else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 38c3c5d6453a..8c100a957ee2 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -189,6 +189,18 @@
    *    solid_fill is set up with 
drm_plane_create_solid_fill_property(). It

    *    contains pixel data that drivers can use to fill a plane.
    *
+ * pixel_source:
+ *    pixel_source is set up with 
drm_plane_create_pixel_source_property().

+ *    It is used to toggle the source of pixel data for the plane.


Other sources than the selected one are ignored?


Yep, the plane will only display the data from the set pixel_source.

So if pixel_source == FB and solid_fill_blob is non-NULL, 
solid_fill_blob will be ignored and the plane will display the FB that 
is set.


correct.



Will add a note about this in the comment docs.




+ *
+ *    Possible values:


Wouldn't hurt to explicitly mention here that this is an enum.


Acked.




+ *
+ *   

Re: [Freedreno] [PATCH 07/12] drm/msm/adreno: Move speedbin mapping to device table

[Rob Clark]

On Thu, Jul 6, 2023 at 7:54 PM Dmitry Baryshkov
 wrote:
>
> On 07/07/2023 00:10, Rob Clark wrote:
> > From: Rob Clark 
> >
> > This simplifies the code.
> >
> > Signed-off-by: Rob Clark 
> > ---
> >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c  | 171 ++---
> >   drivers/gpu/drm/msm/adreno/adreno_device.c |  51 ++
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.h|  25 +++
> >   3 files changed, 92 insertions(+), 155 deletions(-)
>
>
> Interesting hack, I'd say.
>
> Reviewed-by: Dmitry Baryshkov 
>
> Minor nit below.
>
> >
>
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index d5335b99c64c..994ac26ce731 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -72,8 +72,33 @@ struct adreno_info {
> >   u32 inactive_period;
> >   const struct adreno_reglist *hwcg;
> >   u64 address_space_size;
> > + /**
> > +  * @speedbins: Optional table of fuse to speedbin mappings
> > +  *
> > +  * Consists of pairs of fuse, index mappings, terminated with
> > +  * UINT_MAX sentinal.
> > +  */
> > + uint32_t *speedbins;
>
> Would it be better to explicitly list this as pairs of uint32_t? And
> then use braces in ADRENO_SPEEDBIN initialisation.

It would mean the sentinel would take 8 bytes instead of 4.. maybe
that is over-thinking it, but it was the reason I just stuck with a
flat table

BR,
-R

> >   };
> >
> > +/*
> > + * Helper to build a speedbin table, ie. the table:
> > + *  fuse | speedbin
> > + *  -+-
> > + *0  |   0
> > + *   169 |   1
> > + *   174 |   2
> > + *
> > + * would be declared as:
> > + *
> > + * .speedbins = ADRENO_SPEEDBINS(
> > + *  0,   0,
> > + *  169, 1,
> > + *  174, 2
> > + *  ),
> > + */
> > +#define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
> > +
> >   const struct adreno_info *adreno_info(struct adreno_rev rev);
> >
> >   struct adreno_gpu {
>
> --
> With best wishes
> Dmitry
>

Re: [Freedreno] [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property

[Jessica Zhang]

On 6/30/2023 1:27 AM, Pekka Paalanen wrote:

On Fri, 30 Jun 2023 03:42:28 +0300
Dmitry Baryshkov  wrote:


On 30/06/2023 03:25, Jessica Zhang wrote:

Add support for pixel_source property to drm_plane and related
documentation.

This enum property will allow user to specify a pixel source for the
plane. Possible pixel sources will be defined in the
drm_plane_pixel_source enum.

The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.


I think, this should come before the solid fill property addition. First
you tell that there is a possibility to define other pixel sources, then
additional sources are defined.


Hi,

that would be logical indeed.


Hi Dmitry and Pekka,

Sorry for the delay in response, was out of office last week.

Acked.





Signed-off-by: Jessica Zhang 
---
   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
   drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
   drivers/gpu/drm/drm_blend.c   | 81 
+++
   include/drm/drm_blend.h   |  2 +
   include/drm/drm_plane.h   | 21 
   5 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
b/drivers/gpu/drm/drm_atomic_state_helper.c
index fe14be2bd2b2..86fb876efbe6 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct 
drm_plane_state *plane_state,
   
   	plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;

plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
+   plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
   
   	if (plane_state->solid_fill_blob) {

drm_property_blob_put(plane_state->solid_fill_blob);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index a28b4ee79444..6e59c21af66b 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
drm_property_blob_put(solid_fill);
   
   		return ret;

+   } else if (property == plane->pixel_source_property) {
+   state->pixel_source = val;
} else if (property == plane->alpha_property) {
state->alpha = val;
} else if (property == plane->blend_mode_property) {


I think, it was pointed out in the discussion that drm_mode_setplane()
(a pre-atomic IOCTL to turn the plane on and off) should also reset
pixel_source to FB.


I don't remember drm_mode_setplane() being mentioned in the pixel_source 
discussion... can you share where it was mentioned?


I'd prefer to avoid having driver change the pixel_source directly as it 
could cause some unexpected side effects. In general, I would like 
userspace to assign the value of pixel_source without driver doing 
anything "under the hood".





@@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
} else if (property == plane->solid_fill_property) {
*val = state->solid_fill_blob ?
state->solid_fill_blob->base.id : 0;
+   } else if (property == plane->pixel_source_property) {
+   *val = state->pixel_source;
} else if (property == plane->alpha_property) {
*val = state->alpha;
} else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 38c3c5d6453a..8c100a957ee2 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -189,6 +189,18 @@
*   solid_fill is set up with drm_plane_create_solid_fill_property(). It
*   contains pixel data that drivers can use to fill a plane.
*
+ * pixel_source:
+ * pixel_source is set up with drm_plane_create_pixel_source_property().
+ * It is used to toggle the source of pixel data for the plane.


Other sources than the selected one are ignored?


Yep, the plane will only display the data from the set pixel_source.

So if pixel_source == FB and solid_fill_blob is non-NULL, 
solid_fill_blob will be ignored and the plane will display the FB that 
is set.


Will add a note about this in the comment docs.




+ *
+ * Possible values:


Wouldn't hurt to explicitly mention here that this is an enum.


Acked.




+ *
+ * "FB":
+ * Framebuffer source
+ *
+ * "COLOR":
+ * solid_fill source


I think these two should be more explicit. Framebuffer source is the
usual source from the property "FB_ID". Solid fill source comes from
the property "solid_fill".


Acked.



Why "COLOR" and not, say, "SOLID_FILL"?


Ah, that would make more sense :)

I'll change this to "SOLID_FILL".




+ *
* Note that all the property extensions described here apply either to the
* plane or the CRTC (e.g. for the background color, which currently is 

Re: [Freedreno] [PATCH v1 5/5] drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP

[Dmitry Baryshkov]

[Restored CC list]

On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh  wrote:
>
>
> On 7/7/2023 5:32 PM, Dmitry Baryshkov wrote:
> > On 08/07/2023 02:52, Kuogee Hsieh wrote:
> >> Move of_dp_aux_populate_bus() to dp_display_probe() for eDP
> >> from dp_display_bind() so that probe deferral cases can be
> >> handled effectively
> >>
> >> Signed-off-by: Kuogee Hsieh 
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_aux.c | 25 
> >>   drivers/gpu/drm/msm/dp/dp_display.c | 79
> >> +++--
> >>   2 files changed, 65 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> index c592064..c1baffb 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> @@ -505,6 +505,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
> >>   drm_dp_aux_unregister(dp_aux);
> >>   }
> >>   +static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux,
> >> + unsigned long wait_us)
> >> +{
> >> +int ret;
> >> +struct dp_aux_private *aux;
> >> +
> >> +aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> >> +
> >> +pm_runtime_get_sync(aux->dev);
> >> +ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> >> +pm_runtime_put_sync(aux->dev);
> >> +
> >> +return ret;
> >> +}
> >> +
> >>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
> >> *catalog,
> >> bool is_edp)
> >>   {
> >> @@ -528,6 +543,16 @@ struct drm_dp_aux *dp_aux_get(struct device
> >> *dev, struct dp_catalog *catalog,
> >>   aux->catalog = catalog;
> >>   aux->retry_cnt = 0;
> >>   +/*
> >> + * Use the drm_dp_aux_init() to use the aux adapter
> >> + * before registering aux with the DRM device.
> >> + */
> >> +aux->dp_aux.name = "dpu_dp_aux";
> >> +aux->dp_aux.dev = dev;
> >> +aux->dp_aux.transfer = dp_aux_transfer;
> >> +aux->dp_aux.wait_hpd_asserted = dp_wait_hpd_asserted;
> >> +drm_dp_aux_init(>dp_aux);
> >> +
> >>   return >dp_aux;
> >>   }
> >>   diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 185f1eb..7ed4bea 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev,
> >> struct device *master,
> >>   goto end;
> >>   }
> >>   -pm_runtime_enable(dev);
> >> -pm_runtime_set_autosuspend_delay(dev, 1000);
> >> -pm_runtime_use_autosuspend(dev);
> >> -
> >>   return 0;
> >>   end:
> >>   return rc;
> >> @@ -322,8 +318,6 @@ static void dp_display_unbind(struct device *dev,
> >> struct device *master,
> >> kthread_stop(dp->ev_tsk);
> >>   -of_dp_aux_depopulate_bus(dp->aux);
> >> -
> >>   dp_power_client_deinit(dp->power);
> >>   dp_unregister_audio_driver(dev, dp->audio);
> >>   dp_aux_unregister(dp->aux);
> >> @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc
> >> *dp_display_get_desc(struct platform_device *pde
> >>   return NULL;
> >>   }
> >>   +static void of_dp_aux_depopulate_bus_void(void *data)
> >> +{
> >> +of_dp_aux_depopulate_bus(data);
> >> +}
> >> +
> >> +static int dp_display_auxbus_emulation(struct dp_display_private *dp)
> >
> > Why is it called emulation?
> >
> >> +{
> >> +struct device *dev = >pdev->dev;
> >> +struct device_node *aux_bus;
> >> +int ret = 0;
> >> +
> >> +aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> >> +
> >> +if (aux_bus) {
> >> +ret = devm_of_dp_aux_populate_bus(dp->aux, NULL);
> >
> > And here you missed the whole point of why we have been asking for.
> > Please add a sensible `done_probing' callback, which will call
> > component_add(). This way the DP component will only be registered
> > when the panel has been probed. Keeping us from the component binding
> > retries and corresponding side effects.
> >
> >> +
> >> +devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void,
> >> + dp->aux);
> >
> > Useless, it's already handled by the devm_ part of the
> > devm_of_dp_aux_populate_bus().
> >
> >> +}
> >> +
> >> +return ret;
> >> +}
> >> +
> >>   static int dp_display_probe(struct platform_device *pdev)
> >>   {
> >>   int rc = 0;
> >> @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct
> >> platform_device *pdev)
> >> platform_set_drvdata(pdev, >dp_display);
> >>   +pm_runtime_enable(>dev);
> >> +pm_runtime_set_autosuspend_delay(>dev, 1000);
> >> +pm_runtime_use_autosuspend(>dev);
> >
> > Can we have this in probe right from the patch #2?
>
> no, at patch#2, devm_of_dp_aux_populate_bus() is done ta bind timing.
>
> The device used by pm_runtime_get_sync() of generic_edp_panel_probe()
> which is derived from devm_of_dp_aux_populate_bus() is different the
> >dev here.

Excuse me, I don't get your 

Re: [Freedreno] [PATCH v1 3/5] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Dmitry Baryshkov]

On 10/07/2023 19:52, Kuogee Hsieh wrote:


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

On 08/07/2023 02:52, Kuogee Hsieh wrote:

EV_HPD_INIT_SETUP flag is used to trigger the initialization of external
DP host controller. Since external DP host controller initialization had
been incorporated into pm_runtime_resume(), this flag become obsolete.
Lets get rid of it.


And another question. Between patches #2 and #3 we have both 
INIT_SETUP event and the PM doing dp_display_host_init(). Will it work 
correctly?


yes,  i had added  if (!dp->core_initialized)  into dp_display_host_init().

should I merge this patch into patch #2?


You can remove a call to dp_display_host_init() in patch #2 and then 
drop EV_HOST_INIT / msm_dp_irq_postinstall() here.








Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_display.c | 12 
  1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 2c5706a..44580c2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -55,7 +55,6 @@ enum {
  enum {
  EV_NO_EVENT,
  /* hpd events */
-    EV_HPD_INIT_SETUP,
  EV_HPD_PLUG_INT,
  EV_IRQ_HPD_INT,
  EV_HPD_UNPLUG_INT,
@@ -1119,9 +1118,6 @@ static int hpd_event_thread(void *data)
  spin_unlock_irqrestore(_priv->event_lock, flag);
    switch (todo->event_id) {
-    case EV_HPD_INIT_SETUP:
-    dp_display_host_init(dp_priv);
-    break;
  case EV_HPD_PLUG_INT:
  dp_hpd_plug_handle(dp_priv, todo->data);
  break;
@@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void)
    void msm_dp_irq_postinstall(struct msm_dp *dp_display)
  {
-    struct dp_display_private *dp;
-
-    if (!dp_display)
-    return;
-
-    dp = container_of(dp_display, struct dp_display_private, 
dp_display);

  -    if (!dp_display->is_edp)
-    dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
  }
    bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)




--
With best wishes
Dmitry

Re: [Freedreno] [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Dmitry Baryshkov]

On 10/07/2023 19:57, Kuogee Hsieh wrote:


On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote:

On 08/07/2023 02:52, Kuogee Hsieh wrote:

In preparation of moving edp of_dp_aux_populate_bus() to
dp_display_probe(), move dp_display_request_irq(),
dp->parser->parse() and dp_power_client_init() to dp_display_probe()
too.

Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_display.c | 48 
+

  drivers/gpu/drm/msm/dp/dp_display.h |  1 -
  2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 44580c2..185f1eb 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -290,12 +290,6 @@ static int dp_display_bind(struct device *dev, 
struct device *master,

  goto end;
  }
  -    rc = dp_power_client_init(dp->power);
-    if (rc) {
-    DRM_ERROR("Power client create failed\n");
-    goto end;
-    }
-
  rc = dp_register_audio_driver(dev, dp->audio);
  if (rc) {
  DRM_ERROR("Audio registration Dp failed\n");
@@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct 
dp_display_private *dp)

  goto error;
  }
  +    rc = dp->parser->parse(dp->parser);
+    if (rc) {
+    DRM_ERROR("device tree parsing failed\n");
+    goto error;
+    }
+
  dp->catalog = dp_catalog_get(dev, >parser->io);
  if (IS_ERR(dp->catalog)) {
  rc = PTR_ERR(dp->catalog);
@@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct 
dp_display_private *dp)

  goto error;
  }
  +    rc = dp_power_client_init(dp->power);
+    if (rc) {
+    DRM_ERROR("Power client create failed\n");
+    goto error;
+    }
+
  dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
  if (IS_ERR(dp->aux)) {
  rc = PTR_ERR(dp->aux);
@@ -1196,26 +1202,20 @@ static irqreturn_t dp_display_irq_handler(int 
irq, void *dev_id)

  return ret;
  }
  -int dp_display_request_irq(struct msm_dp *dp_display)
+static int dp_display_request_irq(struct dp_display_private *dp)
  {
  int rc = 0;
-    struct dp_display_private *dp;
-
-    if (!dp_display) {
-    DRM_ERROR("invalid input\n");
-    return -EINVAL;
-    }
-
-    dp = container_of(dp_display, struct dp_display_private, 
dp_display);

+    struct device *dev = >pdev->dev;
  -    dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
  if (!dp->irq) {
-    DRM_ERROR("failed to get irq\n");
-    return -EINVAL;
+    dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
+    if (!dp->irq) {
+    DRM_ERROR("failed to get irq\n");
+    return -EINVAL;
+    }
  }


Use platform_get_irq() from probe() function.


  -    rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
-    dp_display_irq_handler,
+    rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
  IRQF_TRIGGER_HIGH, "dp_display_isr", dp);




  if (rc < 0) {
  DRM_ERROR("failed to request IRQ%u: %d\n",
@@ -1290,6 +1290,8 @@ static int dp_display_probe(struct 
platform_device *pdev)

    platform_set_drvdata(pdev, >dp_display);
  +    dp_display_request_irq(dp);
+


Error checking?
Are we completely ready to handle interrupts at this point?
not until dp_display_host_init() is called which will be called from 
pm_runtime_resume() later.


But once you request_irq(), you should be ready for the IRQs to be 
delivered right away.





  rc = component_add(>dev, _display_comp_ops);
  if (rc) {
  DRM_ERROR("component add failed, rc=%d\n", rc);
@@ -1574,12 +1576,6 @@ int msm_dp_modeset_init(struct msm_dp 
*dp_display, struct drm_device *dev,
    dp_priv = container_of(dp_display, struct dp_display_private, 
dp_display);

  -    ret = dp_display_request_irq(dp_display);
-    if (ret) {
-    DRM_ERROR("request_irq failed, ret=%d\n", ret);
-    return ret;
-    }
-
  ret = dp_display_get_next_bridge(dp_display);
  if (ret)
  return ret;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h 
b/drivers/gpu/drm/msm/dp/dp_display.h

index 1e9415a..b3c08de 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -35,7 +35,6 @@ struct msm_dp {
  int dp_display_set_plugged_cb(struct msm_dp *dp_display,
  hdmi_codec_plugged_cb fn, struct device *codec_dev);
  int dp_display_get_modes(struct msm_dp *dp_display);
-int dp_display_request_irq(struct msm_dp *dp_display);
  bool dp_display_check_video_test(struct msm_dp *dp_display);
  int dp_display_get_test_bpp(struct msm_dp *dp_display);
  void dp_display_signal_audio_start(struct msm_dp *dp_display);




--
With best wishes
Dmitry

Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Dmitry Baryshkov]

On 10/07/2023 19:18, Kuogee Hsieh wrote:


On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote:

On 08/07/2023 02:52, Kuogee Hsieh wrote:

Incorporating pm runtime framework into DP driver so that power
and clock resource handling can be centralized allowing easier
control of these resources in preparation of registering aux bus
uring probe.

Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_aux.c |  3 ++
  drivers/gpu/drm/msm/dp/dp_display.c | 75 
+

  2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 8e3b677..c592064 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

  return -EINVAL;
  }
  +    pm_runtime_get_sync(dp_aux->dev);


Let me quote the function's documentation:
Consider using pm_runtime_resume_and_get() instead of it, especially 
if its return value is checked by the caller, as this is likely to 
result in cleaner code.


pm_runtime_resume_and_get() will call pm_runtime_resume()  every time.

Since aux_transfer is called very frequently, is it just simple to call 
pm_runtiem_get_sync() which will call pm_runtime_reusme() if power 
counter is 0 before increased it.


otherwise it just increase power counter?


As you are adding meaningful runtime PM calls, you have to add error 
checking to these calls. Just calling pm_runtime_get_sync() is not enough.


And once you add error handling, you will see what is the difference 
between two mentioned functions and why one is suggested to be used 
instead of the other one.







So two notes concerning the whole patch:
- error checking is missing
- please use pm_runtime_resume_and_get() instead.


  mutex_lock(>mutex);
  if (!aux->initted) {
  ret = -EIO;
@@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

    exit:
  mutex_unlock(>mutex);
+    pm_runtime_mark_last_busy(dp_aux->dev);
+    pm_runtime_put_autosuspend(dp_aux->dev);
    return ret;
  }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 76f1395..2c5706a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, 
struct device *master,

  goto end;
  }
  +    pm_runtime_enable(dev);


devm_pm_runtime_enable() removes need for a cleanup.


+    pm_runtime_set_autosuspend_delay(dev, 1000);
+    pm_runtime_use_autosuspend(dev);


Why do you want to use autosuspend here?


+
  return 0;
  end:
  return rc;
@@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, 
struct device *master,

  struct dp_display_private *dp = dev_get_dp_display_private(dev);
  struct msm_drm_private *priv = dev_get_drvdata(master);
  -    /* disable all HPD interrupts */
-    if (dp->core_initialized)
-    dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, 
false);

+    pm_runtime_dont_use_autosuspend(dev);
+    pm_runtime_disable(dev);
    kthread_stop(dp->ev_tsk);
  @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
dp_display_private *dp)

  dp->dp_display.connector_type, dp->core_initialized,
  dp->phy_initialized);
  -    dp_power_init(dp->power);
-    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
-    dp_aux_init(dp->aux);
-    dp->core_initialized = true;
+    if (!dp->core_initialized) {
+    dp_power_init(dp->power);
+    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+    dp_aux_init(dp->aux);
+    dp->core_initialized = true;
+    }


Is this relevant to PM runtime? I don't think so.


  }
    static void dp_display_host_deinit(struct dp_display_private *dp)
@@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
dp_display_private *dp)

  dp->dp_display.connector_type, dp->core_initialized,
  dp->phy_initialized);
  -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
-    dp_aux_deinit(dp->aux);
-    dp_power_deinit(dp->power);
-    dp->core_initialized = false;
+    if (dp->core_initialized) {
+    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+    dp_aux_deinit(dp->aux);
+    dp_power_deinit(dp->power);
+    dp->core_initialized = false;
+    }
  }
    static int dp_display_usbpd_configure_cb(struct device *dev)
@@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
platform_device *pdev)

  dp_display_deinit_sub_modules(dp);
    platform_set_drvdata(pdev, NULL);
+    pm_runtime_put_sync_suspend(>dev);
+
+    return 0;
+}
+
+static int dp_pm_runtime_suspend(struct device *dev)
+{
+    struct platform_device *pdev = to_platform_device(dev);
+    struct msm_dp *dp_display = platform_get_drvdata(pdev);
+    struct dp_display_private *dp;
+
+    dp = container_of(dp_display, struct dp_display_private, 
dp_display);

+
+    

Re: [Freedreno] [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Dmitry Baryshkov]

On 10/07/2023 20:25, Kuogee Hsieh wrote:


On 7/9/2023 1:32 PM, Abhinav Kumar wrote:



On 7/9/2023 11:00 AM, Dmitry Baryshkov wrote:
On Sun, 9 Jul 2023 at 20:22, Abhinav Kumar 
 wrote:




On 7/7/2023 5:06 PM, Dmitry Baryshkov wrote:

On 08/07/2023 02:52, Kuogee Hsieh wrote:

Since both pm_runtime_resume() and pm_runtime_suspend() are not
populated at dp_pm_ops. Those pm_runtime_get/put() functions within
dp_power.c will not have any effects in addition to increase/decrease
power counter.


Lie.



Even if the commit text is incorrect, review comments like this are not
helping the patch nor the author and will just get ignored anyway.


The review comment might be overreacting, excuse me. I was really
impressed by the commit message, which contradicts the basic source
code. pm_runtime_get() does a lot more than just increasing the power
counter.



It says within dp_power.c. Nonetheless, please let us know what is 
missing in the context of this patch like Bjorn did to make it an 
effective review and we can correct it. In its current form, the 
review comment is adding no value.



I am new in pm.

Any recommendation to revise this commit test?


I'd say, squash patches 1 and 2 and then state in the commit message 
that you are changing pm_runtime code paths because you want to power up 
the device from the runtime callbacks rather than just waking up the 
device in the power up path.


Generally it is much easier to justify changing from A to B rather than 
just dropping A and then adding B.





Also pm_runtime_xxx() should be executed at top
layer.


Why?



I guess he meant to centralize this around dp_display.c. Will elaborate
while posting the next rev.



Signed-off-by: Kuogee Hsieh 
---
   drivers/gpu/drm/msm/dp/dp_power.c | 9 -
   1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_power.c
b/drivers/gpu/drm/msm/dp/dp_power.c
index 5cb84ca..ed2f62a 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -152,8 +152,6 @@ int dp_power_client_init(struct dp_power 
*dp_power)
   power = container_of(dp_power, struct dp_power_private, 
dp_power);

-    pm_runtime_enable(power->dev);
-
   return dp_power_clk_init(power);
   }
@@ -162,8 +160,6 @@ void dp_power_client_deinit(struct dp_power
*dp_power)
   struct dp_power_private *power;
   power = container_of(dp_power, struct dp_power_private, 
dp_power);

-
-    pm_runtime_disable(power->dev);
   }
   int dp_power_init(struct dp_power *dp_power)
@@ -173,11 +169,7 @@ int dp_power_init(struct dp_power *dp_power)
   power = container_of(dp_power, struct dp_power_private, 
dp_power);

-    pm_runtime_get_sync(power->dev);
-
   rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
-    if (rc)
-    pm_runtime_put_sync(power->dev);
   return rc;
   }
@@ -189,7 +181,6 @@ int dp_power_deinit(struct dp_power *dp_power)
   power = container_of(dp_power, struct dp_power_private, 
dp_power);

   dp_power_clk_enable(dp_power, DP_CORE_PM, false);
-    pm_runtime_put_sync(power->dev);
   return 0;
   }








--
With best wishes
Dmitry

Re: [Freedreno] [PATCH v1 1/5] drm/msm/dp: remove pm_runtime_xxx() from dp_power.c

[Kuogee Hsieh]

On 7/9/2023 1:32 PM, Abhinav Kumar wrote:



On 7/9/2023 11:00 AM, Dmitry Baryshkov wrote:
On Sun, 9 Jul 2023 at 20:22, Abhinav Kumar 
 wrote:




On 7/7/2023 5:06 PM, Dmitry Baryshkov wrote:

On 08/07/2023 02:52, Kuogee Hsieh wrote:

Since both pm_runtime_resume() and pm_runtime_suspend() are not
populated at dp_pm_ops. Those pm_runtime_get/put() functions within
dp_power.c will not have any effects in addition to increase/decrease
power counter.


Lie.



Even if the commit text is incorrect, review comments like this are not
helping the patch nor the author and will just get ignored anyway.


The review comment might be overreacting, excuse me. I was really
impressed by the commit message, which contradicts the basic source
code. pm_runtime_get() does a lot more than just increasing the power
counter.



It says within dp_power.c. Nonetheless, please let us know what is 
missing in the context of this patch like Bjorn did to make it an 
effective review and we can correct it. In its current form, the 
review comment is adding no value.



I am new in pm.

Any recommendation to revise this commit test?


Also pm_runtime_xxx() should be executed at top
layer.


Why?



I guess he meant to centralize this around dp_display.c. Will elaborate
while posting the next rev.



Signed-off-by: Kuogee Hsieh 
---
   drivers/gpu/drm/msm/dp/dp_power.c | 9 -
   1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_power.c
b/drivers/gpu/drm/msm/dp/dp_power.c
index 5cb84ca..ed2f62a 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -152,8 +152,6 @@ int dp_power_client_init(struct dp_power 
*dp_power)
   power = container_of(dp_power, struct dp_power_private, 
dp_power);

-    pm_runtime_enable(power->dev);
-
   return dp_power_clk_init(power);
   }
@@ -162,8 +160,6 @@ void dp_power_client_deinit(struct dp_power
*dp_power)
   struct dp_power_private *power;
   power = container_of(dp_power, struct dp_power_private, 
dp_power);

-
-    pm_runtime_disable(power->dev);
   }
   int dp_power_init(struct dp_power *dp_power)
@@ -173,11 +169,7 @@ int dp_power_init(struct dp_power *dp_power)
   power = container_of(dp_power, struct dp_power_private, 
dp_power);

-    pm_runtime_get_sync(power->dev);
-
   rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
-    if (rc)
-    pm_runtime_put_sync(power->dev);
   return rc;
   }
@@ -189,7 +181,6 @@ int dp_power_deinit(struct dp_power *dp_power)
   power = container_of(dp_power, struct dp_power_private, 
dp_power);

   dp_power_clk_enable(dp_power, DP_CORE_PM, false);
-    pm_runtime_put_sync(power->dev);
   return 0;
   }

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

[Jagan Teki]

On Wed, Jul 5, 2023 at 11:09 AM 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.

True, But I'm not quite sure, most of these mode_flags were handled
more on the host. Maybe Marek can comment on this.

Thanks,
Jagan.

Re: [Freedreno] [PATCH] drm/client: Send hotplug event after registering a client

[Limonciello, Mario]

+regressions
On 7/10/2023 04:58, Thomas Zimmermann wrote:

Hi

Am 10.07.23 um 11:52 schrieb Javier Martinez Canillas:

Thomas Zimmermann  writes:

Hello Thomas,


Generate a hotplug event after registering a client to allow the
client to configure its display. Remove the hotplug calls from the
existing clients for fbdev emulation. This change fixes a concurrency
bug between registering a client and receiving events from the DRM
core. The bug is present in the fbdev emulation of all drivers.

The fbdev emulation currently generates a hotplug event before
registering the client to the device. For each new output, the DRM
core sends an additional hotplug event to each registered client.

If the DRM core detects first output between sending the artificial
hotplug and registering the device, the output's hotplug event gets
lost. If this is the first output, the fbdev console display remains
dark. This has been observed with amdgpu and fbdev-generic.

Fix this by adding hotplug generation directly to the client's
register helper drm_client_register(). Registering the client and
receiving events are serialized by struct drm_device.clientlist_mutex.
So an output is either configured by the initial hotplug event, or
the client has already been registered.

The bug was originally added in commit 6e3f17ee73f7 ("drm/fb-helper:
generic: Call drm_client_add() after setup is done"), in which adding
a client and receiving a hotplug event switched order. It was hidden,
as most hardware and drivers have at least on static output configured.
Other drivers didn't use the internal DRM client or still had struct
drm_mode_config_funcs.output_poll_changed set. That callback handled
hotplug events as well. After not setting the callback in amdgpu in
commit 0e3172bac3f4 ("drm/amdgpu: Don't set struct
drm_driver.output_poll_changed"), amdgpu did not show a framebuffer
console if output events got lost. The bug got copy-pasted from
fbdev-generic into the other fbdev emulation.

Reported-by: Moritz Duge 
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2649


Aren't you missing a Fixes: for 0e3172bac3f4 too? Since that's the commit
that unmasked the bug for amdgpu, IMO that is the most important to list.


Well, OK.



Fixes: 6e3f17ee73f7 ("drm/fb-helper: generic: Call drm_client_add() 
after setup is done")
Fixes: 8ab59da26bc0 ("drm/fb-helper: Move generic fbdev emulation 
into separate source file")
Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for 
GEM DMA helpers")
Fixes: 63c381552f69 ("drm/armada: Implement fbdev emulation as 
in-kernel client")
Fixes: 49953b70e7d3 ("drm/exynos: Implement fbdev emulation as 
in-kernel client")
Fixes: 8f1aaccb04b7 ("drm/gma500: Implement client-based fbdev 
emulation")
Fixes: 940b869c2f2f ("drm/msm: Implement fbdev emulation as in-kernel 
client")
Fixes: 9e69bcd88e45 ("drm/omapdrm: Implement fbdev emulation as 
in-kernel client")
Fixes: e317a69fe891 ("drm/radeon: Implement client-based fbdev 
emulation")
Fixes: 71ec16f45ef8 ("drm/tegra: Implement fbdev emulation as 
in-kernel client")

Signed-off-by: Thomas Zimmermann 
Tested-by: Moritz Duge 
Tested-by: Torsten Krah 
Tested-by: Paul Schyska 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Noralf Trønnes 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Javier Martinez Canillas 
Cc: Russell King 
Cc: Inki Dae 
Cc: Seung-Woo Kim 
Cc: Kyungmin Park 
Cc: Krzysztof Kozlowski 
Cc: Patrik Jakobsson 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Tomi Valkeinen 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: Thierry Reding 
Cc: Mikko Perttunen 
Cc: dri-de...@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Cc: amd-...@lists.freedesktop.org
Cc: linux-te...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc:  # v5.2+


While it's true that the but was introduced by commit 6e3f17ee73f7 and 
that
landed in v5.2, I wonder if this patch could even be applied to such 
olders

Linux versions. Probably in practice it would be at most backported to
v6.2, which is the release that exposed the bug for the amdgpu driver.


No idea. The fix looks simple enough, but a lot has changed in the 
surrounding code.




Actually it needs to go to at least 6.1.y.

Moritz found it in 6.1.35 (not present in 6.1.34).



Best regards
Thomas



Your explanation makes sense to me and the patch looks good.

Reviewed-by: Javier Martinez Canillas 

Re: [Freedreno] [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Kuogee Hsieh]

On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote:

On 08/07/2023 02:52, Kuogee Hsieh wrote:

In preparation of moving edp of_dp_aux_populate_bus() to
dp_display_probe(), move dp_display_request_irq(),
dp->parser->parse() and dp_power_client_init() to dp_display_probe()
too.

Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_display.c | 48 
+

  drivers/gpu/drm/msm/dp/dp_display.h |  1 -
  2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 44580c2..185f1eb 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -290,12 +290,6 @@ static int dp_display_bind(struct device *dev, 
struct device *master,

  goto end;
  }
  -    rc = dp_power_client_init(dp->power);
-    if (rc) {
-    DRM_ERROR("Power client create failed\n");
-    goto end;
-    }
-
  rc = dp_register_audio_driver(dev, dp->audio);
  if (rc) {
  DRM_ERROR("Audio registration Dp failed\n");
@@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct 
dp_display_private *dp)

  goto error;
  }
  +    rc = dp->parser->parse(dp->parser);
+    if (rc) {
+    DRM_ERROR("device tree parsing failed\n");
+    goto error;
+    }
+
  dp->catalog = dp_catalog_get(dev, >parser->io);
  if (IS_ERR(dp->catalog)) {
  rc = PTR_ERR(dp->catalog);
@@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct 
dp_display_private *dp)

  goto error;
  }
  +    rc = dp_power_client_init(dp->power);
+    if (rc) {
+    DRM_ERROR("Power client create failed\n");
+    goto error;
+    }
+
  dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
  if (IS_ERR(dp->aux)) {
  rc = PTR_ERR(dp->aux);
@@ -1196,26 +1202,20 @@ static irqreturn_t dp_display_irq_handler(int 
irq, void *dev_id)

  return ret;
  }
  -int dp_display_request_irq(struct msm_dp *dp_display)
+static int dp_display_request_irq(struct dp_display_private *dp)
  {
  int rc = 0;
-    struct dp_display_private *dp;
-
-    if (!dp_display) {
-    DRM_ERROR("invalid input\n");
-    return -EINVAL;
-    }
-
-    dp = container_of(dp_display, struct dp_display_private, 
dp_display);

+    struct device *dev = >pdev->dev;
  -    dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
  if (!dp->irq) {
-    DRM_ERROR("failed to get irq\n");
-    return -EINVAL;
+    dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
+    if (!dp->irq) {
+    DRM_ERROR("failed to get irq\n");
+    return -EINVAL;
+    }
  }


Use platform_get_irq() from probe() function.


  -    rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
-    dp_display_irq_handler,
+    rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
  IRQF_TRIGGER_HIGH, "dp_display_isr", dp);




  if (rc < 0) {
  DRM_ERROR("failed to request IRQ%u: %d\n",
@@ -1290,6 +1290,8 @@ static int dp_display_probe(struct 
platform_device *pdev)

    platform_set_drvdata(pdev, >dp_display);
  +    dp_display_request_irq(dp);
+


Error checking?
Are we completely ready to handle interrupts at this point?
not until dp_display_host_init() is called which will be called from 
pm_runtime_resume() later.



  rc = component_add(>dev, _display_comp_ops);
  if (rc) {
  DRM_ERROR("component add failed, rc=%d\n", rc);
@@ -1574,12 +1576,6 @@ int msm_dp_modeset_init(struct msm_dp 
*dp_display, struct drm_device *dev,
    dp_priv = container_of(dp_display, struct dp_display_private, 
dp_display);

  -    ret = dp_display_request_irq(dp_display);
-    if (ret) {
-    DRM_ERROR("request_irq failed, ret=%d\n", ret);
-    return ret;
-    }
-
  ret = dp_display_get_next_bridge(dp_display);
  if (ret)
  return ret;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h 
b/drivers/gpu/drm/msm/dp/dp_display.h

index 1e9415a..b3c08de 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -35,7 +35,6 @@ struct msm_dp {
  int dp_display_set_plugged_cb(struct msm_dp *dp_display,
  hdmi_codec_plugged_cb fn, struct device *codec_dev);
  int dp_display_get_modes(struct msm_dp *dp_display);
-int dp_display_request_irq(struct msm_dp *dp_display);
  bool dp_display_check_video_test(struct msm_dp *dp_display);
  int dp_display_get_test_bpp(struct msm_dp *dp_display);
  void dp_display_signal_audio_start(struct msm_dp *dp_display);

Re: [Freedreno] [PATCH v1 3/5] drm/msm/dp: delete EV_HPD_INIT_SETUP

[Kuogee Hsieh]

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

On 08/07/2023 02:52, Kuogee Hsieh wrote:

EV_HPD_INIT_SETUP flag is used to trigger the initialization of external
DP host controller. Since external DP host controller initialization had
been incorporated into pm_runtime_resume(), this flag become obsolete.
Lets get rid of it.


And another question. Between patches #2 and #3 we have both 
INIT_SETUP event and the PM doing dp_display_host_init(). Will it work 
correctly?


yes,  i had added  if (!dp->core_initialized)  into dp_display_host_init().

should I merge this patch into patch #2?





Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_display.c | 12 
  1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 2c5706a..44580c2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -55,7 +55,6 @@ enum {
  enum {
  EV_NO_EVENT,
  /* hpd events */
-    EV_HPD_INIT_SETUP,
  EV_HPD_PLUG_INT,
  EV_IRQ_HPD_INT,
  EV_HPD_UNPLUG_INT,
@@ -1119,9 +1118,6 @@ static int hpd_event_thread(void *data)
  spin_unlock_irqrestore(_priv->event_lock, flag);
    switch (todo->event_id) {
-    case EV_HPD_INIT_SETUP:
-    dp_display_host_init(dp_priv);
-    break;
  case EV_HPD_PLUG_INT:
  dp_hpd_plug_handle(dp_priv, todo->data);
  break;
@@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void)
    void msm_dp_irq_postinstall(struct msm_dp *dp_display)
  {
-    struct dp_display_private *dp;
-
-    if (!dp_display)
-    return;
-
-    dp = container_of(dp_display, struct dp_display_private, 
dp_display);

  -    if (!dp_display->is_edp)
-    dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
  }
    bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)

Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Kuogee Hsieh]

On 7/8/2023 7:52 PM, Bjorn Andersson wrote:

On Fri, Jul 07, 2023 at 04:52:20PM -0700, Kuogee Hsieh wrote:

Incorporating pm runtime framework into DP driver so that power
and clock resource handling can be centralized allowing easier
control of these resources in preparation of registering aux bus
uring probe.

Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_aux.c |  3 ++
  drivers/gpu/drm/msm/dp/dp_display.c | 75 +
  2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 8e3b677..c592064 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
return -EINVAL;
}
  
+	pm_runtime_get_sync(dp_aux->dev);

mutex_lock(>mutex);
if (!aux->initted) {
ret = -EIO;
@@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
  
  exit:

mutex_unlock(>mutex);
+   pm_runtime_mark_last_busy(dp_aux->dev);
+   pm_runtime_put_autosuspend(dp_aux->dev);
  
  	return ret;

  }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 76f1395..2c5706a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, struct 
device *master,
goto end;
}
  
+	pm_runtime_enable(dev);

+   pm_runtime_set_autosuspend_delay(dev, 1000);
+   pm_runtime_use_autosuspend(dev);
+
return 0;
  end:
return rc;
@@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, struct 
device *master,
struct dp_display_private *dp = dev_get_dp_display_private(dev);
struct msm_drm_private *priv = dev_get_drvdata(master);
  
-	/* disable all HPD interrupts */

-   if (dp->core_initialized)
-   dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, 
false);
+   pm_runtime_dont_use_autosuspend(dev);
+   pm_runtime_disable(dev);
  
  	kthread_stop(dp->ev_tsk);
  
@@ -466,10 +469,12 @@ static void dp_display_host_init(struct dp_display_private *dp)

dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
  
-	dp_power_init(dp->power);

-   dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
-   dp_aux_init(dp->aux);
-   dp->core_initialized = true;
+   if (!dp->core_initialized) {
+   dp_power_init(dp->power);
+   dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+   dp_aux_init(dp->aux);
+   dp->core_initialized = true;

There are two cases that queries core_initialized, both of those are
done to avoid accessing the DP block without it first being powered up.
With the introduction of runtime PM, it seems reasonable to just power
up the block in those two code paths (and remove the variable).


+   }
  }
  
  static void dp_display_host_deinit(struct dp_display_private *dp)

@@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
  
-	dp_ctrl_reset_irq_ctrl(dp->ctrl, false);

-   dp_aux_deinit(dp->aux);
-   dp_power_deinit(dp->power);
-   dp->core_initialized = false;
+   if (dp->core_initialized) {
+   dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+   dp_aux_deinit(dp->aux);
+   dp_power_deinit(dp->power);
+   dp->core_initialized = false;
+   }
  }
  
  static int dp_display_usbpd_configure_cb(struct device *dev)

@@ -1304,6 +1311,39 @@ static int dp_display_remove(struct platform_device 
*pdev)
dp_display_deinit_sub_modules(dp);
  
  	platform_set_drvdata(pdev, NULL);

+   pm_runtime_put_sync_suspend(>dev);
+
+   return 0;
+}
+
+static int dp_pm_runtime_suspend(struct device *dev)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   struct msm_dp *dp_display = platform_get_drvdata(pdev);

platform_get_drvdata() is a wrapper for dev_get_drvdata(>dev), so
there's no need to resolve the platform_device first...


+   struct dp_display_private *dp;
+
+   dp = container_of(dp_display, struct dp_display_private, dp_display);
+
+   dp_display_host_phy_exit(dp);
+   dp_catalog_ctrl_hpd_enable(dp->catalog);
+   dp_display_host_deinit(dp);
+
+   return 0;
+}
+
+static int dp_pm_runtime_resume(struct device *dev)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   struct msm_dp *dp_display = platform_get_drvdata(pdev);
+   struct dp_display_private *dp;
+
+   dp = container_of(dp_display, struct dp_display_private, dp_display);
+
+   dp_display_host_init(dp);
+   if (dp_display->is_edp) {
+   

Re: [Freedreno] [PATCH v1 2/5] drm/msm/dp: incorporate pm_runtime framework into DP driver

[Kuogee Hsieh]

On 7/7/2023 5:04 PM, Dmitry Baryshkov wrote:

On 08/07/2023 02:52, Kuogee Hsieh wrote:

Incorporating pm runtime framework into DP driver so that power
and clock resource handling can be centralized allowing easier
control of these resources in preparation of registering aux bus
uring probe.

Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_aux.c |  3 ++
  drivers/gpu/drm/msm/dp/dp_display.c | 75 
+

  2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 8e3b677..c592064 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -291,6 +291,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

  return -EINVAL;
  }
  +    pm_runtime_get_sync(dp_aux->dev);


Let me quote the function's documentation:
Consider using pm_runtime_resume_and_get() instead of it, especially 
if its return value is checked by the caller, as this is likely to 
result in cleaner code.


pm_runtime_resume_and_get() will call pm_runtime_resume()  every time.

Since aux_transfer is called very frequently, is it just simple to call 
pm_runtiem_get_sync() which will call pm_runtime_reusme() if power 
counter is 0 before increased it.


otherwise it just increase power counter?




So two notes concerning the whole patch:
- error checking is missing
- please use pm_runtime_resume_and_get() instead.


  mutex_lock(>mutex);
  if (!aux->initted) {
  ret = -EIO;
@@ -364,6 +365,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

    exit:
  mutex_unlock(>mutex);
+    pm_runtime_mark_last_busy(dp_aux->dev);
+    pm_runtime_put_autosuspend(dp_aux->dev);
    return ret;
  }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 76f1395..2c5706a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -309,6 +309,10 @@ static int dp_display_bind(struct device *dev, 
struct device *master,

  goto end;
  }
  +    pm_runtime_enable(dev);


devm_pm_runtime_enable() removes need for a cleanup.


+    pm_runtime_set_autosuspend_delay(dev, 1000);
+    pm_runtime_use_autosuspend(dev);


Why do you want to use autosuspend here?


+
  return 0;
  end:
  return rc;
@@ -320,9 +324,8 @@ static void dp_display_unbind(struct device *dev, 
struct device *master,

  struct dp_display_private *dp = dev_get_dp_display_private(dev);
  struct msm_drm_private *priv = dev_get_drvdata(master);
  -    /* disable all HPD interrupts */
-    if (dp->core_initialized)
-    dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, 
false);

+    pm_runtime_dont_use_autosuspend(dev);
+    pm_runtime_disable(dev);
    kthread_stop(dp->ev_tsk);
  @@ -466,10 +469,12 @@ static void dp_display_host_init(struct 
dp_display_private *dp)

  dp->dp_display.connector_type, dp->core_initialized,
  dp->phy_initialized);
  -    dp_power_init(dp->power);
-    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
-    dp_aux_init(dp->aux);
-    dp->core_initialized = true;
+    if (!dp->core_initialized) {
+    dp_power_init(dp->power);
+    dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+    dp_aux_init(dp->aux);
+    dp->core_initialized = true;
+    }


Is this relevant to PM runtime? I don't think so.


  }
    static void dp_display_host_deinit(struct dp_display_private *dp)
@@ -478,10 +483,12 @@ static void dp_display_host_deinit(struct 
dp_display_private *dp)

  dp->dp_display.connector_type, dp->core_initialized,
  dp->phy_initialized);
  -    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
-    dp_aux_deinit(dp->aux);
-    dp_power_deinit(dp->power);
-    dp->core_initialized = false;
+    if (dp->core_initialized) {
+    dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+    dp_aux_deinit(dp->aux);
+    dp_power_deinit(dp->power);
+    dp->core_initialized = false;
+    }
  }
    static int dp_display_usbpd_configure_cb(struct device *dev)
@@ -1304,6 +1311,39 @@ static int dp_display_remove(struct 
platform_device *pdev)

  dp_display_deinit_sub_modules(dp);
    platform_set_drvdata(pdev, NULL);
+    pm_runtime_put_sync_suspend(>dev);
+
+    return 0;
+}
+
+static int dp_pm_runtime_suspend(struct device *dev)
+{
+    struct platform_device *pdev = to_platform_device(dev);
+    struct msm_dp *dp_display = platform_get_drvdata(pdev);
+    struct dp_display_private *dp;
+
+    dp = container_of(dp_display, struct dp_display_private, 
dp_display);

+
+    dp_display_host_phy_exit(dp);
+    dp_catalog_ctrl_hpd_enable(dp->catalog);


What? NO!


+    dp_display_host_deinit(dp);
+
+    return 0;
+}
+
+static int dp_pm_runtime_resume(struct device *dev)
+{
+    struct platform_device *pdev = to_platform_device(dev);
+    struct msm_dp *dp_display = platform_get_drvdata(pdev);
+    struct dp_display_private *dp;
+
+   

Re: [Freedreno] [PATCH] drm/client: Send hotplug event after registering a client

[Thomas Zimmermann]

Hi

Am 10.07.23 um 11:52 schrieb Javier Martinez Canillas:

Thomas Zimmermann  writes:

Hello Thomas,


Generate a hotplug event after registering a client to allow the
client to configure its display. Remove the hotplug calls from the
existing clients for fbdev emulation. This change fixes a concurrency
bug between registering a client and receiving events from the DRM
core. The bug is present in the fbdev emulation of all drivers.

The fbdev emulation currently generates a hotplug event before
registering the client to the device. For each new output, the DRM
core sends an additional hotplug event to each registered client.

If the DRM core detects first output between sending the artificial
hotplug and registering the device, the output's hotplug event gets
lost. If this is the first output, the fbdev console display remains
dark. This has been observed with amdgpu and fbdev-generic.

Fix this by adding hotplug generation directly to the client's
register helper drm_client_register(). Registering the client and
receiving events are serialized by struct drm_device.clientlist_mutex.
So an output is either configured by the initial hotplug event, or
the client has already been registered.

The bug was originally added in commit 6e3f17ee73f7 ("drm/fb-helper:
generic: Call drm_client_add() after setup is done"), in which adding
a client and receiving a hotplug event switched order. It was hidden,
as most hardware and drivers have at least on static output configured.
Other drivers didn't use the internal DRM client or still had struct
drm_mode_config_funcs.output_poll_changed set. That callback handled
hotplug events as well. After not setting the callback in amdgpu in
commit 0e3172bac3f4 ("drm/amdgpu: Don't set struct
drm_driver.output_poll_changed"), amdgpu did not show a framebuffer
console if output events got lost. The bug got copy-pasted from
fbdev-generic into the other fbdev emulation.

Reported-by: Moritz Duge 
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2649


Aren't you missing a Fixes: for 0e3172bac3f4 too? Since that's the commit
that unmasked the bug for amdgpu, IMO that is the most important to list.


Well, OK.




Fixes: 6e3f17ee73f7 ("drm/fb-helper: generic: Call drm_client_add() after setup is 
done")
Fixes: 8ab59da26bc0 ("drm/fb-helper: Move generic fbdev emulation into separate 
source file")
Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA 
helpers")
Fixes: 63c381552f69 ("drm/armada: Implement fbdev emulation as in-kernel 
client")
Fixes: 49953b70e7d3 ("drm/exynos: Implement fbdev emulation as in-kernel 
client")
Fixes: 8f1aaccb04b7 ("drm/gma500: Implement client-based fbdev emulation")
Fixes: 940b869c2f2f ("drm/msm: Implement fbdev emulation as in-kernel client")
Fixes: 9e69bcd88e45 ("drm/omapdrm: Implement fbdev emulation as in-kernel 
client")
Fixes: e317a69fe891 ("drm/radeon: Implement client-based fbdev emulation")
Fixes: 71ec16f45ef8 ("drm/tegra: Implement fbdev emulation as in-kernel client")
Signed-off-by: Thomas Zimmermann 
Tested-by: Moritz Duge 
Tested-by: Torsten Krah 
Tested-by: Paul Schyska 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Noralf Trønnes 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Javier Martinez Canillas 
Cc: Russell King 
Cc: Inki Dae 
Cc: Seung-Woo Kim 
Cc: Kyungmin Park 
Cc: Krzysztof Kozlowski 
Cc: Patrik Jakobsson 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Tomi Valkeinen 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: Thierry Reding 
Cc: Mikko Perttunen 
Cc: dri-de...@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Cc: amd-...@lists.freedesktop.org
Cc: linux-te...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc:  # v5.2+


While it's true that the but was introduced by commit 6e3f17ee73f7 and that
landed in v5.2, I wonder if this patch could even be applied to such olders
Linux versions. Probably in practice it would be at most backported to
v6.2, which is the release that exposed the bug for the amdgpu driver.


No idea. The fix looks simple enough, but a lot has changed in the 
surrounding code.


Best regards
Thomas



Your explanation makes sense to me and the patch looks good.

Reviewed-by: Javier Martinez Canillas 



--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)


OpenPGP_signature
Description: OpenPGP digital signature

Re: [Freedreno] [PATCH] drm/client: Send hotplug event after registering a client

[Javier Martinez Canillas]

Thomas Zimmermann  writes:

Hello Thomas,

> Generate a hotplug event after registering a client to allow the
> client to configure its display. Remove the hotplug calls from the
> existing clients for fbdev emulation. This change fixes a concurrency
> bug between registering a client and receiving events from the DRM
> core. The bug is present in the fbdev emulation of all drivers.
>
> The fbdev emulation currently generates a hotplug event before
> registering the client to the device. For each new output, the DRM
> core sends an additional hotplug event to each registered client.
>
> If the DRM core detects first output between sending the artificial
> hotplug and registering the device, the output's hotplug event gets
> lost. If this is the first output, the fbdev console display remains
> dark. This has been observed with amdgpu and fbdev-generic.
>
> Fix this by adding hotplug generation directly to the client's
> register helper drm_client_register(). Registering the client and
> receiving events are serialized by struct drm_device.clientlist_mutex.
> So an output is either configured by the initial hotplug event, or
> the client has already been registered.
>
> The bug was originally added in commit 6e3f17ee73f7 ("drm/fb-helper:
> generic: Call drm_client_add() after setup is done"), in which adding
> a client and receiving a hotplug event switched order. It was hidden,
> as most hardware and drivers have at least on static output configured.
> Other drivers didn't use the internal DRM client or still had struct
> drm_mode_config_funcs.output_poll_changed set. That callback handled
> hotplug events as well. After not setting the callback in amdgpu in
> commit 0e3172bac3f4 ("drm/amdgpu: Don't set struct
> drm_driver.output_poll_changed"), amdgpu did not show a framebuffer
> console if output events got lost. The bug got copy-pasted from
> fbdev-generic into the other fbdev emulation.
>
> Reported-by: Moritz Duge 
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2649

Aren't you missing a Fixes: for 0e3172bac3f4 too? Since that's the commit
that unmasked the bug for amdgpu, IMO that is the most important to list.

> Fixes: 6e3f17ee73f7 ("drm/fb-helper: generic: Call drm_client_add() after 
> setup is done")
> Fixes: 8ab59da26bc0 ("drm/fb-helper: Move generic fbdev emulation into 
> separate source file")
> Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA 
> helpers")
> Fixes: 63c381552f69 ("drm/armada: Implement fbdev emulation as in-kernel 
> client")
> Fixes: 49953b70e7d3 ("drm/exynos: Implement fbdev emulation as in-kernel 
> client")
> Fixes: 8f1aaccb04b7 ("drm/gma500: Implement client-based fbdev emulation")
> Fixes: 940b869c2f2f ("drm/msm: Implement fbdev emulation as in-kernel client")
> Fixes: 9e69bcd88e45 ("drm/omapdrm: Implement fbdev emulation as in-kernel 
> client")
> Fixes: e317a69fe891 ("drm/radeon: Implement client-based fbdev emulation")
> Fixes: 71ec16f45ef8 ("drm/tegra: Implement fbdev emulation as in-kernel 
> client")
> Signed-off-by: Thomas Zimmermann 
> Tested-by: Moritz Duge 
> Tested-by: Torsten Krah 
> Tested-by: Paul Schyska 
> Cc: Daniel Vetter 
> Cc: David Airlie 
> Cc: Noralf Trønnes 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Javier Martinez Canillas 
> Cc: Russell King 
> Cc: Inki Dae 
> Cc: Seung-Woo Kim 
> Cc: Kyungmin Park 
> Cc: Krzysztof Kozlowski 
> Cc: Patrik Jakobsson 
> Cc: Rob Clark 
> Cc: Abhinav Kumar 
> Cc: Dmitry Baryshkov 
> Cc: Tomi Valkeinen 
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "Pan, Xinhui" 
> Cc: Thierry Reding 
> Cc: Mikko Perttunen 
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Cc: amd-...@lists.freedesktop.org
> Cc: linux-te...@vger.kernel.org
> Cc: dri-de...@lists.freedesktop.org
> Cc:  # v5.2+

While it's true that the but was introduced by commit 6e3f17ee73f7 and that
landed in v5.2, I wonder if this patch could even be applied to such olders
Linux versions. Probably in practice it would be at most backported to
v6.2, which is the release that exposed the bug for the amdgpu driver.

Your explanation makes sense to me and the patch looks good.

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[Freedreno] [PATCH] drm/client: Send hotplug event after registering a client

[Thomas Zimmermann]

Generate a hotplug event after registering a client to allow the
client to configure its display. Remove the hotplug calls from the
existing clients for fbdev emulation. This change fixes a concurrency
bug between registering a client and receiving events from the DRM
core. The bug is present in the fbdev emulation of all drivers.

The fbdev emulation currently generates a hotplug event before
registering the client to the device. For each new output, the DRM
core sends an additional hotplug event to each registered client.

If the DRM core detects first output between sending the artificial
hotplug and registering the device, the output's hotplug event gets
lost. If this is the first output, the fbdev console display remains
dark. This has been observed with amdgpu and fbdev-generic.

Fix this by adding hotplug generation directly to the client's
register helper drm_client_register(). Registering the client and
receiving events are serialized by struct drm_device.clientlist_mutex.
So an output is either configured by the initial hotplug event, or
the client has already been registered.

The bug was originally added in commit 6e3f17ee73f7 ("drm/fb-helper:
generic: Call drm_client_add() after setup is done"), in which adding
a client and receiving a hotplug event switched order. It was hidden,
as most hardware and drivers have at least on static output configured.
Other drivers didn't use the internal DRM client or still had struct
drm_mode_config_funcs.output_poll_changed set. That callback handled
hotplug events as well. After not setting the callback in amdgpu in
commit 0e3172bac3f4 ("drm/amdgpu: Don't set struct
drm_driver.output_poll_changed"), amdgpu did not show a framebuffer
console if output events got lost. The bug got copy-pasted from
fbdev-generic into the other fbdev emulation.

Reported-by: Moritz Duge 
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2649
Fixes: 6e3f17ee73f7 ("drm/fb-helper: generic: Call drm_client_add() after setup 
is done")
Fixes: 8ab59da26bc0 ("drm/fb-helper: Move generic fbdev emulation into separate 
source file")
Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA 
helpers")
Fixes: 63c381552f69 ("drm/armada: Implement fbdev emulation as in-kernel 
client")
Fixes: 49953b70e7d3 ("drm/exynos: Implement fbdev emulation as in-kernel 
client")
Fixes: 8f1aaccb04b7 ("drm/gma500: Implement client-based fbdev emulation")
Fixes: 940b869c2f2f ("drm/msm: Implement fbdev emulation as in-kernel client")
Fixes: 9e69bcd88e45 ("drm/omapdrm: Implement fbdev emulation as in-kernel 
client")
Fixes: e317a69fe891 ("drm/radeon: Implement client-based fbdev emulation")
Fixes: 71ec16f45ef8 ("drm/tegra: Implement fbdev emulation as in-kernel client")
Signed-off-by: Thomas Zimmermann 
Tested-by: Moritz Duge 
Tested-by: Torsten Krah 
Tested-by: Paul Schyska 
Cc: Daniel Vetter 
Cc: David Airlie 
Cc: Noralf Trønnes 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Javier Martinez Canillas 
Cc: Russell King 
Cc: Inki Dae 
Cc: Seung-Woo Kim 
Cc: Kyungmin Park 
Cc: Krzysztof Kozlowski 
Cc: Patrik Jakobsson 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Tomi Valkeinen 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: Thierry Reding 
Cc: Mikko Perttunen 
Cc: dri-de...@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Cc: amd-...@lists.freedesktop.org
Cc: linux-te...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc:  # v5.2+
---
 drivers/gpu/drm/armada/armada_fbdev.c |  4 
 drivers/gpu/drm/drm_client.c  | 21 +
 drivers/gpu/drm/drm_fbdev_dma.c   |  4 
 drivers/gpu/drm/drm_fbdev_generic.c   |  4 
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |  4 
 drivers/gpu/drm/gma500/fbdev.c|  4 
 drivers/gpu/drm/msm/msm_fbdev.c   |  4 
 drivers/gpu/drm/omapdrm/omap_fbdev.c  |  4 
 drivers/gpu/drm/radeon/radeon_fbdev.c |  4 
 drivers/gpu/drm/tegra/fbdev.c |  4 
 10 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_fbdev.c 
b/drivers/gpu/drm/armada/armada_fbdev.c
index 3943e89cc06c..e40a95e51785 100644
--- a/drivers/gpu/drm/armada/armada_fbdev.c
+++ b/drivers/gpu/drm/armada/armada_fbdev.c
@@ -209,10 +209,6 @@ void armada_fbdev_setup(struct drm_device *dev)
goto err_drm_client_init;
}
 
-   ret = armada_fbdev_client_hotplug(>client);
-   if (ret)
-   drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
-
drm_client_register(>client);
 
return;
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index f6292ba0e6fc..037e36f2049c 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -122,13 +122,34 @@ EXPORT_SYMBOL(drm_client_init);
  * 

Re: [Freedreno] [PATCH 1/5] dt-bindings: display: msm: dp-controller: document SM8250 compatible

[Krzysztof Kozlowski]

On 09/07/2023 06:19, Dmitry Baryshkov wrote:
> It looks like DP controlled on SM8250 is the same as DP controller on
> SM8350. Use the SM8350 compatible as fallback for SM8250.
> 
> Signed-off-by: Dmitry Baryshkov 
> ---


Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof