Re: [PATCH 3/7] drm/msm/dpu: Always flush the slave INTF on the CTL
On 4/16/2024 4:57 PM, Marijn Suijten wrote:
As we can clearly see in a downstream kernel [1], flushing the slave INTF
is skipped /only if/ the PPSPLIT topology is active.
However, when DPU was originally submitted to mainline PPSPLIT was no
longer part of it (seems to have been ripped out before submission), but
this clause was incorrectly ported from the original SDE driver. Given
that there is no support for PPSPLIT (currently), flushing the slave
INTF should /never/ be skipped (as the `if (ppsplit && !master) goto
skip;` clause downstream never becomes true).
[1]:
https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.4.r1-rel/msm/sde/sde_encoder_phys_cmd.c?ref_type=heads#L1131-1139
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 ---
1 file changed, 3 deletions(-)
Yes, I agree with this, even though I did think earlier that intf master
flush was sufficient , I cross-checked the docs and this is the right way.
Reviewed-by: Abhinav Kumar
Re: [PATCH 2/3] drm/msm/mdp4: don't destroy mdp4_kms in mdp4_kms_init error path
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
Since commit 3c74682637e6 ("drm/msm/mdp4: move resource allocation to
the _probe function") the mdp4_kms data is allocated during probe. It is
an error to destroy it during mdp4_kms_init(), as the data is still
referenced by the drivers's data and can be used later in case of probe
deferral.
mdp4_destroy() currently detaches mmu, calls msm_kms_destroy() which
destroys pending timers and releases refcount on the aspace.
It does not touch the mdp4_kms as that one is devm managed.
In the comment
https://patchwork.freedesktop.org/patch/590411/?series=132664=1#comment_1074306,
we had discussed that the last component's reprobe attempt is affected
(which is not dpu or mdp4 or mdp5 right? )
If it was an interface (such as DSI OR DP), is it the aspace detach
which is causing the crash?
Another note is, mdp5 needs the same fix in that case.
dpu_kms_init() looks fine.
Fixes: 3c74682637e6 ("drm/msm/mdp4: move resource allocation to the _probe
function")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 4ba1cb74ad76..4c5f72b7e0e5 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -392,7 +392,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = mdp_kms_init(_kms->base, _funcs);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
- goto fail;
+ return ret;
}
kms = priv->kms;
@@ -403,7 +403,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = regulator_enable(mdp4_kms->vdd);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to enable regulator vdd:
%d\n", ret);
- goto fail;
+ return ret;
}
}
@@ -414,8 +414,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (major != 4) {
DRM_DEV_ERROR(dev->dev, "unexpected MDP version: v%d.%d\n",
major, minor);
- ret = -ENXIO;
- goto fail;
+ return -ENXIO;
}
mdp4_kms->rev = minor;
@@ -423,8 +422,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (mdp4_kms->rev >= 2) {
if (!mdp4_kms->lut_clk) {
DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n");
- ret = -ENODEV;
- goto fail;
+ return -ENODEV;
}
clk_set_rate(mdp4_kms->lut_clk, max_clk);
}
@@ -445,8 +443,7 @@ static int mdp4_kms_init(struct drm_device *dev)
mmu = msm_iommu_new(>dev, 0);
if (IS_ERR(mmu)) {
- ret = PTR_ERR(mmu);
- goto fail;
+ return PTR_ERR(mmu);
} else if (!mmu) {
DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys "
"contig buffers for scanout\n");
@@ -458,8 +455,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (IS_ERR(aspace)) {
if (!IS_ERR(mmu))
mmu->funcs->destroy(mmu);
- ret = PTR_ERR(aspace);
- goto fail;
+ return PTR_ERR(aspace);
}
kms->aspace = aspace;
@@ -468,7 +464,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = modeset_init(mdp4_kms);
if (ret) {
DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
- goto fail;
+ return ret;
}
mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC | MSM_BO_SCANOUT);
@@ -476,14 +472,14 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
DRM_DEV_ERROR(dev->dev, "could not allocate blank-cursor bo:
%d\n", ret);
mdp4_kms->blank_cursor_bo = NULL;
- goto fail;
+ return ret;
}
ret = msm_gem_get_and_pin_iova(mdp4_kms->blank_cursor_bo, kms->aspace,
_kms->blank_cursor_iova);
if (ret) {
DRM_DEV_ERROR(dev->dev, "could not pin blank-cursor bo: %d\n",
ret);
- goto fail;
+ return ret;
}
dev->mode_config.min_width = 0;
@@ -492,12 +488,6 @@ static int mdp4_kms_init(struct drm_device *dev)
dev->mode_config.max_height = 2048;
return 0;
-
-fail:
- if (kms)
- mdp4_destroy(kms);
-
- return ret;
}
static const struct dev_pm_ops mdp4_pm_ops = {
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote:
On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote:
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
MSM display drivers provide kms structure allocated during probe().
Don't clean up priv->kms field in case of an error. Otherwise probe
functions might fail after KMS probe deferral.
So just to understand this more, this will happen when master component
probe (dpu) succeeded but other sub-component probe (dsi) deferred?
Because if master component probe itself deferred it will allocate priv->kms
again isnt it and we will not even hit here.
Master probing succeeds (so priv->kms is set), then kms_init fails at
runtime, during binding of the master device. This results in probe
deferral from the last component's component_add() function and reprobe
attempt when possible (once the next device is added or probed). However
as priv->kms is NULL, probe crashes.
Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()")
Actually, Is this Fixes tag correct?
OR is this one better
Fixes: 506efcba3129 ("drm/msm: carve out KMS code from msm_drv.c")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_kms.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index af6a6fcb1173..6749f0fbca96 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct
drm_driver *drv)
ret = priv->kms_init(ddev);
if (ret) {
DRM_DEV_ERROR(dev, "failed to load kms\n");
- priv->kms = NULL;
return ret;
}
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: MSM display drivers provide kms structure allocated during probe(). Don't clean up priv->kms field in case of an error. Otherwise probe functions might fail after KMS probe deferral. So just to understand this more, this will happen when master component probe (dpu) succeeded but other sub-component probe (dsi) deferred? Because if master component probe itself deferred it will allocate priv->kms again isnt it and we will not even hit here. Master probing succeeds (so priv->kms is set), then kms_init fails at runtime, during binding of the master device. This results in probe deferral from the last component's component_add() function and reprobe attempt when possible (once the next device is added or probed). However as priv->kms is NULL, probe crashes. Got it, a better commit text would have helped here. Either way, Reviewed-by: Abhinav Kumar
Re: [PATCH 3/3] drm/msm/mdp4: correct LCDC regulator name
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
Correct c error from the conversion of LCDC regulators to the bulk
API.
Fixes: 54f1fbcb47d4 ("drm/msm/mdp4: use bulk regulators API for LCDC encoder")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Indeed ! I should have caught this during review :(
Reviewed-by: Abhinav Kumar
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
MSM display drivers provide kms structure allocated during probe().
Don't clean up priv->kms field in case of an error. Otherwise probe
functions might fail after KMS probe deferral.
So just to understand this more, this will happen when master component
probe (dpu) succeeded but other sub-component probe (dsi) deferred?
Because if master component probe itself deferred it will allocate
priv->kms again isnt it and we will not even hit here.
Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_kms.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index af6a6fcb1173..6749f0fbca96 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct
drm_driver *drv)
ret = priv->kms_init(ddev);
if (ret) {
DRM_DEV_ERROR(dev, "failed to load kms\n");
- priv->kms = NULL;
return ret;
}
Re: [PATCH v2 8/9] drm/msm: merge dpu format database to MDP formats
On 4/19/2024 9:01 PM, Dmitry Baryshkov wrote: Finally remove duplication between DPU and generic MDP code by merging DPU format lists to the MDP format database. Signed-off-by: Dmitry Baryshkov --- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 4 +- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 7 +- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c| 602 drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h| 23 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h| 10 - drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 3 +- drivers/gpu/drm/msm/disp/mdp_format.c | 614 ++--- drivers/gpu/drm/msm/disp/mdp_format.h | 10 + drivers/gpu/drm/msm/disp/mdp_kms.h | 2 - drivers/gpu/drm/msm/msm_drv.h | 2 + 11 files changed, 571 insertions(+), 708 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH v2 4/9] drm/msm/dpu: pull format flag definitions to mdp_format.h
On 4/19/2024 9:01 PM, Dmitry Baryshkov wrote: In preparation to merger of formats databases, pull format flag definitions to mdp_format.h header, so that they are visibile to both dpu and mdp drivers. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 98 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 31 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 8 +-- drivers/gpu/drm/msm/disp/mdp_format.c | 6 +- drivers/gpu/drm/msm/disp/mdp_format.h | 39 drivers/gpu/drm/msm/disp/mdp_kms.h | 4 +- drivers/gpu/drm/msm/msm_drv.h | 4 -- 9 files changed, 109 insertions(+), 89 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c
On 4/19/2024 8:06 PM, Dmitry Baryshkov wrote:
On Sat, 20 Apr 2024 at 06:05, Abhinav Kumar wrote:
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
Lift mode_config limits set by the DPU driver to the actual FB limits as
handled by the dpu_plane.c.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7257ac4020d8..e7dda9eca466 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1136,13 +1136,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dev->mode_config.min_width = 0;
dev->mode_config.min_height = 0;
- /*
- * max crtc width is equal to the max mixer width * 2 and max height is
- * is 4K
- */
- dev->mode_config.max_width =
- dpu_kms->catalog->caps->max_mixer_width * 2;
- dev->mode_config.max_height = 4096;
+ dev->mode_config.max_width = DPU_MAX_IMG_WIDTH;
+ dev->mode_config.max_height = DPU_MAX_IMG_HEIGHT;
Can you please explain a little more about why the previous limits did
not work in the multi-monitor case?
We support at the most using 2 LMs per display today. Quad pipe support
is not there yet. So by bounding to 2 * mixer_width should have been
same as rejecting the mixer width in atomic_check.
This is the framebuffer limit, not a CRTC size limit.
As discussed on IRC, the DRM fwk uses this to limit the modes on the
connector, please check
2922if (out_resp->count_modes == 0) {
2923if (is_current_master)
2924connector->funcs->fill_modes(connector,
2925 dev->mode_config.max_width,
2926
dev->mode_config.max_height);
2927else
2928 drm_dbg_kms(dev, "User-space requested a forced probe on
[CONNECTOR:%d:%s] but is not the DRM master, demoting to read-only probe",
2929connector->base.id, connector->name);
2930}
So the documentation of this doesnt really align with the usage.
Unless we alter these pieces, I am hesitant to ack this.
dev->max_vblank_count = 0x;
/* Disable vblank irqs aggressively for power-saving */
Re: [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote: Lift mode_config limits set by the DPU driver to the actual FB limits as handled by the dpu_plane.c. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 7257ac4020d8..e7dda9eca466 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -1136,13 +1136,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms) dev->mode_config.min_width = 0; dev->mode_config.min_height = 0; - /* -* max crtc width is equal to the max mixer width * 2 and max height is -* is 4K -*/ - dev->mode_config.max_width = - dpu_kms->catalog->caps->max_mixer_width * 2; - dev->mode_config.max_height = 4096; + dev->mode_config.max_width = DPU_MAX_IMG_WIDTH; + dev->mode_config.max_height = DPU_MAX_IMG_HEIGHT; Can you please explain a little more about why the previous limits did not work in the multi-monitor case? We support at the most using 2 LMs per display today. Quad pipe support is not there yet. So by bounding to 2 * mixer_width should have been same as rejecting the mixer width in atomic_check. dev->max_vblank_count = 0x; /* Disable vblank irqs aggressively for power-saving */
Re: [PATCH 8/9] drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote: dpu_formats.c defines DPU_MAX_IMG_WIDTH and _HEIGHT, while dpu_hw_catalog.h defines just MAX_IMG_WIDTH and _HEIGHT. Merge these constants to remove duplication. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c| 3 --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 ++-- 3 files changed, 4 insertions(+), 7 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
On 4/19/2024 6:34 PM, Dmitry Baryshkov wrote:
On Fri, Apr 19, 2024 at 05:14:01PM -0700, Abhinav Kumar wrote:
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
Move a call to dpu_format_populate_plane_sizes() to the atomic_check
step, so that any issues with the FB layout can be reported as early as
possible.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index d9631fe90228..a9de1fbd0df3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -673,12 +673,6 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane,
}
}
- ret = dpu_format_populate_plane_sizes(new_state->fb, >layout);
- if (ret) {
- DPU_ERROR_PLANE(pdpu, "failed to get format plane sizes, %d\n",
ret);
- return ret;
- }
-
/* validate framebuffer layout before commit */
ret = dpu_format_populate_addrs(pstate->aspace,
new_state->fb,
@@ -864,6 +858,12 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
return -E2BIG;
}
+ ret = dpu_format_populate_plane_sizes(new_plane_state->fb,
>layout);
+ if (ret) {
+ DPU_ERROR_PLANE(pdpu, "failed to get format plane sizes, %d\n",
ret);
+ return ret;
+ }
+
I think we need another function to do the check. It seems incorrect to
populate the layout to the plane state knowing it can potentially fail.
why? The state is interim object, which is subject to checks. In other
parts of the atomic_check we also fill parts of the state, perform
checks and then destroy it if the check fails.
Yes, the same thing you wrote.
I felt we can perform the validation and reject it before populating it
in the state as it seems thats doable here rather than populating it
without knowing whether it can be discarded.
Maybe I'm missing your point here. Could you please explain what is the
problem from your point of view?
Can we move the validation part of dpu_format_populate_plane_sizes() out to
another helper dpu_format_validate_plane_sizes() and use that?
And then make the remaining dpu_format_populate_plane_sizes() just a void
API to fill the layout?
Re: [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
On 4/19/2024 6:26 PM, Dmitry Baryshkov wrote:
On Fri, Apr 19, 2024 at 04:43:20PM -0700, Abhinav Kumar wrote:
On 3/19/2024 6:21 AM, Dmitry Baryshkov wrote:
The msm_kms_funcs::check_modified_format() callback is not used by the
driver. Drop it completely.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 45
-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 15 --
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 -
drivers/gpu/drm/msm/msm_kms.h | 5
4 files changed, 66 deletions(-)
I think in this case, I am leaning towards completing the implementation
rather than dropping it as usual.
It seems its easier to just add the support to call this like the attached
patch?
Please don't attach patches to the email. It makes it impossible to
respond to them.
I attached it because it was too much to paste over here.
Please review msm_framebuffer_init() in the downstream sources.
The only missing piece I can see is the handling of
DRM_MODE_FB_MODIFIERS flags.
I am unable to trace back why this support was not present.
Anyway, what are we missing with the current codebase? Why wasn't the
callback / function used in the first place?
WDYT?
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index e366ab134249..ff0df478c958 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -960,51 +960,6 @@ int dpu_format_populate_layout(
return ret;
}
-int dpu_format_check_modified_format(
- const struct msm_kms *kms,
- const struct msm_format *msm_fmt,
- const struct drm_mode_fb_cmd2 *cmd,
- struct drm_gem_object **bos)
-{
- const struct drm_format_info *info;
- const struct dpu_format *fmt;
- struct dpu_hw_fmt_layout layout;
- uint32_t bos_total_size = 0;
- int ret, i;
-
- if (!msm_fmt || !cmd || !bos) {
- DRM_ERROR("invalid arguments\n");
- return -EINVAL;
- }
-
- fmt = to_dpu_format(msm_fmt);
- info = drm_format_info(fmt->base.pixel_format);
- if (!info)
- return -EINVAL;
-
- ret = dpu_format_get_plane_sizes(fmt, cmd->width, cmd->height,
- , cmd->pitches);
- if (ret)
- return ret;
-
- for (i = 0; i < info->num_planes; i++) {
- if (!bos[i]) {
- DRM_ERROR("invalid handle for plane %d\n", i);
- return -EINVAL;
- }
- if ((i == 0) || (bos[i] != bos[0]))
- bos_total_size += bos[i]->size;
- }
-
- if (bos_total_size < layout.total_size) {
- DRM_ERROR("buffers total size too small %u expected %u\n",
- bos_total_size, layout.total_size);
- return -EINVAL;
- }
-
- return 0;
-}
-
const struct dpu_format *dpu_get_dpu_format_ext(
const uint32_t format,
const uint64_t modifier)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
index 84b8b3289f18..9442445f1a86 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
@@ -54,21 +54,6 @@ const struct msm_format *dpu_get_msm_format(
const uint32_t format,
const uint64_t modifiers);
-/**
- * dpu_format_check_modified_format - validate format and buffers for
- * dpu non-standard, i.e. modified format
- * @kms: kms driver
- * @msm_fmt: pointer to the msm_fmt base pointer of an dpu_format
- * @cmd: fb_cmd2 structure user request
- * @bos: gem buffer object list
- *
- * Return: error code on failure, 0 on success
- */
-int dpu_format_check_modified_format(
- const struct msm_kms *kms,
- const struct msm_format *msm_fmt,
- const struct drm_mode_fb_cmd2 *cmd,
- struct drm_gem_object **bos);
/**
* dpu_format_populate_layout - populate the given format layout based on
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a1f5d7c4ab91..7257ac4020d8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -969,7 +969,6 @@ static const struct msm_kms_funcs kms_funcs = {
.complete_commit = dpu_kms_complete_commit,
.enable_vblank = dpu_kms_enable_vblank,
.disable_vblank = dpu_kms_disable_vblank,
- .check_modified_format = dpu_format_check_modified_format,
.get_format = dpu_get_msm_format,
.destroy = dpu_kms_destroy,
.snapshot= dpu_kms_mdp_snapshot,
diff --git
Re: [PATCH 5/9] drm/msm/dpu: check for the plane pitch overflow
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
Check that the plane pitch doesn't overflow the maximum pitch size
allowed by the hardware.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 6 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index b7dc52312c39..86b1defa5d21 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -12,6 +12,8 @@
struct dpu_hw_sspp;
+#define DPU_SSPP_MAX_PITCH_SIZE 0x
+
You obtained this value from below code right?
if (pipe->multirect_index == DPU_SSPP_RECT_0) {
487 ystride0 = (ystride0 & 0x) |
488 (layout->plane_pitch[0] & 0x);
489 ystride1 = (ystride1 & 0x)|
490 (layout->plane_pitch[2] & 0x);
491 } else {
492 ystride0 = (ystride0 & 0x) |
493 ((layout->plane_pitch[0] << 16) &
494 0x);
495 ystride1 = (ystride1 & 0x) |
496 ((layout->plane_pitch[2] << 16) &
497 0x);
498 }
Seems correct, but was just curious
Reviewed-by: Abhinav Kumar
Re: [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
Move a call to dpu_format_populate_plane_sizes() to the atomic_check
step, so that any issues with the FB layout can be reported as early as
possible.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index d9631fe90228..a9de1fbd0df3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -673,12 +673,6 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane,
}
}
- ret = dpu_format_populate_plane_sizes(new_state->fb, >layout);
- if (ret) {
- DPU_ERROR_PLANE(pdpu, "failed to get format plane sizes, %d\n",
ret);
- return ret;
- }
-
/* validate framebuffer layout before commit */
ret = dpu_format_populate_addrs(pstate->aspace,
new_state->fb,
@@ -864,6 +858,12 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
return -E2BIG;
}
+ ret = dpu_format_populate_plane_sizes(new_plane_state->fb, >layout);
+ if (ret) {
+ DPU_ERROR_PLANE(pdpu, "failed to get format plane sizes, %d\n",
ret);
+ return ret;
+ }
+
I think we need another function to do the check. It seems incorrect to
populate the layout to the plane state knowing it can potentially fail.
Can we move the validation part of dpu_format_populate_plane_sizes() out
to another helper dpu_format_validate_plane_sizes() and use that?
And then make the remaining dpu_format_populate_plane_sizes() just a
void API to fill the layout?
Re: [PATCH 3/9] drm/msm/dpu: split dpu_format_populate_layout
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote: Split dpu_format_populate_layout() into addess-related and pitch/format-related parts. Signed-off-by: Dmitry Baryshkov --- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 8 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c| 44 +++--- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h| 8 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 -- 4 files changed, 45 insertions(+), 27 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 2/9] drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote: The dpu_plane_prepare_fb() already calls dpu_format_populate_layout(). Store the generated layour in the plane state and drop this call from dpu_plane_sspp_update(). Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 19 --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 3 +++ 2 files changed, 7 insertions(+), 15 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
On 3/19/2024 6:21 AM, Dmitry Baryshkov wrote:
The msm_kms_funcs::check_modified_format() callback is not used by the
driver. Drop it completely.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 45 -
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 15 --
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 -
drivers/gpu/drm/msm/msm_kms.h | 5
4 files changed, 66 deletions(-)
I think in this case, I am leaning towards completing the implementation
rather than dropping it as usual.
It seems its easier to just add the support to call this like the
attached patch?
WDYT?
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index e366ab134249..ff0df478c958 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -960,51 +960,6 @@ int dpu_format_populate_layout(
return ret;
}
-int dpu_format_check_modified_format(
- const struct msm_kms *kms,
- const struct msm_format *msm_fmt,
- const struct drm_mode_fb_cmd2 *cmd,
- struct drm_gem_object **bos)
-{
- const struct drm_format_info *info;
- const struct dpu_format *fmt;
- struct dpu_hw_fmt_layout layout;
- uint32_t bos_total_size = 0;
- int ret, i;
-
- if (!msm_fmt || !cmd || !bos) {
- DRM_ERROR("invalid arguments\n");
- return -EINVAL;
- }
-
- fmt = to_dpu_format(msm_fmt);
- info = drm_format_info(fmt->base.pixel_format);
- if (!info)
- return -EINVAL;
-
- ret = dpu_format_get_plane_sizes(fmt, cmd->width, cmd->height,
- , cmd->pitches);
- if (ret)
- return ret;
-
- for (i = 0; i < info->num_planes; i++) {
- if (!bos[i]) {
- DRM_ERROR("invalid handle for plane %d\n", i);
- return -EINVAL;
- }
- if ((i == 0) || (bos[i] != bos[0]))
- bos_total_size += bos[i]->size;
- }
-
- if (bos_total_size < layout.total_size) {
- DRM_ERROR("buffers total size too small %u expected %u\n",
- bos_total_size, layout.total_size);
- return -EINVAL;
- }
-
- return 0;
-}
-
const struct dpu_format *dpu_get_dpu_format_ext(
const uint32_t format,
const uint64_t modifier)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
index 84b8b3289f18..9442445f1a86 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
@@ -54,21 +54,6 @@ const struct msm_format *dpu_get_msm_format(
const uint32_t format,
const uint64_t modifiers);
-/**
- * dpu_format_check_modified_format - validate format and buffers for
- * dpu non-standard, i.e. modified format
- * @kms: kms driver
- * @msm_fmt: pointer to the msm_fmt base pointer of an dpu_format
- * @cmd: fb_cmd2 structure user request
- * @bos: gem buffer object list
- *
- * Return: error code on failure, 0 on success
- */
-int dpu_format_check_modified_format(
- const struct msm_kms *kms,
- const struct msm_format *msm_fmt,
- const struct drm_mode_fb_cmd2 *cmd,
- struct drm_gem_object **bos);
/**
* dpu_format_populate_layout - populate the given format layout based on
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a1f5d7c4ab91..7257ac4020d8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -969,7 +969,6 @@ static const struct msm_kms_funcs kms_funcs = {
.complete_commit = dpu_kms_complete_commit,
.enable_vblank = dpu_kms_enable_vblank,
.disable_vblank = dpu_kms_disable_vblank,
- .check_modified_format = dpu_format_check_modified_format,
.get_format = dpu_get_msm_format,
.destroy = dpu_kms_destroy,
.snapshot= dpu_kms_mdp_snapshot,
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 0641f6111b93..b794ed918b56 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -96,11 +96,6 @@ struct msm_kms_funcs {
const struct msm_format *(*get_format)(struct msm_kms *kms,
const uint32_t format,
const uint64_t modifiers);
- /* do format checking on format modified through fb_cmd2 modifiers */
- int (*check_modified_format)(const struct msm_kms *kms,
- const struct msm_format *msm_fmt,
-
Re: [PATCH 04/12] drm/msm: add arrays listing formats supported by MDP4/MDP5 hardware
On 4/19/2024 2:21 PM, Dmitry Baryshkov wrote:
On Sat, 20 Apr 2024 at 00:06, Abhinav Kumar wrote:
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
MDP4 and MDP5 drivers enumerate supported formats each time the plane is
created. In preparation to merger of MDP DPU format databases, define
precise formats list, so that changes to the database do not cause the
driver to add unsupported format to the list.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 57 --
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 36 +++---
drivers/gpu/drm/msm/disp/mdp_format.c | 28 ---
drivers/gpu/drm/msm/disp/mdp_kms.h | 1 -
4 files changed, 80 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
index b689b618da78..cebe20c82a54 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
@@ -371,6 +371,47 @@ static const uint64_t supported_format_modifiers[] = {
DRM_FORMAT_MOD_INVALID
};
+const uint32_t mdp4_rgb_formats[] = {
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_RGBA,
+ DRM_FORMAT_BGRA,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_RGBX,
+ DRM_FORMAT_BGRX,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR565,
+};
+
+const uint32_t mdp4_rgb_yuv_formats[] = {
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_RGBA,
+ DRM_FORMAT_BGRA,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_RGBX,
+ DRM_FORMAT_BGRX,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR565,
+
+ DRM_FORMAT_NV12,
+ DRM_FORMAT_NV21,
+ DRM_FORMAT_NV16,
+ DRM_FORMAT_NV61,
+ DRM_FORMAT_VYUY,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_YUYV,
+ DRM_FORMAT_YVYU,
+ DRM_FORMAT_YUV420,
+ DRM_FORMAT_YVU420,
+};
+
/* initialize plane */
struct drm_plane *mdp4_plane_init(struct drm_device *dev,
enum mdp4_pipe pipe_id, bool private_plane)
@@ -379,6 +420,8 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
struct mdp4_plane *mdp4_plane;
int ret;
enum drm_plane_type type;
+ const uint32_t *formats;
+ unsigned int nformats;
mdp4_plane = kzalloc(sizeof(*mdp4_plane), GFP_KERNEL);
if (!mdp4_plane) {
@@ -392,13 +435,17 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
mdp4_plane->name = pipe_names[pipe_id];
mdp4_plane->caps = mdp4_pipe_caps(pipe_id);
- mdp4_plane->nformats = mdp_get_formats(mdp4_plane->formats,
- ARRAY_SIZE(mdp4_plane->formats),
- !pipe_supports_yuv(mdp4_plane->caps));
-
type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
+
+ if (pipe_supports_yuv(mdp4_plane->caps)) {
+ formats = mdp4_rgb_yuv_formats;
+ nformats = ARRAY_SIZE(mdp4_rgb_yuv_formats);
+ } else {
+ formats = mdp4_rgb_formats;
+ nformats = ARRAY_SIZE(mdp4_rgb_formats);
+ }
ret = drm_universal_plane_init(dev, plane, 0xff, _plane_funcs,
- mdp4_plane->formats, mdp4_plane->nformats,
+ formats, nformats,
supported_format_modifiers, type, NULL);
if (ret)
goto fail;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index 0d5ff03cb091..aa8342d93393 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -17,9 +17,6 @@
struct mdp5_plane {
struct drm_plane base;
-
- uint32_t nformats;
- uint32_t formats[32];
};
#define to_mdp5_plane(x) container_of(x, struct mdp5_plane, base)
@@ -1007,6 +1004,32 @@ uint32_t mdp5_plane_get_flush(struct drm_plane *plane)
return mask;
}
+const uint32_t mdp5_plane_formats[] = {
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_RGBA,
+ DRM_FORMAT_BGRA,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_RGBX,
+ DRM_FORMAT_BGRX,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR565,
+
+ DRM_FORMAT_NV12,
+ DRM_FORMAT_NV21,
+ DRM_FORMAT_NV16,
+ DRM_FORMAT_NV61,
+ DRM_FORMAT_VYUY,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_YUYV,
+ DRM_FORMAT_YVYU,
+ DRM_FORMAT_YUV420,
+ DRM_FORMAT_YVU420,
+};
+
/* initialize plane */
struct drm_plane *mdp5_plane_init(struct drm_device *dev,
enum drm_plane_type type)
@@ -1023,12 +1046,9 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
plane = _plane->bas
Re: [PATCH 04/12] drm/msm: add arrays listing formats supported by MDP4/MDP5 hardware
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
MDP4 and MDP5 drivers enumerate supported formats each time the plane is
created. In preparation to merger of MDP DPU format databases, define
precise formats list, so that changes to the database do not cause the
driver to add unsupported format to the list.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 57 --
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 36 +++---
drivers/gpu/drm/msm/disp/mdp_format.c | 28 ---
drivers/gpu/drm/msm/disp/mdp_kms.h | 1 -
4 files changed, 80 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
index b689b618da78..cebe20c82a54 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
@@ -371,6 +371,47 @@ static const uint64_t supported_format_modifiers[] = {
DRM_FORMAT_MOD_INVALID
};
+const uint32_t mdp4_rgb_formats[] = {
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_RGBA,
+ DRM_FORMAT_BGRA,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_RGBX,
+ DRM_FORMAT_BGRX,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR565,
+};
+
+const uint32_t mdp4_rgb_yuv_formats[] = {
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_RGBA,
+ DRM_FORMAT_BGRA,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_RGBX,
+ DRM_FORMAT_BGRX,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR565,
+
+ DRM_FORMAT_NV12,
+ DRM_FORMAT_NV21,
+ DRM_FORMAT_NV16,
+ DRM_FORMAT_NV61,
+ DRM_FORMAT_VYUY,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_YUYV,
+ DRM_FORMAT_YVYU,
+ DRM_FORMAT_YUV420,
+ DRM_FORMAT_YVU420,
+};
+
/* initialize plane */
struct drm_plane *mdp4_plane_init(struct drm_device *dev,
enum mdp4_pipe pipe_id, bool private_plane)
@@ -379,6 +420,8 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
struct mdp4_plane *mdp4_plane;
int ret;
enum drm_plane_type type;
+ const uint32_t *formats;
+ unsigned int nformats;
mdp4_plane = kzalloc(sizeof(*mdp4_plane), GFP_KERNEL);
if (!mdp4_plane) {
@@ -392,13 +435,17 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
mdp4_plane->name = pipe_names[pipe_id];
mdp4_plane->caps = mdp4_pipe_caps(pipe_id);
- mdp4_plane->nformats = mdp_get_formats(mdp4_plane->formats,
- ARRAY_SIZE(mdp4_plane->formats),
- !pipe_supports_yuv(mdp4_plane->caps));
-
type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
+
+ if (pipe_supports_yuv(mdp4_plane->caps)) {
+ formats = mdp4_rgb_yuv_formats;
+ nformats = ARRAY_SIZE(mdp4_rgb_yuv_formats);
+ } else {
+ formats = mdp4_rgb_formats;
+ nformats = ARRAY_SIZE(mdp4_rgb_formats);
+ }
ret = drm_universal_plane_init(dev, plane, 0xff, _plane_funcs,
-mdp4_plane->formats, mdp4_plane->nformats,
+formats, nformats,
supported_format_modifiers, type, NULL);
if (ret)
goto fail;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index 0d5ff03cb091..aa8342d93393 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -17,9 +17,6 @@
struct mdp5_plane {
struct drm_plane base;
-
- uint32_t nformats;
- uint32_t formats[32];
};
#define to_mdp5_plane(x) container_of(x, struct mdp5_plane, base)
@@ -1007,6 +1004,32 @@ uint32_t mdp5_plane_get_flush(struct drm_plane *plane)
return mask;
}
+const uint32_t mdp5_plane_formats[] = {
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_RGBA,
+ DRM_FORMAT_BGRA,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_RGBX,
+ DRM_FORMAT_BGRX,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR565,
+
+ DRM_FORMAT_NV12,
+ DRM_FORMAT_NV21,
+ DRM_FORMAT_NV16,
+ DRM_FORMAT_NV61,
+ DRM_FORMAT_VYUY,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_YUYV,
+ DRM_FORMAT_YVYU,
+ DRM_FORMAT_YUV420,
+ DRM_FORMAT_YVU420,
+};
+
/* initialize plane */
struct drm_plane *mdp5_plane_init(struct drm_device *dev,
enum drm_plane_type type)
@@ -1023,12 +1046,9 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
Re: [PATCH 03/12] drm/msm/dpu: use format-related definitions from mdp_common.xml.h
On 4/10/2024 7:38 PM, Dmitry Baryshkov wrote: On Thu, 11 Apr 2024 at 02:54, Abhinav Kumar wrote: On 4/10/2024 2:12 PM, Dmitry Baryshkov wrote: On Wed, Apr 10, 2024 at 01:18:42PM -0700, Abhinav Kumar wrote: On 4/10/2024 1:16 PM, Dmitry Baryshkov wrote: On Wed, 10 Apr 2024 at 23:00, Abhinav Kumar wrote: On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Instead of having DPU-specific defines, switch to the definitions from the mdp_common.xml.h file. This is the preparation for merged of DPU and MDP format tables. Adding MDP_***__ usages in DPU driver is quite confusing. Can we align to a common naming scheme such as DISP_***? No, it's not something display-generic. It is specific to MDP platforms. In the end DPU is a continuation of the MDP lineup, isn't it? No some aspects of the hw are completely different as you already know between MDP4/MDP5 and DPU. Bringing back MDP usages into DPU does not seem right. MDP4 is different, it's true. But there is a lot of common between MDP5 and DPU. Frakly speaking, I don't see an issue with using the constant that was defined for MDP5 for DPU layer. Especially since we are also going to use mdp_ functions for format handling. All the HW naming etc in the doc has migrated to DPU and in fact it only makes sense to start using DPU for MDP5 as we plan to move mdp5 targets to DPU anyway. Not the other way around. MDP4 remains different. How about MSM_DISP then? I dont get why this is MDP platform specific. Because the term MDP no longer holds true for DPU. I am even looking for future chipsets. We cannot live with MDP5 names. Have to think of generic names for formats. Another point: MDP_ is still frequently used in the DPU driver. See dpu_hwio.h, dpu_hw_catalog.h or dpu_hw_interrupts.c As I wrote in https://patchwork.freedesktop.org/patch/570148/?series=127230=1, lets go ahead with the MDP naming which you have. If we see its not working out later on, please be open to a mass renaming that time. With that expectation set, Reviewed-by: Abhinav Kumar
Re: [PATCH 12/12] drm/msm: drop msm_kms_funcs::get_format() callback
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Now as all subdrivers were converted to use common database of formats, drop the get_format() callback and use mdp_get_format() directly. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c| 2 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 1 - drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 - drivers/gpu/drm/msm/msm_fb.c | 2 +- drivers/gpu/drm/msm/msm_kms.h| 4 8 files changed, 4 insertions(+), 11 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 11/12] drm/msm: merge dpu format database to MDP formats
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
Finally remove duplication between DPU and generic MDP code by merging
DPU format lists to the MDP format database.
Signed-off-by: Dmitry Baryshkov
---
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 602 --
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 23 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 10 -
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 3 +-
drivers/gpu/drm/msm/disp/mdp_format.c | 595 +++--
drivers/gpu/drm/msm/disp/mdp_kms.h| 2 -
drivers/gpu/drm/msm/msm_drv.h | 12 +
10 files changed, 549 insertions(+), 706 deletions(-)
I cross-checked a few macros visually (not each one) and it LGTM in
terms of just moving it from dpu_formats.c to mdp_format.c
Even in this change I had the same concern about whether to use MDP for
dpu formats.
But I think even if we make it MSM_*** then we will have to keep them in
some msm_** header and not mdp_format.c
So lets go ahead with the MDP naming which you have. If we see its not
working out later on, please be open to a mass renaming that time.
index dea6d47854fe..e7651a0e878c 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -267,6 +267,16 @@ enum msm_format_flags {
#define MSM_FORMAT_FLAG_UNPACK_ALIGN_MSB
BIT(MSM_FORMAT_FLAG_UNPACK_ALIGN_MSB_BIT)
#define MSM_FORMAT_FLAG_ALPHA_ENABLE BIT(MSM_FORMAT_FLAG_ALPHA_ENABLE_BIT)
+/**
+ * DPU HW,Component order color map
+ */
+enum {
+ C0_G_Y = 0,
+ C1_B_Cb = 1,
+ C2_R_Cr = 2,
+ C3_ALPHA = 3
+};
+
/**
* struct msm_format: defines the format configuration
* @pixel_format: format fourcc
@@ -305,6 +315,8 @@ struct msm_format {
(((X)->fetch_mode == MDP_FETCH_UBWC) && \
((X)->flags & MSM_FORMAT_FLAG_COMPRESSED))
+const struct msm_format *mdp_get_format(struct msm_kms *kms, uint32_t format, uint64_t modifier);
+
struct msm_pending_timer;
int msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
I am now thinking that do you think it makes sense to move all
MDP_FORMAT macros to a new mdp_formats.h including the RGB/YUV bitfield
macros (even though I already acked that change).
Instead of bloating msm_drv.h even more?
Re: [PATCH 10/12] drm/msm: convert msm_format::alpha_enable to the flag
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
Instead of having a bool field alpha_enable, convert it to the
flag, this save space in the tables and allows us to handle all booleans
in the same way.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 12 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 24 ++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 7 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 4 ++--
drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 2 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 3 ++-
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 9
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 3 ++-
drivers/gpu/drm/msm/disp/mdp_format.c | 2 +-
drivers/gpu/drm/msm/msm_drv.h | 4 ++--
11 files changed, 40 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 9041b0d71b25..201010038660 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -342,7 +342,7 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer
*mixer,
/* default to opaque blending */
if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
- !format->alpha_enable) {
+ !(format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE)) {
blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
DPU_BLEND_BG_ALPHA_BG_CONST;
} else if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) {
@@ -373,8 +373,8 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer
*mixer,
lm->ops.setup_blend_config(lm, pstate->stage,
fg_alpha, bg_alpha, blend_op);
- DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%u blend_op:0x%x\n",
- >pixel_format, format->alpha_enable, blend_op);
+ DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%lu blend_op:0x%x\n",
+ >pixel_format, format->flags &
MSM_FORMAT_FLAG_ALPHA_ENABLE, blend_op);
}
static void _dpu_crtc_program_lm_output_roi(struct drm_crtc *crtc)
@@ -472,7 +472,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
format = msm_framebuffer_format(pstate->base.fb);
- if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
+ if (pstate->stage == DPU_STAGE_BASE &&
+ format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE)
bg_alpha_enable = true;
set_bit(pstate->pipe.sspp->idx, fetch_active);
@@ -495,7 +496,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
_dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate,
format);
- if (bg_alpha_enable && !format->alpha_enable)
+ if (bg_alpha_enable &&
+ !(format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE))
mixer[lm_idx].mixer_op_mode = 0;
else
mixer[lm_idx].mixer_op_mode |=
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index baf0fd67bf42..de9e93cb42c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -36,7 +36,6 @@ bp, flg, fm, np)
\
{ \
.pixel_format = DRM_FORMAT_ ## fmt, \
.fetch_type = MDP_PLANE_INTERLEAVED, \
- .alpha_enable = alpha,\
.element = { (e0), (e1), (e2), (e3) },\
.bpc_g_y = g, \
.bpc_b_cb = b,\
@@ -46,7 +45,9 @@ bp, flg, fm, np)
\
.unpack_count = uc, \
.bpp = bp,\
.fetch_mode = fm, \
- .flags = MSM_FORMAT_FLAG_UNPACK_TIGHT | flg, \
+ .flags = MSM_FORMAT_FLAG_UNPACK_TIGHT | \
+ (alpha ? MSM_FORMAT_FLAG_ALPHA_ENABLE : 0) | \
+ flg, \
In the previous two patches where the same thing was done for
unpack_tight and unpack_align_msb, it was different in the sense that
just on the basis of which macro we were choosing we knew the value of
those flags so you could just
Re: [PATCH 09/12] drm/msm: convert msm_format::unpack_align_msb to the flag
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Instead of having a u8 or bool field unpack_align_msb, convert it to the flag, this save space in the tables and allows us to handle all booleans in the same way. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 12 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 2 +- drivers/gpu/drm/msm/msm_drv.h | 4 ++-- 4 files changed, 6 insertions(+), 14 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 08/12] drm/msm: convert msm_format::unpack_tight to the flag
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Instead of having a u8 or bool field unpack_tight, convert it to the flag, this save space in the tables and allows us to handle all booleans in the same way. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 22 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 2 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 3 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 3 +- drivers/gpu/drm/msm/disp/mdp_format.c | 52 ++--- drivers/gpu/drm/msm/msm_drv.h | 4 +- 7 files changed, 41 insertions(+), 47 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 07/12] drm/msm: merge dpu_format and mdp_format in struct msm_format
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
Structures dpu_format and mdp_format are largely the same structures.
In order to remove duplication between format databases, merge these two
stucture definitions into the global struct msm_format.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +-
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 184 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 41 +---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 14 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 16 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 74 +++
drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 4 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c| 26 +--
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 7 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 54 ++---
drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 4 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 2 +-
drivers/gpu/drm/msm/disp/mdp_format.c | 28 ++-
drivers/gpu/drm/msm/disp/mdp_kms.h| 13 --
drivers/gpu/drm/msm/msm_drv.h | 28 +++
24 files changed, 279 insertions(+), 288 deletions(-)
int mdp5_smp_assign(struct mdp5_smp *smp, struct mdp5_smp_state *state,
diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c
b/drivers/gpu/drm/msm/disp/mdp_format.c
index 30919641c813..5fc55f41e74f 100644
--- a/drivers/gpu/drm/msm/disp/mdp_format.c
+++ b/drivers/gpu/drm/msm/disp/mdp_format.c
@@ -63,26 +63,24 @@ static struct csc_cfg csc_convert[CSC_MAX] = {
};
#define FMT(name, a, r, g, b, e0, e1, e2, e3, alpha, tight, c, cnt, fp, cs, yuv) { \
- .base = {\
- .pixel_format = DRM_FORMAT_ ## name, \
- .flags = yuv ? MSM_FORMAT_FLAG_YUV : 0, \
- }, \
+ .pixel_format = DRM_FORMAT_ ## name, \
.bpc_a = BPC ## a ## A, \
- .bpc_r = BPC ## r, \
- .bpc_g = BPC ## g, \
- .bpc_b = BPC ## b, \
- .unpack = { e0, e1, e2, e3 },\
+ .bpc_r_cr = BPC ## r,\
+ .bpc_g_y = BPC ## g, \
+ .bpc_b_cb = BPC ## b,\
+ .element = { e0, e1, e2, e3 }, \
+ .fetch_type = fp,\
+ .chroma_sample = cs, \
.alpha_enable = alpha, \
.unpack_tight = tight, \
- .cpp = c,\
.unpack_count = cnt, \
- .fetch_type = fp,\
- .chroma_sample = cs, \
Minor nit:
These two lines are only moving the locations of assignment so
unnecessary change?
Rest LGTM,
Reviewed-by: Abhinav Kumar
For validation, are you relying mostly on the CI here OR also other
internal farms? Even though mostly its just making code common, basic
display coming up on one target each of MDP4/MDP5/DPU will be great to
be safe.
Re: [PATCH 06/12] drm/msm/dpu: pull format flag definitions to msm_drv.h
On 4/11/2024 11:41 AM, Abhinav Kumar wrote: On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: In preparation to merger of formats databases, pull format flag definitions to msm_drv.h header, so that they are visibile to both dpu and mdp drivers. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 98 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 28 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 8 +- drivers/gpu/drm/msm/disp/mdp_format.c | 6 +- drivers/gpu/drm/msm/disp/mdp_kms.h | 3 +- drivers/gpu/drm/msm/msm_drv.h | 24 + 8 files changed, 91 insertions(+), 84 deletions(-) +#define DPU_FORMAT_IS_YUV(X) MSM_FORMAT_IS_YUV(&(X)->base) +#define DPU_FORMAT_IS_DX(X) MSM_FORMAT_IS_DX(&(X)->base) +#define DPU_FORMAT_IS_LINEAR(X) MSM_FORMAT_IS_LINEAR(&(X)->base) +#define DPU_FORMAT_IS_TILE(X) MSM_FORMAT_IS_TILE(&(X)->base) +#define DPU_FORMAT_IS_UBWC(X) MSM_FORMAT_IS_UBWC(&(X)->base) Do we need another wrapper macro on top of MSM_FORMAT_*** macros? Why cant we use them directly? Same comment for MDP_FORMAT_IS_YUV macro as well. Rest LGTM. never mind, the next change does exactly this. Hence this one LGTM Reviewed-by: Abhinav Kumar
Re: [PATCH 06/12] drm/msm/dpu: pull format flag definitions to msm_drv.h
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: In preparation to merger of formats databases, pull format flag definitions to msm_drv.h header, so that they are visibile to both dpu and mdp drivers. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 98 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 28 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 8 +- drivers/gpu/drm/msm/disp/mdp_format.c | 6 +- drivers/gpu/drm/msm/disp/mdp_kms.h | 3 +- drivers/gpu/drm/msm/msm_drv.h | 24 + 8 files changed, 91 insertions(+), 84 deletions(-) +#define DPU_FORMAT_IS_YUV(X) MSM_FORMAT_IS_YUV(&(X)->base) +#define DPU_FORMAT_IS_DX(X)MSM_FORMAT_IS_DX(&(X)->base) +#define DPU_FORMAT_IS_LINEAR(X)MSM_FORMAT_IS_LINEAR(&(X)->base) +#define DPU_FORMAT_IS_TILE(X) MSM_FORMAT_IS_TILE(&(X)->base) +#define DPU_FORMAT_IS_UBWC(X) MSM_FORMAT_IS_UBWC(&(X)->base) Do we need another wrapper macro on top of MSM_FORMAT_*** macros? Why cant we use them directly? Same comment for MDP_FORMAT_IS_YUV macro as well. Rest LGTM.
Re: [PATCH 03/12] drm/msm/dpu: use format-related definitions from mdp_common.xml.h
On 4/10/2024 2:12 PM, Dmitry Baryshkov wrote: On Wed, Apr 10, 2024 at 01:18:42PM -0700, Abhinav Kumar wrote: On 4/10/2024 1:16 PM, Dmitry Baryshkov wrote: On Wed, 10 Apr 2024 at 23:00, Abhinav Kumar wrote: On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Instead of having DPU-specific defines, switch to the definitions from the mdp_common.xml.h file. This is the preparation for merged of DPU and MDP format tables. Adding MDP_***__ usages in DPU driver is quite confusing. Can we align to a common naming scheme such as DISP_***? No, it's not something display-generic. It is specific to MDP platforms. In the end DPU is a continuation of the MDP lineup, isn't it? No some aspects of the hw are completely different as you already know between MDP4/MDP5 and DPU. Bringing back MDP usages into DPU does not seem right. MDP4 is different, it's true. But there is a lot of common between MDP5 and DPU. Frakly speaking, I don't see an issue with using the constant that was defined for MDP5 for DPU layer. Especially since we are also going to use mdp_ functions for format handling. All the HW naming etc in the doc has migrated to DPU and in fact it only makes sense to start using DPU for MDP5 as we plan to move mdp5 targets to DPU anyway. Not the other way around. MDP4 remains different. How about MSM_DISP then? I dont get why this is MDP platform specific. Because the term MDP no longer holds true for DPU. I am even looking for future chipsets. We cannot live with MDP5 names. Have to think of generic names for formats.
Re: [PATCH 05/12] drm/msm/dpu: in dpu_format replace bitmap with unsigned long field
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Using bitmap for the flags results in a clumsy syntax on test_bit, replace it with unsigned long type and simple binary ops. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 18 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 16 +++- 2 files changed, 16 insertions(+), 18 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 03/12] drm/msm/dpu: use format-related definitions from mdp_common.xml.h
On 4/10/2024 1:16 PM, Dmitry Baryshkov wrote: On Wed, 10 Apr 2024 at 23:00, Abhinav Kumar wrote: On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Instead of having DPU-specific defines, switch to the definitions from the mdp_common.xml.h file. This is the preparation for merged of DPU and MDP format tables. Adding MDP_***__ usages in DPU driver is quite confusing. Can we align to a common naming scheme such as DISP_***? No, it's not something display-generic. It is specific to MDP platforms. In the end DPU is a continuation of the MDP lineup, isn't it? No some aspects of the hw are completely different as you already know between MDP4/MDP5 and DPU. Bringing back MDP usages into DPU does not seem right.
Re: [PATCH 03/12] drm/msm/dpu: use format-related definitions from mdp_common.xml.h
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
Instead of having DPU-specific defines, switch to the definitions from
the mdp_common.xml.h file. This is the preparation for merged of DPU and
MDP format tables.
Adding MDP_***__ usages in DPU driver is quite confusing.
Can we align to a common naming scheme such as DISP_***?
Signed-off-by: Dmitry Baryshkov
---
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 290 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 64 +---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 12 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +-
6 files changed, 164 insertions(+), 214 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 0b6a761d68b7..4fead04d83a0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -640,7 +640,7 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct
dpu_encoder_phys *phys_enc
wb_cfg->dest.height = job->fb->height;
wb_cfg->dest.num_planes = wb_cfg->dest.format->num_planes;
- if ((wb_cfg->dest.format->fetch_planes == DPU_PLANE_PLANAR) &&
+ if ((wb_cfg->dest.format->fetch_planes == MDP_PLANE_PLANAR) &&
(wb_cfg->dest.format->element[0] == C1_B_Cb))
swap(wb_cfg->dest.plane_addr[1], wb_cfg->dest.plane_addr[2]);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index e366ab134249..05e82f5dd0e6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -35,11 +35,11 @@
bp, flg, fm, np) \
{ \
.base.pixel_format = DRM_FORMAT_ ## fmt, \
- .fetch_planes = DPU_PLANE_INTERLEAVED,\
+ .fetch_planes = MDP_PLANE_INTERLEAVED,\
.alpha_enable = alpha,\
.element = { (e0), (e1), (e2), (e3) },\
.bits = { g, b, r, a }, \
- .chroma_sample = DPU_CHROMA_RGB, \
+ .chroma_sample = CHROMA_FULL, \
.unpack_align_msb = 0,\
.unpack_tight = 1,\
.unpack_count = uc, \
@@ -54,11 +54,11 @@ bp, flg, fm, np)
\
alpha, bp, flg, fm, np, th) \
{ \
.base.pixel_format = DRM_FORMAT_ ## fmt, \
- .fetch_planes = DPU_PLANE_INTERLEAVED,\
+ .fetch_planes = MDP_PLANE_INTERLEAVED,\
.alpha_enable = alpha,\
.element = { (e0), (e1), (e2), (e3) },\
.bits = { g, b, r, a }, \
- .chroma_sample = DPU_CHROMA_RGB, \
+ .chroma_sample = CHROMA_FULL, \
.unpack_align_msb = 0,\
.unpack_tight = 1,\
.unpack_count = uc, \
@@ -74,7 +74,7 @@ alpha, bp, flg, fm, np, th)
\
alpha, chroma, count, bp, flg, fm, np)\
{ \
.base.pixel_format = DRM_FORMAT_ ## fmt, \
- .fetch_planes = DPU_PLANE_INTERLEAVED,\
+ .fetch_planes = MDP_PLANE_INTERLEAVED,\
.alpha_enable = alpha,\
.element = { (e0), (e1), (e2), (e3)}, \
.bits = { g, b, r, a }, \
@@ -92,7 +92,7 @@ alpha, chroma, count, bp, flg, fm, np)
\
#define PSEUDO_YUV_FMT(fmt, a, r, g, b, e0, e1, chroma, flg, fm, np) \
{ \
.base.pixel_format = DRM_FORMAT_ ## fmt, \
- .fetch_planes =
Re: [PATCH 02/12] drm/msm/disp: add mdp_fetch_mode enum
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Pull in new enum from the mesa registers. This commit should be replaced with the registers sync with Mesa instead. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/mdp_common.xml.h | 6 ++ 1 file changed, 6 insertions(+) Reviewed-by: Abhinav Kumar
Re: [PATCH 01/12] drm/msm: fix BPC1 -> BPC4
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Fix enum mdp_bpc::BPC1 value to be BPC4 instead (as shown in the DPU driver). This commit should be replaced with the registers sync with Mesa instead. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/mdp_common.xml.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 3/3] drm/msm/dsi: simplify connector creation
On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote: Instead of having two functions, msm_dsi_manager_bridge_init() and msm_dsi_manager_ext_bridge_init(), merge them into msm_dsi_manager_connector_init(), moving drm_bridge_attach() to be called from the bridge's attach callback (as most other bridges do). Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/dsi.c | 10 + drivers/gpu/drm/msm/dsi/dsi.h | 5 ++--- drivers/gpu/drm/msm/dsi/dsi_manager.c | 41 +++ 3 files changed, 21 insertions(+), 35 deletions(-) LGTM, Reviewed-by: Abhinav Kumar
Re: [PATCH 2/3] drm/msm/dsi: move next bridge acquisition to dsi_bind
On 4/5/2024 11:15 AM, Dmitry Baryshkov wrote:
On Fri, 5 Apr 2024 at 20:35, Abhinav Kumar wrote:
On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:
Currently the MSM DSI driver looks for the next bridge during
msm_dsi_modeset_init(). If the bridge is not registered at that point,
this might result in -EPROBE_DEFER, which can be problematic that late
during the device probe process. Move next bridge acquisition to the
dsi_bind state so that probe deferral is returned as early as possible.
But msm_dsi_modeset)init() is also called during msm_drm_bind() only.
What issue are you suggesting will be fixed by moving this from
msm_drm_bind() to dsi_bind()?
The goal is to return as early as possible as not not cause
probe-deferral loops. See commit fbc35b45f9f6 ("Add documentation on
meaning of -EPROBE_DEFER"). It discusses returning from probe() but
the same logic applies to bind().
Understood. I was trying to make sure the purpose of the patch is that
"deferral in component_bind() is better than component_master_bind()"
But yes, overall that is better since the unbounding path will be more
in the master case.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi.c | 16
drivers/gpu/drm/msm/dsi/dsi.h | 2 ++
drivers/gpu/drm/msm/dsi/dsi_manager.c | 8 +---
3 files changed, 19 insertions(+), 7 deletions(-)
Reviewed-by: Abhinav Kumar
Re: [PATCH v2 6/6] drm/msm/dp: Use function arguments for audio operations
On 3/28/2024 7:40 AM, Bjorn Andersson wrote: From: Bjorn Andersson The dp_audio read and write operations uses members in struct dp_catalog for passing arguments and return values. This adds unnecessary complexity to the implementation, as it turns out after detangling the logic that no state is actually held in these variables. Clean this up by using function arguments and return values for passing the data. Reviewed-by: Dmitry Baryshkov Signed-off-by: Bjorn Andersson --- drivers/gpu/drm/msm/dp/dp_audio.c | 20 +-- drivers/gpu/drm/msm/dp/dp_catalog.c | 39 + drivers/gpu/drm/msm/dp/dp_catalog.h | 18 + 3 files changed, 28 insertions(+), 49 deletions(-) One quick question, was DP audio re-tested after this patch?
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On 4/8/2024 2:12 PM, Dmitry Baryshkov wrote:
On Mon, 8 Apr 2024 at 22:43, Abhinav Kumar wrote:
On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
From: Kuogee Hsieh
[..]
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
-dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+dp_hpd_plug_handle(dp, 0);
If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.
Will that not cause the same problem?
Regards,
Bjorn
Yes, your observation is correct and I had asked the same question to
kuogee before taking over this change and posting.
Should it then have the Co-developed-by trailers?
hmmm, perhaps but that didnt result in any code change between v2 and
v3, so I didnt add it.
We will have to handle that case separately. I don't have a good
solution yet for it without requiring further rework or we drop the
below snippet.
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
mutex_unlock(>event_mutex);
return 0;
}
I will need sometime to address that use-case as I need to see if we can
handle that better and then drop the the DISCONNECT_PENDING state to
address this fully. But it needs more testing.
But, we will need this patch anyway because without this we will not be
able to fix even the most regular and commonly seen case of basic
connect/disconnect receiving complementary events.
Hmm, no. We need to drop the HPD state machine, not to patch it. Once
the driver has proper detect() callback, there will be no
complementary events. That is a proper way to fix the code, not these
kinds of band-aids patches.
I had discussed this part too :)
I totally agree we should fix .detect()'s behavior to just match cable
connect/disconnect and not link_ready status.
But that alone would not have fixed this issue. If HPD thread does not
get scheduled and plug_handle() was not executed, .detect() would have
still returned old status as we will update the cable status only in
plug_handle() / unplug_handle() to have a common API between internal
and external hpd execution.
So we need to do both, make .detect() return correct status AND drop hpd
event thread processing.
But, dropping the hpd event thread processing alone was fixing the
complimentary events issue. So kuogee had only this part in this patch.
else if (dp_display->link_ready && status ==
connector_status_disconnected)
-dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On 4/8/2024 2:13 PM, Dmitry Baryshkov wrote:
On Tue, 9 Apr 2024 at 00:08, Abhinav Kumar wrote:
On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
From: Kuogee Hsieh
[..]
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ dp_hpd_plug_handle(dp, 0);
If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.
Will that not cause the same problem?
Regards,
Bjorn
Yes, your observation is correct and I had asked the same question to kuogee
before taking over this change and posting.
We will have to handle that case separately. I don't have a good solution
yet for it without requiring further rework or we drop the below snippet.
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
mutex_unlock(>event_mutex);
return 0;
}
I will need sometime to address that use-case as I need to see if we can
handle that better and then drop the the DISCONNECT_PENDING state to address
this fully. But it needs more testing.
But, we will need this patch anyway because without this we will not be able
to fix even the most regular and commonly seen case of basic
connect/disconnect receiving complementary events.
I did some more testing on this patch. Connecting and disconnecting the
cable while in fbcon works reliably, except for:
Thanks for the tests !
- edid/modes are not read before we bring up the link so I always end up
with 640x480
hmmm, I wonder why this should be affected due to this patch. We always
read the EDID during hpd_connect() and the selected resolution will be
programmed with the modeset. We will retry this with our x1e80100 and see.
BTW, why is EDID read during HPD handling? I always supposed that it
can be read much later, when the DRM framework calls the get_modes /
get_edid callbacks.
Well, dp_panel_read_sink_caps() is in dp_display_process_hpd_high()
currently. We read the edid there.
get_modes(), parses the EDID and adds the modes using drm_add_edid_modes().
- if I run modetest -s : the link is brought up with the new
resolution and I get my test image on the screen.
But as we're shutting down the link for the resolution chance I end up
in dp_bridge_hpd_notify() with link_ready && state = disconnected.
This triggers an unplug which hangs on the event_mutex, such that as
soon as I get the test image, the state machine enters
DISCONNECT_PENDING. This is immediately followed by another
!link_ready && status = connected, which attempts to perform the plug
operation, but as we're in DISCONNECT_PENDING this is posted on the
event queue. From there I get a log entry from my PLUG_INT, every
100ms stating that we're still in DISCONNECT_PENDING. If I exit
modetest the screen goes black, and no new mode can be selected,
because we're in DISCONNECT_PENDING. The only way out is to disconnect
the cable to complete the DISCONNECT_PENDING.
I am going to run this test-case and see what we can do.
Regards,
Bjorn
else if (dp_display->link_ready && status ==
connector_status_disconnected)
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On 4/8/2024 1:41 PM, Bjorn Andersson wrote:
On Mon, Apr 08, 2024 at 12:43:34PM -0700, Abhinav Kumar wrote:
On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
From: Kuogee Hsieh
[..]
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ dp_hpd_plug_handle(dp, 0);
If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.
Will that not cause the same problem?
Regards,
Bjorn
Yes, your observation is correct and I had asked the same question to kuogee
before taking over this change and posting.
We will have to handle that case separately. I don't have a good solution
yet for it without requiring further rework or we drop the below snippet.
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
mutex_unlock(>event_mutex);
return 0;
}
I will need sometime to address that use-case as I need to see if we can
handle that better and then drop the the DISCONNECT_PENDING state to address
this fully. But it needs more testing.
But, we will need this patch anyway because without this we will not be able
to fix even the most regular and commonly seen case of basic
connect/disconnect receiving complementary events.
I did some more testing on this patch. Connecting and disconnecting the
cable while in fbcon works reliably, except for:
Thanks for the tests !
- edid/modes are not read before we bring up the link so I always end up
with 640x480
hmmm, I wonder why this should be affected due to this patch. We always
read the EDID during hpd_connect() and the selected resolution will be
programmed with the modeset. We will retry this with our x1e80100 and see.
- if I run modetest -s : the link is brought up with the new
resolution and I get my test image on the screen.
But as we're shutting down the link for the resolution chance I end up
in dp_bridge_hpd_notify() with link_ready && state = disconnected.
This triggers an unplug which hangs on the event_mutex, such that as
soon as I get the test image, the state machine enters
DISCONNECT_PENDING. This is immediately followed by another
!link_ready && status = connected, which attempts to perform the plug
operation, but as we're in DISCONNECT_PENDING this is posted on the
event queue. From there I get a log entry from my PLUG_INT, every
100ms stating that we're still in DISCONNECT_PENDING. If I exit
modetest the screen goes black, and no new mode can be selected,
because we're in DISCONNECT_PENDING. The only way out is to disconnect
the cable to complete the DISCONNECT_PENDING.
I am going to run this test-case and see what we can do.
Regards,
Bjorn
else if (dp_display->link_ready && status ==
connector_status_disconnected)
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
Re: [PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
On 4/7/2024 11:48 AM, Bjorn Andersson wrote:
On Fri, Apr 05, 2024 at 08:15:47PM -0700, Abhinav Kumar wrote:
From: Kuogee Hsieh
[..]
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ dp_hpd_plug_handle(dp, 0);
If I read the code correctly, and we get an external connect event
inbetween a previous disconnect and the related disable call, this
should result in a PLUG_INT being injected into the queue still.
Will that not cause the same problem?
Regards,
Bjorn
Yes, your observation is correct and I had asked the same question to
kuogee before taking over this change and posting.
We will have to handle that case separately. I don't have a good
solution yet for it without requiring further rework or we drop the
below snippet.
if (state == ST_DISCONNECT_PENDING) {
/* wait until ST_DISCONNECTED */
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 1); /* delay = 1 */
mutex_unlock(>event_mutex);
return 0;
}
I will need sometime to address that use-case as I need to see if we can
handle that better and then drop the the DISCONNECT_PENDING state to
address this fully. But it needs more testing.
But, we will need this patch anyway because without this we will not be
able to fix even the most regular and commonly seen case of basic
connect/disconnect receiving complementary events.
else if (dp_display->link_ready && status ==
connector_status_disconnected)
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
Re: [PATCH] drm: ci: fix the xfails for apq8016
Hi Helen Gentle reminder on this. If you are okay, we can land it via msm-next tree... Thanks Abhinav On 4/1/2024 1:48 PM, Abhinav Kumar wrote: After IGT migrating to dynamic sub-tests, the pipe prefixes in the expected fails list are incorrect. Lets drop those to accurately match the expected fails. In addition, update the xfails list to match the current passing list. This should have ideally failed in the CI run because some tests were marked as fail even though they passed but due to the mismatch in test names, the matching didn't correctly work and was resulting in those failures not being seen. Here is the passing pipeline for apq8016 with this change: https://gitlab.freedesktop.org/drm/msm/-/jobs/57050562 Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt index 44a5c62dedad..b14d4e884971 100644 --- a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt +++ b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt @@ -1,17 +1,6 @@ kms_3d,Fail kms_addfb_basic@addfb25-bad-modifier,Fail -kms_cursor_legacy@all-pipes-forked-bo,Fail -kms_cursor_legacy@all-pipes-forked-move,Fail -kms_cursor_legacy@all-pipes-single-bo,Fail -kms_cursor_legacy@all-pipes-single-move,Fail -kms_cursor_legacy@all-pipes-torture-bo,Fail -kms_cursor_legacy@all-pipes-torture-move,Fail -kms_cursor_legacy@pipe-A-forked-bo,Fail -kms_cursor_legacy@pipe-A-forked-move,Fail -kms_cursor_legacy@pipe-A-single-bo,Fail -kms_cursor_legacy@pipe-A-single-move,Fail -kms_cursor_legacy@pipe-A-torture-bo,Fail -kms_cursor_legacy@pipe-A-torture-move,Fail +kms_cursor_legacy@torture-bo,Fail kms_force_connector_basic@force-edid,Fail kms_hdmi_inject@inject-4k,Fail kms_selftest@drm_format,Timeout
Re: [PATCH] drm/msm/dpu: Add callback function pointer check before its call
On 4/8/2024 1:55 AM, Aleksandr Mishin wrote:
In dpu_core_irq_callback_handler() callback function pointer is compared to
NULL,
but then callback function is unconditionally called by this pointer.
Fix this bug by adding conditional return.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Yes , as dmitry wrote, this should be Reported-by.
But rest LGTM.
Fixes: c929ac60b3ed ("drm/msm/dpu: allow just single IRQ callback")
Signed-off-by: Aleksandr Mishin
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 946dd0135dff..03a16fbd4c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -223,9 +223,11 @@ static void dpu_core_irq_callback_handler(struct dpu_kms
*dpu_kms, unsigned int
VERB("IRQ=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
- if (!irq_entry->cb)
+ if (!irq_entry->cb) {
DRM_ERROR("no registered cb, IRQ=[%d, %d]\n",
DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
+ return;
+ }
atomic_inc(_entry->count);
[PATCH v3] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
From: Kuogee Hsieh
For HPD events coming from external modules using drm_bridge_hpd_notify(),
the sequence of calls leading to dp_bridge_hpd_notify() is like below:
dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_connector_hotplug_event+0x30/0x3c [drm_kms_helper]
drm_bridge_connector_handle_hpd+0x84/0x94 [drm_kms_helper]
drm_bridge_connector_hpd_cb+0xc/0x14 [drm_kms_helper]
drm_bridge_hpd_notify+0x38/0x50 [drm]
drm_aux_hpd_bridge_notify+0x14/0x20 [aux_hpd_bridge]
pmic_glink_altmode_worker+0xec/0x27c [pmic_glink_altmode]
process_scheduled_works+0x17c/0x2cc
worker_thread+0x2ac/0x2d0
kthread+0xfc/0x120
There are three notifications delivered to DP driver for each notification
event.
1) From the drm_aux_hpd_bridge_notify() itself as shown above
2) From output_poll_execute() thread which arises due to
drm_helper_probe_single_connector_modes() call of the above stacktrace
as shown in more detail here.
dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_hotplug_event+0x30/0x3c [drm_kms_helper]
output_poll_execute+0xe0/0x210 [drm_kms_helper]
3) From the DP driver as the dp_bridge_hpd_notify() callback today triggers
the hpd_event_thread for connect and disconnect events respectively via below
stack
dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect_ctx+0x98/0x110 [drm_kms_helper]
check_connector_changed+0x4c/0x20c [drm_kms_helper]
drm_helper_hpd_irq_event+0x98/0x120 [drm_kms_helper]
hpd_event_thread+0x478/0x5bc [msm]
dp_bridge_hpd_notify() delivered from output_poll_execute() thread
returns the incorrect HPD status as the MSM DP driver returns the value
of link_ready and not the HPD status currently in the .detect() callback.
And because the HPD event thread has not run yet, this results in two
complementary
events.
To address this, fix dp_bridge_hpd_notify() to call
dp_hpd_plug_handle/unplug_handle()
directly to return consistent values for the above scenarios.
changes in v3:
- Fix the commit message as per submitting guidelines.
- remove extra line added
changes in v2:
- Fix the commit message to explain the scenario
- Fix the subject a little as well
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Signed-off-by: Kuogee Hsieh
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..bfb6dfff27e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,7 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ dp_hpd_plug_handle(dp, 0);
else if (dp_display->link_ready && status ==
connector_status_disconnected)
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ dp_hpd_unplug_handle(dp, 0);
}
--
2.43.2
[PATCH v2] drm/msm/dp: call dp_hpd_plug_handle()/unplug_handle() directly for external HPD
From: Kuogee Hsieh
In the external HPD case, hotplug event is delivered by pmic glink to DP driver
using drm_aux_hpd_bridge_notify().
The stacktrace showing the sequence of events is below:
dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_connector_hotplug_event+0x30/0x3c [drm_kms_helper]
drm_bridge_connector_handle_hpd+0x84/0x94 [drm_kms_helper]
drm_bridge_connector_hpd_cb+0xc/0x14 [drm_kms_helper]
drm_bridge_hpd_notify+0x38/0x50 [drm]
drm_aux_hpd_bridge_notify+0x14/0x20 [aux_hpd_bridge]
pmic_glink_altmode_worker+0xec/0x27c [pmic_glink_altmode]
process_scheduled_works+0x17c/0x2cc
worker_thread+0x2ac/0x2d0
kthread+0xfc/0x120
There are three notifications delivered to DP driver for each notification
event.
1) From the drm_aux_hpd_bridge_notify() itself as shown above
2) From output_poll_execute() thread which arises due to
drm_helper_probe_single_connector_modes() call of the above stacktrace
as shown in more detail here.
dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect+0x94/0xc0 [drm_kms_helper]
drm_helper_probe_single_connector_modes+0x43c/0x53c [drm_kms_helper]
drm_client_modeset_probe+0x240/0x1114 [drm]
drm_fb_helper_hotplug_event.part.26+0x9c/0xe8 [drm_kms_helper]
drm_fb_helper_hotplug_event+0x24/0x38 [drm_kms_helper]
msm_fbdev_client_hotplug+0x24/0xd4 [msm]
drm_client_dev_hotplug+0xd8/0x148 [drm]
drm_kms_helper_hotplug_event+0x30/0x3c [drm_kms_helper]
output_poll_execute+0xe0/0x210 [drm_kms_helper]
3) From the DP driver as the dp_bridge_hpd_notify() callback today triggers
the hpd_event_thread for connect and disconnect events respectively via below
stack
dp_bridge_hpd_notify+0x18/0x70 [msm]
drm_bridge_connector_detect+0x60/0xe8 [drm_kms_helper]
drm_helper_probe_detect_ctx+0x98/0x110 [drm_kms_helper]
check_connector_changed+0x4c/0x20c [drm_kms_helper]
drm_helper_hpd_irq_event+0x98/0x120 [drm_kms_helper]
hpd_event_thread+0x478/0x5bc [msm]
We have to address why we end up with 3 events for every single event so
something
is broken with how we work with the drm_bridge_connector.
But, the dp_bridge_hpd_notify() delivered from output_poll_execute() thread will
return the incorrect HPD status DP driver because the .detect() returns the
value
of link_ready and not the HPD status currently.
And because the HPD event thread has not run yet and this results in the two
complementary
events.
To fix this problem lets have dp_bridge_hpd_notify() call
dp_hpd_plug_handle/unplug_handle() directly instead of going through the
event thread.
Then the following .detect() called by drm_kms_helper_connector_hotplug_event()
will return correct value of HPD status since it uses the correct link_ready
value.
With this change, the HPD status delivered by both
drm_bridge_connector_hpd_notify()
and drm_kms_helper_connector_hotplug_event() will match each other consistently.
changes in v2:
- Fix the commit message to explain the scenario
- Fix the subject a little as well
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Signed-off-by: Kuogee Hsieh
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d80f89581760..dfb10584ff97 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1665,7 +1665,8 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
return;
if (!dp_display->link_ready && status == connector_status_connected)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ dp_hpd_plug_handle(dp, 0);
else if (dp_display->link_ready && status ==
connector_status_disconnected)
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ dp_hpd_unplug_handle(dp, 0);
+
}
--
2.43.2
Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly
On 3/28/2024 10:47 PM, Abhinav Kumar wrote:
On 3/28/2024 8:23 PM, Dmitry Baryshkov wrote:
On Fri, 29 Mar 2024 at 04:16, Abhinav Kumar
wrote:
On 3/28/2024 5:10 PM, Dmitry Baryshkov wrote:
On Fri, 29 Mar 2024 at 01:42, Abhinav Kumar
wrote:
On 3/28/2024 3:50 PM, Dmitry Baryshkov wrote:
On Thu, 28 Mar 2024 at 23:21, Abhinav Kumar
wrote:
On 3/28/2024 1:58 PM, Stephen Boyd wrote:
Quoting Abhinav Kumar (2024-03-28 13:24:34)
+ Johan and Bjorn for FYI
On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
For internal HPD case, hpd_event_thread is created to handle HPD
interrupts generated by HPD block of DP controller. It converts
HPD interrupts into events and executed them under
hpd_event_thread
context. For external HPD case, HPD events is delivered by way of
dp_bridge_hpd_notify() under thread context. Since they are
executed
under thread context already, there is no reason to hand over
those
events to hpd_event_thread. Hence dp_hpd_plug_handle() and
dp_hpd_unplug_hanlde() are called directly at
dp_bridge_hpd_notify().
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Is this a bug fix or an optimization? The commit text doesn't
tell me.
I would say both.
optimization as it avoids the need to go through the hpd_event
thread
processing.
bug fix because once you go through the hpd event thread
processing it
exposes and often breaks the already fragile hpd handling state
machine
which can be avoided in this case.
Please add a description for the particular issue that was observed
and how it is fixed by the patch.
Otherwise consider there to be an implicit NAK for all HPD-related
patches unless it is a series that moves link training to the enable
path and drops the HPD state machine completely.
I really mean it. We should stop beating a dead horse unless there is
a grave bug that must be fixed.
I think the commit message is explaining the issue well enough.
This was not fixing any issue we saw to explain you the exact scenario
of things which happened but this is just from code walkthrough.
Like kuogee wrote, hpd event thread was there so handle events coming
out of the hpd_isr for internal hpd cases. For the hpd_notify coming
from pmic_glink or any other extnernal hpd cases, there is no need to
put this through the hpd event thread because this will only make
things
worse of exposing the race conditions of the state machine.
Moving link training to enable and removal of hpd event thread will be
worked on but delaying obvious things we can fix does not make sense.
From the commit message this feels like an optimisation rather than a
fix. And granted the fragility of the HPD state machine, I'd prefer to
stay away from optimisations. As far as I understood from the history
of the last revert, we'd better make sure that HPD handling goes only
through the HPD event thread.
I think you are mixing the two. We tried to send the events through
DRM's hpd_notify which ended up in a bad way and btw, thats still not
resolved even though I have seen reports that things are fine with the
revert, we are consistently able to see us ending up in a disconnected
state with all the reverts and fixes in our x1e80100 DP setup.
I plan to investigate that issue properly in the next week and try to
make some sense of it all.
In fact, this patch is removing one more user of the hpd event thread
which is the direction in which we all want to head towards.
As I stated earlier, from my point of view it doesn't make sense to
rework the HPD thread in small steps.
On whether this is an optimization or a bug fix. I think by avoiding hpd
event thread (which should have never been used for hpd_notify updates,
hence a bug) we are avoiding the possibility of more race conditions.
I think that the HPD event thread serializes handling of events, so
avoiding it increases the possibility of a race condition.
So, this has my R-b and it holds. Upto you.
I'd wait for a proper description of the issue that was observed and
how it is solved by this patch.
This was a code walkthrough fix as I wrote a few times. If there no
merit in pushing this, lets ignore it and stop discussing.
Ok, so after we debugged the HPD issue on we have found the issue and
why actually this change will help. I am going to post a V2 with more
details on the commit text. We can discuss after that.
Re: (subset) [PATCH v3 0/4] arm64: dts: fix several display-related schema warnings
On Tue, 02 Apr 2024 05:57:14 +0300, Dmitry Baryshkov wrote: > Fix several warnings produced by the display nodes. > > Please excuse me for the spam for sending v3 soon after v2. > > Applied, thanks! [1/4] dt-bindings: display/msm: sm8150-mdss: add DP node https://gitlab.freedesktop.org/drm/msm/-/commit/be1b7acb9291 Best regards, -- Abhinav Kumar
Re: [PATCH] drm/msm/dp: fix typo in dp_display_handle_port_status_changed()
On Wed, 06 Mar 2024 11:35:15 -0800, Abhinav Kumar wrote: > Fix the typo in the name of dp_display_handle_port_status_changed(). > > Applied, thanks! [1/1] drm/msm/dp: fix typo in dp_display_handle_port_status_changed() (no commit info) https://gitlab.freedesktop.org/drm/msm/-/commit/cd49cca222bc Best regards, -- Abhinav Kumar
Re: [PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible
On Sat, 30 Mar 2024 05:53:22 +0200, Dmitry Baryshkov wrote: > There is little point in using %ps to print a value known to be NULL. On > the other hand it makes sense to print the callback symbol in the > 'invalid IRQ' message. Correct those two error messages to make more > sense. > > Applied, thanks! [1/1] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible https://gitlab.freedesktop.org/drm/msm/-/commit/8844f467d6a5 Best regards, -- Abhinav Kumar
Re: [PATCH v3] drm/msm/dp: assign correct DP controller ID to x1e80100 interface table
On Fri, 29 Mar 2024 12:46:26 -0700, Kuogee Hsieh wrote: > At current x1e80100 interface table, interface #3 is wrongly > connected to DP controller #0 and interface #4 wrongly connected > to DP controller #2. Fix this problem by connect Interface #3 to > DP controller #0 and interface #4 connect to DP controller #1. > Also add interface #6, #7 and #8 connections to DP controller to > complete x1e80100 interface table. > > [...] Applied, thanks! [1/1] drm/msm/dp: assign correct DP controller ID to x1e80100 interface table https://gitlab.freedesktop.org/drm/msm/-/commit/ee15c8bf5d77 Best regards, -- Abhinav Kumar
Re: (subset) [PATCH v3 0/5] drm/msm/dpu: rework debugfs interface of dpu_core_perf
On Thu, 14 Mar 2024 03:10:40 +0200, Dmitry Baryshkov wrote: > Bring back a set of patches extracted from [1] per Abhinav's suggestion. > > Rework debugging overrides for the bandwidth and clock settings. Instead > of specifying the 'mode' and some values, allow one to set the affected > value directly. > > [1] https://patchwork.freedesktop.org/series/119552/#rev2 > > [...] Applied, thanks! [1/5] drm/msm/dpu: don't allow overriding data from catalog https://gitlab.freedesktop.org/drm/msm/-/commit/4f3b77ae5ff5 Best regards, -- Abhinav Kumar
Re: [PATCH] drm/msm: Add newlines to some debug prints
On Mon, 25 Mar 2024 14:08:09 -0700, Stephen Boyd wrote: > These debug prints are missing newlines, leading to multiple messages > being printed on one line and hard to read logs. Add newlines to have > the debug prints on separate lines. The DBG macro used to add a newline, > but I missed that while migrating to drm_dbg wrappers. > > Applied, thanks! [1/1] drm/msm: Add newlines to some debug prints https://gitlab.freedesktop.org/drm/msm/-/commit/c588f7d67044 Best regards, -- Abhinav Kumar
Re: [PATCH 0/2] drm/msm/dp: fix runtime PM leaks on hotplug
On Wed, 13 Mar 2024 17:43:04 +0100, Johan Hovold wrote: > As I've reported elsewhere, I've been hitting runtime PM usage count > leaks when investigated a DisplayPort hotplug regression on the Lenovo > ThinkPad X13s. [1] > > This series addresses two obvious leaks on disconnect and on connect > failures, respectively. > > [...] Applied, thanks! [1/2] drm/msm/dp: fix runtime PM leak on disconnect https://gitlab.freedesktop.org/drm/msm/-/commit/0640f47b7426 [2/2] drm/msm/dp: fix runtime PM leak on connect failure https://gitlab.freedesktop.org/drm/msm/-/commit/e86750b01a15 Best regards, -- Abhinav Kumar
Re: [PATCH 1/3] drm/msm/dsi: remove the drm_bridge_attach fallback
On 4/5/2024 11:19 AM, Dmitry Baryshkov wrote:
On Fri, 5 Apr 2024 at 21:17, Abhinav Kumar wrote:
On 4/5/2024 11:16 AM, Dmitry Baryshkov wrote:
On Fri, 5 Apr 2024 at 20:20, Abhinav Kumar wrote:
On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:
All the bridges that are being used with the MSM DSI hosts have been
converted to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. Drop the fallback
code and require DRM_BRIDGE_ATTACH_NO_CONNECTOR to be supported by the
downstream bridges.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 36
+++
1 file changed, 11 insertions(+), 25 deletions(-)
There are the bridges I checked by looking at the dts:
1) lontium,lt9611
2) lontium,lt9611uxc
3) adi,adv7533
4) ti,sn65dsi86
Are there any more?
If not, this LGTM
Reviewed-by: Abhinav Kumar
From your message it looks more like Tested-by rather than just Reviewed-by
No, I only cross-checked the dts.
So, its only Reviewed-by :)
But I wanted to list down all the bridges
Then I'd also nominate the panel bridge to the list of bridges for
cross-checking. It is created automatically when we request a bridge,
but DT has only a panel.
Yes, that one is fine too
58 static int panel_bridge_attach(struct drm_bridge *bridge,
59 enum drm_bridge_attach_flags flags)
60 {
61 struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
62 struct drm_connector *connector = _bridge->connector;
63 int ret;
64
65 if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
66 return 0;
67
Re: [PATCH 1/3] drm/msm/dsi: remove the drm_bridge_attach fallback
On 4/5/2024 11:16 AM, Dmitry Baryshkov wrote: On Fri, 5 Apr 2024 at 20:20, Abhinav Kumar wrote: On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote: All the bridges that are being used with the MSM DSI hosts have been converted to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. Drop the fallback code and require DRM_BRIDGE_ATTACH_NO_CONNECTOR to be supported by the downstream bridges. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 36 +++ 1 file changed, 11 insertions(+), 25 deletions(-) There are the bridges I checked by looking at the dts: 1) lontium,lt9611 2) lontium,lt9611uxc 3) adi,adv7533 4) ti,sn65dsi86 Are there any more? If not, this LGTM Reviewed-by: Abhinav Kumar From your message it looks more like Tested-by rather than just Reviewed-by No, I only cross-checked the dts. So, its only Reviewed-by :) But I wanted to list down all the bridges
Re: [PATCH 2/3] drm/msm/dsi: move next bridge acquisition to dsi_bind
On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote:
Currently the MSM DSI driver looks for the next bridge during
msm_dsi_modeset_init(). If the bridge is not registered at that point,
this might result in -EPROBE_DEFER, which can be problematic that late
during the device probe process. Move next bridge acquisition to the
dsi_bind state so that probe deferral is returned as early as possible.
But msm_dsi_modeset)init() is also called during msm_drm_bind() only.
What issue are you suggesting will be fixed by moving this from
msm_drm_bind() to dsi_bind()?
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi.c | 16
drivers/gpu/drm/msm/dsi/dsi.h | 2 ++
drivers/gpu/drm/msm/dsi/dsi_manager.c | 8 +---
3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 37c4c07005fe..38f10f7a10d3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -120,6 +120,22 @@ static int dsi_bind(struct device *dev, struct device
*master, void *data)
struct msm_drm_private *priv = dev_get_drvdata(master);
struct msm_dsi *msm_dsi = dev_get_drvdata(dev);
+ /*
+* Next bridge doesn't exist for the secondary DSI host in a bonded
+* pair.
+*/
+ if (!msm_dsi_is_bonded_dsi(msm_dsi) ||
+ msm_dsi_is_master_dsi(msm_dsi)) {
+ struct drm_bridge *ext_bridge;
+
+ ext_bridge = devm_drm_of_get_bridge(_dsi->pdev->dev,
+ msm_dsi->pdev->dev.of_node,
1, 0);
+ if (IS_ERR(ext_bridge))
+ return PTR_ERR(ext_bridge);
+
+ msm_dsi->next_bridge = ext_bridge;
+ }
+
priv->dsi[msm_dsi->id] = msm_dsi;
return 0;
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 2ad9a842c678..0adef65be1de 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -38,6 +38,8 @@ struct msm_dsi {
struct mipi_dsi_host *host;
struct msm_dsi_phy *phy;
+ struct drm_bridge *next_bridge;
+
struct device *phy_dev;
bool phy_enabled;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index a7c7f85b73e4..b7c52b14c790 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -464,18 +464,12 @@ int msm_dsi_manager_ext_bridge_init(u8 id, struct
drm_bridge *int_bridge)
struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
struct drm_device *dev = msm_dsi->dev;
struct drm_encoder *encoder;
- struct drm_bridge *ext_bridge;
struct drm_connector *connector;
int ret;
- ext_bridge = devm_drm_of_get_bridge(_dsi->pdev->dev,
- msm_dsi->pdev->dev.of_node, 1, 0);
- if (IS_ERR(ext_bridge))
- return PTR_ERR(ext_bridge);
-
encoder = int_bridge->encoder;
- ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
+ ret = drm_bridge_attach(encoder, msm_dsi->next_bridge, int_bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (ret)
return ret;
Re: [PATCH 1/3] drm/msm/dsi: remove the drm_bridge_attach fallback
On 3/9/2024 7:09 AM, Dmitry Baryshkov wrote: All the bridges that are being used with the MSM DSI hosts have been converted to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. Drop the fallback code and require DRM_BRIDGE_ATTACH_NO_CONNECTOR to be supported by the downstream bridges. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 36 +++ 1 file changed, 11 insertions(+), 25 deletions(-) There are the bridges I checked by looking at the dts: 1) lontium,lt9611 2) lontium,lt9611uxc 3) adi,adv7533 4) ti,sn65dsi86 Are there any more? If not, this LGTM Reviewed-by: Abhinav Kumar
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
On 4/3/2024 1:04 PM, Stephen Boyd wrote: Quoting Abhinav Kumar (2024-04-03 12:58:50) On 4/3/2024 12:51 PM, Stephen Boyd wrote: Quoting Kuogee Hsieh (2024-03-29 12:50:35) Currently qmp_combo_dp_power_on() always return 0 in regardless of return value of cfg->configure_dp_phy(). This patch propagate return value of cfg->configure_dp_phy() all the way back to caller. Is this found via code inspection or because the phy is failing to power on sometimes? I ask because I'm looking at a DP bug on Trogdor with chromeos' v6.6 based kernel and wondering if this is related. No, we actually hit an issue. This issue was originally reported as a link training issue while bringing up DP on x1e80100. While debugging that we noticed that we should not have even proceeded to link training because the PLL was not getting locked and it was failing silently since there are no other error prints (and hence the second part of the patch to improve the error logs), and we do not return any error code from this driver, we could not catch the PLL unlocked issue. Did link training succeed in that case and the screen was black? Also, did you figure out why the PLL failed to lock? I sometimes see reports now with an "Unexpected interrupt:" message from the DP driver and the interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). No the link training had failed. Yes, root-cause was that the PLL registers were misconfigured in the x1e80100 DP PHY for HBR2. Once we programmed the correct values it worked. This was specific to x1e80100. Yes, Doug mentioned this to me on IRC that this issue is still there. Surprising because I thought we had pushed https://patchwork.freedesktop.org/patch/551847/ long ago and it was fixed. It certainly did that time when I had tested this. Also, is the call to phy_power_on() going to be checked in the DP driver? $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); Yes, this is a good point. We should also post the patch to add the error checking in DP driver to fail if phy_power_on fails. Sounds great, thanks.
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
On 4/3/2024 12:51 PM, Stephen Boyd wrote: Quoting Kuogee Hsieh (2024-03-29 12:50:35) Currently qmp_combo_dp_power_on() always return 0 in regardless of return value of cfg->configure_dp_phy(). This patch propagate return value of cfg->configure_dp_phy() all the way back to caller. Is this found via code inspection or because the phy is failing to power on sometimes? I ask because I'm looking at a DP bug on Trogdor with chromeos' v6.6 based kernel and wondering if this is related. No, we actually hit an issue. This issue was originally reported as a link training issue while bringing up DP on x1e80100. While debugging that we noticed that we should not have even proceeded to link training because the PLL was not getting locked and it was failing silently since there are no other error prints (and hence the second part of the patch to improve the error logs), and we do not return any error code from this driver, we could not catch the PLL unlocked issue. Also, is the call to phy_power_on() going to be checked in the DP driver? $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); Yes, this is a good point. We should also post the patch to add the error checking in DP driver to fail if phy_power_on fails.
[PATCH] drm: ci: fix the xfails for apq8016
After IGT migrating to dynamic sub-tests, the pipe prefixes in the expected fails list are incorrect. Lets drop those to accurately match the expected fails. In addition, update the xfails list to match the current passing list. This should have ideally failed in the CI run because some tests were marked as fail even though they passed but due to the mismatch in test names, the matching didn't correctly work and was resulting in those failures not being seen. Here is the passing pipeline for apq8016 with this change: https://gitlab.freedesktop.org/drm/msm/-/jobs/57050562 Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt index 44a5c62dedad..b14d4e884971 100644 --- a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt +++ b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt @@ -1,17 +1,6 @@ kms_3d,Fail kms_addfb_basic@addfb25-bad-modifier,Fail -kms_cursor_legacy@all-pipes-forked-bo,Fail -kms_cursor_legacy@all-pipes-forked-move,Fail -kms_cursor_legacy@all-pipes-single-bo,Fail -kms_cursor_legacy@all-pipes-single-move,Fail -kms_cursor_legacy@all-pipes-torture-bo,Fail -kms_cursor_legacy@all-pipes-torture-move,Fail -kms_cursor_legacy@pipe-A-forked-bo,Fail -kms_cursor_legacy@pipe-A-forked-move,Fail -kms_cursor_legacy@pipe-A-single-bo,Fail -kms_cursor_legacy@pipe-A-single-move,Fail -kms_cursor_legacy@pipe-A-torture-bo,Fail -kms_cursor_legacy@pipe-A-torture-move,Fail +kms_cursor_legacy@torture-bo,Fail kms_force_connector_basic@force-edid,Fail kms_hdmi_inject@inject-4k,Fail kms_selftest@drm_format,Timeout -- 2.43.2
Re: [PATCH] drm/msm/dpu: fix vblank IRQ handling for command panels
On 3/30/2024 9:49 AM, Marijn Suijten wrote: On 2024-03-30 05:52:29, Dmitry Baryshkov wrote: In case of CMD DSI panels, the vblank IRQ can be used outside of irq_enable/irq_disable pair. This results in the following kind of Can you clarify when exactly that is? Is it via ops.control_vblank_irq in dpu_encoder_toggle_vblank_for_crtc()? Yes, please explain the sequence of events a litte bit more. messages. Move assignment of IRQ indices to atomic_enable / atomic_disable callbacks. [dpu error]invalid IRQ=[134217727, 31] [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable true/0 [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable false/0 You are right that such messages are common, both at random but also seemingly around toggling the `ACTIVE` property on the CRTC: [ 45.878300] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_disable [ 45.909941] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_unprepare [ 46.093234] [drm:dpu_encoder_helper_wait_for_irq] *ERROR* encoder is disabled id=31, callback=dpu_encoder_phys_cmd_ctl_start_irq, IRQ=[134217727, 31] [ 46.130421] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_prepare [ 46.340457] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_enable [ 65.520323] [dpu error]invalid IRQ=[134217727, 31] irq_cb:dpu_encoder_phys_cmd_te_rd_ptr_irq [ 65.520463] [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable true/0 [ 65.630199] [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable false/0 [ 166.576465] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_disable [ 166.609674] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_unprepare [ 166.781967] [drm:dpu_encoder_helper_wait_for_irq] *ERROR* encoder is disabled id=31, callback=dpu_encoder_phys_cmd_ctl_start_irq, IRQ=[134217727, 31] [ 166.829805] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_prepare [ 167.040476] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_enable [ 337.449827] [dpu error]invalid IRQ=[134217727, 31] irq_cb:dpu_encoder_phys_cmd_te_rd_ptr_irq [ 337.450434] [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable true/0 [ 337.569526] [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable false/0 [ 354.980357] [dpu error]invalid IRQ=[134217727, 31] irq_cb:dpu_encoder_phys_cmd_te_rd_ptr_irq [ 354.980495] [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable true/0 [ 355.090460] [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable false/0 Unfortunately with this patch, turning the CRTC off via ./modetest -M msm -a -w 81:ACTIVE:0 immediately triggers a bunch of WARNs (note that the CRTC turns on immediately again when the command returns, that's probably the framebuffer console taking over again). Running it a few times in succession this may or may not happen, or reboot the phone (Xperia Griffin) entirely: [ 23.423930] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_disable [ 23.461013] [dpu error]invalid IRQ=[134217727, 31] [ 23.461144] [dpu error]invalid IRQ=[134217727, 31] [ 23.461208] [drm:dpu_encoder_phys_cmd_control_vblank_irq] *ERROR* vblank irq err id:31 pp:0 ret:-22, enable false/1 [ 23.461340] [dpu error]invalid IRQ=[134217727, 31] [ 23.461406] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_unprepare [ 23.641721] [drm:dpu_encoder_helper_wait_for_irq] *ERROR* encoder is disabled id=31, callback=dpu_encoder_phys_cmd_ctl_start_irq, IRQ=[134217727, 31] [ 23.679938] panel-samsung-souxp ae94000.dsi.0: samsung_souxp00_prepare [ 23.900465] [ cut here ] [ 23.900813] WARNING: CPU: 1 PID: 747 at drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c:545 dpu_core_irq_register_callback+0x1b4/0x244 [ 23.901450] Modules linked in: [ 23.901814] CPU: 1 PID: 747 Comm: modetest Tainted: G U 6.9.0-rc1-next-20240328-SoMainline-02555-g27abbea53b6b #19 [ 23.902402] Hardware name: Sony Xperia 1 (DT) [ 23.902674] pstate: 804000c5 (Nzcv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 23.903133] pc : dpu_core_irq_register_callback+0x1b4/0x244 [ 23.903455] lr : dpu_encoder_phys_cmd_irq_enable+0x30/0x8c [ 23.903880] sp : 800086833930 [ 23.904123] x29: 800086833930 x28: 0001 x27: 0273834522d0 [ 23.904604] x26: d46ebdb5edc8 x25: d46ebe0f1228 x24: 02738106b280 [ 23.904973] x23:
Re: [PATCH] drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible
On 3/29/2024 8:53 PM, Dmitry Baryshkov wrote:
There is little point in using %ps to print a value known to be NULL. On
the other hand it makes sense to print the callback symbol in the
'invalid IRQ' message. Correct those two error messages to make more
sense.
Fixes: 6893199183f8 ("drm/msm/dpu: stop using raw IRQ indices in the kernel
output")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Abhinav Kumar
Re: [PATCH] drm/msm/dp: allow voltage swing / pre emphasis of 3
Hi Doug
On 3/29/2024 4:07 PM, Doug Anderson wrote:
Hi,
On Sat, Feb 3, 2024 at 5:47 AM Dmitry Baryshkov
wrote:
Both dp_link_adjust_levels() and dp_ctrl_update_vx_px() limit swing and
pre-emphasis to 2, while the real maximum value for the sum of the
voltage swing and pre-emphasis is 3. Fix the DP code to remove this
limitation.
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 6 +++---
drivers/gpu/drm/msm/dp/dp_link.c | 22 +++---
drivers/gpu/drm/msm/dp/dp_link.h | 14 +-
3 files changed, 15 insertions(+), 27 deletions(-)
What ever happened with this patch? It seemed important so I've been
trying to check back on it, but it seems to still be in limbo. I was
assuming that (maybe?) Abhinav would check things against the hardware
documentation and give it a Reviewed-by and then it would land...
-Doug
The issue for which this patch was originally made (DP link training
issue on x1e80100) was not getting fixed by this patch.
That one turned out as actually a PLL locking issue. So this kind of
went off the radar as it was not immediately needed to fix anything.
I will wait for Kuogee's response on this patch. He was OOO entire Feb
so this got missed.
Re: [PATCH] drm/msm: fix the `CRASHDUMP_READ` target of `a6xx_get_shader_block()`
On 3/26/2024 2:23 PM, Miguel Ojeda wrote:
Clang 14 in an (essentially) defconfig arm64 build for next-20240326
reports [1]:
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error:
variable 'out' set but not used [-Werror,-Wunused-but-set-variable]
The variable `out` in these functions is meant to compute the `target` of
`CRASHDUMP_READ()`, but in this case only the initial value (`dumper->iova
+ A6XX_CD_DATA_OFFSET`) was being passed.
Thus use `out` as it was intended by Connor [2].
There was an alternative patch at [3] that removed the variable
altogether, but that would only use the initial value.
Fixes: 64d6255650d4 ("drm/msm: More fully implement devcoredump for a7xx")
Closes:
https://lore.kernel.org/lkml/caniq72mjc5t4n25sqvysroehxxpxypz4ppznesjhenc3qap...@mail.gmail.com/
[1]
Link:
https://lore.kernel.org/lkml/cacu1e7hhckmjd6fixzspinaz6ekoznkmthtclfvmbz-9vol...@mail.gmail.com/
[2]
Link:
https://lore.kernel.org/lkml/20240307093727.1978126-1-colin.i.k...@gmail.com/
[3]
Signed-off-by: Miguel Ojeda
---
LGTM,
Reviewed-by: Abhinav Kumar
Re: [PATCH v2] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
On 3/29/2024 9:41 AM, Kuogee Hsieh wrote:
Currently qmp_combo_dp_power_on() always return 0 in regardless of
return value of cfg->configure_dp_phy(). This patch propagate
return value of cfg->configure_dp_phy() all the way back to caller.
Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy")
Signed-off-by: Kuogee Hsieh
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
Thanks for the cleanup!
Reviewed-by: Abhinav Kumar
Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly
On 3/28/2024 8:23 PM, Dmitry Baryshkov wrote:
On Fri, 29 Mar 2024 at 04:16, Abhinav Kumar wrote:
On 3/28/2024 5:10 PM, Dmitry Baryshkov wrote:
On Fri, 29 Mar 2024 at 01:42, Abhinav Kumar wrote:
On 3/28/2024 3:50 PM, Dmitry Baryshkov wrote:
On Thu, 28 Mar 2024 at 23:21, Abhinav Kumar wrote:
On 3/28/2024 1:58 PM, Stephen Boyd wrote:
Quoting Abhinav Kumar (2024-03-28 13:24:34)
+ Johan and Bjorn for FYI
On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
For internal HPD case, hpd_event_thread is created to handle HPD
interrupts generated by HPD block of DP controller. It converts
HPD interrupts into events and executed them under hpd_event_thread
context. For external HPD case, HPD events is delivered by way of
dp_bridge_hpd_notify() under thread context. Since they are executed
under thread context already, there is no reason to hand over those
events to hpd_event_thread. Hence dp_hpd_plug_handle() and
dp_hpd_unplug_hanlde() are called directly at dp_bridge_hpd_notify().
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Is this a bug fix or an optimization? The commit text doesn't tell me.
I would say both.
optimization as it avoids the need to go through the hpd_event thread
processing.
bug fix because once you go through the hpd event thread processing it
exposes and often breaks the already fragile hpd handling state machine
which can be avoided in this case.
Please add a description for the particular issue that was observed
and how it is fixed by the patch.
Otherwise consider there to be an implicit NAK for all HPD-related
patches unless it is a series that moves link training to the enable
path and drops the HPD state machine completely.
I really mean it. We should stop beating a dead horse unless there is
a grave bug that must be fixed.
I think the commit message is explaining the issue well enough.
This was not fixing any issue we saw to explain you the exact scenario
of things which happened but this is just from code walkthrough.
Like kuogee wrote, hpd event thread was there so handle events coming
out of the hpd_isr for internal hpd cases. For the hpd_notify coming
from pmic_glink or any other extnernal hpd cases, there is no need to
put this through the hpd event thread because this will only make things
worse of exposing the race conditions of the state machine.
Moving link training to enable and removal of hpd event thread will be
worked on but delaying obvious things we can fix does not make sense.
From the commit message this feels like an optimisation rather than a
fix. And granted the fragility of the HPD state machine, I'd prefer to
stay away from optimisations. As far as I understood from the history
of the last revert, we'd better make sure that HPD handling goes only
through the HPD event thread.
I think you are mixing the two. We tried to send the events through
DRM's hpd_notify which ended up in a bad way and btw, thats still not
resolved even though I have seen reports that things are fine with the
revert, we are consistently able to see us ending up in a disconnected
state with all the reverts and fixes in our x1e80100 DP setup.
I plan to investigate that issue properly in the next week and try to
make some sense of it all.
In fact, this patch is removing one more user of the hpd event thread
which is the direction in which we all want to head towards.
As I stated earlier, from my point of view it doesn't make sense to
rework the HPD thread in small steps.
On whether this is an optimization or a bug fix. I think by avoiding hpd
event thread (which should have never been used for hpd_notify updates,
hence a bug) we are avoiding the possibility of more race conditions.
I think that the HPD event thread serializes handling of events, so
avoiding it increases the possibility of a race condition.
So, this has my R-b and it holds. Upto you.
I'd wait for a proper description of the issue that was observed and
how it is solved by this patch.
This was a code walkthrough fix as I wrote a few times. If there no
merit in pushing this, lets ignore it and stop discussing.
Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly
On 3/28/2024 6:46 PM, Bjorn Andersson wrote:
On Thu, Mar 28, 2024 at 02:21:14PM -0700, Abhinav Kumar wrote:
On 3/28/2024 1:58 PM, Stephen Boyd wrote:
Quoting Abhinav Kumar (2024-03-28 13:24:34)
+ Johan and Bjorn for FYI
On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
For internal HPD case, hpd_event_thread is created to handle HPD
interrupts generated by HPD block of DP controller. It converts
HPD interrupts into events and executed them under hpd_event_thread
context. For external HPD case, HPD events is delivered by way of
dp_bridge_hpd_notify() under thread context. Since they are executed
under thread context already, there is no reason to hand over those
events to hpd_event_thread. Hence dp_hpd_plug_handle() and
dp_hpd_unplug_hanlde() are called directly at dp_bridge_hpd_notify().
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Is this a bug fix or an optimization? The commit text doesn't tell me.
I would say both.
optimization as it avoids the need to go through the hpd_event thread
processing.
bug fix because once you go through the hpd event thread processing it
exposes and often breaks the already fragile hpd handling state machine
which can be avoided in this case.
It removes the main users of the thread, but there's still code paths
which will post events on the thread.
I think I like the direction this is taking, but does it really fix the
whole problem, or just patch one case?
So kuogee's idea behind this that NON-hpd_isr events need not go through
event thread at all.
We did not run into any special scenario or issue without this. It was a
code walkthrough fix.
PS. Please read go/upstream and switch to b4, to avoid some practical
issues with the way you posted this patch.
Thanks,
Bjorn
Just to elaborate the practical issues so that developers know what you
encountered:
-> no need of v1 on the PATCH
-> somehow this patch was linked "in-reply-to" another patch
https://lore.kernel.org/all/1711656246-3483-2-git-send-email-quic_khs...@quicinc.com/
. This is quite strange and not sure how it happened. But will double
check if we did something wrong here.
Thanks for sharing these.
Looks right to me,
Reviewed-by: Abhinav Kumar
Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly
On 3/28/2024 5:10 PM, Dmitry Baryshkov wrote:
On Fri, 29 Mar 2024 at 01:42, Abhinav Kumar wrote:
On 3/28/2024 3:50 PM, Dmitry Baryshkov wrote:
On Thu, 28 Mar 2024 at 23:21, Abhinav Kumar wrote:
On 3/28/2024 1:58 PM, Stephen Boyd wrote:
Quoting Abhinav Kumar (2024-03-28 13:24:34)
+ Johan and Bjorn for FYI
On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
For internal HPD case, hpd_event_thread is created to handle HPD
interrupts generated by HPD block of DP controller. It converts
HPD interrupts into events and executed them under hpd_event_thread
context. For external HPD case, HPD events is delivered by way of
dp_bridge_hpd_notify() under thread context. Since they are executed
under thread context already, there is no reason to hand over those
events to hpd_event_thread. Hence dp_hpd_plug_handle() and
dp_hpd_unplug_hanlde() are called directly at dp_bridge_hpd_notify().
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Is this a bug fix or an optimization? The commit text doesn't tell me.
I would say both.
optimization as it avoids the need to go through the hpd_event thread
processing.
bug fix because once you go through the hpd event thread processing it
exposes and often breaks the already fragile hpd handling state machine
which can be avoided in this case.
Please add a description for the particular issue that was observed
and how it is fixed by the patch.
Otherwise consider there to be an implicit NAK for all HPD-related
patches unless it is a series that moves link training to the enable
path and drops the HPD state machine completely.
I really mean it. We should stop beating a dead horse unless there is
a grave bug that must be fixed.
I think the commit message is explaining the issue well enough.
This was not fixing any issue we saw to explain you the exact scenario
of things which happened but this is just from code walkthrough.
Like kuogee wrote, hpd event thread was there so handle events coming
out of the hpd_isr for internal hpd cases. For the hpd_notify coming
from pmic_glink or any other extnernal hpd cases, there is no need to
put this through the hpd event thread because this will only make things
worse of exposing the race conditions of the state machine.
Moving link training to enable and removal of hpd event thread will be
worked on but delaying obvious things we can fix does not make sense.
From the commit message this feels like an optimisation rather than a
fix. And granted the fragility of the HPD state machine, I'd prefer to
stay away from optimisations. As far as I understood from the history
of the last revert, we'd better make sure that HPD handling goes only
through the HPD event thread.
I think you are mixing the two. We tried to send the events through
DRM's hpd_notify which ended up in a bad way and btw, thats still not
resolved even though I have seen reports that things are fine with the
revert, we are consistently able to see us ending up in a disconnected
state with all the reverts and fixes in our x1e80100 DP setup.
I plan to investigate that issue properly in the next week and try to
make some sense of it all.
In fact, this patch is removing one more user of the hpd event thread
which is the direction in which we all want to head towards.
On whether this is an optimization or a bug fix. I think by avoiding hpd
event thread (which should have never been used for hpd_notify updates,
hence a bug) we are avoiding the possibility of more race conditions.
So, this has my R-b and it holds. Upto you.
Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly
On 3/28/2024 3:50 PM, Dmitry Baryshkov wrote:
On Thu, 28 Mar 2024 at 23:21, Abhinav Kumar wrote:
On 3/28/2024 1:58 PM, Stephen Boyd wrote:
Quoting Abhinav Kumar (2024-03-28 13:24:34)
+ Johan and Bjorn for FYI
On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
For internal HPD case, hpd_event_thread is created to handle HPD
interrupts generated by HPD block of DP controller. It converts
HPD interrupts into events and executed them under hpd_event_thread
context. For external HPD case, HPD events is delivered by way of
dp_bridge_hpd_notify() under thread context. Since they are executed
under thread context already, there is no reason to hand over those
events to hpd_event_thread. Hence dp_hpd_plug_handle() and
dp_hpd_unplug_hanlde() are called directly at dp_bridge_hpd_notify().
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Is this a bug fix or an optimization? The commit text doesn't tell me.
I would say both.
optimization as it avoids the need to go through the hpd_event thread
processing.
bug fix because once you go through the hpd event thread processing it
exposes and often breaks the already fragile hpd handling state machine
which can be avoided in this case.
Please add a description for the particular issue that was observed
and how it is fixed by the patch.
Otherwise consider there to be an implicit NAK for all HPD-related
patches unless it is a series that moves link training to the enable
path and drops the HPD state machine completely.
I really mean it. We should stop beating a dead horse unless there is
a grave bug that must be fixed.
I think the commit message is explaining the issue well enough.
This was not fixing any issue we saw to explain you the exact scenario
of things which happened but this is just from code walkthrough.
Like kuogee wrote, hpd event thread was there so handle events coming
out of the hpd_isr for internal hpd cases. For the hpd_notify coming
from pmic_glink or any other extnernal hpd cases, there is no need to
put this through the hpd event thread because this will only make things
worse of exposing the race conditions of the state machine.
Moving link training to enable and removal of hpd event thread will be
worked on but delaying obvious things we can fix does not make sense.
Looks right to me,
Reviewed-by: Abhinav Kumar
Re: [PATCH v1] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
On 3/28/2024 2:07 PM, Kuogee Hsieh wrote:
Currently qmp_combo_dp_power_on() always return 0 in regardless of
return value of cfg->configure_dp_phy(). This patch propagate
return value of cfg->configure_dp_phy() all the way back to caller.
This is good. But I am also thinking if we should add some prints in
this driver like it doesnt even tell where it failed like here
ret = qmp_v456_configure_dp_phy(qmp);
if (ret < 0)
return ret;
Signed-off-by: Kuogee Hsieh
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Also, I think we should have
Fixes: 94a407cc17a4 ("phy: qcom-qmp: create copies of QMP PHY driver")
If there is a better fixes tag for this, please let me know.
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 36632fa..884973a 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -2754,6 +2754,7 @@ static int qmp_combo_dp_power_on(struct phy *phy)
const struct qmp_phy_cfg *cfg = qmp->cfg;
void __iomem *tx = qmp->dp_tx;
void __iomem *tx2 = qmp->dp_tx2;
+ int ret = 0;
mutex_lock(>phy_mutex);
@@ -2766,11 +2767,11 @@ static int qmp_combo_dp_power_on(struct phy *phy)
cfg->configure_dp_tx(qmp);
/* Configure link rate, swing, etc. */
- cfg->configure_dp_phy(qmp);
+ ret = cfg->configure_dp_phy(qmp);
mutex_unlock(>phy_mutex);
- return 0;
+ return ret;
}
static int qmp_combo_dp_power_off(struct phy *phy)
Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly
On 3/28/2024 1:58 PM, Stephen Boyd wrote:
Quoting Abhinav Kumar (2024-03-28 13:24:34)
+ Johan and Bjorn for FYI
On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
For internal HPD case, hpd_event_thread is created to handle HPD
interrupts generated by HPD block of DP controller. It converts
HPD interrupts into events and executed them under hpd_event_thread
context. For external HPD case, HPD events is delivered by way of
dp_bridge_hpd_notify() under thread context. Since they are executed
under thread context already, there is no reason to hand over those
events to hpd_event_thread. Hence dp_hpd_plug_handle() and
dp_hpd_unplug_hanlde() are called directly at dp_bridge_hpd_notify().
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Is this a bug fix or an optimization? The commit text doesn't tell me.
I would say both.
optimization as it avoids the need to go through the hpd_event thread
processing.
bug fix because once you go through the hpd event thread processing it
exposes and often breaks the already fragile hpd handling state machine
which can be avoided in this case.
Looks right to me,
Reviewed-by: Abhinav Kumar
Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly
+ Johan and Bjorn for FYI
On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
For internal HPD case, hpd_event_thread is created to handle HPD
interrupts generated by HPD block of DP controller. It converts
HPD interrupts into events and executed them under hpd_event_thread
context. For external HPD case, HPD events is delivered by way of
dp_bridge_hpd_notify() under thread context. Since they are executed
under thread context already, there is no reason to hand over those
events to hpd_event_thread. Hence dp_hpd_plug_handle() and
dp_hpd_unplug_hanlde() are called directly at dp_bridge_hpd_notify().
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Looks right to me,
Reviewed-by: Abhinav Kumar
Re: [PATCH v1] drm/msm/dp: assign correct DP controller ID to interface table
On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
At current x1e80100 interface table, interface #3 is wrongly
connected to DP controller #0 and interface #4 wrongly connected
to DP controller #2. Fix this problem by connect Interface #3 to
DP controller #0 and interface #4 connect to DP controller #1.
Also add interface #6, #7 and #8 connections to DP controller to
complete x1e80100 interface table.
Signed-off-by: Kuogee Hsieh
---
.../drm/msm/disp/dpu1/catalog/dpu_9_2_x1e80100.h | 34 --
1 file changed, 31 insertions(+), 3 deletions(-)
Fixes: e3b1f369db5a ("drm/msm/dpu: Add X1E80100 support")
I cross-checked all the entries and this looks right.
Thanks for fixing this.
Reviewed-by: Abhinav Kumar
Re: [PATCH v4 10/16] drm/msm: generate headers on the fly
On 3/22/2024 3:57 PM, Dmitry Baryshkov wrote: Generate DRM/MSM headers on the fly during kernel build. This removes a need to push register changes to Mesa with the following manual synchronization step. Existing headers will be removed in the following commits (split away to ease reviews). This change does two things: 1) move adreno folder compilation under "adreno-y", move display related files compilation undere "msm-display-y", move common files under "msm-y" 2) changes to generate the header using gen_header.py Why not split it into two changes? Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/.gitignore | 1 + drivers/gpu/drm/msm/Makefile | 97 +- drivers/gpu/drm/msm/msm_drv.c | 3 +- drivers/gpu/drm/msm/msm_gpu.c | 2 +- 4 files changed, 80 insertions(+), 23 deletions(-) Are below two changes related to this patch? +targets += $(ADRENO_HEADERS) $(DISPLAY_HEADERS) diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 97790faffd23..9c33f4e3f822 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -17,8 +17,9 @@ #include "msm_drv.h" #include "msm_debugfs.h" +#include "msm_gem.h" +#include "msm_gpu.h" #include "msm_kms.h" -#include "adreno/adreno_gpu.h" /* * MSM driver version: diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 655002b21b0d..cd185b9636d2 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -11,7 +11,7 @@ #include "msm_mmu.h" #include "msm_fence.h" #include "msm_gpu_trace.h" -#include "adreno/adreno_gpu.h" +//#include "adreno/adreno_gpu.h" you can just drop this line #include #include
Re: [PATCH v4 09/16] drm/msm: import gen_header.py script from Mesa
On 3/26/2024 3:25 PM, Dmitry Baryshkov wrote: On Wed, 27 Mar 2024 at 00:19, Abhinav Kumar wrote: On 3/22/2024 3:57 PM, Dmitry Baryshkov wrote: Import the gen_headers.py script from Mesa, commit FIXME. This script will be used to generate MSM register files on the fly during compilation. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/registers/gen_header.py | 957 1 file changed, 957 insertions(+) diff --git a/drivers/gpu/drm/msm/registers/gen_header.py b/drivers/gpu/drm/msm/registers/gen_header.py new file mode 100644 index ..ae39b7e6cde8 --- /dev/null +++ b/drivers/gpu/drm/msm/registers/gen_header.py @@ -0,0 +1,957 @@ +#!/usr/bin/python3 + We need a licence and copyright here. Yes, this is going to be fixed in the next revision. Mesa already got the proper SPDX header here. Also is something like a "based on" applicable here? +import xml.parsers.expat +import sys +import os +import collections +import argparse +import time +import datetime + +class Error(Exception): +This file was generated by the rules-ng-ng gen_header.py tool in this git repository: +http://gitlab.freedesktop.org/mesa/mesa/ +git clone https://gitlab.freedesktop.org/mesa/mesa.git + +The rules-ng-ng source files this header was generated from are: Is this still applicable ? Now gen_header.py is moved to kernel. Copied, not moved. So Mesa remains the primary source for Adreno headers and gen_header.py But all future development and code review on gen_header.py will be done in kernel itself OR periodically we will sync it up with mesa?
Re: [PATCH v4 01/16] drm/msm/mdp5: add writeback block bases
On 3/26/2024 2:52 PM, Dmitry Baryshkov wrote: On Tue, 26 Mar 2024 at 23:39, Abhinav Kumar wrote: On 3/22/2024 3:56 PM, Dmitry Baryshkov wrote: In order to stop patching the mdp5 headers, import definitions for the writeback blocks. This part is extracted from the old Rob's patch. Co-developed-by: Rob Clark Signed-off-by: Rob Clark Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h | 11 +++ 1 file changed, 11 insertions(+) This is unused today right? Is it just being migrated now in advance as all the mesa mdp5 headers are moving to kernel? Exactly. I had three options: pick up this patch, implement applying 'fixup' patches or drop corresponding doffests from the mdp5.xml. I've chosen the first option. Yes, this is fine Reviewed-by: Abhinav Kumar -- With best wishes Dmitry
Re: [PATCH v4 09/16] drm/msm: import gen_header.py script from Mesa
On 3/22/2024 3:57 PM, Dmitry Baryshkov wrote: Import the gen_headers.py script from Mesa, commit FIXME. This script will be used to generate MSM register files on the fly during compilation. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/registers/gen_header.py | 957 1 file changed, 957 insertions(+) diff --git a/drivers/gpu/drm/msm/registers/gen_header.py b/drivers/gpu/drm/msm/registers/gen_header.py new file mode 100644 index ..ae39b7e6cde8 --- /dev/null +++ b/drivers/gpu/drm/msm/registers/gen_header.py @@ -0,0 +1,957 @@ +#!/usr/bin/python3 + We need a licence and copyright here. Also is something like a "based on" applicable here? +import xml.parsers.expat +import sys +import os +import collections +import argparse +import time +import datetime + +class Error(Exception): +This file was generated by the rules-ng-ng gen_header.py tool in this git repository: +http://gitlab.freedesktop.org/mesa/mesa/ +git clone https://gitlab.freedesktop.org/mesa/mesa.git + +The rules-ng-ng source files this header was generated from are: Is this still applicable ? Now gen_header.py is moved to kernel.
Re: [PATCH v4 03/16] drm/msm/dsi: drop mmss_cc.xml.h
On 3/22/2024 3:56 PM, Dmitry Baryshkov wrote: The mmss_cc.xml.h file describes bits of the MMSS clock controller on APQ8064 / MSM8960 platforms. They are not used by the driver and do not belong to the DRM MSM driver. Drop the file. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/mmss_cc.xml.h | 131 -- 1 file changed, 131 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH v4 02/16] drm/msm/hdmi: drop qfprom.xml.h
On 3/22/2024 3:56 PM, Dmitry Baryshkov wrote: The qfprom.xml.h contains definitions for the nvmem code. They are not used in the existing code. Also if we were to use them later, we should have used nvmem cell API instead of using these defs. Drop the file. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/hdmi/qfprom.xml.h | 61 --- 1 file changed, 61 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH v4 01/16] drm/msm/mdp5: add writeback block bases
On 3/22/2024 3:56 PM, Dmitry Baryshkov wrote:
In order to stop patching the mdp5 headers, import definitions for the
writeback blocks. This part is extracted from the old Rob's patch.
Co-developed-by: Rob Clark
Signed-off-by: Rob Clark
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h | 11 +++
1 file changed, 11 insertions(+)
This is unused today right?
Is it just being migrated now in advance as all the mesa mdp5 headers
are moving to kernel?
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
index 26c5d8b4ab46..4b988e69fbfc 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
@@ -69,6 +69,16 @@ struct mdp5_mdp_block {
uint32_t caps; /* MDP capabilities: MDP_CAP_xxx bits */
};
+struct mdp5_wb_instance {
+ int id;
+ int lm;
+};
+
+struct mdp5_wb_block {
+ MDP5_SUB_BLOCK_DEFINITION;
+ struct mdp5_wb_instance instances[MAX_BASES];
+};
+
#define MDP5_INTF_NUM_MAX 5
struct mdp5_intf_block {
@@ -98,6 +108,7 @@ struct mdp5_cfg_hw {
struct mdp5_sub_block pp;
struct mdp5_sub_block dsc;
struct mdp5_sub_block cdm;
+ struct mdp5_wb_block wb;
struct mdp5_intf_block intf;
struct mdp5_perf_block perf;
Re: drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable 'out' set but not used
On 3/26/2024 12:47 PM, Dmitry Baryshkov wrote:
On Tue, 26 Mar 2024 at 21:32, Abhinav Kumar wrote:
On 3/26/2024 12:10 PM, Dmitry Baryshkov wrote:
On Tue, 26 Mar 2024 at 20:31, Abhinav Kumar wrote:
On 3/26/2024 11:19 AM, Dmitry Baryshkov wrote:
On Tue, 26 Mar 2024 at 20:05, Miguel Ojeda
wrote:
Hi,
In today's next, I got:
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable
'out' set but not used [-Werror,-Wunused-but-set-variable]
`out` seems to be there since commit 64d6255650d4 ("drm/msm: More
fully implement devcoredump for a7xx").
Untested diff below assuming `dumper->iova` is constant -- if you want
a formal patch, please let me know.
Please send a proper patch that we can pick up.
This should be fixed with https://patchwork.freedesktop.org/patch/581853/.
Is that a correct fix? If you check other usage locations for
CRASHDUMP_READ, you'll see that `out` is the last parameter and it is
being incremented.
Right but in this function out is not the last parameter of CRASHDUMP_READ.
Yes. I think in this case the patch from this email is more correct.
Alright, in that case, Miguel can you please repost this with the Fixes
tags and in a patch form.
Maybe you or Rob can correct me but I thought the fix looked sane
although noone commented on that patch.
We can pickup that one with a Fixes tag applied.
Cheers,
Miguel
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index 1f5245fc2cdc..a847a0f7a73c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -852,7 +852,7 @@ static void a6xx_get_shader_block(struct msm_gpu *gpu,
(block->type << 8) | i);
in += CRASHDUMP_READ(in, REG_A6XX_HLSQ_DBG_AHB_READ_APERTURE,
-block->size, dumper->iova + A6XX_CD_DATA_OFFSET);
+block->size, out);
out += block->size * sizeof(u32);
}
Re: drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable 'out' set but not used
On 3/26/2024 12:10 PM, Dmitry Baryshkov wrote:
On Tue, 26 Mar 2024 at 20:31, Abhinav Kumar wrote:
On 3/26/2024 11:19 AM, Dmitry Baryshkov wrote:
On Tue, 26 Mar 2024 at 20:05, Miguel Ojeda
wrote:
Hi,
In today's next, I got:
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable
'out' set but not used [-Werror,-Wunused-but-set-variable]
`out` seems to be there since commit 64d6255650d4 ("drm/msm: More
fully implement devcoredump for a7xx").
Untested diff below assuming `dumper->iova` is constant -- if you want
a formal patch, please let me know.
Please send a proper patch that we can pick up.
This should be fixed with https://patchwork.freedesktop.org/patch/581853/.
Is that a correct fix? If you check other usage locations for
CRASHDUMP_READ, you'll see that `out` is the last parameter and it is
being incremented.
Right but in this function out is not the last parameter of CRASHDUMP_READ.
Maybe you or Rob can correct me but I thought the fix looked sane
although noone commented on that patch.
We can pickup that one with a Fixes tag applied.
Cheers,
Miguel
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index 1f5245fc2cdc..a847a0f7a73c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -852,7 +852,7 @@ static void a6xx_get_shader_block(struct msm_gpu *gpu,
(block->type << 8) | i);
in += CRASHDUMP_READ(in, REG_A6XX_HLSQ_DBG_AHB_READ_APERTURE,
-block->size, dumper->iova + A6XX_CD_DATA_OFFSET);
+block->size, out);
out += block->size * sizeof(u32);
}
Re: drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable 'out' set but not used
On 3/26/2024 11:19 AM, Dmitry Baryshkov wrote:
On Tue, 26 Mar 2024 at 20:05, Miguel Ojeda
wrote:
Hi,
In today's next, I got:
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable
'out' set but not used [-Werror,-Wunused-but-set-variable]
`out` seems to be there since commit 64d6255650d4 ("drm/msm: More
fully implement devcoredump for a7xx").
Untested diff below assuming `dumper->iova` is constant -- if you want
a formal patch, please let me know.
Please send a proper patch that we can pick up.
This should be fixed with https://patchwork.freedesktop.org/patch/581853/.
We can pickup that one with a Fixes tag applied.
Cheers,
Miguel
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index 1f5245fc2cdc..a847a0f7a73c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -852,7 +852,7 @@ static void a6xx_get_shader_block(struct msm_gpu *gpu,
(block->type << 8) | i);
in += CRASHDUMP_READ(in, REG_A6XX_HLSQ_DBG_AHB_READ_APERTURE,
-block->size, dumper->iova + A6XX_CD_DATA_OFFSET);
+block->size, out);
out += block->size * sizeof(u32);
}
Re: [PATCH] drm/msm: Add newlines to some debug prints
On 3/25/2024 2:08 PM, Stephen Boyd wrote:
These debug prints are missing newlines, leading to multiple messages
being printed on one line and hard to read logs. Add newlines to have
the debug prints on separate lines. The DBG macro used to add a newline,
but I missed that while migrating to drm_dbg wrappers.
Fixes: 7cb017db1896 ("drm/msm: Move FB debug prints to drm_dbg_state()")
Fixes: 721c6e0c6aed ("drm/msm: Move vblank debug prints to drm_dbg_vbl()")
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/msm_fb.c | 6 +++---
drivers/gpu/drm/msm/msm_kms.c | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Abhinav Kumar
Re: [PATCH v3 5/5] drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib
On 3/13/2024 6:10 PM, Dmitry Baryshkov wrote:
The max_per_pipe_ib is a constant across all CRTCs and is read from the
catalog. Drop corresponding calculations and read the value directly at
icc_set_bw() time.
Suggested-by: Konrad Dybcio
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 17 +
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 --
3 files changed, 5 insertions(+), 16 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 2e78e57665fc..2fc05665dc7a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -105,13 +105,12 @@ static void _dpu_core_perf_calc_crtc(const struct
dpu_core_perf *core_perf,
}
perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
- perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
DRM_DEBUG_ATOMIC(
- "crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
+ "crtc=%d clk_rate=%llu core_ab=%llu\n",
crtc->base.id, perf->core_clk_rate,
- perf->max_per_pipe_ib, perf->bw_ctl);
+ perf->bw_ctl);
}
int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
@@ -190,9 +189,6 @@ static void dpu_core_perf_aggregate(struct drm_device *ddev,
curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
- perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
-
dpu_cstate->new_perf.max_per_pipe_ib);
during the override case
perf->max_per_pipe_ib = core_perf->fix_core_ib_vote
So this is one case where max_per_pipe_ib can actually be changed right?
Now, fix_core_ib_vote will not be used then?
-
perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
@@ -216,7 +212,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms
*kms,
dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc),
);
avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
- peak_bw = perf.max_per_pipe_ib;
+ peak_bw = kms->catalog->perf->min_dram_ib;
if (kms->perf.fix_core_ab_vote)
avg_bw = kms->perf.fix_core_ab_vote;
@@ -321,15 +317,12 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
* 2. new bandwidth vote - "ab or ib vote" is lower
*than current vote at end of commit or stop.
*/
- if ((params_changed && ((new->bw_ctl > old->bw_ctl) ||
- (new->max_per_pipe_ib > old->max_per_pipe_ib)))
||
- (!params_changed && ((new->bw_ctl < old->bw_ctl) ||
- (new->max_per_pipe_ib < old->max_per_pipe_ib {
+ if ((params_changed && new->bw_ctl > old->bw_ctl) ||
+ (!params_changed && new->bw_ctl < old->bw_ctl)) {
DRM_DEBUG_ATOMIC("crtc=%d p=%d
new_bw=%llu,old_bw=%llu\n",
crtc->base.id, params_changed,
new->bw_ctl, old->bw_ctl);
old->bw_ctl = new->bw_ctl;
- old->max_per_pipe_ib = new->max_per_pipe_ib;
update_bus = true;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 5a3d18ca9555..a5a9c3389718 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -14,12 +14,10 @@
/**
* struct dpu_core_perf_params - definition of performance parameters
- * @max_per_pipe_ib: maximum instantaneous bandwidth request
* @bw_ctl: arbitrated bandwidth request
* @core_clk_rate: core clock rate request
*/
struct dpu_core_perf_params {
- u64 max_per_pipe_ib;
u64 bw_ctl;
u64 core_clk_rate;
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 88c2e51ab166..771c04c1a5ea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1389,8 +1389,6 @@ static int dpu_crtc_debugfs_state_show(struct seq_file
*s, void *v)
seq_printf(s, "core_clk_rate: %llu\n",
dpu_crtc->cur_perf.core_clk_rate);
seq_printf(s, "bw_ctl: %llu\n", dpu_crtc->cur_perf.bw_ctl);
- seq_printf(s, "max_per_pipe_ib: %llu\n",
- dpu_crtc->cur_perf.max_per_pipe_ib);
return 0;
}
Re: [PATCH v3 4/5] drm/msm/dpu: rework core_perf debugfs overrides
On 3/13/2024 6:10 PM, Dmitry Baryshkov wrote: Currently debugfs provides separate 'modes' to override calculated MDP_CLK rate and interconnect bandwidth votes. Change that to allow overriding individual values (e.g. one can override just clock or just average bandwidth vote). I am not opposed to the idea of dropping modes and overriding individual values. But, we cannot expect the user to know the max clock value and max BW value for each chipset by looking into the catalog or devicetree. That was the whole point of the MODE_MINIMUM perf mode. User doesnt need to know "what values to program", just sets the "max" mode. If you are also going to expose a "max_core_clk" and "max_bw" debugfs read nodes along with this along with the --help I spoke about in the prev patch, that will make this approach complete from my PoV. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 87 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 10 --- 2 files changed, 9 insertions(+), 88 deletions(-)
Re: [PATCH v3 3/5] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()
On 3/19/2024 3:25 PM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 23:35, Abhinav Kumar wrote:
On 3/19/2024 1:43 PM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 22:34, Abhinav Kumar wrote:
On 3/13/2024 6:10 PM, Dmitry Baryshkov wrote:
Move perf mode handling for the bandwidth to
_dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
and then aggregating known values.
Note, this changes the fix_core_ab_vote. Previously it would be
multiplied per the CRTC number, now it will be used directly for
interconnect voting.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 39
+--
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 87b892069526..ff2942a6a678 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -118,21 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct
dpu_core_perf *core_perf,
return;
}
- memset(perf, 0, sizeof(struct dpu_core_perf_params));
-
- if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
- perf->bw_ctl = 0;
- perf->max_per_pipe_ib = 0;
- perf->core_clk_rate = 0;
- } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
- perf->bw_ctl = core_perf->fix_core_ab_vote;
- perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
- perf->core_clk_rate = core_perf->fix_core_clk_rate;
- } else {
- perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
- perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
- perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc,
state);
- }
+ perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
+ perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
+ perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
DRM_DEBUG_ATOMIC(
"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
@@ -233,18 +221,29 @@ 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;
+ u32 peak_bw;
Why were avg_bw and peak_bw values brought down to u32?
I think we might go higher so u64 was better.
First of all, icc_set_bw takes u32, not u64. The unit is 1000 bps, not
1 bps, so sensible values fit into u32.
True and agreed.
Would have been better to send this update as a separate patch so that
its clear why you are actually doing this downgrade instead of this
being hidden in this cleanup perhaps even with a Fixes tag then.
if (!kms->num_paths)
return 0;
- dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), );
+ if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
+ avg_bw = 0;
+ peak_bw = 0;
+ } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
+ avg_bw = kms->perf.fix_core_ab_vote;
+ peak_bw = kms->perf.fix_core_ib_vote;
Instead of changing the value of avg_bw like mentioned in commit text,
why cant we do avg_bw = fix_core_ab * (drm_mode_config::num_crtc);
Any reason you want to change it from "per CRTC fixed" to just "fixed"?
Now, the user who wants to hard-code this also needs to first account
for number of CRTCs from the dri state and then program the fixed value
using debugfs. Thats not convenient.
Different CRTCs have different bandwidth values, so programming as
value-per-CRTC is not efficient. In the end we care for the overall
bandwidth, so one has to calculate the expected value then divide it
per num_crtc.
Yes, different CRTCs will have different bandwidth values as each CRTC
might be driving a different resolution.
So you are expecting the user to program the total bandwidth they are
expecting (sum_of_crtcs).
Then why would they have to divide it per num_crtc?
After this change, I think you are expecting the overall bandwidth right?
I think some --help option for this debugfs should be written now or
later to explain what to do with node,
+ } else {
+ dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc),
);
Where is this function dpu_core_perf_aggregate() defined? I dont see it
in msm-next
In the previous patch.
Sorry, my bad. I thought it had a different name in the prev patch :/
No problems.
+
+ avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
+ peak_bw = perf.max_per_pipe_ib;
+ }
- avg_bw = perf.bw_ctl;
- do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
+ avg_bw /= kms->num_paths;
Re: [PATCH] drm/msm/dpu: add support for 4:2:2 and 4:4:4 planar YCbCr plane formats
On 3/21/2024 11:09 AM, Dmitry Baryshkov wrote: On Thu, 21 Mar 2024 at 19:36, Abhinav Kumar wrote: On 3/21/2024 8:43 AM, Dmitry Baryshkov wrote: On Fri, 23 Feb 2024 at 22:48, Abhinav Kumar wrote: On 2/22/2024 3:43 AM, Dmitry Baryshkov wrote: The DPU driver provides support for 4:2:0 planar YCbCr plane formats. Extend it to also support 4:2:2 and 4:4:4 plat formats. I checked myself and also internally on this. On sm8250, the DPU planes do not support YUV444 and YUV422 (and the corresponding YVU formats). May I know what was the reference to add these formats to DPU considering that even downstream sources didn't add them? Signed-off-by: Dmitry Baryshkov --- Full-screen (1080p@60) YV24 gave me underruns on SM8250 until I bumped the clock inefficiency factor from 105 to 117. I'm not sure that it is a correct way to handle it, so I'm sending this as an RFC. If we agree that bumping the .clk_inefficiency_factor is a correct way, I'll send v2, including catalog changes. I had no such issues for the YV16/YU16 formats. We don't support this too on sm8250. But interesting it worked. I have been cross-checking DPU formats list against the format list from the display overview docs. The DPU (and SDE FWIW) drivers supported NV16/61 and UYVY/YUY2/YVYU/VYUY formats for ages, although overview does not mention these semi-planar formats at all and interleaved YUV formats are marked as unsupported. For reference, NV24 and NV42 also seem to work. Thanks for the update. I cross-checked sm8250 format list in our internal docs to make sure there is no discrepancy between those and the display overview doc. NV16 / NV61 (linear) are marked "NOT supported" by DPU. UYVY/YUY2/YVYU/VYUY (linear) are also marked "NOT supported". But all of these image formats are handled by the DPU _driver_ as supported. Ok, I see where this discrepancy is happening now. So I took another chipset, sc8280xp and checked these formats. Those are marked "supported" in that. Our dpu_formats listed in the driver is not chipset specific and that is causing this discrepancy between the display overview docs and what is in the driver. I will plan to move the formats list to the catalog to eliminate this and prioritize that change. Till then, I think we should stick to the display overview doc in terms which formats should be validated on which chipsets. So the markings are correct. If you notice a discrepancy between our dpu formats list in the driver and what is marked as "supported" in the display overview docs, that is something we can investigate and get fixed. If you are running some standalone tests and reporting that formats marked as "unsupported" in the display overview docs still work, we cannot simply add those formats on the basis of your modetest validation as your validation alone shall not supersede the marking of the design teams as the system level validation of those formats is what we have to go by. The formats marked unsupported shall remain unsupported by the driver and QC shall not ack adding any of those.
Re: [PATCH] drm/msm/dpu: add support for 4:2:2 and 4:4:4 planar YCbCr plane formats
On 3/21/2024 8:43 AM, Dmitry Baryshkov wrote: On Fri, 23 Feb 2024 at 22:48, Abhinav Kumar wrote: On 2/22/2024 3:43 AM, Dmitry Baryshkov wrote: The DPU driver provides support for 4:2:0 planar YCbCr plane formats. Extend it to also support 4:2:2 and 4:4:4 plat formats. I checked myself and also internally on this. On sm8250, the DPU planes do not support YUV444 and YUV422 (and the corresponding YVU formats). May I know what was the reference to add these formats to DPU considering that even downstream sources didn't add them? Signed-off-by: Dmitry Baryshkov --- Full-screen (1080p@60) YV24 gave me underruns on SM8250 until I bumped the clock inefficiency factor from 105 to 117. I'm not sure that it is a correct way to handle it, so I'm sending this as an RFC. If we agree that bumping the .clk_inefficiency_factor is a correct way, I'll send v2, including catalog changes. I had no such issues for the YV16/YU16 formats. We don't support this too on sm8250. But interesting it worked. I have been cross-checking DPU formats list against the format list from the display overview docs. The DPU (and SDE FWIW) drivers supported NV16/61 and UYVY/YUY2/YVYU/VYUY formats for ages, although overview does not mention these semi-planar formats at all and interleaved YUV formats are marked as unsupported. For reference, NV24 and NV42 also seem to work. Thanks for the update. I cross-checked sm8250 format list in our internal docs to make sure there is no discrepancy between those and the display overview doc. NV16 / NV61 (linear) are marked "NOT supported" by DPU. UYVY/YUY2/YVYU/VYUY (linear) are also marked "NOT supported". So the markings are correct. If you notice a discrepancy between our dpu formats list in the driver and what is marked as "supported" in the display overview docs, that is something we can investigate and get fixed. If you are running some standalone tests and reporting that formats marked as "unsupported" in the display overview docs still work, we cannot simply add those formats on the basis of your modetest validation as your validation alone shall not supersede the marking of the design teams as the system level validation of those formats is what we have to go by. The formats marked unsupported shall remain unsupported by the driver and QC shall not ack adding any of those. -- With best wishes Dmitry
Re: [PATCH v3 3/5] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()
On 3/19/2024 1:43 PM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 22:34, Abhinav Kumar wrote:
On 3/13/2024 6:10 PM, Dmitry Baryshkov wrote:
Move perf mode handling for the bandwidth to
_dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
and then aggregating known values.
Note, this changes the fix_core_ab_vote. Previously it would be
multiplied per the CRTC number, now it will be used directly for
interconnect voting.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 39
+--
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 87b892069526..ff2942a6a678 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -118,21 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct
dpu_core_perf *core_perf,
return;
}
- memset(perf, 0, sizeof(struct dpu_core_perf_params));
-
- if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
- perf->bw_ctl = 0;
- perf->max_per_pipe_ib = 0;
- perf->core_clk_rate = 0;
- } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
- perf->bw_ctl = core_perf->fix_core_ab_vote;
- perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
- perf->core_clk_rate = core_perf->fix_core_clk_rate;
- } else {
- perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
- perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
- perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc,
state);
- }
+ perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
+ perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
+ perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
DRM_DEBUG_ATOMIC(
"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
@@ -233,18 +221,29 @@ 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;
+ u32 peak_bw;
Why were avg_bw and peak_bw values brought down to u32?
I think we might go higher so u64 was better.
if (!kms->num_paths)
return 0;
- dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), );
+ if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
+ avg_bw = 0;
+ peak_bw = 0;
+ } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
+ avg_bw = kms->perf.fix_core_ab_vote;
+ peak_bw = kms->perf.fix_core_ib_vote;
Instead of changing the value of avg_bw like mentioned in commit text,
why cant we do avg_bw = fix_core_ab * (drm_mode_config::num_crtc);
Any reason you want to change it from "per CRTC fixed" to just "fixed"?
Now, the user who wants to hard-code this also needs to first account
for number of CRTCs from the dri state and then program the fixed value
using debugfs. Thats not convenient.
+ } else {
+ dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc),
);
Where is this function dpu_core_perf_aggregate() defined? I dont see it
in msm-next
In the previous patch.
Sorry, my bad. I thought it had a different name in the prev patch :/
+
+ avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
+ peak_bw = perf.max_per_pipe_ib;
+ }
- avg_bw = perf.bw_ctl;
- do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
+ avg_bw /= kms->num_paths;
for (i = 0; i < kms->num_paths; i++)
- icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
+ icc_set_bw(kms->path[i], avg_bw, peak_bw);
return ret;
}
Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
On 3/19/2024 1:16 PM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 21:02, Abhinav Kumar wrote:
On 3/19/2024 11:35 AM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 20:15, Douglas Anderson wrote:
In response to my patch removing the "wait for HPD" logic at the
beginning of the MSM DP transfer() callback [1], we had some debate
about what the "This is an optional function" meant in the
documentation of the wait_hpd_asserted() callback. Let's clarify.
As talked about in the MSM DP patch [1], before wait_hpd_asserted()
was introduced there was no great way for panel drivers to wait for
HPD in the case that the "built-in" HPD signal was used. Panel drivers
could only wait for HPD if a GPIO was used. At the time, we ended up
just saying that if we were using the "built-in" HPD signal that DP
AUX controllers needed to wait for HPD themselves at the beginning of
their transfer() callback. The fact that the wait for HPD at the
beginning of transfer() was awkward/problematic was the whole reason
wait_hpd_asserted() was added.
Let's make it obvious that if a DP AUX controller implements
wait_hpd_asserted() that they don't need a loop waiting for HPD at the
start of their transfer() function. We'll still allow DP controllers
to work the old way but mark it as deprecated.
[1]
https://lore.kernel.org/r/20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid
Signed-off-by: Douglas Anderson
---
I would consider changing the docs to say that implementing
wait_hpd_asserted() is actually _required_ for any DP controllers that
want to support eDP panels parented on the DP AUX bus. The issue is
that one DP controller (tegra/dpaux.c, found by looking for those that
include display/drm_dp_aux_bus.h) does populate the DP AUX bus but
doesn't implement wait_hpd_asserted(). I'm actually not sure how/if
this work on tegra since I also don't see any delay loop for HPD in
tegra's transfer() callback. For now, I've left wait_hpd_asserted() as
optional and described the old/deprecated way things used to work
before wait_hpd_asserted().
include/drm/display/drm_dp_helper.h | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/drm/display/drm_dp_helper.h
b/include/drm/display/drm_dp_helper.h
index a62fcd051d4d..b170efa1f5d2 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -422,7 +422,13 @@ struct drm_dp_aux {
* @wait_hpd_asserted: wait for HPD to be asserted
*
* This is mainly useful for eDP panels drivers to wait for an eDP
-* panel to finish powering on. This is an optional function.
+* panel to finish powering on. It is optional for DP AUX controllers
+* to implement this function but required for DP AUX endpoints (panel
+* drivers) to call it after powering up but before doing AUX transfers.
+* If a DP AUX controller does not implement this function then it
+* may still support eDP panels that use the AUX controller's built-in
+* HPD signal by implementing a long wait for HPD in the transfer()
+* callback, though this is deprecated.
It doesn't cover a valid case when the panel driver handles HPD signal
on its own.
This doc is only for wait_for_hpd_asserted(). If panel driver handles
HPD signal on its own, this will not be called. Do we need a doc for that?
This comment declares that this callback must be called by the panel
driver: '...but required for DP AUX endpoints [...] to call it after
powering up but before doing AUX transfers.'
If we were to follow documentation changes from this patch, we'd have
to patch panel-edp to always call wait_for_hpd_asserted, even if HPD
GPIO is used. However this is not correct from my POV.
hmmm I dont mind explicitly saying "unless the panel can independently
check the HPD state" but not required in my opinion because if panel was
capable of checking the HPD gpio (its self-capable) why would it even
call wait_for_hpd_asserted?
I will let you and Doug discuss this but fwiw, I am fine without this
additional clarification. So the R-b stands with or without this
additional clause.
*
* This function will efficiently wait for the HPD signal to be
* asserted. The `wait_us` parameter that is passed in says that we
--
2.44.0.291.gc1ea87d7ee-goog
Re: [PATCH v3 3/5] drm/msm/dpu: handle perf mode in _dpu_core_perf_crtc_update_bus()
On 3/13/2024 6:10 PM, Dmitry Baryshkov wrote:
Move perf mode handling for the bandwidth to
_dpu_core_perf_crtc_update_bus() rather than overriding per-CRTC data
and then aggregating known values.
Note, this changes the fix_core_ab_vote. Previously it would be
multiplied per the CRTC number, now it will be used directly for
interconnect voting.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 39 +--
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 87b892069526..ff2942a6a678 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -118,21 +118,9 @@ static void _dpu_core_perf_calc_crtc(const struct
dpu_core_perf *core_perf,
return;
}
- memset(perf, 0, sizeof(struct dpu_core_perf_params));
-
- if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
- perf->bw_ctl = 0;
- perf->max_per_pipe_ib = 0;
- perf->core_clk_rate = 0;
- } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
- perf->bw_ctl = core_perf->fix_core_ab_vote;
- perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
- perf->core_clk_rate = core_perf->fix_core_clk_rate;
- } else {
- perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
- perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
- perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc,
state);
- }
+ perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
+ perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
+ perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
DRM_DEBUG_ATOMIC(
"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
@@ -233,18 +221,29 @@ 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;
+ u32 peak_bw;
if (!kms->num_paths)
return 0;
- dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), );
+ if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
+ avg_bw = 0;
+ peak_bw = 0;
+ } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
+ avg_bw = kms->perf.fix_core_ab_vote;
+ peak_bw = kms->perf.fix_core_ib_vote;
+ } else {
+ dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc),
);
Where is this function dpu_core_perf_aggregate() defined? I dont see it
in msm-next
+
+ avg_bw = div_u64(perf.bw_ctl, 1000); /*Bps_to_icc*/
+ peak_bw = perf.max_per_pipe_ib;
+ }
- avg_bw = perf.bw_ctl;
- do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
+ avg_bw /= kms->num_paths;
for (i = 0; i < kms->num_paths; i++)
- icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
+ icc_set_bw(kms->path[i], avg_bw, peak_bw);
return ret;
}
Re: [PATCH v3 2/5] drm/msm/dpu: core_perf: extract bandwidth aggregation function
On 3/13/2024 6:10 PM, Dmitry Baryshkov wrote:
In preparation to refactoring the dpu_core_perf debugfs interface,
extract the bandwidth aggregation function from
_dpu_core_perf_crtc_update_bus().
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 45 +++
1 file changed, 25 insertions(+), 20 deletions(-)
no need of core_perf : in the subject line.
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 68fae048a9a8..87b892069526 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -204,36 +204,41 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
return 0;
}
-static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
- struct drm_crtc *crtc)
+static void dpu_core_perf_aggregate(struct drm_device *ddev,
+ enum dpu_crtc_client_type curr_client_type,
+ struct dpu_core_perf_params *perf)
{
- struct dpu_core_perf_params perf = { 0 };
- enum dpu_crtc_client_type curr_client_type
- = dpu_crtc_get_client_type(crtc);
- struct drm_crtc *tmp_crtc;
struct dpu_crtc_state *dpu_cstate;
- int i, ret = 0;
- u64 avg_bw;
-
- if (!kms->num_paths)
- return 0;
+ struct drm_crtc *tmp_crtc;
- drm_for_each_crtc(tmp_crtc, crtc->dev) {
+ drm_for_each_crtc(tmp_crtc, ddev) {
if (tmp_crtc->enabled &&
- curr_client_type ==
- dpu_crtc_get_client_type(tmp_crtc)) {
+ curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
- perf.max_per_pipe_ib = max(perf.max_per_pipe_ib,
- dpu_cstate->new_perf.max_per_pipe_ib);
+ perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
+
dpu_cstate->new_perf.max_per_pipe_ib);
- perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
+ perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
- DRM_DEBUG_ATOMIC("crtc=%d bw=%llu paths:%d\n",
- tmp_crtc->base.id,
- dpu_cstate->new_perf.bw_ctl, kms->num_paths);
+ DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
+tmp_crtc->base.id,
+dpu_cstate->new_perf.bw_ctl);
}
}
+}
+
+static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
+ struct drm_crtc *crtc)
+{
since you have fixed some of the formatting inside the function, do you
also want to align struct drm_crtc *crtc with the opening braces?
With that fixed,
Reviewed-by: Abhinav Kumar
+ struct dpu_core_perf_params perf = { 0 };
+ int i, ret = 0;
+ u64 avg_bw;
+
+ if (!kms->num_paths)
+ return 0;
+
+ dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc),
);
avg_bw = perf.bw_ctl;
do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
On 3/19/2024 11:35 AM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 20:15, Douglas Anderson wrote:
In response to my patch removing the "wait for HPD" logic at the
beginning of the MSM DP transfer() callback [1], we had some debate
about what the "This is an optional function" meant in the
documentation of the wait_hpd_asserted() callback. Let's clarify.
As talked about in the MSM DP patch [1], before wait_hpd_asserted()
was introduced there was no great way for panel drivers to wait for
HPD in the case that the "built-in" HPD signal was used. Panel drivers
could only wait for HPD if a GPIO was used. At the time, we ended up
just saying that if we were using the "built-in" HPD signal that DP
AUX controllers needed to wait for HPD themselves at the beginning of
their transfer() callback. The fact that the wait for HPD at the
beginning of transfer() was awkward/problematic was the whole reason
wait_hpd_asserted() was added.
Let's make it obvious that if a DP AUX controller implements
wait_hpd_asserted() that they don't need a loop waiting for HPD at the
start of their transfer() function. We'll still allow DP controllers
to work the old way but mark it as deprecated.
[1]
https://lore.kernel.org/r/20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid
Signed-off-by: Douglas Anderson
---
I would consider changing the docs to say that implementing
wait_hpd_asserted() is actually _required_ for any DP controllers that
want to support eDP panels parented on the DP AUX bus. The issue is
that one DP controller (tegra/dpaux.c, found by looking for those that
include display/drm_dp_aux_bus.h) does populate the DP AUX bus but
doesn't implement wait_hpd_asserted(). I'm actually not sure how/if
this work on tegra since I also don't see any delay loop for HPD in
tegra's transfer() callback. For now, I've left wait_hpd_asserted() as
optional and described the old/deprecated way things used to work
before wait_hpd_asserted().
include/drm/display/drm_dp_helper.h | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/drm/display/drm_dp_helper.h
b/include/drm/display/drm_dp_helper.h
index a62fcd051d4d..b170efa1f5d2 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -422,7 +422,13 @@ struct drm_dp_aux {
* @wait_hpd_asserted: wait for HPD to be asserted
*
* This is mainly useful for eDP panels drivers to wait for an eDP
-* panel to finish powering on. This is an optional function.
+* panel to finish powering on. It is optional for DP AUX controllers
+* to implement this function but required for DP AUX endpoints (panel
+* drivers) to call it after powering up but before doing AUX transfers.
+* If a DP AUX controller does not implement this function then it
+* may still support eDP panels that use the AUX controller's built-in
+* HPD signal by implementing a long wait for HPD in the transfer()
+* callback, though this is deprecated.
It doesn't cover a valid case when the panel driver handles HPD signal
on its own.
This doc is only for wait_for_hpd_asserted(). If panel driver handles
HPD signal on its own, this will not be called. Do we need a doc for that?
*
* This function will efficiently wait for the HPD signal to be
* asserted. The `wait_us` parameter that is passed in says that we
--
2.44.0.291.gc1ea87d7ee-goog
Re: [PATCH v3 1/5] drm/msm/dpu: don't allow overriding data from catalog
On 3/13/2024 6:10 PM, Dmitry Baryshkov wrote:
The data from catalog is marked as const, so it is a part of the RO
segment. Allowing userspace to write to it through debugfs can cause
protection faults. Set debugfs file mode to read-only for debug entries
corresponding to perf_cfg coming from catalog.
Fixes: abda0d925f9c ("drm/msm/dpu: Mark various data tables as const")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
Thanks for the fix,
Reviewed-by: Abhinav Kumar
Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
On 3/19/2024 11:15 AM, Doug Anderson wrote:
Hi,
On Tue, Mar 19, 2024 at 10:27 AM Dmitry Baryshkov
wrote:
On Tue, 19 Mar 2024 at 19:13, Abhinav Kumar wrote:
On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar wrote:
+bjorn, johan as fyi for sc8280xp
On 3/15/2024 2:36 PM, Douglas Anderson wrote:
Before the introduction of the wait_hpd_asserted() callback in commit
841d742f094e ("drm/dp: Add wait_hpd_asserted() callback to struct
drm_dp_aux") the API between panel drivers and DP AUX bus drivers was
that it was up to the AUX bus driver to wait for HPD in the transfer()
function.
Now wait_hpd_asserted() has been added. The two panel drivers that are
DP AUX endpoints use it. See commit 2327b13d6c47 ("drm/panel-edp: Take
advantage of wait_hpd_asserted() in struct drm_dp_aux") and commit
3b5765df375c ("drm/panel: atna33xc20: Take advantage of
wait_hpd_asserted() in struct drm_dp_aux"). We've implemented
wait_hpd_asserted() in the MSM DP driver as of commit e2969ee30252
("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()"). There is
no longer any reason for long wait in the AUX transfer() function.
Remove it.
NOTE: the wait_hpd_asserted() is listed as "optional". That means it's
optional for the DP AUX bus to implement. In the case of the MSM DP
driver we implement it so we can assume it will be called.
How do we enforce that for any new edp panels to be used with MSM, the
panels should atleast invoke wait_hpd_asserted()?
I agree that since MSM implements it, even though its listed as
optional, we can drop this additional wait. So nothing wrong with this
patch for current users including sc8280xp, sc7280 and sc7180.
But, does there need to be some documentation that the edp panels not
using the panel-edp framework should still invoke wait_hpd_asserted()?
Since its marked as optional, what happens if the edp panel driver,
skips calling wait_hpd_asserted()?
It is optional for the DP AUX implementations, not for the panel to be called.
Yes, I understood that part, but is there anything from the panel side
which mandates calling wait_hpd_asserted()?
Is this documented somewhere for all edp panels to do:
if (aux->wait_hpd_asserted)
aux->wait_hpd_asserted(aux, wait_us);
That's obviously not true, e.g. if panel-edp.c handled HPD signal via
the GPIO pin.
But the documentation explicitly says that the host will be powered up
automatically, but the caller must ensure that the panel is powered
on. `It's up to the caller of this code to make sure that the panel is
powered on if getting an error back is not OK.'
It wouldn't hurt to send out a documentation patch that makes this
more explicit. OK, I sent:
https://lore.kernel.org/r/20240319111432.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid
-Doug
Thanks, with that in place, this is
Reviewed-by: Abhinav Kumar
Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
On 3/19/2024 11:14 AM, Douglas Anderson wrote: In response to my patch removing the "wait for HPD" logic at the beginning of the MSM DP transfer() callback [1], we had some debate about what the "This is an optional function" meant in the documentation of the wait_hpd_asserted() callback. Let's clarify. As talked about in the MSM DP patch [1], before wait_hpd_asserted() was introduced there was no great way for panel drivers to wait for HPD in the case that the "built-in" HPD signal was used. Panel drivers could only wait for HPD if a GPIO was used. At the time, we ended up just saying that if we were using the "built-in" HPD signal that DP AUX controllers needed to wait for HPD themselves at the beginning of their transfer() callback. The fact that the wait for HPD at the beginning of transfer() was awkward/problematic was the whole reason wait_hpd_asserted() was added. Let's make it obvious that if a DP AUX controller implements wait_hpd_asserted() that they don't need a loop waiting for HPD at the start of their transfer() function. We'll still allow DP controllers to work the old way but mark it as deprecated. [1] https://lore.kernel.org/r/20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid Signed-off-by: Douglas Anderson --- I would consider changing the docs to say that implementing wait_hpd_asserted() is actually _required_ for any DP controllers that want to support eDP panels parented on the DP AUX bus. The issue is that one DP controller (tegra/dpaux.c, found by looking for those that include display/drm_dp_aux_bus.h) does populate the DP AUX bus but doesn't implement wait_hpd_asserted(). I'm actually not sure how/if this work on tegra since I also don't see any delay loop for HPD in tegra's transfer() callback. For now, I've left wait_hpd_asserted() as optional and described the old/deprecated way things used to work before wait_hpd_asserted(). include/drm/display/drm_dp_helper.h | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) Thanks for the change, Reviewed-by: Abhinav Kumar
Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar wrote:
+bjorn, johan as fyi for sc8280xp
On 3/15/2024 2:36 PM, Douglas Anderson wrote:
Before the introduction of the wait_hpd_asserted() callback in commit
841d742f094e ("drm/dp: Add wait_hpd_asserted() callback to struct
drm_dp_aux") the API between panel drivers and DP AUX bus drivers was
that it was up to the AUX bus driver to wait for HPD in the transfer()
function.
Now wait_hpd_asserted() has been added. The two panel drivers that are
DP AUX endpoints use it. See commit 2327b13d6c47 ("drm/panel-edp: Take
advantage of wait_hpd_asserted() in struct drm_dp_aux") and commit
3b5765df375c ("drm/panel: atna33xc20: Take advantage of
wait_hpd_asserted() in struct drm_dp_aux"). We've implemented
wait_hpd_asserted() in the MSM DP driver as of commit e2969ee30252
("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()"). There is
no longer any reason for long wait in the AUX transfer() function.
Remove it.
NOTE: the wait_hpd_asserted() is listed as "optional". That means it's
optional for the DP AUX bus to implement. In the case of the MSM DP
driver we implement it so we can assume it will be called.
How do we enforce that for any new edp panels to be used with MSM, the
panels should atleast invoke wait_hpd_asserted()?
I agree that since MSM implements it, even though its listed as
optional, we can drop this additional wait. So nothing wrong with this
patch for current users including sc8280xp, sc7280 and sc7180.
But, does there need to be some documentation that the edp panels not
using the panel-edp framework should still invoke wait_hpd_asserted()?
Since its marked as optional, what happens if the edp panel driver,
skips calling wait_hpd_asserted()?
It is optional for the DP AUX implementations, not for the panel to be called.
Yes, I understood that part, but is there anything from the panel side
which mandates calling wait_hpd_asserted()?
Is this documented somewhere for all edp panels to do:
if (aux->wait_hpd_asserted)
aux->wait_hpd_asserted(aux, wait_us);
Now, since the wait from MSM is removed, it has a potential to fail.
ALSO NOTE: the wait wasn't actually _hurting_ anything and wasn't even
causing long timeouts, but it's still nice to get rid of unneeded
code. Specificaly it's not truly needed because to handle other DP
drivers that can't power on as quickly (specifically parade-ps8640) we
already avoid DP AUX transfers for eDP panels that aren't powered
on. See commit 8df1ddb5bf11 ("drm/dp: Don't attempt AUX transfers when
eDP panels are not powered").
Signed-off-by: Douglas Anderson
---
(no changes since v1)
drivers/gpu/drm/msm/dp/dp_aux.c | 17 -
1 file changed, 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 75c51f3ee106..ecefd1922d6d 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -313,23 +313,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
goto exit;
}
- /*
- * For eDP it's important to give a reasonably long wait here for HPD
- * to be asserted. This is because the panel driver may have _just_
- * turned on the panel and then tried to do an AUX transfer. The panel
- * driver has no way of knowing when the panel is ready, so it's up
- * to us to wait. For DP we never get into this situation so let's
- * avoid ever doing the extra long wait for DP.
- */
- if (aux->is_edp) {
- ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog,
- 50);
- if (ret) {
- DRM_DEBUG_DP("Panel not ready for aux transactions\n");
- goto exit;
- }
- }
-
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);
Re: [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status)
On 3/18/2024 12:37 PM, Doug Anderson wrote:
Hi,
On Mon, Mar 18, 2024 at 12:26 PM Stephen Boyd wrote:
Quoting Douglas Anderson (2024-03-15 14:36:32)
This is a no-op change to just fix a typo in the name of a static function.
Signed-off-by: Douglas Anderson
---
Changes in v2:
- ("Fix typo in static function (ststus => status)") new for v2.
This was sent at
https://lore.kernel.org/r/20240306193515.455388-1-quic_abhin...@quicinc.com
Whoops! I guess we both noticed it at about the same time. My patch
should be dropped then. The rest of my series (patches #1 - #3) are
still relevant. I won't repost them since they can be applied just
fine even if this patch is dropped.
-Doug
Thanks for the patch.
I will pick up
https://lore.kernel.org/r/20240306193515.455388-1-quic_abhin...@quicinc.com
for -fixes so you can drop this one.
