Re: [PATCH v2] Revert "drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set"
On 5/22/2024 3:24 AM, Dmitry Baryshkov wrote:
In the DPU driver blank IRQ handling is called from a vblank worker and
can happen outside of the irq_enable / irq_disable pair. Using the
worker makes that completely asynchronous with the rest of the code.
Revert commit d13f638c9b88 ("drm/msm/dpu: drop
dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for
CMD DSI panels.
Call trace:
dpu_encoder_phys_cmd_control_vblank_irq+0x218/0x294
dpu_encoder_toggle_vblank_for_crtc+0x160/0x194
dpu_crtc_vblank+0xbc/0x228
dpu_kms_enable_vblank+0x18/0x24
vblank_ctrl_worker+0x34/0x6c
process_one_work+0x218/0x620
worker_thread+0x1ac/0x37c
kthread+0x114/0x118
ret_from_fork+0x10/0x20
Thanks for the stack.
Agreed that vblank can be controlled asynchronously through the worker.
And I am guessing that the worker thread ran and printed this error
message because phys_enc->irq[INTR_IDX_VSYNC] was not valid as modeset
had not happened by then?
272 end:
273 if (ret) {
274 DRM_ERROR("vblank irq err id:%u pp:%d ret:%d, enable %s/%d\n",
275 DRMID(phys_enc->parent),
276 phys_enc->hw_pp->idx - PINGPONG_0, ret,
277 enable ? "true" : "false", refcount);
278 }
But how come this did not happen even with this revert.
IOW, I am still missing how moving the irq assignment back to modeset
from enable is fixing this?
Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
Signed-off-by: Dmitry Baryshkov
---
Changes in v2:
- Expanded commit message to describe the reason for revert and added a
call trace (Abhinav)
- Link to v1:
https://lore.kernel.org/r/20240514-dpu-revert-ams-v1-1-b13623d6c...@linaro.org
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 --
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++--
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 11 +++-
5 files changed, 46 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..a7d8ecf3f5be 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct
drm_encoder *drm_enc,
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
phys->cached_mode = crtc_state->adjusted_mode;
+ if (phys->ops.atomic_mode_set)
+ phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
}
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 002e89cc1705..30470cd15a48 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -69,6 +69,8 @@ struct dpu_encoder_phys;
* @is_master:Whether this phys_enc is the current
master
*encoder. Can be switched at enable time. Based
*on split_role and current mode (CMD/VID).
+ * @atomic_mode_set: DRM Call. Set a DRM mode.
+ * This likely caches the mode, for use at enable.
* @enable: DRM Call. Enable a DRM mode.
* @disable: DRM Call. Disable mode.
* @control_vblank_irqRegister/Deregister for VBLANK IRQ
@@ -93,6 +95,9 @@ struct dpu_encoder_phys;
struct dpu_encoder_phys_ops {
void (*prepare_commit)(struct dpu_encoder_phys *encoder);
bool (*is_master)(struct dpu_encoder_phys *encoder);
+ void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state);
void (*enable)(struct dpu_encoder_phys *encoder);
void (*disable)(struct dpu_encoder_phys *encoder);
int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 489be1c0c704..95cd39b49668 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg)
dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
}
+static void dpu_encoder_phys_cmd_atomic_mode_set(
+ struct dpu_encoder_phys *phys_enc,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
+
+ phys_enc->irq[INTR_IDX_PINGPONG] =
Re: [BUG] Build failure and alleged fix for next-20240523
Hello
On 5/24/2024 12:55 PM, Paul E. McKenney wrote:
Hello!
I get the following allmodconfig build error on x86 in next-20240523:
Traceback (most recent call last):
File "drivers/gpu/drm/msm/registers/gen_header.py", line 970, in
main()
File "drivers/gpu/drm/msm/registers/gen_header.py", line 951, in main
parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
AttributeError: module 'argparse' has no attribute 'BooleanOptionalAction'
The following patch allows the build to complete successfully:
https://patchwork.kernel.org/project/dri-devel/patch/20240508091751.336654-1-jonath...@nvidia.com/#25842751
As to whether this is a proper fix, I must defer to the DRM folks on CC.
Thanx, Paul
Thanks for the report.
I have raised a merge request for
https://patchwork.freedesktop.org/patch/593057/ to make it available for
the next fixes release for msm.
Abhinav
Re: [PATCH] drm/msm/dpu: drop duplicate drm formats from wb2_formats arrays
On 5/24/2024 8:01 AM, Junhao Xie wrote:
There are duplicate items in wb2_formats_rgb and wb2_formats_rgb_yuv,
which cause weston assertions failed.
weston: libweston/drm-formats.c:131: weston_drm_format_array_add_format:
Assertion `!weston_drm_format_array_find_format(formats, format)' failed.
Signed-off-by: Junhao Xie
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 6 --
1 file changed, 6 deletions(-)
I think we need two fixes tag here, one for the RGB array and the other
one for the RGB+YUV array.
Fixes: 8c16b988ba2d ("drm/msm/dpu: introduce separate wb2_format arrays
for rgb and yuv")
Fixes: 53324b99bd7b ("drm/msm/dpu: add writeback blocks to the sm8250
DPU catalog")
Reviewed-by: Abhinav Kumar
(pls ignore the line breaks in the fixes line, I will fix it while applying)
Re: [PATCH 2/7] drm/msm/dpu: convert vsync source defines to the enum
On 5/22/2024 1:01 PM, Dmitry Baryshkov wrote:
On Wed, 22 May 2024 at 21:41, Abhinav Kumar wrote:
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
Add enum dpu_vsync_source instead of a series of defines. Use this enum
to pass vsync information.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 2 +-
5 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index 66759623fc42..a2eff36a2224 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -54,18 +54,20 @@
#define DPU_BLEND_BG_INV_MOD_ALPHA (1 << 12)
#define DPU_BLEND_BG_TRANSP_EN (1 << 13)
-#define DPU_VSYNC0_SOURCE_GPIO 0
-#define DPU_VSYNC1_SOURCE_GPIO 1
-#define DPU_VSYNC2_SOURCE_GPIO 2
-#define DPU_VSYNC_SOURCE_INTF_0 3
-#define DPU_VSYNC_SOURCE_INTF_1 4
-#define DPU_VSYNC_SOURCE_INTF_2 5
-#define DPU_VSYNC_SOURCE_INTF_3 6
-#define DPU_VSYNC_SOURCE_WD_TIMER_4 11
-#define DPU_VSYNC_SOURCE_WD_TIMER_3 12
-#define DPU_VSYNC_SOURCE_WD_TIMER_2 13
-#define DPU_VSYNC_SOURCE_WD_TIMER_1 14
-#define DPU_VSYNC_SOURCE_WD_TIMER_0 15
+enum dpu_vsync_source {
+ DPU_VSYNC_SOURCE_GPIO_0,
+ DPU_VSYNC_SOURCE_GPIO_1,
+ DPU_VSYNC_SOURCE_GPIO_2,
+ DPU_VSYNC_SOURCE_INTF_0 = 3,
Do we need this assignment to 3?
It is redundant, but it points out that if at some point another GPIO
is added, it should go to the end (or to some other place, having the
proper value).
Ack, makes sense.
Rest LGTM,
Reviewed-by: Abhinav Kumar
Re: [PATCH 1/7] dt-bindings: display/msm/dsi: allow specifying TE source
On 5/22/2024 1:05 PM, Dmitry Baryshkov wrote:
On Wed, 22 May 2024 at 21:38, Abhinav Kumar wrote:
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
Command mode panels provide TE signal back to the DSI host to signal
that the frame display has completed and update of the image will not
cause tearing. Usually it is connected to the first GPIO with the
mdp_vsync function, which is the default. In such case the property can
be skipped.
This is a good addition overall. Some comments below.
Signed-off-by: Dmitry Baryshkov
---
.../bindings/display/msm/dsi-controller-main.yaml| 16
1 file changed, 16 insertions(+)
diff --git
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 1fa28e976559..c1771c69b247 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -162,6 +162,21 @@ properties:
items:
enum: [ 0, 1, 2, 3 ]
+ qcom,te-source:
+$ref: /schemas/types.yaml#/definitions/string
+description:
+ Specifies the source of vsync signal from the panel used for
+ tearing elimination. The default is mdp_gpio0.
panel --> command mode panel?
+enum:
+ - mdp_gpio0
+ - mdp_gpio1
+ - mdp_gpio2
are gpio0, gpio1 and gpio2 referring to the vsync_p, vsync_s and vsync_e
sources?
No idea, unfortunately. They are gpioN or just mdp_vsync all over the
place. For the reference, in case of the SDM845 and Pixel3 the signal
is routed through SoC GPIO12.
GPIO12 on sdm845 is mdp_vsync_e.
Thats why I think its better we use mdp_vsync_p/s/e instead of mdp_gpio0/1/2
In that case wouldnt it be better to name it like that?
+ - timer0
+ - timer1
+ - timer2
+ - timer3
+ - timer4
+
These are indicating watchdog timer sources right?
Yes.
required:
- port@0
- port@1
@@ -452,6 +467,7 @@ examples:
dsi0_out: endpoint {
remote-endpoint = <_in>;
data-lanes = <0 1 2 3>;
+ qcom,te-source = "mdp_gpio2";
I have a basic doubt on this. Should te-source should be in the input
port or the output one for the controller? Because TE is an input to the
DSI. And if the source is watchdog timer then it aligns even more as a
property of the input endpoint.
I don't really want to split this. Both data-lanes and te-source are
properties of the link between the DSI and panel. You can not really
say which side has which property.
TE is an input to the DSI from the panel. Between input and output port,
I think it belongs more to the input port.
I didnt follow why this is a link property. Sorry , I didnt follow the
split part.
If we are unsure about input vs output port, do you think its better we
make it a property of the main dsi node instead?
};
};
};
Re: [PATCH 5/7] drm/msm/dpu: rework vsync_source handling
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote: The struct msm_display_info has is_te_using_watchdog_timer field which is neither used anywhere nor is flexible enough to specify different sources. Replace it with the field specifying the vsync source using enum dpu_vsync_source. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 ++ 3 files changed, 5 insertions(+), 7 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 4/7] drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote: Setting vsync source makes sense only for DSI CMD panels. Pull the is_cmd_mode condition out of the function into the calling code, so that it becomes more explicit. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 3/7] drm/msm/dsi: drop unused GPIOs handling
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote: Neither disp-enable-gpios nor disp-te-gpios are defined in the schema. None of the board DT files use those GPIO pins. Drop them from the driver. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/dsi_host.c | 37 - 1 file changed, 37 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 2/7] drm/msm/dpu: convert vsync source defines to the enum
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
Add enum dpu_vsync_source instead of a series of defines. Use this enum
to pass vsync information.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 2 +-
5 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..4988a1029431 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -747,7 +747,7 @@ static void _dpu_encoder_update_vsync_source(struct
dpu_encoder_virt *dpu_enc,
if (disp_info->is_te_using_watchdog_timer)
vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
else
- vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
+ vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
hw_mdptop->ops.setup_vsync_source(hw_mdptop, _cfg);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 225c1c7768ff..96f6160cf607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -462,7 +462,7 @@ static int dpu_hw_intf_get_vsync_info(struct dpu_hw_intf
*intf,
}
static void dpu_hw_intf_vsync_sel(struct dpu_hw_intf *intf,
- u32 vsync_source)
+ enum dpu_vsync_source vsync_source)
{
struct dpu_hw_blk_reg_map *c;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index f9015c67a574..ac244f0b33fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -107,7 +107,7 @@ struct dpu_hw_intf_ops {
int (*connect_external_te)(struct dpu_hw_intf *intf, bool enable_external_te);
- void (*vsync_sel)(struct dpu_hw_intf *intf, u32 vsync_source);
+ void (*vsync_sel)(struct dpu_hw_intf *intf, enum dpu_vsync_source
vsync_source);
/**
* Disable autorefresh if enabled
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index 66759623fc42..a2eff36a2224 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -54,18 +54,20 @@
#define DPU_BLEND_BG_INV_MOD_ALPHA(1 << 12)
#define DPU_BLEND_BG_TRANSP_EN(1 << 13)
-#define DPU_VSYNC0_SOURCE_GPIO 0
-#define DPU_VSYNC1_SOURCE_GPIO 1
-#define DPU_VSYNC2_SOURCE_GPIO 2
-#define DPU_VSYNC_SOURCE_INTF_03
-#define DPU_VSYNC_SOURCE_INTF_14
-#define DPU_VSYNC_SOURCE_INTF_25
-#define DPU_VSYNC_SOURCE_INTF_36
-#define DPU_VSYNC_SOURCE_WD_TIMER_411
-#define DPU_VSYNC_SOURCE_WD_TIMER_312
-#define DPU_VSYNC_SOURCE_WD_TIMER_213
-#define DPU_VSYNC_SOURCE_WD_TIMER_114
-#define DPU_VSYNC_SOURCE_WD_TIMER_015
+enum dpu_vsync_source {
+ DPU_VSYNC_SOURCE_GPIO_0,
+ DPU_VSYNC_SOURCE_GPIO_1,
+ DPU_VSYNC_SOURCE_GPIO_2,
+ DPU_VSYNC_SOURCE_INTF_0 = 3,
Do we need this assignment to 3?
Rest LGTM,
Reviewed-by: Abhinav Kumar
Re: [PATCH 0/7] drm/msm/dpu: handle non-default TE source pins
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote: Command-mode DSI panels need to signal the display controlller when vsync happens, so that the device can start sending the next frame. Some devices (Google Pixel 3) use a non-default pin, so additional configuration is required. Add a way to specify this information in DT and handle it in the DSI and DPU drivers. Which pin is the pixel 3 using? Just wanted to know .. is it gpio0 or gpio1? Signed-off-by: Dmitry Baryshkov --- Dmitry Baryshkov (7): dt-bindings: display/msm/dsi: allow specifying TE source drm/msm/dpu: convert vsync source defines to the enum drm/msm/dsi: drop unused GPIOs handling drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source() drm/msm/dpu: rework vsync_source handling drm/msm/dsi: parse vsync source from device tree drm/msm/dpu: support setting the TE source .../bindings/display/msm/dsi-controller-main.yaml | 16 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 11 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 5 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h| 26 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 44 drivers/gpu/drm/msm/dsi/dsi.h | 1 + drivers/gpu/drm/msm/dsi/dsi_host.c | 48 +- drivers/gpu/drm/msm/dsi/dsi_manager.c | 5 +++ drivers/gpu/drm/msm/msm_drv.h | 6 +++ 12 files changed, 106 insertions(+), 62 deletions(-) --- base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd change-id: 20240514-dpu-handle-te-signal-82663c0211bd Best regards,
Re: [PATCH 1/7] dt-bindings: display/msm/dsi: allow specifying TE source
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
Command mode panels provide TE signal back to the DSI host to signal
that the frame display has completed and update of the image will not
cause tearing. Usually it is connected to the first GPIO with the
mdp_vsync function, which is the default. In such case the property can
be skipped.
This is a good addition overall. Some comments below.
Signed-off-by: Dmitry Baryshkov
---
.../bindings/display/msm/dsi-controller-main.yaml| 16
1 file changed, 16 insertions(+)
diff --git
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 1fa28e976559..c1771c69b247 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -162,6 +162,21 @@ properties:
items:
enum: [ 0, 1, 2, 3 ]
+ qcom,te-source:
+$ref: /schemas/types.yaml#/definitions/string
+description:
+ Specifies the source of vsync signal from the panel used for
+ tearing elimination. The default is mdp_gpio0.
panel --> command mode panel?
+enum:
+ - mdp_gpio0
+ - mdp_gpio1
+ - mdp_gpio2
are gpio0, gpio1 and gpio2 referring to the vsync_p, vsync_s and vsync_e
sources?
In that case wouldnt it be better to name it like that?
+ - timer0
+ - timer1
+ - timer2
+ - timer3
+ - timer4
+
These are indicating watchdog timer sources right?
required:
- port@0
- port@1
@@ -452,6 +467,7 @@ examples:
dsi0_out: endpoint {
remote-endpoint = <_in>;
data-lanes = <0 1 2 3>;
+ qcom,te-source = "mdp_gpio2";
I have a basic doubt on this. Should te-source should be in the input
port or the output one for the controller? Because TE is an input to the
DSI. And if the source is watchdog timer then it aligns even more as a
property of the input endpoint.
};
};
};
[RFC PATCH 4/4] drm/msm: switch msm_kms to use msm_iommu_disp_new()
Switch msm_kms to use msm_iommu_disp_new() so that the newly registered fault handler will kick-in during any mmu faults. Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/msm/msm_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c index 62c8e6163e81..1859efbbff1d 100644 --- a/drivers/gpu/drm/msm/msm_kms.c +++ b/drivers/gpu/drm/msm/msm_kms.c @@ -181,7 +181,7 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev) else iommu_dev = mdss_dev; - mmu = msm_iommu_new(iommu_dev, 0); + mmu = msm_iommu_disp_new(iommu_dev, 0); if (IS_ERR(mmu)) return ERR_CAST(mmu); -- 2.44.0
[RFC PATCH 3/4] drm/msm/iommu: introduce msm_iommu_disp_new() for msm_kms
Introduce a new API msm_iommu_disp_new() for display use-cases.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/msm_iommu.c | 28
drivers/gpu/drm/msm/msm_mmu.h | 1 +
2 files changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index a79cd18bc4c9..3d5c1bb4c013 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -343,6 +343,19 @@ static int msm_gpu_fault_handler(struct iommu_domain
*domain, struct device *dev
return 0;
}
+static int msm_disp_fault_handler(struct iommu_domain *domain, struct device
*dev,
+ unsigned long iova, int flags, void *arg)
+{
+ struct msm_iommu *iommu = arg;
+
+ if (iommu->base.handler)
+ return iommu->base.handler(iommu->base.arg, iova, flags, NULL);
+
+ pr_warn_ratelimited("*** fault: iova=%16lx, flags=%d\n", iova, flags);
+
+ return 0;
+}
+
static void msm_iommu_resume_translation(struct msm_mmu *mmu)
{
struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(mmu->dev);
@@ -434,6 +447,21 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned
long quirks)
return >base;
}
+struct msm_mmu *msm_iommu_disp_new(struct device *dev, unsigned long quirks)
+{
+ struct msm_iommu *iommu;
+ struct msm_mmu *mmu;
+
+ mmu = msm_iommu_new(dev, quirks);
+ if (IS_ERR_OR_NULL(mmu))
+ return mmu;
+
+ iommu = to_msm_iommu(mmu);
+ iommu_set_fault_handler(iommu->domain, msm_disp_fault_handler, iommu);
+
+ return mmu;
+}
+
struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu,
unsigned long quirks)
{
struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(dev);
diff --git a/drivers/gpu/drm/msm/msm_mmu.h b/drivers/gpu/drm/msm/msm_mmu.h
index 88af4f490881..730458d08d6b 100644
--- a/drivers/gpu/drm/msm/msm_mmu.h
+++ b/drivers/gpu/drm/msm/msm_mmu.h
@@ -42,6 +42,7 @@ static inline void msm_mmu_init(struct msm_mmu *mmu, struct
device *dev,
struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks);
struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu,
unsigned long quirks);
+struct msm_mmu *msm_iommu_disp_new(struct device *dev, unsigned long quirks);
static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
int (*handler)(void *arg, unsigned long iova, int flags, void
*data))
--
2.44.0
[RFC PATCH 2/4] drm/msm/iommu: rename msm_fault_handler to msm_gpu_fault_handler
In preparation of registering a separate fault handler for
display, lets rename the existing msm_fault_handler to
msm_gpu_fault_handler.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/msm_iommu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index d5512037c38b..a79cd18bc4c9 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -243,7 +243,7 @@ static const struct iommu_flush_ops tlb_ops = {
.tlb_add_page = msm_iommu_tlb_add_page,
};
-static int msm_fault_handler(struct iommu_domain *domain, struct device *dev,
+static int msm_gpu_fault_handler(struct iommu_domain *domain, struct device
*dev,
unsigned long iova, int flags, void *arg);
struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent)
@@ -319,7 +319,7 @@ struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu
*parent)
return >base;
}
-static int msm_fault_handler(struct iommu_domain *domain, struct device *dev,
+static int msm_gpu_fault_handler(struct iommu_domain *domain, struct device
*dev,
unsigned long iova, int flags, void *arg)
{
struct msm_iommu *iommu = arg;
@@ -445,7 +445,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev,
struct msm_gpu *gpu, unsig
return mmu;
iommu = to_msm_iommu(mmu);
- iommu_set_fault_handler(iommu->domain, msm_fault_handler, iommu);
+ iommu_set_fault_handler(iommu->domain, msm_gpu_fault_handler, iommu);
/* Enable stall on iommu fault: */
if (adreno_smmu->set_stall)
--
2.44.0
[RFC PATCH 0/4] drm/msm: add a display mmu fault handler
To debug display mmu faults, this series introduces a display fault handler similar to the gpu one. This is only compile tested at the moment, till a suitable method to trigger the fault is found and see if this handler does the needful on the device. Abhinav Kumar (4): drm/msm: register a fault handler for display mmu faults drm/msm/iommu: rename msm_fault_handler to msm_gpu_fault_handler drm/msm/iommu: introduce msm_iommu_disp_new() for msm_kms drm/msm: switch msm_kms to use msm_iommu_disp_new() drivers/gpu/drm/msm/msm_iommu.c | 34 ++--- drivers/gpu/drm/msm/msm_kms.c | 27 +- drivers/gpu/drm/msm/msm_mmu.h | 1 + 3 files changed, 58 insertions(+), 4 deletions(-) -- 2.44.0
[RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults
In preparation to register a iommu fault handler for display
related modules, register a fault handler for the backing
mmu object of msm_kms.
Currently, the fault handler only captures the display snapshot
but we can expand this later if more information needs to be
added to debug display mmu faults.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/msm_kms.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index af6a6fcb1173..62c8e6163e81 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct
drm_device *dev)
return aspace;
}
+static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags,
void *data)
+{
+ struct msm_kms *kms = arg;
+ struct msm_disp_state *state;
+ int ret;
+
+ ret = mutex_lock_interruptible(>dump_mutex);
+ if (ret)
+ return ret;
+
+ state = msm_disp_snapshot_state_sync(kms);
+
+ mutex_unlock(>dump_mutex);
+
+ if (IS_ERR(state)) {
+ DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
+ return PTR_ERR(state);
+ }
+
+ return 0;
+}
+
void msm_drm_kms_uninit(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const struct
drm_driver *drv)
goto err_msm_uninit;
}
+ if (kms->aspace)
+ msm_mmu_set_fault_handler(kms->aspace->mmu, kms,
msm_kms_fault_handler);
+
drm_helper_move_panel_connectors_to_head(ddev);
drm_for_each_crtc(crtc, ddev) {
--
2.44.0
Re: [PATCH] Revert "drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set"
On 5/14/2024 12:56 AM, Dmitry Baryshkov wrote:
In the DPU driver blank IRQ handling is called from a vblank worker and
can happen outside of the irq_enable / irq_disable pair. Revert commit
d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
to fix vblank IRQ assignment for CMD DSI panels.
Can you please explain the sequence of events causing an issue due to
moving the irq assignment to init and how is moving it back to modeset
helping?
Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 --
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++--
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 11 +++-
5 files changed, 46 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..a7d8ecf3f5be 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct
drm_encoder *drm_enc,
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
phys->cached_mode = crtc_state->adjusted_mode;
+ if (phys->ops.atomic_mode_set)
+ phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
}
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 002e89cc1705..30470cd15a48 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -69,6 +69,8 @@ struct dpu_encoder_phys;
* @is_master:Whether this phys_enc is the current
master
*encoder. Can be switched at enable time. Based
*on split_role and current mode (CMD/VID).
+ * @atomic_mode_set: DRM Call. Set a DRM mode.
+ * This likely caches the mode, for use at enable.
* @enable: DRM Call. Enable a DRM mode.
* @disable: DRM Call. Disable mode.
* @control_vblank_irqRegister/Deregister for VBLANK IRQ
@@ -93,6 +95,9 @@ struct dpu_encoder_phys;
struct dpu_encoder_phys_ops {
void (*prepare_commit)(struct dpu_encoder_phys *encoder);
bool (*is_master)(struct dpu_encoder_phys *encoder);
+ void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state);
void (*enable)(struct dpu_encoder_phys *encoder);
void (*disable)(struct dpu_encoder_phys *encoder);
int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 489be1c0c704..95cd39b49668 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg)
dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
}
+static void dpu_encoder_phys_cmd_atomic_mode_set(
+ struct dpu_encoder_phys *phys_enc,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
+
+ phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
+
+ if (phys_enc->has_intf_te)
+ phys_enc->irq[INTR_IDX_RDPTR] =
phys_enc->hw_intf->cap->intr_tear_rd_ptr;
+ else
+ phys_enc->irq[INTR_IDX_RDPTR] =
phys_enc->hw_pp->caps->intr_rdptr;
+
+ phys_enc->irq[INTR_IDX_UNDERRUN] =
phys_enc->hw_intf->cap->intr_underrun;
+}
+
static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
struct dpu_encoder_phys *phys_enc)
{
@@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct
dpu_encoder_phys *phys_enc)
phys_enc->hw_pp->idx - PINGPONG_0,
phys_enc->vblank_refcount);
- phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
- phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
-
- if (phys_enc->has_intf_te)
- phys_enc->irq[INTR_IDX_RDPTR] =
phys_enc->hw_intf->cap->intr_tear_rd_ptr;
- else
- phys_enc->irq[INTR_IDX_RDPTR] =
phys_enc->hw_pp->caps->intr_rdptr;
-
dpu_core_irq_register_callback(phys_enc->dpu_kms,
Re: [PATCH v2] docs: document python version used for compilation
On 5/11/2024 3:32 PM, Dmitry Baryshkov wrote: The drm/msm driver had adopted using Python3 script to generate register header files instead of shipping pre-generated header files. Document the minimal Python version supported by the script. Per request by Jon Hunter, the script is required to be compatible with Python 3.5. Python is documented as an optional dependency, as it is required only in a limited set of kernel configurations (following the example of other optional dependencies). Cc: Jon Hunter Signed-off-by: Dmitry Baryshkov --- Depends: https://lore.kernel.org/dri-devel/20240507230440.3384949-1-quic_abhin...@quicinc.com/ --- Changes in v2: - Expanded documentation for the Python usage. - Link to v1: https://lore.kernel.org/r/20240509-python-version-v1-1-a7dda3a95...@linaro.org --- Documentation/process/changes.rst | 8 1 file changed, 8 insertions(+) Reviewed-by: Abhinav Kumar
Re: [PATCH v2] drm/msm/dpu: fix encoder irq wait skip
On 5/9/2024 12:40 PM, Barnabás Czémán wrote:
The irq_idx is unsigned so it cannot be lower than zero, better
to change the condition to check if it is equal with zero.
It could not cause any issue because a valid irq index starts from one.
Fixes: 5a9d50150c2c ("drm/msm/dpu: shift IRQ indices by 1")
Signed-off-by: Barnabás Czémán
---
Changes in v2:
- Add Fixes in commit message.
- Link to v1:
https://lore.kernel.org/r/20240509-irq_wait-v1-1-41d653e37...@gmail.com
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
no functional change, so you could have retained by R-b, but here it is
again,
Reviewed-by: Abhinav Kumar
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..cf7d769ab3b9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -428,7 +428,7 @@ int dpu_encoder_helper_wait_for_irq(struct dpu_encoder_phys
*phys_enc,
return -EWOULDBLOCK;
}
- if (irq_idx < 0) {
+ if (irq_idx == 0) {
DRM_DEBUG_KMS("skip irq wait id=%u, callback=%ps\n",
DRMID(phys_enc->parent), func);
return 0;
---
base-commit: 704ba27ac55579704ba1289392448b0c66b56258
change-id: 20240509-irq_wait-49444cea77e2
Best regards,
Re: [PATCH] drm/msm/dpu: fix encoder irq wait skip
On 5/9/2024 10:39 AM, Barnabás Czémán wrote:
The irq_idx is unsigned so it cannot be lower than zero, better
to change the condition to check if it is equal with zero.
It could not cause any issue because a valid irq index starts from one.
Signed-off-by: Barnabás Czémán
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I think we also need
Fixes: 5a9d50150c2c ("drm/msm/dpu: shift IRQ indices by 1")
With that,
Reviewed-by: Abhinav Kumar
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..cf7d769ab3b9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -428,7 +428,7 @@ int dpu_encoder_helper_wait_for_irq(struct dpu_encoder_phys
*phys_enc,
return -EWOULDBLOCK;
}
- if (irq_idx < 0) {
+ if (irq_idx == 0) {
DRM_DEBUG_KMS("skip irq wait id=%u, callback=%ps\n",
DRMID(phys_enc->parent), func);
return 0;
---
base-commit: 704ba27ac55579704ba1289392448b0c66b56258
change-id: 20240509-irq_wait-49444cea77e2
Best regards,
Re: [PATCH] drm/msm/dpu: guard ctl irq callback register/unregister
On 5/9/2024 10:52 AM, Barnabás Czémán wrote: CTLs on older qualcomm SOCs like msm8953 and msm8996 has not got interrupts, so better to skip CTL irq callback register/unregister make dpu_ctl_cfg be able to define without intr_start. Thanks for the patch. Have msm8953 and msm8996 migrated to DPU or is there a series planned to migrate them? The change itself is correct but without the catalogs for those chipsets merged, we will never hit this path. Signed-off-by: Barnabás Czémán --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index 489be1c0c704..250d83af53a4 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -298,7 +298,7 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc) phys_enc); dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true); - if (dpu_encoder_phys_cmd_is_master(phys_enc)) + if (dpu_encoder_phys_cmd_is_master(phys_enc) && phys_enc->irq[INTR_IDX_CTL_START]) dpu_core_irq_register_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_CTL_START], dpu_encoder_phys_cmd_ctl_start_irq, @@ -311,7 +311,7 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc) phys_enc->hw_pp->idx - PINGPONG_0, phys_enc->vblank_refcount); - if (dpu_encoder_phys_cmd_is_master(phys_enc)) + if (dpu_encoder_phys_cmd_is_master(phys_enc) && phys_enc->irq[INTR_IDX_CTL_START]) dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_CTL_START]); --- base-commit: 704ba27ac55579704ba1289392448b0c66b56258 change-id: 20240509-ctl_irq-a90b2d7a0bf5 Best regards,
Re: [PATCH] docs: document python version used for compilation
On 5/9/2024 9:48 AM, Jonathan Corbet wrote: Dmitry Baryshkov writes: The drm/msm driver had adopted using Python3 script to generate register header files instead of shipping pre-generated header files. Document the minimal Python version supported by the script. Signed-off-by: Dmitry Baryshkov --- Documentation/process/changes.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/process/changes.rst b/Documentation/process/changes.rst index 5685d7bfe4d0..8d225a9f65a2 100644 --- a/Documentation/process/changes.rst +++ b/Documentation/process/changes.rst @@ -63,6 +63,7 @@ cpio any cpio --version GNU tar1.28 tar --version gtags (optional) 6.6.5gtags --version mkimage (optional) 2017.01 mkimage --version +Python (optional) 3.5.xpython3 --version == === Is it really optional - can you build the driver without it? True, we cannot build the driver now without it. So we should be dropping the optional tag. With that addressed, Reviewed-by: Abhinav Kumar This document needs some help... I'm missing a number of things that are *not* marked as "optional" (jfsutils, reiserfsprogs, pcmciautils, ppp, ...) and somehow my system works fine :) It would be nice to document *why* users might need a specific tool. But I guess we aren't going to do that now. I can apply this, but I do wonder about the "optional" marking. Thanks, jon
Re: [PATCH v2 1/2] drm/msm/gen_header: allow skipping the validation
On 5/8/2024 3:41 PM, Doug Anderson wrote:
Hi,
On Fri, May 3, 2024 at 11:15 AM Dmitry Baryshkov
wrote:
@@ -941,6 +948,7 @@ def main():
parser = argparse.ArgumentParser()
parser.add_argument('--rnn', type=str, required=True)
parser.add_argument('--xml', type=str, required=True)
+ parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
FWIW, the above (argparse.BooleanOptionalAction) appears to be a
python 3.9 thing. My own build environment happens to have python3
default to python 3.8 and thus I get a build error related to this. I
have no idea what the kernel usually assumes for a baseline, but
others might get build errors too. I don't even see python listed in:
https://docs.kernel.org/process/changes.html
...in any case, if it's easy to change this to not require python3.9
that would at least help for my build environment. :-P
Yes, I had posted this y'day as I also ran into this
https://patchwork.freedesktop.org/patch/593057/
-Doug
Re: [PATCH] drm/msm: remove python 3.9 dependency for compiling msm
On 5/8/2024 1:43 AM, Jani Nikula wrote:
On Tue, 07 May 2024, Abhinav Kumar wrote:
Since commit 5acf49119630 ("drm/msm: import gen_header.py script from Mesa"),
compilation is broken on machines having python versions older than 3.9
due to dependency on argparse.BooleanOptionalAction.
Is it now okay to require Python for the build? Not listed in
Documentation/process/changes.rst.
BR,
Jani.
The change to move gen_header.py to kernel is already part of the v6.10
pull request.
This change only fixes the version dependency.
But, I agree we should update the changes.rst (better late than never).
Dmitry, can you pls suggest which version we want to list there?
I am hoping that after this change there are no further dependencies on
python versions, so anything > EOL should be fine.
I am leaning towards v3.8
Switch to use simple bool for the validate flag to remove the dependency.
Fixes: 5acf49119630 ("drm/msm: import gen_header.py script from Mesa")
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/registers/gen_header.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/registers/gen_header.py
b/drivers/gpu/drm/msm/registers/gen_header.py
index fc3bfdc991d2..3926485bb197 100644
--- a/drivers/gpu/drm/msm/registers/gen_header.py
+++ b/drivers/gpu/drm/msm/registers/gen_header.py
@@ -538,7 +538,7 @@ class Parser(object):
self.variants.add(reg.domain)
def do_validate(self, schemafile):
- if self.validate == False:
+ if not self.validate:
return
try:
@@ -948,7 +948,8 @@ def main():
parser = argparse.ArgumentParser()
parser.add_argument('--rnn', type=str, required=True)
parser.add_argument('--xml', type=str, required=True)
- parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
+ parser.add_argument('--validate', default=False, action='store_true')
+ parser.add_argument('--no-validate', dest='validate',
action='store_false')
subparsers = parser.add_subparsers()
subparsers.required = True
Re: [PATCH] drm/msm: Fix gen_header.py for python earlier than v3.9
On 5/8/2024 2:17 AM, Jon Hunter wrote:
Building the kernel with python3 versions earlier than v3.9 fails with ...
Traceback (most recent call last):
File "drivers/gpu/drm/msm/registers/gen_header.py", line 970, in
main()
File "drivers/gpu/drm/msm/registers/gen_header.py", line 951, in main
parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
AttributeError: module 'argparse' has no attribute 'BooleanOptionalAction'
The argparse attribute 'BooleanOptionalAction' is only supported for
python v3.9 and later. Fix support for earlier python3 versions by
explicitly defining '--validate' and '--no-validate' arguments.
Signed-off-by: Jon Hunter
---
drivers/gpu/drm/msm/registers/gen_header.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Thanks for your patch, I had sent something similar y'day.
If you are alright with https://patchwork.freedesktop.org/patch/593057/,
we can use that one.
diff --git a/drivers/gpu/drm/msm/registers/gen_header.py
b/drivers/gpu/drm/msm/registers/gen_header.py
index fc3bfdc991d2..64f67d2e3f1c 100644
--- a/drivers/gpu/drm/msm/registers/gen_header.py
+++ b/drivers/gpu/drm/msm/registers/gen_header.py
@@ -948,7 +948,8 @@ def main():
parser = argparse.ArgumentParser()
parser.add_argument('--rnn', type=str, required=True)
parser.add_argument('--xml', type=str, required=True)
- parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
+ parser.add_argument('--validate', dest='validate', action='store_true')
+ parser.add_argument('--no-validate', dest='validate',
action='store_false')
subparsers = parser.add_subparsers()
subparsers.required = True
[PATCH] drm/msm: remove python 3.9 dependency for compiling msm
Since commit 5acf49119630 ("drm/msm: import gen_header.py script from Mesa"),
compilation is broken on machines having python versions older than 3.9
due to dependency on argparse.BooleanOptionalAction.
Switch to use simple bool for the validate flag to remove the dependency.
Fixes: 5acf49119630 ("drm/msm: import gen_header.py script from Mesa")
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/registers/gen_header.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/registers/gen_header.py
b/drivers/gpu/drm/msm/registers/gen_header.py
index fc3bfdc991d2..3926485bb197 100644
--- a/drivers/gpu/drm/msm/registers/gen_header.py
+++ b/drivers/gpu/drm/msm/registers/gen_header.py
@@ -538,7 +538,7 @@ class Parser(object):
self.variants.add(reg.domain)
def do_validate(self, schemafile):
- if self.validate == False:
+ if not self.validate:
return
try:
@@ -948,7 +948,8 @@ def main():
parser = argparse.ArgumentParser()
parser.add_argument('--rnn', type=str, required=True)
parser.add_argument('--xml', type=str, required=True)
- parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
+ parser.add_argument('--validate', default=False, action='store_true')
+ parser.add_argument('--no-validate', dest='validate',
action='store_false')
subparsers = parser.add_subparsers()
subparsers.required = True
--
2.44.0
Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema
On 5/3/2024 5:02 PM, Dmitry Baryshkov wrote: On Sat, 4 May 2024 at 01:38, Abhinav Kumar wrote: On 5/3/2024 1:20 PM, Dmitry Baryshkov wrote: On Fri, 3 May 2024 at 22:42, Abhinav Kumar wrote: On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote: In order to validate drm/msm register definition files against schema, reuse the nodebugfs build step. The validation entry is guarded by the EXPERT Kconfig option and we don't want to enable that option for all the builds. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/ci/build.sh | 3 +++ drivers/gpu/drm/ci/build.yml | 1 + 2 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh index 106f2d40d222..28a495c0c39c 100644 --- a/drivers/gpu/drm/ci/build.sh +++ b/drivers/gpu/drm/ci/build.sh @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply apt-get update apt-get install -y libssl-dev +# for msm header validation +apt-get install -y python3-lxml + if [[ "$KERNEL_ARCH" = "arm64" ]]; then GCC_ARCH="aarch64-linux-gnu" DEBIAN_ARCH="arm64" diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml index 17ab38304885..9c198239033d 100644 --- a/drivers/gpu/drm/ci/build.yml +++ b/drivers/gpu/drm/ci/build.yml @@ -106,6 +106,7 @@ build-nodebugfs:arm64: extends: .build:arm64 variables: DISABLE_KCONFIGS: "DEBUG_FS" +ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML" Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 device. Cant we make this build rule msm specific? No need to. We just need to validate the files at least once during the whole pipeline build. ah okay, today the arm64 config anyway sets all arm64 vendor drm configs to y. A couple of more questions: 1) Why is this enabled only for no-debugfs option? 2) Will there be any concerns from other vendors to enable CONFIG_EXPERT in their CI runs as the arm64 config is shared across all arm64 vendors. I don't get the second question. This option is only enabled for no-debugfs, which isn't used for execution. Ah I see, makes sense. I didn't want to add an extra build stage, just for the sake of validating regs against the schema, nor did I want EXPERT to find its way into the actual running kernels. This answered my second question actually. That basically I didnt also want EXPERT to find its way into actual running kernels. Hence, I am fine with this change now Reviewed-by: Abhinav Kumar But, I will wait to hear from helen, vignesh about what they think of this.
Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema
On 5/3/2024 1:20 PM, Dmitry Baryshkov wrote: On Fri, 3 May 2024 at 22:42, Abhinav Kumar wrote: On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote: In order to validate drm/msm register definition files against schema, reuse the nodebugfs build step. The validation entry is guarded by the EXPERT Kconfig option and we don't want to enable that option for all the builds. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/ci/build.sh | 3 +++ drivers/gpu/drm/ci/build.yml | 1 + 2 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh index 106f2d40d222..28a495c0c39c 100644 --- a/drivers/gpu/drm/ci/build.sh +++ b/drivers/gpu/drm/ci/build.sh @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply apt-get update apt-get install -y libssl-dev +# for msm header validation +apt-get install -y python3-lxml + if [[ "$KERNEL_ARCH" = "arm64" ]]; then GCC_ARCH="aarch64-linux-gnu" DEBIAN_ARCH="arm64" diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml index 17ab38304885..9c198239033d 100644 --- a/drivers/gpu/drm/ci/build.yml +++ b/drivers/gpu/drm/ci/build.yml @@ -106,6 +106,7 @@ build-nodebugfs:arm64: extends: .build:arm64 variables: DISABLE_KCONFIGS: "DEBUG_FS" +ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML" Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 device. Cant we make this build rule msm specific? No need to. We just need to validate the files at least once during the whole pipeline build. ah okay, today the arm64 config anyway sets all arm64 vendor drm configs to y. A couple of more questions: 1) Why is this enabled only for no-debugfs option? 2) Will there be any concerns from other vendors to enable CONFIG_EXPERT in their CI runs as the arm64 config is shared across all arm64 vendors.
Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema
On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote: In order to validate drm/msm register definition files against schema, reuse the nodebugfs build step. The validation entry is guarded by the EXPERT Kconfig option and we don't want to enable that option for all the builds. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/ci/build.sh | 3 +++ drivers/gpu/drm/ci/build.yml | 1 + 2 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh index 106f2d40d222..28a495c0c39c 100644 --- a/drivers/gpu/drm/ci/build.sh +++ b/drivers/gpu/drm/ci/build.sh @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply apt-get update apt-get install -y libssl-dev +# for msm header validation +apt-get install -y python3-lxml + if [[ "$KERNEL_ARCH" = "arm64" ]]; then GCC_ARCH="aarch64-linux-gnu" DEBIAN_ARCH="arm64" diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml index 17ab38304885..9c198239033d 100644 --- a/drivers/gpu/drm/ci/build.yml +++ b/drivers/gpu/drm/ci/build.yml @@ -106,6 +106,7 @@ build-nodebugfs:arm64: extends: .build:arm64 variables: DISABLE_KCONFIGS: "DEBUG_FS" +ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML" Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 device. Cant we make this build rule msm specific?
Re: [PATCH v2 1/2] drm/msm/gen_header: allow skipping the validation
On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote: We don't need to run the validation of the XML files if we are just compiling the kernel. Skip the validation unless the user enables corresponding Kconfig option. This removes a warning from gen_header.py about lxml being not installed. Reported-by: Stephen Rothwell Closes: https://lore.kernel.org/all/20240409120108.2303d...@canb.auug.org.au/ Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/Kconfig | 8 drivers/gpu/drm/msm/Makefile| 9 - drivers/gpu/drm/msm/registers/gen_header.py | 14 +++--- 3 files changed, 27 insertions(+), 4 deletions(-) Looks reasonable to me, only developers need to worry about or fix the xml files Reviewed-by: Abhinav Kumar
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] phy: qcom: qmp-combo: Fix register base for QSERDES_DP_PHY_MODE
On 4/4/2024 5:01 PM, Stephen Boyd wrote:
The register base that was used to write to the QSERDES_DP_PHY_MODE
register was 'dp_dp_phy' before commit 815891eee668 ("phy:
qcom-qmp-combo: Introduce orientation variable"). There isn't any
explanation in the commit why this is changed, so I suspect it was an
oversight or happened while being extracted from some other series.
Oddly the value being 0x4c or 0x5c doesn't seem to matter for me, so I
suspect this is dead code, but that can be fixed in another patch. It's
not good to write to the wrong register space, and maybe some other
version of this phy relies on this.
Cc: Douglas Anderson
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Neil Armstrong
Cc: Abel Vesa
Cc: Steev Klimaszewski
Cc: Johan Hovold
Cc: Bjorn Andersson
Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
Signed-off-by: Stephen Boyd
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Yes I dont know why the commit 815891eee668 ("phy:
qcom-qmp-combo: Introduce orientation variable") changed the base in
below code. Certainly looks like a bug to me because we should be
writing to DP_PHY_MODE which is at an offset 0x1c from the dp_phy base.
Hence, this LGTM,
Reviewed-by: Abhinav Kumar
Re: [PATCH] phy: qcom: qmp-combo: Fix VCO div offset on v3
On 4/4/2024 4:43 PM, Stephen Boyd wrote:
Commit ec17373aebd0 ("phy: qcom: qmp-combo: extract common function to
setup clocks") changed the offset that is used to write to
DP_PHY_VCO_DIV from QSERDES_V3_DP_PHY_VCO_DIV to
QSERDES_V4_DP_PHY_VCO_DIV. Unfortunately, this offset is different
between v3 and v4 phys:
#define QSERDES_V3_DP_PHY_VCO_DIV 0x064
#define QSERDES_V4_DP_PHY_VCO_DIV 0x070
meaning that we write the wrong register on v3 phys now. Add another
generic register to 'regs' and use it here instead of a version specific
define to fix this.
This was discovered after Abhinav looked over register dumps with me
from sc7180 Trogdor devices that started failing to light up the
external display with v6.6 based kernels. It turns out that some
monitors are very specific about their link clk frequency and if the
default power on reset value is still there the monitor will show a
blank screen or a garbled display. Other monitors are perfectly happy to
get a bad clock signal.
Cc: Douglas Anderson
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Fixes: ec17373aebd0 ("phy: qcom: qmp-combo: extract common function to setup
clocks")
Signed-off-by: Stephen Boyd
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
I cross-checked the foll chipsets which use qmp_v3_usb3phy_regs_layout:
-> sdm845
-> sc7180
-> sm6350
All of them have VCO_DIV at offset 0x64.
And, I cross-checked the foll chipsets which use
qmp_v45_usb3phy_regs_layout:
-> sc8180x
-> x1e80100
-> sm8250
-> sm8350
All of them have VCO_DIV at offset 0x70.
Now, thing look in order to me.
Hence,
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
