Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
On 02/07/2024 14:43, Maxime Ripard wrote: Hi Tomi, On Wed, Jun 26, 2024 at 06:53:40PM GMT, Tomi Valkeinen wrote: On 26/06/2024 18:07, Maxime Ripard wrote: On Wed, Jun 26, 2024 at 12:55:39PM GMT, Tomi Valkeinen wrote: On 26/06/2024 11:49, Maxime Ripard wrote: Hi, On Wed, Jun 19, 2024 at 12:07:48PM GMT, Tomi Valkeinen wrote: From: Tomi Valkeinen When a bridge driver uses devm_mipi_dsi_device_register_full() or devm_mipi_dsi_attach(), the resource management is moved to devres, which releases the resource automatically when the bridge driver is unbound. However, if the DSI host goes away first, the host unregistration code will automatically detach and unregister any DSI peripherals, without notifying the devres about it. So when the bridge driver later is unbound, the resources are released a second time, leading to crash. That's super surprising. mipi_dsi_device_unregister calls device_unregister, which calls device_del, which in turn calls devres_release_all. Hmm, right. If that doesn't work like that, then it's what needs to be fixed, and not worked around in the MIPI-DSI bus. Well, something causes a crash for both the device register/unregister case and the attach/detach case, and the call stacks and debug prints showed a double unregister/detach... I need to dig up the board and check again why the devres_release_all() in device_del() doesn't solve this. But I can probably only get back to this in August, so it's perhaps best to ignore this patch for now. However, the attach/detach case is still valid? I see no devres calls in the detach paths. I'm not sure what you mean by the attach/detach case. Do you expect device resources allocated in attach to be freed when detach run? Ah, never mind, the devres_release_all() would of course deal with that too. However, I just realized/remembered why it crashes. devm_mipi_dsi_device_register_full() and devm_mipi_dsi_attach() are given a device which is used for the devres. This device is probably always the bridge device. So when the bridge device goes away, so do those resources. The mipi_dsi_device_unregister() call deals with a DSI device, which was created in devm_mipi_dsi_device_register_full(). Unregistering that DSI device, which does happen when the DSI host is removed, does not affect the devres of the bridge. So, unloading the DSI host driver causes mipi_dsi_device_unregister() and mipi_dsi_detach() to be called (as part of mipi_dsi_host_unregister()), and unloading the bridge driver causes them to be called again via devres. Sorry, that's one of the things I don't quite get. Both functions are exclusively(?) called from I2C bridges, so the device passed there should be a i2c_client instance, and thus the MIPI-DSI host going away will not remove those i2c devices, only the MIPI-DSI ones, right? Yes. So if we remove the host, the MIPI-DSI device will be detached and removed through the path you were explaing with the i2c client lingering around. And if we remove the I2C device, then devm will kick in and will detach and remove the MIPI-DSI device. Right. Or is it the other way around? That if you remove the host, the device is properly detached and removed, but there's still the devm actions lingering around in the i2c device with pointers to the mipi_dsi_device that was first created, but since destroyed? And thus, if the i2c device ever goes away, we get a use-after-free? Hmm, I'm not sure I understand what you mean here... Aren't you describing the same thing in both of these cases? In any case, to expand the description a bit, module unloading is quite fragile. I do get a crash if I first unload the i2c bridge module, and only then go and unload the other ones in the DRM pipeline. But I think module unloading will very easily crash, whatever the DRM drivers being used are, so it's not related to this particular issue. In my view, the unload sequence that should be supported (for development purposes, not for production) is to start the unload from the display controller module, which tears down the DRM pipeline, and going from there towards the panels/connectors. Of course, it would be very nice if the module unloading worked perfectly, but afaics fixing all that's related to module unloading would be a multi-year project... So, I just want to keep the sequence I described above working, which allows using modules while doing driver development. Tomi
Re: [PATCH 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter
On 14/05/2024 08:08, Aradhya Bhatia wrote:
Hi Rob,
Thank you for reviewing the patches!
On 14/05/24 01:00, Rob Herring wrote:
On Mon, May 13, 2024 at 02:07:44PM +0530, Aradhya Bhatia wrote:
Hi Laurent,
Thank you for reviewing the patches!
On 13-May-24 01:04, Laurent Pinchart wrote:
Hi Aradhya,
Thank you for the patch.
On Sun, May 12, 2024 at 01:00:53AM +0530, Aradhya Bhatia wrote:
Add devicetree binding schema for AM625 OLDI Transmitters.
Signed-off-by: Aradhya Bhatia
---
.../bindings/display/ti/ti,am625-oldi.yaml| 153 ++
MAINTAINERS | 1 +
2 files changed, 154 insertions(+)
create mode 100644
Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
diff --git a/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
new file mode 100644
index ..0a96e600bc0b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
@@ -0,0 +1,153 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/ti/ti,am625-oldi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments AM625 OLDI Transmitter
+
+maintainers:
+ - Tomi Valkeinen
+ - Aradhya Bhatia
+
+description: |
+ The AM625 TI Keystone OpenLDI transmitter (OLDI TX) supports serialized RGB
+ pixel data transmission between host and flat panel display over LVDS (Low
+ Voltage Differential Sampling) interface. The OLDI TX consists of 7-to-1 data
+ serializers, and 4-data and 1-clock LVDS outputs. It supports the LVDS output
+ formats "jeida-18", "jeida-24" and "vesa-18", and can accept 24-bit RGB or
+ padded and un-padded 18-bit RGB bus formats as input.
+
+properties:
+ reg:
+maxItems: 1
+
+ clocks:
+maxItems: 1
+description: serial clock input for the OLDI transmitters
+
+ clock-names:
+const: s_clk
+
+ ti,companion-oldi:
+$ref: /schemas/types.yaml#/definitions/phandle
+description:
+ phandle to companion OLDI transmitter. This property is mandatory for the
+ primarty OLDI TX if the OLDI TXes are expected to work either in
dual-lvds
+ mode or in clone mode. This property should point to the secondary OLDI
+ TX.
+
+ ti,secondary-oldi:
+type: boolean
+description: Boolean property to mark an OLDI TX as secondary node.
+
+ ti,oldi-io-ctrl:
+$ref: /schemas/types.yaml#/definitions/phandle
+description:
+ phandle to syscon device node mapping OLDI IO_CTRL registers found in the
+ control MMR region. This property is needed for OLDI interface to work.
+
+ ports:
+$ref: /schemas/graph.yaml#/properties/ports
+
+properties:
+ port@0:
+$ref: /schemas/graph.yaml#/properties/port
+description: Parallel RGB input port
+
+ port@1:
+$ref: /schemas/graph.yaml#/properties/port
+description: LVDS output port
+
+required:
+ - port@0
+ - port@1
+
+allOf:
+ - if:
+ properties:
+ti,secondary-oldi: true
+then:
+ properties:
+ti,companion-oldi: false
+ti,oldi-io-ctrl: false
+clocks: false
+clock-names: false
+
+else:
+ required:
+- ti,oldi-io-ctrl
+- clocks
+- clock-names
+
+required:
+ - reg
+ - ports
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+
+oldi_txes {
+#address-cells = <1>;
+#size-cells = <0>;
+oldi: oldi@0 {
+reg = <0>;
+clocks = <_clks 186 0>;
+clock-names = "s_clk";
+ti,oldi-io-ctrl = <_oldi_io_ctrl>;
What bus does this device live on ? Couldn't the I/O register space be
referenced by the reg property ?.
These registers are a part of the system-controller register space
(ctrl_mmr0). The whole register set is owned by the main_conf[0]
devicetree node, with sub-nodes pointing to specific regions. That's why
I cannot reference these registers directly.
Then what does 'reg' represent? Looks like you just made up an index. If
so, then this should probably be a child of _oldi_io_ctrl instead.
Or it should just be merged into that node.
I did make up an index when I used the 'reg' property. Similar to how
ports can be indexed. The AM65 has 1 OLDI TX. AM62 and AM62P have 2 OLDI
TXes each. The index is to help the driver parse through each of them.
If I push these OLDI TX nodes as child nodes under _oldi_io_ctrl,
then that would be an inaccurate representation of the hardware.
The OLDI TXes are very well a part of the DSS hardware. As such, the
(three) registers that control the functionality have been made a part
of the DSS video-port (VP) register space, leaving OLDI TXes with no
direct access to the primary bus (cbass_main) where the DSS sits.
The IO control registers, on the o
Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup
On 11/05/2024 22:30, Aradhya Bhatia wrote:
Reduce tab size from 8 spaces to 4 spaces to make the bindings
consistent, and easy to expand.
Signed-off-by: Aradhya Bhatia
---
.../bindings/display/ti/ti,am65x-dss.yaml | 54 +--
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 55e3e490d0e6..399d68986326 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -142,32 +142,32 @@ examples:
#include
dss: dss@4a0 {
-compatible = "ti,am65x-dss";
-reg = <0x04a0 0x1000>, /* common */
-<0x04a02000 0x1000>, /* vidl1 */
-<0x04a06000 0x1000>, /* vid */
-<0x04a07000 0x1000>, /* ovr1 */
-<0x04a08000 0x1000>, /* ovr2 */
-<0x04a0a000 0x1000>, /* vp1 */
-<0x04a0b000 0x1000>, /* vp2 */
-<0x04a01000 0x1000>; /* common1 */
-reg-names = "common", "vidl1", "vid",
-"ovr1", "ovr2", "vp1", "vp2", "common1";
-ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>;
-power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>;
-clocks =<_clks 67 1>,
-<_clks 216 1>,
-<_clks 67 2>;
-clock-names = "fck", "vp1", "vp2";
-interrupts = ;
-ports {
-#address-cells = <1>;
-#size-cells = <0>;
-port@0 {
-reg = <0>;
-oldi_out0: endpoint {
-remote-endpoint = <_in0>;
-};
-};
+compatible = "ti,am65x-dss";
+reg = <0x04a0 0x1000>, /* common */
+ <0x04a02000 0x1000>, /* vidl1 */
+ <0x04a06000 0x1000>, /* vid */
+ <0x04a07000 0x1000>, /* ovr1 */
+ <0x04a08000 0x1000>, /* ovr2 */
+ <0x04a0a000 0x1000>, /* vp1 */
+ <0x04a0b000 0x1000>, /* vp2 */
+ <0x04a01000 0x1000>; /* common1 */
+reg-names = "common", "vidl1", "vid",
+"ovr1", "ovr2", "vp1", "vp2", "common1";
+ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>;
+power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>;
+clocks =<_clks 67 1>,
+<_clks 216 1>,
+ <_clks 67 2>;
+clock-names = "fck", "vp1", "vp2";
+interrupts = ;
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+port@0 {
+reg = <0>;
+oldi_out0: endpoint {
+remote-endpoint = <_in0>;
+};
};
+};
};
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH 4/4] drm/tidss: Add OLDI bridge support
On 11/05/2024 22:30, Aradhya Bhatia wrote:
Up till now, the OLDI support in tidss was integrated within the tidss dispc.
This was fine till the OLDI was one-to-mapped with the DSS video-port (VP).
The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone
modes.
Add OLDI TXes as separate DRM bridge entities to better support the new LVDS
configurations.
Signed-off-by: Aradhya Bhatia
---
Note:
The OLDI configuration should happen before the video-port configuration takes
place in tidss_crtc_atomic_enable hook. I have posted a patch allowing DRM
bridges to get enabled before the CRTC of that bridge is enabled[0]. This patch
uses the bridge hooks introduced in [0], and hence will not compile without [0].
[0]: Dependency Patch: Introduce early_enable / late_disable drm bridge APIs
https://lore.kernel.org/all/20240511153051.1355825-7-a-bhat...@ti.com/
---
drivers/gpu/drm/tidss/Makefile | 3 +-
drivers/gpu/drm/tidss/tidss_dispc.c | 11 +-
drivers/gpu/drm/tidss/tidss_dispc.h | 4 +
drivers/gpu/drm/tidss/tidss_drv.c | 13 +-
drivers/gpu/drm/tidss/tidss_drv.h | 4 +
drivers/gpu/drm/tidss/tidss_oldi.c | 568
drivers/gpu/drm/tidss/tidss_oldi.h | 73
7 files changed, 673 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c
create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h
diff --git a/drivers/gpu/drm/tidss/Makefile b/drivers/gpu/drm/tidss/Makefile
index 312645271014..b6d6becf1683 100644
--- a/drivers/gpu/drm/tidss/Makefile
+++ b/drivers/gpu/drm/tidss/Makefile
@@ -7,6 +7,7 @@ tidss-y := tidss_crtc.o \
tidss_irq.o \
tidss_plane.o \
tidss_scale_coefs.o \
- tidss_dispc.o
+ tidss_dispc.o \
+ tidss_oldi.o
obj-$(CONFIG_DRM_TIDSS) += tidss.o
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
b/drivers/gpu/drm/tidss/tidss_dispc.c
index 1ad711f8d2a8..4961da3989c0 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -466,6 +466,16 @@ static u32 dispc_vp_read(struct dispc_device *dispc, u32
hw_videoport, u16 reg)
return ioread32(base + reg);
}
+u32 tidss_get_status(struct tidss_device *tidss)
+{
+ return dispc_read(tidss->dispc, DSS_SYSSTATUS);
+}
+
+void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport, u32
val)
+{
+ return dispc_vp_write(tidss->dispc, hw_videoport,
DISPC_VP_DSS_OLDI_CFG, val);
+}
+
/*
* TRM gives bitfields as start:end, where start is the higher bit
* number. For example 7:0
@@ -1310,7 +1320,6 @@ void dispc_vp_disable_clk(struct dispc_device *dispc, u32
hw_videoport)
* Calculate the percentage difference between the requested pixel clock rate
* and the effective rate resulting from calculating the clock divider value.
*/
-static
unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate)
{
int r = rate / 100, rr = real_rate / 100;
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h
b/drivers/gpu/drm/tidss/tidss_dispc.h
index 086327d51a90..800a73457aff 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -94,6 +94,10 @@ extern const struct dispc_features dispc_am62a7_feats;
extern const struct dispc_features dispc_am65x_feats;
extern const struct dispc_features dispc_j721e_feats;
+u32 tidss_get_status(struct tidss_device *tidss);
+void tidss_configure_oldi(struct tidss_device *tidss, u32 hw_videoport, u32
val);
+unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate);
+
void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask);
dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index d15f836dca95..fd90e8498cc2 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -23,6 +23,7 @@
#include "tidss_drv.h"
#include "tidss_kms.h"
#include "tidss_irq.h"
+#include "tidss_oldi.h"
/* Power management */
@@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device *pdev)
spin_lock_init(>wait_lock);
+ ret = tidss_oldi_init(tidss);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to init OLDI (%d)\n", ret);
+ return ret;
+ }
+
ret = dispc_init(tidss);
if (ret) {
dev_err(dev, "failed to initialize dispc: %d\n", ret);
- return ret;
+ goto err_oldi_deinit;
}
pm_runtime_enable(dev);
@@ -202,6 +210,9 @@ static int tidss_probe(struct platform_device *pdev)
pm_runtime_dont_use_autosuspend(dev);
pm_runtime_disable(dev);
+err_oldi_deinit:
+ tidss_oldi_deinit(tidss);
Shouldn't this be called tidss_remove()?
+
return ret;
}
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h
Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
On 26/06/2024 18:07, Maxime Ripard wrote: On Wed, Jun 26, 2024 at 12:55:39PM GMT, Tomi Valkeinen wrote: On 26/06/2024 11:49, Maxime Ripard wrote: Hi, On Wed, Jun 19, 2024 at 12:07:48PM GMT, Tomi Valkeinen wrote: From: Tomi Valkeinen When a bridge driver uses devm_mipi_dsi_device_register_full() or devm_mipi_dsi_attach(), the resource management is moved to devres, which releases the resource automatically when the bridge driver is unbound. However, if the DSI host goes away first, the host unregistration code will automatically detach and unregister any DSI peripherals, without notifying the devres about it. So when the bridge driver later is unbound, the resources are released a second time, leading to crash. That's super surprising. mipi_dsi_device_unregister calls device_unregister, which calls device_del, which in turn calls devres_release_all. Hmm, right. If that doesn't work like that, then it's what needs to be fixed, and not worked around in the MIPI-DSI bus. Well, something causes a crash for both the device register/unregister case and the attach/detach case, and the call stacks and debug prints showed a double unregister/detach... I need to dig up the board and check again why the devres_release_all() in device_del() doesn't solve this. But I can probably only get back to this in August, so it's perhaps best to ignore this patch for now. However, the attach/detach case is still valid? I see no devres calls in the detach paths. I'm not sure what you mean by the attach/detach case. Do you expect device resources allocated in attach to be freed when detach run? Ah, never mind, the devres_release_all() would of course deal with that too. However, I just realized/remembered why it crashes. devm_mipi_dsi_device_register_full() and devm_mipi_dsi_attach() are given a device which is used for the devres. This device is probably always the bridge device. So when the bridge device goes away, so do those resources. The mipi_dsi_device_unregister() call deals with a DSI device, which was created in devm_mipi_dsi_device_register_full(). Unregistering that DSI device, which does happen when the DSI host is removed, does not affect the devres of the bridge. So, unloading the DSI host driver causes mipi_dsi_device_unregister() and mipi_dsi_detach() to be called (as part of mipi_dsi_host_unregister()), and unloading the bridge driver causes them to be called again via devres. Tomi
Re: [PATCH v4 11/11] drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable
On 22/06/2024 14:09, Aradhya Bhatia wrote:
The cdns-dsi controller requires that it be turned on completely before
the input DPI's source has begun streaming[0]. Not having that, allows
for a small window before cdns-dsi enable and after cdns-dsi disable
where the previous entity (in this case tidss's videoport) to continue
streaming DPI video signals. This small window where cdns-dsi is
disabled but is still receiving signals causes the input FIFO of
cdns-dsi to get corrupted. This causes the colors to shift on the output
display. The colors can either shift by one color component (R->G, G->B,
B->R), or by two color components (R->B, G->R, B->G).
Since tidss's videoport starts streaming via crtc enable hooks, we need
cdns-dsi to be up and running before that. Now that the bridges are
pre_enabled before crtc is enabled, and post_disabled after crtc is
disabled, use the pre_enable and post_disable hooks to get cdns-dsi
ready and running before the tidss videoport to get pass the color shift
issues.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
I think it makes sense to explain a bit about this in a comment in the
driver code. Otherwise doing all of this in pre_enable and post_disable
looks a bit odd.
Reviewed-by: Tomi Valkeinen
Tomi
Signed-off-by: Aradhya Bhatia
---
.../gpu/drm/bridge/cadence/cdns-dsi-core.c| 32 +++
1 file changed, 4 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index c9697818308e..c352ea7db4ed 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -655,8 +655,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
}
-static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
- struct drm_bridge_state
*old_bridge_state)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -680,15 +680,6 @@ static void cdns_dsi_bridge_atomic_disable(struct
drm_bridge *bridge,
pm_runtime_put(dsi->base.dev);
}
-static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
- struct drm_bridge_state
*old_bridge_state)
-{
- struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
- struct cdns_dsi *dsi = input_to_dsi(input);
-
- pm_runtime_put(dsi->base.dev);
-}
-
static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
{
struct cdns_dsi_output *output = >output;
@@ -757,8 +748,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
dsi->link_initialized = true;
}
-static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
- struct drm_bridge_state
*old_bridge_state)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -909,19 +900,6 @@ static void cdns_dsi_bridge_atomic_enable(struct
drm_bridge *bridge,
writel(tmp, dsi->regs + MCTL_MAIN_EN);
}
-static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
- struct drm_bridge_state
*old_bridge_state)
-{
- struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
- struct cdns_dsi *dsi = input_to_dsi(input);
-
- if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
- return;
-
- cdns_dsi_init_link(dsi);
- cdns_dsi_hs_init(dsi);
-}
-
static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
struct drm_bridge_state
*bridge_state,
struct drm_crtc_state
*crtc_state,
@@ -952,9 +930,7 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct
drm_bridge *bridge,
static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
.attach = cdns_dsi_bridge_attach,
.mode_valid = cdns_dsi_bridge_mode_valid,
- .atomic_disable = cdns_dsi_bridge_atomic_disable,
.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
- .atomic_enable = cdns_dsi_bridge_atomic_enable,
.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Move the bridge pre_enable call before crtc enable, and the bridge
post_disable call after the crtc disable.
The sequence of enable after this patch will look like:
bridge[n]_pre_enable
...
bridge[1]_pre_enable
crtc_enable
encoder_enable
bridge[1]_enable
...
bridge[n]__enable
and vice-versa for the bridge chain disable sequence.
The definition of bridge pre_enable hook says that,
"The display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called".
Since CRTC is also a source feeding the bridge, it should not be enabled
before the bridges in the pipeline are pre_enabled. Fix that by
re-ordering the sequence of bridge pre_enable and bridge post_disable.
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/drm_atomic_helper.c | 165 ++--
include/drm/drm_atomic_helper.h | 7 ++
2 files changed, 114 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index fb97b51b38f1..e8ad08634f58 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -74,6 +74,7 @@
* also shares the drm_plane_helper_funcs function table with the
plane
* helpers.
*/
+
Extra change.
static void
drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
struct drm_plane_state *old_plane_state,
@@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
}
static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+disable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state
*old_state,
+ enum bridge_chain_operation_type op_type)
{
struct drm_connector *connector;
struct drm_connector_state *old_conn_state, *new_conn_state;
- struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
int i;
@@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
if (WARN_ON(!encoder))
continue;
- funcs = encoder->helper_private;
-
- drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
- encoder->base.id, encoder->name);
-
/*
* Each encoder has at most one connector (since we always steal
* it away), so we won't call disable hooks twice.
*/
bridge = drm_bridge_chain_get_first_bridge(encoder);
- drm_atomic_bridge_chain_disable(bridge, old_state);
- /* Right function depends upon target state. */
- if (funcs) {
- if (funcs->atomic_disable)
- funcs->atomic_disable(encoder, old_state);
- else if (new_conn_state->crtc && funcs->prepare)
- funcs->prepare(encoder);
- else if (funcs->disable)
- funcs->disable(encoder);
- else if (funcs->dpms)
- funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
- }
+ switch (op_type) {
+ case DRM_ENCODER_BRIDGE_DISABLE:
+ funcs = encoder->helper_private;
+
+ drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
+ encoder->base.id, encoder->name);
+
+ drm_atomic_bridge_chain_disable(bridge, old_state);
+
+ /* Right function depends upon target state. */
+ if (funcs) {
+ if (funcs->atomic_disable)
+ funcs->atomic_disable(encoder,
old_state);
+ else if (new_conn_state->crtc && funcs->prepare)
+ funcs->prepare(encoder);
+ else if (funcs->disable)
+ funcs->disable(encoder);
+ else if (funcs->dpms)
+ funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+ }
+
+ break;
+
+ case DRM_BRIDGE_POST_DISABLE:
+ drm_atomic_bridge_chain_post_disable(bridge, old_state);
- drm_atomic_bridge_chain_post_disable(bridge, old_state);
+ break;
+
+ default:
+ drm_err(dev, "Unrecognized Encoder/Bridge Operation
(%d).\n", op_type);
+ break;
+ }
}
+}
+
+static void
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+ struct drm_crtc *crtc;
+ struct
Re: [PATCH v4 09/11] drm/bridge: cdns-dsi: Support atomic bridge APIs
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Change the existing (and deprecated) bridge hooks, to the bridge
atomic APIs.
Add drm helpers for duplicate_state, destroy_state, and bridge_reset
bridge hooks.
Further add support for the input format negotiation hook.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Aradhya Bhatia
Reviewed-by: Tomi Valkeinen
Tomi
---
.../gpu/drm/bridge/cadence/cdns-dsi-core.c| 51 ---
1 file changed, 43 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index cad0c1478ef0..c9697818308e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -655,7 +655,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
}
-static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -679,7 +680,8 @@ static void cdns_dsi_bridge_disable(struct drm_bridge
*bridge)
pm_runtime_put(dsi->base.dev);
}
-static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -755,7 +757,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
dsi->link_initialized = true;
}
-static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -906,7 +909,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge
*bridge)
writel(tmp, dsi->regs + MCTL_MAIN_EN);
}
-static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -918,13 +922,44 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge
*bridge)
cdns_dsi_hs_init(dsi);
}
+static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
+ struct drm_bridge_state
*bridge_state,
+ struct drm_crtc_state
*crtc_state,
+ struct drm_connector_state
*conn_state,
+ u32 output_fmt,
+ unsigned int *num_input_fmts)
+{
+ struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+ struct cdns_dsi *dsi = input_to_dsi(input);
+ struct cdns_dsi_output *output = >output;
+ u32 *input_fmts;
+
+ *num_input_fmts = 0;
+
+ input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+ if (!input_fmts)
+ return NULL;
+
+ input_fmts[0] = drm_mipi_dsi_get_input_bus_fmt(output->dev->format);
+ if (!input_fmts[0])
+ return NULL;
+
+ *num_input_fmts = 1;
+
+ return input_fmts;
+}
+
static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
.attach = cdns_dsi_bridge_attach,
.mode_valid = cdns_dsi_bridge_mode_valid,
- .disable = cdns_dsi_bridge_disable,
- .pre_enable = cdns_dsi_bridge_pre_enable,
- .enable = cdns_dsi_bridge_enable,
- .post_disable = cdns_dsi_bridge_post_disable,
+ .atomic_disable = cdns_dsi_bridge_atomic_disable,
+ .atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
+ .atomic_enable = cdns_dsi_bridge_atomic_enable,
+ .atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
};
static int cdns_dsi_attach(struct mipi_dsi_host *host,
Re: [PATCH v4 08/11] drm/mipi-dsi: Add helper to find input format
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Add a helper API that can be used by the DSI hosts to find the required
input bus format for the given output dsi pixel format.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/drm_mipi_dsi.c | 37 ++
include/drm/drm_mipi_dsi.h | 1 +
2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index a471c46f5ca6..937aa16dfcf6 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -36,6 +36,8 @@
#include
#include
+#include
+
#include
/**
@@ -866,6 +868,41 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi,
const void *params,
}
EXPORT_SYMBOL(mipi_dsi_generic_read);
+/**
+ * drm_mipi_dsi_get_input_bus_fmt() - Get the required MEDIA_BUS_FMT_* based
+ * input pixel format for a given DSI output
+ * pixel format
+ * @dsi_format: pixel format that a DSI host needs to output
+ *
+ * Various DSI hosts can use this function during their
+ * _bridge_funcs.atomic_get_input_bus_fmts operation to ascertain
+ * the MEDIA_BUS_FMT_* pixel format required as input.
+ *
+ * RETURNS:
+ * a 32-bit MEDIA_BUS_FMT_* value on success or 0 in case of failure.
+ */
+u32 drm_mipi_dsi_get_input_bus_fmt(enum mipi_dsi_pixel_format dsi_format)
+{
+ switch (dsi_format) {
+ case MIPI_DSI_FMT_RGB888:
+ return MEDIA_BUS_FMT_RGB888_1X24;
+
+ case MIPI_DSI_FMT_RGB666:
+ return MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+
+ case MIPI_DSI_FMT_RGB666_PACKED:
+ return MEDIA_BUS_FMT_RGB666_1X18;
+
+ case MIPI_DSI_FMT_RGB565:
+ return MEDIA_BUS_FMT_RGB565_1X16;
+
+ default:
+ /* Unsupported DSI Format */
+ return 0;
+ }
+}
+EXPORT_SYMBOL(drm_mipi_dsi_get_input_bus_fmt);
+
/**
* mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
* @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 71d121aeef24..78a2c7d9eefb 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -290,6 +290,7 @@ void mipi_dsi_generic_write_multi(struct
mipi_dsi_multi_context *ctx,
const void *payload, size_t size);
ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
size_t num_params, void *data, size_t size);
+u32 drm_mipi_dsi_get_input_bus_fmt(enum mipi_dsi_pixel_format dsi_format);
#define mipi_dsi_msleep(ctx, delay) \
do {\
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v4 07/11] drm/bridge: cdns-dsi: Reset the DCS write FIFO
On 22/06/2024 14:09, Aradhya Bhatia wrote: If any normal DCS write command has already been transmitted prior to transmitting any Zero-Parameter DCS command, then it is necessary to clear the TX FIFO by resetting it. Otherwise, the FIFO points to another location, and the DCS command transmits unnecessary data causing the panel to not work[0]. Allow the DCS Write FIFO in the cdns-dsi controller to reset as a rule, before any DCS packet is transmitted to the DSI peripheral. [0]: Section 12.6.5.7.5.2: "Command Mode Settings" in TDA4VM Technical Reference Manual: https://www.ti.com/lit/zip/spruil1 Hmm so if I read the doc right, it says: if sending zero-parameter dcs command, clear the FIFO and write zero to direct_cmd_wrdat. Your patch seems to always clear the FIFO, not only for zero-parameter commands. Is that a problem (I don't think so, but...)? Also, is the direct_cmd_wrdat written at all when sending zero-parameter dcs command? Tomi Signed-off-by: Aradhya Bhatia --- drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c index 126e4bccd868..cad0c1478ef0 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c @@ -1018,6 +1018,9 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host, cdns_dsi_init_link(dsi); + /* Reset the DCS Write FIFO */ + writel(0x00, dsi->regs + DIRECT_CMD_FIFO_RST); + ret = mipi_dsi_create_packet(, msg); if (ret) goto out;
Re: [PATCH v4 06/11] drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Once the DSI Link and DSI Phy are initialized, the code needs to wait
for Clk and Data Lanes to be ready, before continuing configuration.
This is in accordance with the DSI Start-up procedure, found in the
Technical Reference Manual of Texas Instrument's J721E SoC[0] which
houses this DSI TX controller.
If the previous bridge (or crtc/encoder) are configured pre-maturely,
the input signal FIFO gets corrupt. This introduces a color-shift on the
display.
Allow the driver to wait for the clk and data lanes to get ready during
DSI enable.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Tested-by: Dominik Haller
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 426f77092341..126e4bccd868 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -764,7 +764,7 @@ static void cdns_dsi_bridge_enable(struct drm_bridge
*bridge)
struct phy_configure_opts_mipi_dphy *phy_cfg =
>phy_opts.mipi_dphy;
unsigned long tx_byte_period;
struct cdns_dsi_cfg dsi_cfg;
- u32 tmp, reg_wakeup, div;
+ u32 tmp, reg_wakeup, div, status;
int nlanes;
if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
@@ -781,6 +781,17 @@ static void cdns_dsi_bridge_enable(struct drm_bridge
*bridge)
cdns_dsi_init_link(dsi);
cdns_dsi_hs_init(dsi);
+ /*
+* Now that the DSI Link and DSI Phy are initialized,
+* wait for the CLK and Data Lanes to be ready.
+*/
+ tmp = CLK_LANE_RDY;
+ for (int i = 0; i < nlanes; i++)
+ tmp |= DATA_LANE_RDY(i);
+
+ WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
+ status & tmp, 100, 0));
I think an error print is more suitable than WARN_ON_ONCE(). Other than
that:
Reviewed-by: Tomi Valkeinen
Tomi
+
writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
dsi->regs + VID_HSIZE1);
writel(HFP_LEN(dsi_cfg.hfp) | HACT_LEN(dsi_cfg.hact),
Re: [PATCH v4 05/11] drm/bridge: cdns-dsi: Fix the clock variable for mode_valid()
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Allow the D-Phy config checks to use mode->clock instead of
mode->crtc_clock during mode_valid checks, like everywhere else in the
driver.
Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 03a5af52ec0b..426f77092341 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
if (ret)
return ret;
- phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
+ phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock :
mode->crtc_clock) * 1000,
mipi_dsi_pixel_format_to_bpp(output->dev->format),
nlanes, phy_cfg);
I think this is fine as a fix.
Reviewed-by: Tomi Valkeinen
However... The code looks a bit messy. Maybe the first one is something
that could be addressed in this series.
- Return value of phy_mipi_dphy_get_default_config() is not checked
- Using the non-crtc and crtc versions of the timings this way looks
bad, but that's not a problem of the driver. It would be better to have
a struct that contains the timings, and struct drm_display_mode would
contain two instances of that struct. The driver code could then just
pick the correct instance, instead of making the choice for each and
every field. This would be an interesting coccinelle project ;)
- Calling cdns_dsi_check_conf() in cdns_dsi_bridge_enable() is odd.
Everything should already have been checked. In fact, at the check phase
the resulting config values could have been stored somewhere, so that
they're ready for use by cdns_dsi_bridge_enable(). But this rises the
question if the non-crtc and crtc timings can actually be different, and
if they are... doesn't it break everything if at the check phase we use
the non-crtc ones, but at enable phase we use crtc ones?
Ah, I see, this is with non-atomic. Maybe after you switch to atomic
callbacks, atomic_check could be used so that there's no need for the
WARN_ON_ONCE() in enable callback.
Tomi
Re: [PATCH v4 04/11] drm/bridge: cdns-dsi: Fix the link and phy init order
On 22/06/2024 14:09, Aradhya Bhatia wrote:
The order of init of DSI link and DSI phy is wrong. The DSI link needs
to be configured before the DSI phy is getting configured. Otherwise,
the D-Phy is unable to lock in on the incoming PLL Reference clock[0].
Fix the order of inits.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index d89c32bae2b9..03a5af52ec0b 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -778,8 +778,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge
*bridge)
WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, _cfg, false));
- cdns_dsi_hs_init(dsi);
cdns_dsi_init_link(dsi);
+ cdns_dsi_hs_init(dsi);
writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
dsi->regs + VID_HSIZE1);
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v4 03/11] drm/bridge: cdns-dsi: Fix Phy _init() and _exit()
Hi,
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Initialize the Phy during the cdns-dsi _resume(), and de-initialize it
during the _suspend().
Also power-off the Phy from bridge_disable.
Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 5159c3f0853e..d89c32bae2b9 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct drm_bridge
*bridge)
if (dsi->platform_ops && dsi->platform_ops->disable)
dsi->platform_ops->disable(dsi);
+ phy_power_off(dsi->dphy);
+ dsi->link_initialized = false;
+ dsi->phy_initialized = false;
+
pm_runtime_put(dsi->base.dev);
}
@@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
DPHY_CMN_PDN | DPHY_PLL_PDN,
dsi->regs + MCTL_DPHY_CFG0);
- phy_init(dsi->dphy);
phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
phy_configure(dsi->dphy, >phy_opts);
phy_power_on(dsi->dphy);
@@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct device
*dev)
clk_prepare_enable(dsi->dsi_p_clk);
clk_prepare_enable(dsi->dsi_sys_clk);
+ phy_init(dsi->dphy);
+
return 0;
}
@@ -1127,10 +1132,11 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
{
struct cdns_dsi *dsi = dev_get_drvdata(dev);
+ phy_exit(dsi->dphy);
+
clk_disable_unprepare(dsi->dsi_sys_clk);
clk_disable_unprepare(dsi->dsi_p_clk);
reset_control_assert(dsi->dsi_p_rst);
- dsi->link_initialized = false;
return 0;
}
So with this patch, phy_init/exit will be called in the resume/suspend
functions. That looks fine.
But the phy_power_on/phy_power_off looks odd to me. Here you add
phy_power_off() to cdns_dsi_bridge_disable(), which sounds fine. But
phy_power_on() is called in cdns_dsi_hs_init(), and that is called in
cdns_dsi_bridge_enable() (which sounds fine), but also in
cdns_dsi_bridge_pre_enable().
So doesn't that mean cdns_dsi_hs_init() call in cdns_dsi_bridge_enable()
is extra, as it effectively does nothing (it exists right away if
dsi->phy_initialized == true)?
Tomi
Re: [PATCH v4 02/11] drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Instead of manually finding the next bridge/panel, and maintaining the
panel-bridge (in-case the next entity is a panel), switch to using the
automatically managing devm_drm_of_get_bridge() API.
Drop the drm_panel support completely from the driver while at it.
Signed-off-by: Aradhya Bhatia
Reviewed-by: Tomi Valkeinen
Tomi
---
.../gpu/drm/bridge/cadence/cdns-dsi-core.c| 28 ++-
.../gpu/drm/bridge/cadence/cdns-dsi-core.h| 2 --
2 files changed, 3 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index b016f2ba06bb..5159c3f0853e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -920,8 +920,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
struct cdns_dsi_output *output = >output;
struct cdns_dsi_input *input = >input;
struct drm_bridge *bridge;
- struct drm_panel *panel;
- struct device_node *np;
int ret;
/*
@@ -939,26 +937,10 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
/*
* The host <-> device link might be described using an OF-graph
* representation, in this case we extract the device of_node from
-* this representation, otherwise we use dsidev->dev.of_node which
-* should have been filled by the core.
+* this representation.
*/
- np = of_graph_get_remote_node(dsi->base.dev->of_node, DSI_OUTPUT_PORT,
- dev->channel);
- if (!np)
- np = of_node_get(dev->dev.of_node);
-
- panel = of_drm_find_panel(np);
- if (!IS_ERR(panel)) {
- bridge = drm_panel_bridge_add_typed(panel,
- DRM_MODE_CONNECTOR_DSI);
- } else {
- bridge = of_drm_find_bridge(np);
- if (!bridge)
- bridge = ERR_PTR(-EINVAL);
- }
-
- of_node_put(np);
-
+ bridge = devm_drm_of_get_bridge(dsi->base.dev, dsi->base.dev->of_node,
+ DSI_OUTPUT_PORT, dev->channel);
if (IS_ERR(bridge)) {
ret = PTR_ERR(bridge);
dev_err(host->dev, "failed to add DSI device %s (err = %d)",
@@ -968,7 +950,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
output->dev = dev;
output->bridge = bridge;
- output->panel = panel;
/*
* The DSI output has been properly configured, we can now safely
@@ -984,12 +965,9 @@ static int cdns_dsi_detach(struct mipi_dsi_host *host,
struct mipi_dsi_device *dev)
{
struct cdns_dsi *dsi = to_cdns_dsi(host);
- struct cdns_dsi_output *output = >output;
struct cdns_dsi_input *input = >input;
drm_bridge_remove(>bridge);
- if (output->panel)
- drm_panel_bridge_remove(output->bridge);
return 0;
}
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
index ca7ea2da635c..5db5dbbbcaad 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
@@ -10,7 +10,6 @@
#include
#include
-#include
#include
#include
@@ -21,7 +20,6 @@ struct reset_control;
struct cdns_dsi_output {
struct mipi_dsi_device *dev;
- struct drm_panel *panel;
struct drm_bridge *bridge;
union phy_configure_opts phy_opts;
};
Re: [PATCH v4 01/11] drm/bridge: cdns-dsi: Fix OF node pointer
Hi,
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Fix the OF node pointer passed to the of_drm_find_bridge() call to find
the next bridge in the display chain.
To find the next bridge in the pipeline, we need to pass "np" - the OF
node pointer of the next entity in the devicetree chain. Passing
"of_node" to of_drm_find_bridge will make the function try to fetch the
bridge for the cdns-dsi which is not what's required.
Fix that.
The code looks fine, but I'd write the subject and desc from a different
perspective. The subject could be something like "Fix connecting to a
sink bridge", and the desc could first say that connecting the sink to a
DSI panel works, but connecting to a bridge fails, as wrong OF node is
passed to of_drm_find_bridge().
Reviewed-by: Tomi Valkeinen
Tomi
Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 7457d38622b0..b016f2ba06bb 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -952,7 +952,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
bridge = drm_panel_bridge_add_typed(panel,
DRM_MODE_CONNECTOR_DSI);
} else {
- bridge = of_drm_find_bridge(dev->dev.of_node);
+ bridge = of_drm_find_bridge(np);
if (!bridge)
bridge = ERR_PTR(-EINVAL);
}
Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
On 26/06/2024 11:49, Maxime Ripard wrote: Hi, On Wed, Jun 19, 2024 at 12:07:48PM GMT, Tomi Valkeinen wrote: From: Tomi Valkeinen When a bridge driver uses devm_mipi_dsi_device_register_full() or devm_mipi_dsi_attach(), the resource management is moved to devres, which releases the resource automatically when the bridge driver is unbound. However, if the DSI host goes away first, the host unregistration code will automatically detach and unregister any DSI peripherals, without notifying the devres about it. So when the bridge driver later is unbound, the resources are released a second time, leading to crash. That's super surprising. mipi_dsi_device_unregister calls device_unregister, which calls device_del, which in turn calls devres_release_all. Hmm, right. If that doesn't work like that, then it's what needs to be fixed, and not worked around in the MIPI-DSI bus. Well, something causes a crash for both the device register/unregister case and the attach/detach case, and the call stacks and debug prints showed a double unregister/detach... I need to dig up the board and check again why the devres_release_all() in device_del() doesn't solve this. But I can probably only get back to this in August, so it's perhaps best to ignore this patch for now. However, the attach/detach case is still valid? I see no devres calls in the detach paths. Tomi
Re: [PATCH 2/4] drm: ti-sn65dsi86: Check bridge connection failure
On 20/06/2024 13:42, Laurent Pinchart wrote:
On Thu, Jun 20, 2024 at 09:43:05AM +0300, Tomi Valkeinen wrote:
On 19/06/2024 22:32, Laurent Pinchart wrote:
Hi Jacopo,
Thank you for the patch.
On Wed, Jun 19, 2024 at 12:22:16PM +0200, Jacopo Mondi wrote:
From: Phong Hoang
Add a check to the register access function when attaching a bridge
device.
I think the desc is missing the "why". I'm guessing it's the first
register access to the IC, and thus verifies that it is accessible.
Isn't it a good idea in general to always check if I2C reads succeeded ?
It is. But if there are tens of other i2c accesses for which the return
value is ignored, the question remains: why this single one was
specifically fixed?
Tomi
Signed-off-by: Phong Hoang
Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 84698a0b27a8..b7df53577987 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -696,6 +696,7 @@ static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct
drm_bridge *bridge)
static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86 *pdata)
{
+ int ret;
int val;
struct mipi_dsi_host *host;
struct mipi_dsi_device *dsi;
@@ -720,8 +721,11 @@ static int ti_sn_attach_host(struct auxiliary_device
*adev, struct ti_sn65dsi86
/* check if continuous dsi clock is required or not */
pm_runtime_get_sync(dev);
- regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, );
+ ret = regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, );
pm_runtime_put_autosuspend(dev);
+ if (ret)
+ return ret;
+
if (!(val & DPPLL_CLK_SRC_DSICLK))
dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
Re: [PATCH 2/4] drm: ti-sn65dsi86: Check bridge connection failure
On 19/06/2024 22:32, Laurent Pinchart wrote:
Hi Jacopo,
Thank you for the patch.
On Wed, Jun 19, 2024 at 12:22:16PM +0200, Jacopo Mondi wrote:
From: Phong Hoang
Add a check to the register access function when attaching a bridge
device.
I think the desc is missing the "why". I'm guessing it's the first
register access to the IC, and thus verifies that it is accessible.
Tomi
Signed-off-by: Phong Hoang
Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 84698a0b27a8..b7df53577987 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -696,6 +696,7 @@ static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct
drm_bridge *bridge)
static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86 *pdata)
{
+ int ret;
int val;
struct mipi_dsi_host *host;
struct mipi_dsi_device *dsi;
@@ -720,8 +721,11 @@ static int ti_sn_attach_host(struct auxiliary_device
*adev, struct ti_sn65dsi86
/* check if continuous dsi clock is required or not */
pm_runtime_get_sync(dev);
- regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, );
+ ret = regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, );
pm_runtime_put_autosuspend(dev);
+ if (ret)
+ return ret;
+
if (!(val & DPPLL_CLK_SRC_DSICLK))
dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
Re: [PATCH 1/4] drm: rcar-mipi-dsi: Fix CLOCKSET1_LOCK definition
On 19/06/2024 22:29, Laurent Pinchart wrote: Hi Jacopo, Thank you for the patch. CC'ing Tomi. On Wed, Jun 19, 2024 at 12:22:15PM +0200, Jacopo Mondi wrote: From: Takeshi Kihara Version 0.51 of the Renesas R-Car Gen4 TRM reports bit 16 of the CLOCKSET1 register of the DSI transmitter module to be a reserved field. Fix this by correcting the CLOCKSET1_LOCK definition to match the TRM and remove the CLOCKSET1_LOCK_PHY definition, as the register is simply called "lock" in the datasheet. Signed-off-by: Takeshi Kihara Signed-off-by: Jacopo Mondi --- drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h index f8114d11f2d1..1bf9c4717d5a 100644 --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h @@ -141,8 +141,7 @@ #define PHYSETUP_RSTZ (1 << 0) #define CLOCKSET1 0x101c -#define CLOCKSET1_LOCK_PHY (1 << 17) -#define CLOCKSET1_LOCK (1 << 16) +#define CLOCKSET1_LOCK (1 << 17) This matches the documentation, but we should get it tested on V4H to make sure it doesn't cause a regression. Tomi, would you be able to test the patch ? Works for me. Tested-by: Tomi Valkeinen Tomi
[PATCH] drm/mipi-dsi: Fix devm unregister & detach
From: Tomi Valkeinen
When a bridge driver uses devm_mipi_dsi_device_register_full() or
devm_mipi_dsi_attach(), the resource management is moved to devres,
which releases the resource automatically when the bridge driver is
unbound.
However, if the DSI host goes away first, the host unregistration code
will automatically detach and unregister any DSI peripherals, without
notifying the devres about it. So when the bridge driver later is
unbound, the resources are released a second time, leading to crash.
Fix this by recording the device that was used when calling the above
mentioned functions into the struct mipi_dsi_device, and when the
unregister or detach is called, remove the devres action if needed.
Signed-off-by: Tomi Valkeinen
---
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/drm_mipi_dsi.c | 44 --
include/drm/drm_mipi_dsi.h | 6 ++
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 795001bb7ff1..a78c4b6cae70 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -237,13 +237,15 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
}
EXPORT_SYMBOL(mipi_dsi_device_register_full);
+static void mipi_dsi_do_device_unregister(struct mipi_dsi_device *dsi, bool
devres);
+
/**
* mipi_dsi_device_unregister - unregister MIPI DSI device
* @dsi: DSI peripheral device
*/
void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
{
- device_unregister(>dev);
+ mipi_dsi_do_device_unregister(dsi, false);
}
EXPORT_SYMBOL(mipi_dsi_device_unregister);
@@ -251,7 +253,15 @@ static void devm_mipi_dsi_device_unregister(void *arg)
{
struct mipi_dsi_device *dsi = arg;
- mipi_dsi_device_unregister(dsi);
+ mipi_dsi_do_device_unregister(dsi, true);
+}
+
+static void mipi_dsi_do_device_unregister(struct mipi_dsi_device *dsi, bool
devres)
+{
+ if (!devres && dsi->devres_register_dev)
+ devm_remove_action(dsi->devres_register_dev,
devm_mipi_dsi_device_unregister, dsi);
+
+ device_unregister(>dev);
}
/**
@@ -289,6 +299,8 @@ devm_mipi_dsi_device_register_full(struct device *dev,
if (ret)
return ERR_PTR(ret);
+ dsi->devres_register_dev = dev;
+
return dsi;
}
EXPORT_SYMBOL_GPL(devm_mipi_dsi_device_register_full);
@@ -386,17 +398,35 @@ int mipi_dsi_attach(struct mipi_dsi_device *dsi)
}
EXPORT_SYMBOL(mipi_dsi_attach);
+static int mipi_dsi_do_detach(struct mipi_dsi_device *dsi, bool devres);
+
/**
* mipi_dsi_detach - detach a DSI device from its DSI host
* @dsi: DSI peripheral
*/
int mipi_dsi_detach(struct mipi_dsi_device *dsi)
+{
+ return mipi_dsi_do_detach(dsi, false);
+}
+EXPORT_SYMBOL(mipi_dsi_detach);
+
+static void devm_mipi_dsi_detach(void *arg)
+{
+ struct mipi_dsi_device *dsi = arg;
+
+ mipi_dsi_do_detach(dsi, true);
+}
+
+static int mipi_dsi_do_detach(struct mipi_dsi_device *dsi, bool devres)
{
const struct mipi_dsi_host_ops *ops = dsi->host->ops;
if (WARN_ON(!dsi->attached))
return -EINVAL;
+ if (!devres && dsi->devres_attach_dev)
+ devm_remove_action(dsi->devres_attach_dev,
devm_mipi_dsi_detach, dsi);
+
if (!ops || !ops->detach)
return -ENOSYS;
@@ -404,14 +434,6 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
return ops->detach(dsi->host, dsi);
}
-EXPORT_SYMBOL(mipi_dsi_detach);
-
-static void devm_mipi_dsi_detach(void *arg)
-{
- struct mipi_dsi_device *dsi = arg;
-
- mipi_dsi_detach(dsi);
-}
/**
* devm_mipi_dsi_attach - Attach a MIPI-DSI device to its DSI Host
@@ -437,6 +459,8 @@ int devm_mipi_dsi_attach(struct device *dev,
if (ret)
return ret;
+ dsi->devres_attach_dev = dev;
+
return 0;
}
EXPORT_SYMBOL_GPL(devm_mipi_dsi_attach);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 82b1cc434ea3..f68aee6813db 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -181,6 +181,9 @@ struct mipi_dsi_device_info {
* be set to the real limits of the hardware, zero is only accepted for
* legacy drivers
* @dsc: panel/bridge DSC pps payload to be sent
+ * devres_register_dev: device that was used with
+ * devm_mipi_dsi_device_register_full() or NULL
+ * devres_attach_dev: device that was used with devm_mipi_dsi_attach() or NULL
*/
struct mipi_dsi_device {
struct mipi_dsi_host *host;
@@ -195,6 +198,9 @@ struct mipi_dsi_device {
unsigned long hs_rate;
unsigned long lp_rate;
struct drm_dsc_config *dsc;
+
+ struct device *devres_register_dev;
+ struct device *devres_attach_dev;
};
#define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
---
base-commit: 6ba59ff4227927d3a853
Re: [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support
Hi Sean, On 03/05/2024 22:29, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. I tested this, and the "drm: zynqmp_dp: Convert to a hard IRQ" causes a hang for me when unloading the drivers. Unfortunately I'm not in the condition to debug it at the moment. I have picked the first three patches into drm-misc-next, though, to decrease the number of patches in the series a bit. They looked independent and safe enough to apply. Tomi Changes in v5: - Fix AUX bus not getting unregistered - Rebase onto drm-misc/drm-misc-next Changes in v4: - Rebase onto drm/drm-next Changes in v3: - Don't delay work - Convert to a hard IRQ - Use AUX IRQs instead of polling - Take dp->lock in zynqmp_dp_hpd_work_func Changes in v2: - Rearrange zynqmp_dp for better padding - Split off the HPD IRQ work into another commit - Expand the commit message - Document hpd_irq_work - Document debugfs files - Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier implicit functionality - Attempt to fix unreproducable, spurious build warning - Drop "Optionally ignore DPCD errors" in favor of a debugfs file directly affecting zynqmp_dp_aux_transfer. Sean Anderson (10): drm: zynqmp_kms: Fix AUX bus not getting unregistered drm: zynqmp_dp: Rearrange zynqmp_dp for better padding drm: zynqmp_dp: Don't delay work drm: zynqmp_dp: Add locking drm: zynqmp_dp: Don't retrain the link in our IRQ drm: zynqmp_dp: Convert to a hard IRQ drm: zynqmp_dp: Use AUX IRQs instead of polling drm: zynqmp_dp: Split off several helper functions drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func drm: zynqmp_dp: Add debugfs interface for compliance testing Documentation/gpu/drivers.rst | 1 + Documentation/gpu/zynqmp.rst | 149 + MAINTAINERS | 1 + drivers/gpu/drm/xlnx/zynqmp_dp.c | 883 +++--- drivers/gpu/drm/xlnx/zynqmp_kms.c | 12 +- 5 files changed, 977 insertions(+), 69 deletions(-) create mode 100644 Documentation/gpu/zynqmp.rst
Re: [PATCH v2] drm: xlnx: zynqmp_dpsub: Enable plane in atomic update
Hi,
On 24/05/2024 02:49, Anatoliy Klymenko wrote:
Unconditionally enable the DPSUB layer in the corresponding atomic plane
update callback. Setting the new display mode may require disabling and
re-enabling the CRTC. This effectively resets DPSUB to the default state
with all layers disabled. The original implementation of the plane atomic
update enables the corresponding DPSUB layer only if the framebuffer
format has changed. This would leave the layer disabled after switching to
a different display mode with the same framebuffer format.
Signed-off-by: Anatoliy Klymenko
---
Changes in v2:
- Added comment around DPSUB layer enablement explaining why it should be
done unconditionally.
- Link to v1:
https://lore.kernel.org/r/20240520-dp-layer-enable-v1-1-c9b481209...@amd.com
---
drivers/gpu/drm/xlnx/zynqmp_kms.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index 43bf416b33d5..0b57ab5451a9 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
@@ -120,9 +120,13 @@ static void zynqmp_dpsub_plane_atomic_update(struct
drm_plane *plane,
zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
plane->state->alpha >> 8);
- /* Enable or re-enable the plane if the format has changed. */
- if (format_changed)
- zynqmp_disp_layer_enable(layer);
+ /*
+* Unconditionally enable the layer, as it may have been disabled
+* previously either explicitly to reconfigure layer format, or
+* implicitly after DPSUB reset during display mode change. DRM
+* framework calls this callback for enabled planes only.
+*/
+ zynqmp_disp_layer_enable(layer);
}
static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs = {
---
base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
change-id: 20240520-dp-layer-enable-7b561af29ca8
Best regards,
Thanks, I have pushed this to drm-misc-next.
Tomi
Re: [PATCH] drm: zynqmp_dpsub: Fix an error handling path in zynqmp_dpsub_probe()
Hi,
On 16/06/2024 21:43, Laurent Pinchart wrote:
On Thu, Jun 13, 2024 at 11:05:01AM -0400, Sean Anderson wrote:
On 5/20/24 11:05, Sean Anderson wrote:
On 5/20/24 05:40, Christophe JAILLET wrote:
If zynqmp_dpsub_drm_init() fails, we must undo the previous
drm_bridge_add() call.
Fixes: be3f3042391d ("drm: zynqmp_dpsub: Always register bridge")
Signed-off-by: Christophe JAILLET
---
Compile tested only
---
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index face8d6b2a6f..f5781939de9c 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -269,6 +269,7 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
return 0;
err_disp:
+ drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
err_dp:
zynqmp_dp_remove(dpsub);
Reviewed-by: Sean Anderson
Will this be applied soon? The patch it fixes has made its way into
the stable tree already.
If someone can merge it in drm-misc that would be the fastest way.
Otherwise I'll send a pull request at some point, but I'm overworked at
the moment.
Thanks, I have pushed this to drm-misc-next.
Tomi
Re: [PATCH RFC 2/2] pmdomain: ti-sci: Support retaining PD boot time state
Hi, On 03/05/2024 16:45, Ulf Hansson wrote: + Abel, Saravanna, Stephen On Mon, 15 Apr 2024 at 19:17, Tomi Valkeinen wrote: On 15/04/2024 19:00, Tomi Valkeinen wrote: Add a new flag, TI_SCI_PD_KEEP_BOOT_STATE, which can be set in the dts when referring to power domains. When this flag is set, the ti-sci driver will check if the PD is currently enabled in the HW, and if so, set the GENPD_FLAG_ALWAYS_ON flag so that the PD will stay enabled. The main issue I'm trying to solve here is this: If the Display Subsystem (DSS) has been enabled by the bootloader, the related PD has also been enabled in the HW. When the tidss driver probes, the driver framework will automatically enable the PD. While executing the probe function it is very common for the probe to return EPROBE_DEFER, and, in rarer cases, an actual error. When this happens (probe() returns an error), the driver framework will automatically disable the related PD. Powering off the PD while the DSS is enabled and displaying a picture will cause the DSS HW to enter a bad state, from which (afaik) it can't be woken up except with full power-cycle. Trying to access the DSS in this state (e.g. when retrying the probe) will usually cause the board to hang sooner or later. Even if we wouldn't have this board-hangs issue, it's nice to be able to keep the DSS PD enabled: we want to keep the DSS enabled when the bootloader has enabled the screen. If, instead, we disable the PD at the first EPROBE_DEFER, the screen will (probably) go black. A few things occurred to me. The driver is supposed to clear the GENPD_FLAG_ALWAYS_ON when the driver has probed successfully. There are two possible issues with that: - Afaics, there's no API to do that, and currently I just clear the bit in genpd->flags. There's a clear race there, so some locking would be required. - This uses the GENPD_FLAG_ALWAYS_ON flag to say "PD is always on, until the driver has started". If the PD would have GENPD_FLAG_ALWAYS_ON set for other reasons, the driver would still go and clear the flag, which might break things. Also, unrelated to the above and not a problem in practice at the very moment, but I think clocks should also be dealt with somehow. Something, at early-ish boot stage, should mark the relevant clocks as in use, so that there's no chance they would be turned off when the main kernel has started (the main display driver is often a module). It would be nice to deal with all the above in a single place. I wonder if the tidss driver itself could somehow be split into two parts, an early part that would probe with minimal dependencies, mainly to reserve the core resources without doing any kind of DRM init. And a main part which would (somehow) finish the initialization at a later point, when we have the filesystem (for firmware) and the other bridge/panel drivers have probed. That can be somewhat achieved with simplefb or simpledrm, though, but we can't do any TI DSS specific things there, and it also creates a requirement to have either of those drivers built-in, and the related DT nodes to be added. Without going into too much detail, this and similar problems have been discussed in the past. With the fw_devlink and the ->sync_state() callback we are getting closer to a solution, but for genpd a solution is still pending. If you want to read up on earlier discussions and join us moving forward, that would be great. The last attempt for genpd to move this forward was posted by Abel Vesa: https://lore.kernel.org/linux-pm/20230621144019.3219858-1-abel.v...@linaro.org/ Beyond that, we have also discussed various solutions at the last LPC in Richmond. I think the consensus at that point was that Saravana targeted to post something for clocks - and when that was done, we should do the similar thing for genpd. Anyway, I have looped them into this thread, so they can share any updates on their side of the matter. If I understand the series correctly, it has an issue at least for this case/platform. The devlinks are between the consumer devices and the PD provider device. TI SCI PD provider has quite a lot of PDs, and all the consumers would have to be probed before any of the PDs could be disabled. So, to get the display PD disabled, I would have to load, e.g., the GPU driver (which I don't even have). I believe this is the case for the clocks also. Perhaps that can be considered a feature, but I fear that in practice it would mean that most of the time for most users all the boot-time enabled powerdomains would be always on. Nevertheless, I believe the series would fix the issue mentioned in this patch, so I'll see if I can get the series working on the TI platform to get a bit more experience on this whole issue. Tomi Tomi Kind regards Uffe Another option here would perhaps be to change the driver framework (drivers/base/platform.c) which attaches and detaches the PD, and make it somehow optional, allowing the d
Re: [BUG] drm: zynqmp_dp: Lockup in zynqmp_dp_bridge_detect when device is unbound
Hi,
On 04/05/2024 00:54, Sean Anderson wrote:
Hi,
I have discovered a bug in the displayport driver on drm-misc-next. To
trigger it, run
echo fd4a.display > /sys/bus/platform/drivers/zynqmp-dpsub/unbind
The system will become unresponsive and (after a bit) splat with a hard
LOCKUP. One core will be unresponsive at the first zynqmp_dp_read in
zynqmp_dp_bridge_detect.
I believe the issue is due the registers being unmapped and the block
put into reset in zynqmp_dp_remove instead of zynqmp_dpsub_release. This
could be resolved by deferring things until zynqmp_dpsub_release
(requiring us to skip devm_*), or by adding a flag to struct zynqmp_dp
and checking it before each callback. A subsystem-level implementation
might be better for the latter.
For a better traceback, try applying the below patch and running the
following commands before triggering the lockup:
echo 4 > /sys/module/drm/parameters/debug
echo 8 > /proc/sys/kernel/printk
I wasn't able to reproduce. Where does the detect call come in your
case? Looking at the code, afaics, it shouldn't happen.
Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 9df068a413f3..17b477b14ab5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -296,6 +296,7 @@ struct zynqmp_dp_config {
* @train_set: set of training data
*/
struct zynqmp_dp {
+ unsigned long long magic;
struct device *dev;
struct zynqmp_dpsub *dpsub;
void __iomem *iomem;
@@ -1533,6 +1534,8 @@ static enum drm_connector_status
zynqmp_dp_bridge_detect(struct drm_bridge *brid
u32 state, i;
int ret;
+ WARN_ON(dp->magic != 0x0123456789abcdefULL);
+
/*
* This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
* get the HPD signal with some monitors.
@@ -1723,6 +1726,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
if (!dp)
return -ENOMEM;
+ dp->magic = 0x0123456789abcdefULL;
dp->dev = >dev;
dp->dpsub = dpsub;
dp->status = connector_status_disconnected;
@@ -1839,4 +1843,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
zynqmp_dp_phy_exit(dp);
zynqmp_dp_reset(dp, true);
+ dp->magic = 0xdeadbeefdeadbeefULL;
}
Re: [PATCH v3 0/2] Fix Kernel CI issues
On 03/05/2024 19:27, Nathan Chancellor wrote:
Hi Tomi,
On Sat, Apr 27, 2024 at 10:48:16AM +0300, Tomi Valkeinen wrote:
On 26/04/2024 22:27, Anatoliy Klymenko wrote:
Fix number of CI reported W=1 build issues.
Patch 1/2: Fix function arguments description.
Closes:
https://lore.kernel.org/oe-kbuild-all/202404260616.kfgdpcdn-...@intel.com/
Patch 2/2: Fix clang compilation error.
Closes:
https://lore.kernel.org/oe-kbuild-all/202404260946.4ozxvhd2-...@intel.com/
Signed-off-by: Anatoliy Klymenko
---
Changes in v3:
- Add Signed-off-by tag.
- Link to v2:
https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v2-0-6048e8121...@amd.com
Changes in v2:
- Compilation error fix added.
- Link to v1:
https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v1-1-405f352d3...@amd.com
---
Anatoliy Klymenko (2):
drm: xlnx: zynqmp_dpsub: Fix few function comments
drm: xlnx: zynqmp_dpsub: Fix compilation error
drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
---
base-commit: 2bdb481bf7a93c22b9fea8daefa2834aab23a70f
change-id: 20240425-dp-live-fmt-fix-a10bf7973596
Best regards,
Thanks, pushed to drm-misc-next.
I think the second patch also needs to go to drm-misc-next-fixes? The
clang warning fixed by it has returned in next-20240503 because it
appears that for-linux-next was switch from drm-misc-next to
drm-misc-next-fixes, as I see for-linux-next was pointing to commit
235e60653f8d ("drm/debugfs: Drop conditionals around of_node pointers")
on drm-misc-next in next-20240502 but it is now pointing to commit
be3f3042391d ("drm: zynqmp_dpsub: Always register bridge") on
drm-misc-next-fixes in next-20240503.
Oh. Hmm, did I just hit the feature-freeze point with the fixes...
Now I'm unsure where I should push these (if anywhere), as they already
are in drm-misc-next.
DRM Misc maintainers, can you give me a hint? =)
Tomi
Re: [PATCH v3 0/2] Fix Kernel CI issues
On 26/04/2024 22:27, Anatoliy Klymenko wrote: Fix number of CI reported W=1 build issues. Patch 1/2: Fix function arguments description. Closes: https://lore.kernel.org/oe-kbuild-all/202404260616.kfgdpcdn-...@intel.com/ Patch 2/2: Fix clang compilation error. Closes: https://lore.kernel.org/oe-kbuild-all/202404260946.4ozxvhd2-...@intel.com/ Signed-off-by: Anatoliy Klymenko --- Changes in v3: - Add Signed-off-by tag. - Link to v2: https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v2-0-6048e8121...@amd.com Changes in v2: - Compilation error fix added. - Link to v1: https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v1-1-405f352d3...@amd.com --- Anatoliy Klymenko (2): drm: xlnx: zynqmp_dpsub: Fix few function comments drm: xlnx: zynqmp_dpsub: Fix compilation error drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- base-commit: 2bdb481bf7a93c22b9fea8daefa2834aab23a70f change-id: 20240425-dp-live-fmt-fix-a10bf7973596 Best regards, Thanks, pushed to drm-misc-next. Tomi
Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
On 26/04/2024 12:30, Laurent Pinchart wrote:
On Fri, Mar 22, 2024 at 08:01:44AM +0200, Tomi Valkeinen wrote:
On 08/03/2024 22:47, Sean Anderson wrote:
We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
initialized. We do this before zynqmp_dpsub_drm_init since that calls
drm_bridge_attach. This fixes the following lockdep warning:
[ 19.217084] [ cut here ]
[ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582
__mutex_lock+0x4bc/0x550
[ 19.241696] Modules linked in:
[ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
[ 19.252046] Hardware name: xlnx,zynqmp (DT)
[ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
[ 19.261795] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 19.269104] pc : __mutex_lock+0x4bc/0x550
[ 19.273364] lr : __mutex_lock+0x4bc/0x550
[ 19.277592] sp : ffc085c5bbe0
[ 19.281066] x29: ffc085c5bbe0 x28: x27: ff88009417f8
[ 19.288624] x26: ff8800941788 x25: ff8800020008 x24: ffc082aa3000
[ 19.296227] x23: ffc080d90e3c x22: 0002 x21:
[ 19.303744] x20: x19: ff88002f5210 x18:
[ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
[ 19.318922] x14: x13: 284e4f5f4e524157 x12: 0001
[ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
[ 19.334003] x8 : 0001ff88003f3888 x7 : x6 :
[ 19.341537] x5 : x4 : 1668 x3 :
[ 19.349054] x2 : x1 : x0 : ff88003f3880
[ 19.356581] Call trace:
[ 19.359160] __mutex_lock+0x4bc/0x550
[ 19.363032] mutex_lock_nested+0x24/0x30
[ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
[ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
[ 19.376364] process_one_work+0x3ac/0x988
[ 19.380660] worker_thread+0x398/0x694
[ 19.384736] kthread+0x1bc/0x1c0
[ 19.388241] ret_from_fork+0x10/0x20
[ 19.392031] irq event stamp: 183
[ 19.395450] hardirqs last enabled at (183): []
finish_task_switch.isra.0+0xa8/0x2d4
[ 19.405140] hardirqs last disabled at (182): []
__schedule+0x714/0xd04
[ 19.413612] softirqs last enabled at (114): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.423128] softirqs last disabled at (110): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.432614] ---[ end trace ]---
Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
Signed-off-by: Sean Anderson
---
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..639fff2c693f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device
*pdev)
if (ret)
goto err_dp;
+ drm_bridge_add(dpsub->bridge);
A blank line here would be nice.
if (dpsub->dma_enabled) {
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device
*pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
+ drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
I sent a similar patch:
https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03...@ideasonboard.com/
I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where
the bridge is set up, so it felt like a logical place. You add it later,
just before the bridge is used the first time.
I like mine a bit more as it has all the bridge code in the same place,
but I also wonder if there might be some risks in adding the bridge
early (before zynqmp_disp_probe()), although I can't see any issue right
away...
Seems we have the same concerns :-) I've reviewed your patch and wrote
pretty much the same. I would be more comfortable with this version,
even if I like gathering all bridge code in the same location.
I guess there's no reason to take the risk here, so I have pushed this
one to drm-misc-next.
Tomi
In any case, as this works for me too:
Reviewed-by: Tomi Valkeinen
Reviewed-by: Laurent Pinchart
Re: [PATCH v4 00/13] drm: zynqmp_dp: IRQ cleanups and debugfs support
On 25/04/2024 18:17, Sean Anderson wrote: On 4/24/24 14:54, Tomi Valkeinen wrote: Hi Sean, On 23/04/2024 20:18, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. If it's ok to you, I'd like to pick the first four patches to drm-misc already. Fine by me. I have pushed the first four to drm-misc. I'll try to look at the rest of the patches next week. Tomi
Re: [PATCH v2 2/2] drm: xlnx: zynqmp_dpsub: Fix compilation error
On 26/04/2024 04:46, Anatoliy Klymenko wrote:
Fix W=1 clang 19 compilation error in zynqmp_disp_layer_drm_formats().
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202404260946.4ozxvhd2-...@intel.com/
---
This is missing your signed-off-by.
Tomi
drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 423f5f4943cc..c9fb432d4cbd 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -981,7 +981,7 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer
*layer,
unsigned int i;
u32 *formats;
- if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
+ if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE)) {
*num_formats = 0;
return NULL;
}
Re: [PATCH v4 00/13] drm: zynqmp_dp: IRQ cleanups and debugfs support
Hi Sean, On 23/04/2024 20:18, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. If it's ok to you, I'd like to pick the first four patches to drm-misc already. Tomi Changes in v4: - Rebase onto drm/drm-next Changes in v3: - Store base pointers in zynqmp_disp directly - Don't delay work - Convert to a hard IRQ - Use AUX IRQs instead of polling - Take dp->lock in zynqmp_dp_hpd_work_func Changes in v2: - Fix kerneldoc - Rearrange zynqmp_dp for better padding - Split off the HPD IRQ work into another commit - Expand the commit message - Document hpd_irq_work - Document debugfs files - Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier implicit functionality - Attempt to fix unreproducable, spurious build warning - Drop "Optionally ignore DPCD errors" in favor of a debugfs file directly affecting zynqmp_dp_aux_transfer. Sean Anderson (13): drm: xlnx: Store base pointers in zynqmp_disp directly drm: xlnx: Fix kerneldoc drm: zynqmp_dp: Downgrade log level for aux retries message drm: zynqmp_dp: Adjust training values per-lane drm: zynqmp_dp: Rearrange zynqmp_dp for better padding drm: zynqmp_dp: Don't delay work drm: zynqmp_dp: Add locking drm: zynqmp_dp: Don't retrain the link in our IRQ drm: zynqmp_dp: Convert to a hard IRQ drm: zynqmp_dp: Use AUX IRQs instead of polling drm: zynqmp_dp: Split off several helper functions drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func drm: zynqmp_dp: Add debugfs interface for compliance testing Documentation/gpu/drivers.rst | 1 + Documentation/gpu/zynqmp.rst| 149 + MAINTAINERS | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 44 +- drivers/gpu/drm/xlnx/zynqmp_dp.c| 908 +--- drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 + drivers/gpu/drm/xlnx/zynqmp_kms.h | 4 +- 7 files changed, 999 insertions(+), 109 deletions(-) create mode 100644 Documentation/gpu/zynqmp.rst
Re: [PATCH v4 0/7] Managing live video input format for ZynqMP DPSUB
Hi, On 16/04/2024 23:31, Anatoliy Klymenko wrote: Implement live video input format setting for ZynqMP DPSUB. ZynqMP DPSUB can operate in 2 modes: DMA-based and live. In the live mode, DPSUB receives a live video signal from FPGA-based CRTC. DPSUB acts as a DRM encoder bridge in such a scenario. To properly tune into the incoming video signal, DPSUB should be programmed with the proper media bus format. This patch series addresses this task. Patch 1/7: Set the DPSUB layer mode of operation prior to enabling the layer. Allows to use layer operational mode before its enablement. Patch 2/7: Update some IP register defines. Patch 3/7: Factor out some code into a helper function. Patch 4/7: Announce supported input media bus formats via drm_bridge_funcs.atomic_get_input_bus_fmts callback. Patch 5/7: Minimize usage of a global flag. Minor improvement. Patch 6/7: Program DPSUB live video input format based on selected bus config in the new atomic bridge state. Patch 7/7: New optional CRTC atomic helper proposal that will allow to negotiate video signal format between CRTC and connected encoder. Incorporate this callback into the DRM bridge format negotiation process. Save negotiated output format in drm_crtc_state. Reference usage of this API is available here: https://github.com/onotole/linux/tree/dpsub-live-in The patches up to and including patch 6 look ready to me. I'll pick them up to drm-misc. Tomi To: Laurent Pinchart To: Maarten Lankhorst To: Maxime Ripard To: Thomas Zimmermann To: David Airlie To: Daniel Vetter To: Michal Simek To: Andrzej Hajda To: Neil Armstrong To: Robert Foss To: Jonas Karlman To: Jernej Skrabec To: Rob Herring To: Krzysztof Kozlowski To: Conor Dooley To: Mauro Carvalho Chehab Cc: Tomi Valkeinen Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Cc: devicet...@vger.kernel.org Cc: linux-me...@vger.kernel.org Signed-off-by: Anatoliy Klymenko Changes in v4: - Replace controversial reference driver patches with the private repository link. - Split display layer format manipulation functions into 2 separate cases for diferet layer modes. - Address misc review comments (typos, comments, etc.) Link to v3: https://lore.kernel.org/r/20240321-dp-live-fmt-v3-0-d5090d796...@amd.com Changes in v3: - Add connected live layer helper - Include reference DRM format in zynqmp_disp_format for live layerss. - Add default bus format list for non-live case. - Explain removal of redundant checks in the commit message. - Minor fixes and improvements from review comments. Link to v2: https://lore.kernel.org/r/20240312-dp-live-fmt-v2-0-a9c35dc5c...@amd.com Changes in v2: - Factor out register defines update into separate patch. - Add some improvements minimizing ithe usage of a global flag. - Reuse existing format setting API instead of introducing new versions. - Add warning around NULL check on new bridge state within atomic enable callback. - Add drm_helper_crtc_select_output_bus_format() that wraps drm_crtc_helper_funcs.select_output_bus_format(). - Update API comments per review recommendations. - Address some minor review comments. - Add reference CRTC driver that demonstrates the usage of the proposed drm_crtc_helper_funcs.select_output_bus_format() API. - Link to v1: https://lore.kernel.org/r/20240226-dp-live-fmt-v1-0-b78c3f69c...@amd.com --- Anatoliy Klymenko (7): drm: xlnx: zynqmp_dpsub: Set layer mode during creation drm: xlnx: zynqmp_dpsub: Update live format defines drm: xlnx: zynqmp_dpsub: Add connected live layer helper drm: xlnx: zynqmp_dpsub: Anounce supported input formats drm: xlnx: zynqmp_dpsub: Minimize usage of global flag drm: xlnx: zynqmp_dpsub: Set input live format drm/atomic-helper: Add select_output_bus_format callback drivers/gpu/drm/drm_bridge.c | 14 +- drivers/gpu/drm/drm_crtc_helper.c| 38 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 231 +++ drivers/gpu/drm/xlnx/zynqmp_disp.h | 17 +-- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 81 --- drivers/gpu/drm/xlnx/zynqmp_kms.c| 2 +- include/drm/drm_crtc.h | 11 ++ include/drm/drm_crtc_helper.h| 5 + include/drm/drm_modeset_helper_vtables.h | 30 10 files changed, 372 insertions(+), 65 deletions(-) --- base-commit: bfa4437fd3938ae2e186e7664b2db65bb8775670 change-id: 20240226-dp-live-fmt-6415773b5a68 Best regards,
Re: [PATCH v4 10/13] drm: zynqmp_dp: Use AUX IRQs instead of polling
On 23/04/2024 20:18, Sean Anderson wrote:
Instead of polling the status register for the AUX status, just enable
the IRQs and signal a completion.
Signed-off-by: Sean Anderson
---
This one seems to cause a hang when I unload the modules. I didn't debug
it further yet, but most likely we get an AUX interrupt when disabling
the hardware, and the driver hasn't disabled the IRQ handler.
Tomi
(no changes since v3)
Changes in v3:
- New
drivers/gpu/drm/xlnx/zynqmp_dp.c | 35 +++-
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 9d61b6b8f2d4..863668642190 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -285,6 +285,7 @@ struct zynqmp_dp_config {
* @next_bridge: The downstream bridge
* @config: IP core configuration from DTS
* @aux: aux channel
+ * @aux_done: Completed when we get an AUX reply or timeout
* @phy: PHY handles for DP lanes
* @num_lanes: number of enabled phy lanes
* @hpd_work: hot plug detection worker
@@ -305,6 +306,7 @@ struct zynqmp_dp {
struct drm_bridge bridge;
struct work_struct hpd_work;
struct work_struct hpd_irq_work;
+ struct completion aux_done;
struct mutex lock;
struct drm_bridge *next_bridge;
@@ -941,12 +943,15 @@ static int zynqmp_dp_aux_cmd_submit(struct zynqmp_dp *dp,
u32 cmd, u16 addr,
u8 *buf, u8 bytes, u8 *reply)
{
bool is_read = (cmd & AUX_READ_BIT) ? true : false;
+ unsigned long time_left;
u32 reg, i;
reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REQUEST)
return -EBUSY;
+ reinit_completion(>aux_done);
+
zynqmp_dp_write(dp, ZYNQMP_DP_AUX_ADDRESS, addr);
if (!is_read)
for (i = 0; i < bytes; i++)
@@ -961,17 +966,14 @@ static int zynqmp_dp_aux_cmd_submit(struct zynqmp_dp *dp,
u32 cmd, u16 addr,
zynqmp_dp_write(dp, ZYNQMP_DP_AUX_COMMAND, reg);
/* Wait for reply to be delivered upto 2ms */
- for (i = 0; ; i++) {
- reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
- if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY)
- break;
+ time_left = wait_for_completion_timeout(>aux_done,
+ msecs_to_jiffies(2));
+ if (!time_left)
+ return -ETIMEDOUT;
- if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT ||
- i == 2)
- return -ETIMEDOUT;
-
- usleep_range(1000, 1100);
- }
+ reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
+ if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT)
+ return -ETIMEDOUT;
reg = zynqmp_dp_read(dp, ZYNQMP_DP_AUX_REPLY_CODE);
if (reply)
@@ -1055,6 +1057,9 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp)
(w << ZYNQMP_DP_AUX_CLK_DIVIDER_AUX_FILTER_SHIFT) |
(rate / (1000 * 1000)));
+ zynqmp_dp_write(dp, ZYNQMP_DP_INT_EN, ZYNQMP_DP_INT_REPLY_RECEIVED |
+ ZYNQMP_DP_INT_REPLY_TIMEOUT);
+
dp->aux.name = "ZynqMP DP AUX";
dp->aux.dev = dp->dev;
dp->aux.drm_dev = dp->bridge.dev;
@@ -1072,6 +1077,9 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp)
static void zynqmp_dp_aux_cleanup(struct zynqmp_dp *dp)
{
drm_dp_aux_unregister(>aux);
+
+ zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_REPLY_RECEIVED |
+ ZYNQMP_DP_INT_REPLY_TIMEOUT);
}
/* -
@@ -1685,6 +1693,12 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void
*data)
if (status & ZYNQMP_DP_INT_HPD_IRQ)
schedule_work(>hpd_irq_work);
+ if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY)
+ complete(>aux_done);
+
+ if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT)
+ complete(>aux_done);
+
return IRQ_HANDLED;
}
@@ -1708,6 +1722,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
dp->dpsub = dpsub;
dp->status = connector_status_disconnected;
mutex_init(>lock);
+ init_completion(>aux_done);
INIT_WORK(>hpd_work, zynqmp_dp_hpd_work_func);
INIT_WORK(>hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
On 23/04/2024 23:50, Sean Anderson wrote:
Hi,
On 3/22/24 02:01, Tomi Valkeinen wrote:
Hi,
On 08/03/2024 22:47, Sean Anderson wrote:
We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
initialized. We do this before zynqmp_dpsub_drm_init since that calls
drm_bridge_attach. This fixes the following lockdep warning:
[ 19.217084] [ cut here ]
[ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582
__mutex_lock+0x4bc/0x550
[ 19.241696] Modules linked in:
[ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
[ 19.252046] Hardware name: xlnx,zynqmp (DT)
[ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
[ 19.261795] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 19.269104] pc : __mutex_lock+0x4bc/0x550
[ 19.273364] lr : __mutex_lock+0x4bc/0x550
[ 19.277592] sp : ffc085c5bbe0
[ 19.281066] x29: ffc085c5bbe0 x28: x27: ff88009417f8
[ 19.288624] x26: ff8800941788 x25: ff8800020008 x24: ffc082aa3000
[ 19.296227] x23: ffc080d90e3c x22: 0002 x21:
[ 19.303744] x20: x19: ff88002f5210 x18:
[ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
[ 19.318922] x14: x13: 284e4f5f4e524157 x12: 0001
[ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
[ 19.334003] x8 : 0001ff88003f3888 x7 : x6 :
[ 19.341537] x5 : x4 : 1668 x3 :
[ 19.349054] x2 : x1 : x0 : ff88003f3880
[ 19.356581] Call trace:
[ 19.359160] __mutex_lock+0x4bc/0x550
[ 19.363032] mutex_lock_nested+0x24/0x30
[ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
[ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
[ 19.376364] process_one_work+0x3ac/0x988
[ 19.380660] worker_thread+0x398/0x694
[ 19.384736] kthread+0x1bc/0x1c0
[ 19.388241] ret_from_fork+0x10/0x20
[ 19.392031] irq event stamp: 183
[ 19.395450] hardirqs last enabled at (183): []
finish_task_switch.isra.0+0xa8/0x2d4
[ 19.405140] hardirqs last disabled at (182): []
__schedule+0x714/0xd04
[ 19.413612] softirqs last enabled at (114): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.423128] softirqs last disabled at (110): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.432614] ---[ end trace ]---
Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
Signed-off-by: Sean Anderson
---
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..639fff2c693f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device
*pdev)
if (ret)
goto err_dp;
+ drm_bridge_add(dpsub->bridge);
if (dpsub->dma_enabled) {
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device
*pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
+ drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
I sent a similar patch:
https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03...@ideasonboard.com/
I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where the
bridge is set up, so it felt like a logical place. You add it later, just
before the bridge is used the first time.
I like mine a bit more as it has all the bridge code in the same place, but I
also wonder if there might be some risks in adding the bridge early (before
zynqmp_disp_probe()), although I can't see any issue right away...
In any case, as this works for me too:
Reviewed-by: Tomi Valkeinen
Tomi
Can someone pick up this fix before the release? I am still running into this
bug on linux-next/master.
Can you check my version (link above)? After looking at both versions
today, I'd rather push mine: it adds and removes the bridge in the same
file where all the bridge code resides, and it'd be nice to manage the
bridge in that file as much as possible.
Tomi
Re: [PATCH v3 00/13] drm: zynqmp_dp: IRQ cleanups and debugfs support
On 23/04/2024 17:59, Sean Anderson wrote: On 4/23/24 09:33, Tomi Valkeinen wrote: Hi Sean, On 22/04/2024 21:45, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. What is the base for this series? I'm having trouble applying it. I managed to mostly apply it, but I see the board hang when I unload the modules. I didn't debug it as it might as well be caused by my conflict resolution. The base is v6.8-rc1, but it should probably be v6.9. I can rebase and resend. Did you have something extra in your branch before the series? I got "error: sha1 information is lacking or useless". Tomi
Re: [PATCH v3 00/13] drm: zynqmp_dp: IRQ cleanups and debugfs support
Hi Sean, On 22/04/2024 21:45, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. What is the base for this series? I'm having trouble applying it. I managed to mostly apply it, but I see the board hang when I unload the modules. I didn't debug it as it might as well be caused by my conflict resolution. Tomi Changes in v3: - Store base pointers in zynqmp_disp directly - Don't delay work - Convert to a hard IRQ - Use AUX IRQs instead of polling - Take dp->lock in zynqmp_dp_hpd_work_func Changes in v2: - Fix kerneldoc - Rearrange zynqmp_dp for better padding - Split off the HPD IRQ work into another commit - Expand the commit message - Document hpd_irq_work - Document debugfs files - Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier implicit functionality - Attempt to fix unreproducable, spurious build warning - Drop "Optionally ignore DPCD errors" in favor of a debugfs file directly affecting zynqmp_dp_aux_transfer. Sean Anderson (13): drm: xlnx: Store base pointers in zynqmp_disp directly drm: xlnx: Fix kerneldoc drm: zynqmp_dp: Downgrade log level for aux retries message drm: zynqmp_dp: Adjust training values per-lane drm: zynqmp_dp: Rearrange zynqmp_dp for better padding drm: zynqmp_dp: Don't delay work drm: zynqmp_dp: Add locking drm: zynqmp_dp: Don't retrain the link in our IRQ drm: zynqmp_dp: Convert to a hard IRQ drm: zynqmp_dp: Use AUX IRQs instead of polling drm: zynqmp_dp: Split off several helper functions drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func drm: zynqmp_dp: Add debugfs interface for compliance testing Documentation/gpu/drivers.rst | 1 + Documentation/gpu/zynqmp.rst| 149 + MAINTAINERS | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 44 +- drivers/gpu/drm/xlnx/zynqmp_dp.c| 909 +--- drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 + drivers/gpu/drm/xlnx/zynqmp_kms.h | 4 +- 7 files changed, 1000 insertions(+), 109 deletions(-) create mode 100644 Documentation/gpu/zynqmp.rst
Re: [PATCH v3 06/13] drm: zynqmp_dp: Don't delay work
On 22/04/2024 21:45, Sean Anderson wrote:
We always call scheduled_delayed_work with no delay, so just use a
non-delayed work_struct instead.
Signed-off-by: Sean Anderson
---
Reviewed-by: Tomi Valkeinen
Tomi
Changes in v3:
- New
drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index f1834c8e3c02..59fed00a8f89 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -297,7 +297,7 @@ struct zynqmp_dp_config {
struct zynqmp_dp {
struct drm_dp_aux aux;
struct drm_bridge bridge;
- struct delayed_work hpd_work;
+ struct work_struct hpd_work;
struct drm_bridge *next_bridge;
struct device *dev;
@@ -1467,7 +1467,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct
drm_bridge *bridge,
struct zynqmp_dp *dp = bridge_to_dp(bridge);
dp->enabled = false;
- cancel_delayed_work(>hpd_work);
+ cancel_work(>hpd_work);
zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
drm_dp_dpcd_writeb(>aux, DP_SET_POWER, DP_SET_POWER_D3);
zynqmp_dp_write(dp, ZYNQMP_DP_TX_PHY_POWER_DOWN,
@@ -1603,8 +1603,7 @@ void zynqmp_dp_disable_vblank(struct zynqmp_dp *dp)
static void zynqmp_dp_hpd_work_func(struct work_struct *work)
{
- struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
- hpd_work.work);
+ struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp, hpd_work);
enum drm_connector_status status;
status = zynqmp_dp_bridge_detect(>bridge);
@@ -1633,7 +1632,7 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void
*data)
zynqmp_dpsub_drm_handle_vblank(dp->dpsub);
if (status & ZYNQMP_DP_INT_HPD_EVENT)
- schedule_delayed_work(>hpd_work, 0);
+ schedule_work(>hpd_work);
if (status & ZYNQMP_DP_INT_HPD_IRQ) {
int ret;
@@ -1675,7 +1674,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
dp->dpsub = dpsub;
dp->status = connector_status_disconnected;
- INIT_DELAYED_WORK(>hpd_work, zynqmp_dp_hpd_work_func);
+ INIT_WORK(>hpd_work, zynqmp_dp_hpd_work_func);
/* Acquire all resources (IOMEM, IRQ and PHYs). */
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
@@ -1775,7 +1774,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
disable_irq(dp->irq);
- cancel_delayed_work_sync(>hpd_work);
+ cancel_work_sync(>hpd_work);
zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, 0x);
Re: [PATCH v3 02/13] drm: xlnx: Fix kerneldoc
On 22/04/2024 21:45, Sean Anderson wrote:
Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
members.
Signed-off-by: Sean Anderson
---
Reviewed-by: Tomi Valkeinen
Tomi
Changes in v3:
- Split off documentation for base pointers to previous commit
Changes in v2:
- New
drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
drivers/gpu/drm/xlnx/zynqmp_kms.h | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
index 09ea01878f2a..b18554467e9c 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
@@ -53,6 +53,7 @@ enum zynqmp_dpsub_format {
* @drm: The DRM/KMS device data
* @bridge: The DP encoder bridge
* @disp: The display controller
+ * @layers: Video and graphics layers
* @dp: The DisplayPort controller
* @dma_align: DMA alignment constraint (must be a power of 2)
*/
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.h
b/drivers/gpu/drm/xlnx/zynqmp_kms.h
index 01be96b00e3f..cb13c6b8008e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.h
@@ -22,9 +22,9 @@
struct zynqmp_dpsub;
/**
- * struct zynqmp_dpsub - ZynqMP DisplayPort Subsystem DRM/KMS data
+ * struct zynqmp_dpsub_drm - ZynqMP DisplayPort Subsystem DRM/KMS data
* @dpsub: Backpointer to the DisplayPort subsystem
- * @drm: The DRM/KMS device
+ * @dev: The DRM/KMS device
* @planes: The DRM planes
* @crtc: The DRM CRTC
* @encoder: The dummy DRM encoder
Re: [PATCH v3 01/13] drm: xlnx: Store base pointers in zynqmp_disp directly
On 22/04/2024 21:45, Sean Anderson wrote:
The blend, avbuf, and audio members of zynqmp_disp are anonymous structs
with only one member each. This is rather pointless, so move the members
up a level.
Signed-off-by: Sean Anderson
---
Changes in v3:
- New
I would have renamed the fields to, e.g., "blend_base", but it doesn't
really matter as they are accessed only in a couple of places.
Reviewed-by: Tomi Valkeinen
Tomi
drivers/gpu/drm/xlnx/zynqmp_disp.c | 44 +-
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..94a3ac046373 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -128,24 +128,18 @@ struct zynqmp_disp_layer {
* struct zynqmp_disp - Display controller
* @dev: Device structure
* @dpsub: Display subsystem
- * @blend.base: Register I/O base address for the blender
- * @avbuf.base: Register I/O base address for the audio/video buffer manager
- * @audio.base: Registers I/O base address for the audio mixer
+ * @blend: Register I/O base address for the blender
+ * @avbuf: Register I/O base address for the audio/video buffer manager
+ * @audio: Registers I/O base address for the audio mixer
* @layers: Layers (planes)
*/
struct zynqmp_disp {
struct device *dev;
struct zynqmp_dpsub *dpsub;
- struct {
- void __iomem *base;
- } blend;
- struct {
- void __iomem *base;
- } avbuf;
- struct {
- void __iomem *base;
- } audio;
+ void __iomem *blend;
+ void __iomem *avbuf;
+ void __iomem *audio;
struct zynqmp_disp_layer layers[ZYNQMP_DPSUB_NUM_LAYERS];
};
@@ -356,12 +350,12 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] =
{
static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
{
- return readl(disp->avbuf.base + reg);
+ return readl(disp->avbuf + reg);
}
static void zynqmp_disp_avbuf_write(struct zynqmp_disp *disp, int reg, u32 val)
{
- writel(val, disp->avbuf.base + reg);
+ writel(val, disp->avbuf + reg);
}
static bool zynqmp_disp_layer_is_video(const struct zynqmp_disp_layer *layer)
@@ -587,7 +581,7 @@ static void zynqmp_disp_avbuf_disable(struct zynqmp_disp
*disp)
static void zynqmp_disp_blend_write(struct zynqmp_disp *disp, int reg, u32 val)
{
- writel(val, disp->blend.base + reg);
+ writel(val, disp->blend + reg);
}
/*
@@ -813,7 +807,7 @@ static void zynqmp_disp_blend_layer_disable(struct
zynqmp_disp *disp,
static void zynqmp_disp_audio_write(struct zynqmp_disp *disp, int reg, u32 val)
{
- writel(val, disp->audio.base + reg);
+ writel(val, disp->audio + reg);
}
/**
@@ -1237,21 +1231,21 @@ int zynqmp_disp_probe(struct zynqmp_dpsub *dpsub)
disp->dev = >dev;
disp->dpsub = dpsub;
- disp->blend.base = devm_platform_ioremap_resource_byname(pdev, "blend");
- if (IS_ERR(disp->blend.base)) {
- ret = PTR_ERR(disp->blend.base);
+ disp->blend = devm_platform_ioremap_resource_byname(pdev, "blend");
+ if (IS_ERR(disp->blend)) {
+ ret = PTR_ERR(disp->blend);
goto error;
}
- disp->avbuf.base = devm_platform_ioremap_resource_byname(pdev, "av_buf");
- if (IS_ERR(disp->avbuf.base)) {
- ret = PTR_ERR(disp->avbuf.base);
+ disp->avbuf = devm_platform_ioremap_resource_byname(pdev, "av_buf");
+ if (IS_ERR(disp->avbuf)) {
+ ret = PTR_ERR(disp->avbuf);
goto error;
}
- disp->audio.base = devm_platform_ioremap_resource_byname(pdev, "aud");
- if (IS_ERR(disp->audio.base)) {
- ret = PTR_ERR(disp->audio.base);
+ disp->audio = devm_platform_ioremap_resource_byname(pdev, "aud");
+ if (IS_ERR(disp->audio)) {
+ ret = PTR_ERR(disp->audio);
goto error;
}
Re: [PATCH v4 6/7] drm: xlnx: zynqmp_dpsub: Set input live format
On 16/04/2024 23:31, Anatoliy Klymenko wrote:
Program live video input format according to selected media bus format.
In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
almost certainly supports a single media bus format as its output. Expect
this to be delivered via the new bridge atomic state. Program DPSUB
registers accordingly.
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 92 --
drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +
drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 --
3 files changed, 90 insertions(+), 17 deletions(-)
Reviewed-by: Tomi Valkeinen
Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8cdd74a9b772..13157da0089e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -436,19 +436,29 @@ static void zynqmp_disp_avbuf_set_format(struct
zynqmp_disp *disp,
const struct zynqmp_disp_format *fmt)
{
unsigned int i;
- u32 val;
+ u32 val, reg;
- val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
- val &= zynqmp_disp_layer_is_video(layer)
- ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
- : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
- val |= fmt->buf_fmt;
- zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val);
+ layer->disp_fmt = fmt;
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
+ reg = ZYNQMP_DISP_AV_BUF_FMT;
+ val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
+ val &= zynqmp_disp_layer_is_video(layer)
+ ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
+ : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
+ val |= fmt->buf_fmt;
+ zynqmp_disp_avbuf_write(disp, reg, val);
+ } else {
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
+ : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
+ val = fmt->buf_fmt;
+ zynqmp_disp_avbuf_write(disp, reg, val);
+ }
for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
- unsigned int reg = zynqmp_disp_layer_is_video(layer)
-? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
-: ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
+ : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
}
@@ -926,6 +936,31 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer
*layer,
return NULL;
}
+/**
+ * zynqmp_disp_layer_find_live_format - Find format information for given
+ * media bus format
+ * @layer: The layer
+ * @drm_fmt: Media bus format to search
+ *
+ * Search display subsystem format information corresponding to the given media
+ * bus format @media_bus_format for the @layer, and return a pointer to the
+ * format descriptor.
+ *
+ * Return: A pointer to the format descriptor if found, NULL otherwise
+ */
+static const struct zynqmp_disp_format *
+zynqmp_disp_layer_find_live_format(struct zynqmp_disp_layer *layer,
+ u32 media_bus_format)
+{
+ unsigned int i;
+
+ for (i = 0; i < layer->info->num_formats; i++)
+ if (layer->info->formats[i].bus_fmt == media_bus_format)
+ return >info->formats[i];
+
+ return NULL;
+}
+
/**
* zynqmp_disp_layer_drm_formats - Return the DRM formats supported by the
layer
* @layer: The layer
@@ -1040,6 +1075,9 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer
*layer)
* @layer: The layer
* @info: The format info
*
+ * NOTE: Use zynqmp_disp_layer_set_live_format() to set media bus format for
+ * live video layers.
+ *
* Set the format for @layer to @info. The layer must be disabled.
*/
void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
@@ -1047,14 +1085,16 @@ void zynqmp_disp_layer_set_format(struct
zynqmp_disp_layer *layer,
{
unsigned int i;
+ if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE))
+ return;
+
layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
+ if (WARN_ON(!layer->disp_fmt))
+ return;
layer->drm_fmt = info;
zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
- if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
- return;
-
/*
* Set pconfig for each DMA channel to indicate they're part of a
* video group.
@@ -1074,6 +1114,32 @@ void zynqmp_disp_layer_set_format(struct
zynqmp_disp_layer *layer,
}
}
+/**
+ * zynqmp_disp_layer_set_liv
Re: [PATCH v4 4/7] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
On 16/04/2024 23:31, Anatoliy Klymenko wrote:
DPSUB in bridge mode supports multiple input media bus formats.
Announce the list of supported input media bus formats via
drm_bridge.atomic_get_input_bus_fmts callback. Introduce a set of live
input formats supported by DPSUB. Add safeguards to format list functions
to prevent their misuse in the different layer modes contexts.
Reviewed-by: Laurent Pinchart
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 110 +++--
drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +
drivers/gpu/drm/xlnx/zynqmp_dp.c | 31 +++
3 files changed, 139 insertions(+), 4 deletions(-)
Reviewed-by: Tomi Valkeinen
Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 891577475349..24f1f367b1d3 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
+ * @bus_fmt: Media bus format
* @buf_fmt: AV buffer format
* @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
* @sf: Scaling factors for color components
*/
struct zynqmp_disp_format {
u32 drm_fmt;
+ u32 bus_fmt;
u32 buf_fmt;
bool swap;
const u32 *sf;
@@ -182,6 +185,12 @@ static const u32 scaling_factors_565[] = {
ZYNQMP_DISP_AV_BUF_5BIT_SF,
};
+static const u32 scaling_factors_666[] = {
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+};
+
static const u32 scaling_factors_888[] = {
ZYNQMP_DISP_AV_BUF_8BIT_SF,
ZYNQMP_DISP_AV_BUF_8BIT_SF,
@@ -364,6 +373,41 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
},
};
+/* List of live video layer formats */
+static const struct zynqmp_disp_format avbuf_live_fmts[] = {
+ {
+ .drm_fmt= DRM_FORMAT_RGB565,
+ .bus_fmt= MEDIA_BUS_FMT_RGB666_1X18,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
+ .sf = scaling_factors_666,
+ }, {
+ .drm_fmt= DRM_FORMAT_RGB888,
+ .bus_fmt= MEDIA_BUS_FMT_RGB888_1X24,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_YUV422,
+ .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X16,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_YUV444,
+ .bus_fmt= MEDIA_BUS_FMT_VUY8_1X24,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_P210,
+ .bus_fmt= MEDIA_BUS_FMT_UYVY10_1X20,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_101010,
+ },
+};
+
static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
{
return readl(disp->avbuf.base + reg);
@@ -887,6 +931,11 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer
*layer,
* @layer: The layer
* @num_formats: Pointer to the returned number of formats
*
+ * NOTE: This function doesn't make sense for live video layers and will
+ * always return an empty list in such cases. zynqmp_disp_live_layer_formats()
+ * should be used to query a list of media bus formats supported by the live
+ * video input layer.
+ *
* Return: A newly allocated u32 array that stores all the DRM formats
* supported by the layer. The number of formats in the array is returned
* through the num_formats argument.
@@ -897,10 +946,17 @@ u32 *zynqmp_disp_layer_drm_formats(struct
zynqmp_disp_layer *layer,
unsigned int i;
u32 *formats;
+ if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
+ *num_formats = 0;
+ return NULL;
+ }
+
formats = kcalloc(layer->info->num_formats, sizeof(*formats),
GFP_KERNEL);
- if (!formats)
+ if (!formats) {
+
Re: [PATCH v4 2/7] drm: xlnx: zynqmp_dpsub: Update live format defines
On 16/04/2024 23:31, Anatoliy Klymenko wrote: Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register layout. These defines were never referenced before, so no other changes required. Reviewed-by: Laurent Pinchart Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h index f92a006d5070..fa3935384834 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h @@ -165,10 +165,10 @@ #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 0x1 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 0x2 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8) #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400 Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH RFC 2/2] pmdomain: ti-sci: Support retaining PD boot time state
On 15/04/2024 19:00, Tomi Valkeinen wrote: Add a new flag, TI_SCI_PD_KEEP_BOOT_STATE, which can be set in the dts when referring to power domains. When this flag is set, the ti-sci driver will check if the PD is currently enabled in the HW, and if so, set the GENPD_FLAG_ALWAYS_ON flag so that the PD will stay enabled. The main issue I'm trying to solve here is this: If the Display Subsystem (DSS) has been enabled by the bootloader, the related PD has also been enabled in the HW. When the tidss driver probes, the driver framework will automatically enable the PD. While executing the probe function it is very common for the probe to return EPROBE_DEFER, and, in rarer cases, an actual error. When this happens (probe() returns an error), the driver framework will automatically disable the related PD. Powering off the PD while the DSS is enabled and displaying a picture will cause the DSS HW to enter a bad state, from which (afaik) it can't be woken up except with full power-cycle. Trying to access the DSS in this state (e.g. when retrying the probe) will usually cause the board to hang sooner or later. Even if we wouldn't have this board-hangs issue, it's nice to be able to keep the DSS PD enabled: we want to keep the DSS enabled when the bootloader has enabled the screen. If, instead, we disable the PD at the first EPROBE_DEFER, the screen will (probably) go black. A few things occurred to me. The driver is supposed to clear the GENPD_FLAG_ALWAYS_ON when the driver has probed successfully. There are two possible issues with that: - Afaics, there's no API to do that, and currently I just clear the bit in genpd->flags. There's a clear race there, so some locking would be required. - This uses the GENPD_FLAG_ALWAYS_ON flag to say "PD is always on, until the driver has started". If the PD would have GENPD_FLAG_ALWAYS_ON set for other reasons, the driver would still go and clear the flag, which might break things. Also, unrelated to the above and not a problem in practice at the very moment, but I think clocks should also be dealt with somehow. Something, at early-ish boot stage, should mark the relevant clocks as in use, so that there's no chance they would be turned off when the main kernel has started (the main display driver is often a module). It would be nice to deal with all the above in a single place. I wonder if the tidss driver itself could somehow be split into two parts, an early part that would probe with minimal dependencies, mainly to reserve the core resources without doing any kind of DRM init. And a main part which would (somehow) finish the initialization at a later point, when we have the filesystem (for firmware) and the other bridge/panel drivers have probed. That can be somewhat achieved with simplefb or simpledrm, though, but we can't do any TI DSS specific things there, and it also creates a requirement to have either of those drivers built-in, and the related DT nodes to be added. Tomi Another option here would perhaps be to change the driver framework (drivers/base/platform.c) which attaches and detaches the PD, and make it somehow optional, allowing the driver the manage the PD. That option has two downsides: 1) the driver _has_ to manage the PD, which would rule out the use of simplefb and simpledrm, and 2) it would leave the PD in off state from Linux's perspective until a driver enables the PD, and that might mean that the PD gets actually disabled as part of normal system wide power management (disabling unused resources). Yet another option would be to do this outside the ti_sci_pm_domains driver: a piece of code that would somehow be ran after the ti_sci_pm_domains driver has probed (so that we have the PDs), but before tidss/simplefb/simpledrm probes. The problem here is the "somehow" part. Also, this would partly have the same issue 2) as mentioned above. TODO: If this approach is ok, sci-pm-domain.yaml needs to be extended. Also, it sounds a bit like the cell value is not a bit-mask, so maybe adding TI_SCI_PD_KEEP_BOOT_STATE flag this way is not fine. Signed-off-by: Tomi Valkeinen --- drivers/pmdomain/ti/ti_sci_pm_domains.c| 27 +-- include/dt-bindings/soc/ti,sci_pm_domain.h | 1 + 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c index 1510d5ddae3d..b71b390aaa39 100644 --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c @@ -103,7 +103,7 @@ static struct generic_pm_domain *ti_sci_pd_xlate( return ERR_PTR(-ENOENT); genpd_to_ti_sci_pd(genpd_data->domains[idx])->exclusive = - genpdspec->args[1]; + genpdspec->args[1] & TI_SCI_PD_EXCLUSIVE; return genpd_data->domains[idx]; } @@ -161,6 +161,8 @@ static int ti_sci_pm_domain_probe(struct platform_device *p
[PATCH RFC 0/2] pmdomain: ti-sci: Handling DSS boot splash
The first patch here is a fix for ti_sci_pm_domains to handle the case where the dts has two nodes which refer to the same PD. The second is the interesting one, and very much an RFC. Tomi Signed-off-by: Tomi Valkeinen --- Tomi Valkeinen (2): pmdomain: ti-sci: Fix duplicate PD referrals pmdomain: ti-sci: Support retaining PD boot time state drivers/pmdomain/ti/ti_sci_pm_domains.c| 47 -- include/dt-bindings/soc/ti,sci_pm_domain.h | 1 + 2 files changed, 45 insertions(+), 3 deletions(-) --- base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680 change-id: 20240415-ti-sci-pd-33b39f6b0586 Best regards, -- Tomi Valkeinen
[PATCH RFC 1/2] pmdomain: ti-sci: Fix duplicate PD referrals
When the dts file has multiple referrers to a single PD (e.g.
simple-framebuffer and dss nodes both point to the DSS power-domain) the
ti-sci driver will create two power domains, both with the same ID, and
that will cause problems as one of the power domains will hide the other
one.
Fix this checking if a PD with the ID has already been created, and only
create a PD for new IDs.
Fixes: efa5c01cd7ee ("soc: ti: ti_sci_pm_domains: switch to use multiple genpds
instead of one")
Signed-off-by: Tomi Valkeinen
---
drivers/pmdomain/ti/ti_sci_pm_domains.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c
b/drivers/pmdomain/ti/ti_sci_pm_domains.c
index 9dddf227a3a6..1510d5ddae3d 100644
--- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
+++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
@@ -114,6 +114,18 @@ static const struct of_device_id
ti_sci_pm_domain_matches[] = {
};
MODULE_DEVICE_TABLE(of, ti_sci_pm_domain_matches);
+static bool ti_sci_pm_idx_exists(struct ti_sci_genpd_provider *pd_provider,
u32 idx)
+{
+ struct ti_sci_pm_domain *pd;
+
+ list_for_each_entry(pd, _provider->pd_list, node) {
+ if (pd->idx == idx)
+ return true;
+ }
+
+ return false;
+}
+
static int ti_sci_pm_domain_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
@@ -149,8 +161,14 @@ static int ti_sci_pm_domain_probe(struct platform_device
*pdev)
break;
if (args.args_count >= 1 && args.np == dev->of_node) {
- if (args.args[0] > max_id)
+ if (args.args[0] > max_id) {
max_id = args.args[0];
+ } else {
+ if (ti_sci_pm_idx_exists(pd_provider,
args.args[0])) {
+ index++;
+ continue;
+ }
+ }
pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
if (!pd) {
--
2.34.1
[PATCH RFC 2/2] pmdomain: ti-sci: Support retaining PD boot time state
Add a new flag, TI_SCI_PD_KEEP_BOOT_STATE, which can be set in the dts
when referring to power domains. When this flag is set, the ti-sci
driver will check if the PD is currently enabled in the HW, and if so,
set the GENPD_FLAG_ALWAYS_ON flag so that the PD will stay enabled.
The main issue I'm trying to solve here is this:
If the Display Subsystem (DSS) has been enabled by the bootloader, the
related PD has also been enabled in the HW. When the tidss driver
probes, the driver framework will automatically enable the PD. While
executing the probe function it is very common for the probe to return
EPROBE_DEFER, and, in rarer cases, an actual error. When this happens
(probe() returns an error), the driver framework will automatically
disable the related PD.
Powering off the PD while the DSS is enabled and displaying a picture
will cause the DSS HW to enter a bad state, from which (afaik) it can't
be woken up except with full power-cycle. Trying to access the DSS in
this state (e.g. when retrying the probe) will usually cause the board
to hang sooner or later.
Even if we wouldn't have this board-hangs issue, it's nice to be able to
keep the DSS PD enabled: we want to keep the DSS enabled when the
bootloader has enabled the screen. If, instead, we disable the PD at the
first EPROBE_DEFER, the screen will (probably) go black.
Another option here would perhaps be to change the driver framework
(drivers/base/platform.c) which attaches and detaches the PD, and make
it somehow optional, allowing the driver the manage the PD. That option
has two downsides: 1) the driver _has_ to manage the PD, which would
rule out the use of simplefb and simpledrm, and 2) it would leave the PD
in off state from Linux's perspective until a driver enables the PD, and
that might mean that the PD gets actually disabled as part of normal
system wide power management (disabling unused resources).
Yet another option would be to do this outside the ti_sci_pm_domains
driver: a piece of code that would somehow be ran after the
ti_sci_pm_domains driver has probed (so that we have the PDs), but
before tidss/simplefb/simpledrm probes. The problem here is the
"somehow" part. Also, this would partly have the same issue 2) as
mentioned above.
TODO: If this approach is ok, sci-pm-domain.yaml needs to be extended.
Also, it sounds a bit like the cell value is not a bit-mask, so maybe
adding TI_SCI_PD_KEEP_BOOT_STATE flag this way is not fine.
Signed-off-by: Tomi Valkeinen
---
drivers/pmdomain/ti/ti_sci_pm_domains.c| 27 +--
include/dt-bindings/soc/ti,sci_pm_domain.h | 1 +
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c
b/drivers/pmdomain/ti/ti_sci_pm_domains.c
index 1510d5ddae3d..b71b390aaa39 100644
--- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
+++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
@@ -103,7 +103,7 @@ static struct generic_pm_domain *ti_sci_pd_xlate(
return ERR_PTR(-ENOENT);
genpd_to_ti_sci_pd(genpd_data->domains[idx])->exclusive =
- genpdspec->args[1];
+ genpdspec->args[1] & TI_SCI_PD_EXCLUSIVE;
return genpd_data->domains[idx];
}
@@ -161,6 +161,8 @@ static int ti_sci_pm_domain_probe(struct platform_device
*pdev)
break;
if (args.args_count >= 1 && args.np == dev->of_node) {
+ bool is_on = false;
+
if (args.args[0] > max_id) {
max_id = args.args[0];
} else {
@@ -189,7 +191,28 @@ static int ti_sci_pm_domain_probe(struct platform_device
*pdev)
pd->idx = args.args[0];
pd->parent = pd_provider;
- pm_genpd_init(>pd, NULL, true);
+ /*
+* If TI_SCI_PD_KEEP_BOOT_STATE is set and the
+* PD has been enabled by the bootloader, set
+* the PD to GENPD_FLAG_ALWAYS_ON. This will
+* make sure the PD stays enabled until a driver
+* takes over and clears the
GENPD_FLAG_ALWAYS_ON
+* flag.
+*/
+ if (args.args_count > 1 &&
+ args.args[1] & TI_SCI_PD_KEEP_BOOT_STATE) {
+ /*
+* We ignore any error here, and in case
+* of error just assume the PD is off.
+*/
+
pd_provider->ti_sci->ops.dev_ops.is_on(pd_provider
Re: [PATCH 12/21] drm/tilcdc: Allow build without __iowmb()
On 10/04/2024 20:04, Ville Syrjälä wrote: On Wed, Apr 10, 2024 at 06:25:17PM +0300, Ville Syrjälä wrote: On Wed, Apr 10, 2024 at 12:06:29PM +0300, Tomi Valkeinen wrote: On 08/04/2024 20:04, Ville Syrjala wrote: From: Ville Syrjälä __iowmb() isn't available on most architectures. Make its use optional so that the driver can be built on other architectures with COMPILE_TEST=y. Cc: Jyri Sarha Cc: Tomi Valkeinen Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f90e2dc3457c..44e4ada30fba 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) iowrite64(data, addr); #else +#ifdef __iowmb __iowmb(); +#endif /* This compiles to strd (=64-bit write) on ARM7 */ *(volatile u64 __force *)addr = __cpu_to_le64(data); #endif As the memory barrier is an important part there, would it be better to ifdef based on COMPILE_TEST, to make it clear why it's being done? I can do that if you prefer. What if someone tries to actually boot a kernel built with COMPILE_TEST=y on a machine with this hardware? Ah, right... #ifndef __iowmb #define __iowmb BUG #endif ? =) Maybe go with the original one, but with a comment like "allow compilation without __iowmb() for COMPILE_TEST" or such. Tomi
Re: [PATCH 12/21] drm/tilcdc: Allow build without __iowmb()
On 10/04/2024 18:25, Ville Syrjälä wrote: On Wed, Apr 10, 2024 at 12:06:29PM +0300, Tomi Valkeinen wrote: On 08/04/2024 20:04, Ville Syrjala wrote: From: Ville Syrjälä __iowmb() isn't available on most architectures. Make its use optional so that the driver can be built on other architectures with COMPILE_TEST=y. Cc: Jyri Sarha Cc: Tomi Valkeinen Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f90e2dc3457c..44e4ada30fba 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) iowrite64(data, addr); #else +#ifdef __iowmb __iowmb(); +#endif /* This compiles to strd (=64-bit write) on ARM7 */ *(volatile u64 __force *)addr = __cpu_to_le64(data); #endif As the memory barrier is an important part there, would it be better to ifdef based on COMPILE_TEST, to make it clear why it's being done? I can do that if you prefer. I suppose the real question is why iowrite64() doesn't work if a hand rolled version does work? If I recall right, there is (was?) no iowrite64. The bus is 32 bit anyway. But the two 32 bit registers written with the tilcdc_write64() have an annoying HW race, so we tried to find a method of writing them that reduces the chance of race to a minimum. Tomi
Re: [PATCH] drm: tilcdc: don't use devm_pinctrl_get_select_default() in probe
On 22/09/2023 10:37, Wolfram Sang wrote:
Since commit ab78029ecc34 ("drivers/pinctrl: grab default handles from
device core"), we can rely on device core for setting the default pins.
Signed-off-by: Wolfram Sang
---
drivers/gpu/drm/tilcdc/tilcdc_panel.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 9aefd010acde..68093d6b6b16 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -6,7 +6,6 @@
#include
#include
-#include
#include
#include
@@ -308,7 +307,6 @@ static int panel_probe(struct platform_device *pdev)
struct backlight_device *backlight;
struct panel_module *panel_mod;
struct tilcdc_module *mod;
- struct pinctrl *pinctrl;
int ret;
/* bail out early if no DT data: */
@@ -342,10 +340,6 @@ static int panel_probe(struct platform_device *pdev)
tilcdc_module_init(mod, "panel", _module_ops);
- pinctrl = devm_pinctrl_get_select_default(>dev);
- if (IS_ERR(pinctrl))
- dev_warn(>dev, "pins are not configured\n");
-
panel_mod->timings = of_get_display_timings(node);
if (!panel_mod->timings) {
dev_err(>dev, "could not get panel timings\n");
Thanks, applying to drm-misc-next.
Tomi
Re: [PATCH] drm/omap: dmm_tiler: drop driver owner assignment
On 30/03/2024 22:28, Krzysztof Kozlowski wrote:
Core in platform_driver_register() already sets the .owner, so driver
does not need to. Whatever is set here will be anyway overwritten by
main driver calling platform_driver_register().
Signed-off-by: Krzysztof Kozlowski
---
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 9753c1e1f994..1aca3060333e 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -1212,7 +1212,6 @@ struct platform_driver omap_dmm_driver = {
.probe = omap_dmm_probe,
.remove_new = omap_dmm_remove,
.driver = {
- .owner = THIS_MODULE,
.name = DMM_DRIVER_NAME,
.of_match_table = of_match_ptr(dmm_of_match),
.pm = _dmm_pm_ops,
Thanks, applying to drm-misc-next.
Tomi
Re: [PATCH] drm: xlnx: db: fix a memory leak in probe
On 04/04/2024 10:32, Dan Carpenter wrote:
Free "dp" before returning.
Fixes: be318d01a903 ("drm: xlnx: dp: Reset DisplayPort IP")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 5a40aa1d4283..8a15d18a65a6 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1716,7 +1716,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
ret = zynqmp_dp_reset(dp, true);
if (ret < 0)
- return ret;
+ goto err_free;
ret = zynqmp_dp_reset(dp, false);
if (ret < 0)
Thanks, applying to drm-misc-next.
Tomi
Re: [PATCH 15/21] drm/omap: Allow build with COMPILE_TEST=y
On 08/04/2024 20:04, Ville Syrjala wrote: From: Ville Syrjälä Allow omapdrm to be built with COMPILE_TEST=y for greater coverage. FIXME: Still borked due to ? Cc: Tomi Valkeinen Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/omapdrm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig index 6c49270cb290..85ed92042b74 100644 --- a/drivers/gpu/drm/omapdrm/Kconfig +++ b/drivers/gpu/drm/omapdrm/Kconfig @@ -2,7 +2,7 @@ config DRM_OMAP tristate "OMAP DRM" depends on DRM && OF - depends on ARCH_OMAP2PLUS + depends on ARCH_OMAP2PLUS || COMPILE_TEST select DRM_KMS_HELPER select FB_DMAMEM_HELPERS_DEFERRED if DRM_FBDEV_EMULATION select VIDEOMODE_HELPERS Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 14/21] drm/omap: Open code phys_to_page()
On 08/04/2024 20:04, Ville Syrjala wrote:
From: Ville Syrjälä
phys_to_page() is not available on most architectures.
Just open code it like msm does. Allows COMPILE_TEST=y
builds of omapdrm on other architectures.
Cc: Tomi Valkeinen
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
b/drivers/gpu/drm/omapdrm/omap_gem.c
index 3421e8389222..c4454e7f1c94 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1022,8 +1022,8 @@ struct sg_table *omap_gem_get_sg(struct drm_gem_object
*obj,
if (addr) {
for_each_sg(sgt->sgl, sg, count, i) {
- sg_set_page(sg, phys_to_page(addr), len,
- offset_in_page(addr));
+ sg_set_page(sg, pfn_to_page(__phys_to_pfn(addr)),
+ len, offset_in_page(addr));
sg_dma_address(sg) = addr;
sg_dma_len(sg) = len;
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH 13/21] drm/tilcdc: Allow build with COMPILE_TEST=y
On 08/04/2024 20:04, Ville Syrjala wrote:
From: Ville Syrjälä
Allow tilcdc to be built with COMPILE_TEST=y for greater
coverage. Builds fine on x86/x86_64 at least.
Cc: Jyri Sarha
Cc: Tomi Valkeinen
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/tilcdc/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
index d3bd2d7a181e..1897ef91c70b 100644
--- a/drivers/gpu/drm/tilcdc/Kconfig
+++ b/drivers/gpu/drm/tilcdc/Kconfig
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
config DRM_TILCDC
tristate "DRM Support for TI LCDC Display Controller"
- depends on DRM && OF && ARM
+ depends on DRM && OF && (ARM || COMPILE_TEST)
select DRM_KMS_HELPER
select DRM_GEM_DMA_HELPER
select DRM_BRIDGE
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH 12/21] drm/tilcdc: Allow build without __iowmb()
On 08/04/2024 20:04, Ville Syrjala wrote: From: Ville Syrjälä __iowmb() isn't available on most architectures. Make its use optional so that the driver can be built on other architectures with COMPILE_TEST=y. Cc: Jyri Sarha Cc: Tomi Valkeinen Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f90e2dc3457c..44e4ada30fba 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) iowrite64(data, addr); #else +#ifdef __iowmb __iowmb(); +#endif /* This compiles to strd (=64-bit write) on ARM7 */ *(volatile u64 __force *)addr = __cpu_to_le64(data); #endif As the memory barrier is an important part there, would it be better to ifdef based on COMPILE_TEST, to make it clear why it's being done? Tomi
Re: [PATCH v3 6/9] drm: xlnx: zynqmp_dpsub: Set input live format
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Program live video input format according to selected media bus format.
In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
almost certainly supports a single media bus format as its output. Expect
this to be delivered via the new bridge atomic state. Program DPSUB
registers accordingly. Update zynqmp_disp_layer_set_format() API to fit
both live and non-live layer types.
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 66 +-
drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 +---
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 0c2b3f4bffa6..a385d22d428e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -436,19 +436,28 @@ static void zynqmp_disp_avbuf_set_format(struct
zynqmp_disp *disp,
const struct zynqmp_disp_format *fmt)
{
unsigned int i;
- u32 val;
+ u32 val, reg;
- val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
- val &= zynqmp_disp_layer_is_video(layer)
- ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
- : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
- val |= fmt->buf_fmt;
- zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val);
+ layer->disp_fmt = fmt;
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
+ reg = ZYNQMP_DISP_AV_BUF_FMT;
+ val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
+ val &= zynqmp_disp_layer_is_video(layer)
+ ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
+ : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
+ val |= fmt->buf_fmt;
+ } else {
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
+ : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
+ val = fmt->buf_fmt;
+ }
+ zynqmp_disp_avbuf_write(disp, reg, val);
Just write the registers inside the above if-else blocks.
for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
- unsigned int reg = zynqmp_disp_layer_is_video(layer)
-? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
-: ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
+ : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
}
@@ -902,25 +911,33 @@ static void zynqmp_disp_audio_disable(struct zynqmp_disp
*disp)
*/
/**
- * zynqmp_disp_layer_find_format - Find format information for a DRM format
+ * zynqmp_disp_layer_find_format - Find format information for a DRM or media
+ * bus format
* @layer: The layer
- * @drm_fmt: DRM format to search
+ * @drm_or_bus_format: DRM or media bus format
*
* Search display subsystem format information corresponding to the given DRM
- * format @drm_fmt for the @layer, and return a pointer to the format
- * descriptor.
+ * or media bus format @drm_or_bus_format for the @layer, and return a pointer
+ * to the format descriptor. Search key choice depends on @layer mode, for live
+ * layers search is done by zynqmp_disp_format.bus_fmt, and for non-live layers
+ * zynqmp_disp_format.drm_fmt is used.
Here also I recommend creating separate funcs for the fourcc and mbus
versions. They are different types, even if they happen to fit into u32.
*
* Return: A pointer to the format descriptor if found, NULL otherwise
*/
static const struct zynqmp_disp_format *
zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer,
- u32 drm_fmt)
+ u32 drm_or_bus_format)
{
unsigned int i;
+ const struct zynqmp_disp_format *disp_format;
for (i = 0; i < layer->info->num_formats; i++) {
- if (layer->info->formats[i].drm_fmt == drm_fmt)
- return >info->formats[i];
+ disp_format = >info->formats[i];
+ if ((layer->mode == ZYNQMP_DPSUB_LAYER_LIVE &&
+disp_format->bus_fmt == drm_or_bus_format) ||
+ (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE &&
+disp_format->drm_fmt == drm_or_bus_format))
+ return disp_format;
}
return NULL;
@@ -992,20 +1009,25 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer
*layer)
/**
* zynqmp_disp_layer_set_format - Set the layer format
* @layer: The layer
- * @info: The format info
+ * @drm_or_bus_format: DRM or media bus format
*
* Set the format for @layer to @info. The layer must be
Re: [PATCH v3 5/9] drm: xlnx: zynqmp_dpsub: Minimize usage of global flag
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer
context. This flag signals whether the driver runs in DRM CRTC or DRM
bridge mode, assuming that all display layers share the same live or
non-live mode of operation. Using per-layer mode instead of global flag
will simplify future support of the hybrid scenario.
Remove redundant checks in DMA request/release contexts as
zynqmp_disp_layer.info is well-defined for all layer types, including the
correct number of DMA channels required for each particular layer.
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index abdc3971b193..0c2b3f4bffa6 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -980,7 +980,7 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer
*layer)
{
unsigned int i;
- if (layer->disp->dpsub->dma_enabled) {
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
for (i = 0; i < layer->drm_fmt->num_planes; i++)
dmaengine_terminate_sync(layer->dmas[i].chan);
}
@@ -1006,7 +1006,7 @@ void zynqmp_disp_layer_set_format(struct
zynqmp_disp_layer *layer,
zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
- if (!layer->disp->dpsub->dma_enabled)
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
return;
/*
@@ -1044,7 +1044,7 @@ int zynqmp_disp_layer_update(struct zynqmp_disp_layer
*layer,
const struct drm_format_info *info = layer->drm_fmt;
unsigned int i;
- if (!layer->disp->dpsub->dma_enabled)
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
return 0;
for (i = 0; i < info->num_planes; i++) {
@@ -1094,9 +1094,6 @@ static void zynqmp_disp_layer_release_dma(struct
zynqmp_disp *disp,
{
unsigned int i;
- if (!layer->info || !disp->dpsub->dma_enabled)
- return;
-
for (i = 0; i < layer->info->num_channels; i++) {
struct zynqmp_disp_layer_dma *dma = >dmas[i];
@@ -1137,9 +1134,6 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp *disp,
unsigned int i;
int ret;
- if (!disp->dpsub->dma_enabled)
- return 0;
-
for (i = 0; i < layer->info->num_channels; i++) {
struct zynqmp_disp_layer_dma *dma = >dmas[i];
char dma_channel_name[16];
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v3 4/9] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
DPSUB in bridge mode supports multiple input media bus formats.
Announce the list of supported input media bus formats via
drm_bridge.atomic_get_input_bus_fmts callback.
Introduce a set of live input formats, supported by DPSUB.
Rename zynqmp_disp_layer_drm_formats() to zynqmp_disp_layer_formats() to
reflect semantics for both live and non-live layer format lists.
Reviewed-by: Laurent Pinchart
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 76 +-
drivers/gpu/drm/xlnx/zynqmp_disp.h | 4 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c | 31
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 101 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index e6d26ef60e89..abdc3971b193 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
+ * @bus_fmt: Media bus format
* @buf_fmt: AV buffer format
* @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
* @sf: Scaling factors for color components
*/
struct zynqmp_disp_format {
u32 drm_fmt;
+ u32 bus_fmt;
u32 buf_fmt;
bool swap;
const u32 *sf;
@@ -182,6 +185,12 @@ static const u32 scaling_factors_565[] = {
ZYNQMP_DISP_AV_BUF_5BIT_SF,
};
+static const u32 scaling_factors_666[] = {
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+};
+
static const u32 scaling_factors_888[] = {
ZYNQMP_DISP_AV_BUF_8BIT_SF,
ZYNQMP_DISP_AV_BUF_8BIT_SF,
@@ -364,6 +373,41 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
},
};
+/* List of live video layer formats */
+static const struct zynqmp_disp_format avbuf_live_fmts[] = {
+ {
+ .drm_fmt= DRM_FORMAT_RGB565,
+ .bus_fmt= MEDIA_BUS_FMT_RGB666_1X18,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
+ .sf = scaling_factors_666,
+ }, {
+ .drm_fmt= DRM_FORMAT_RGB888,
+ .bus_fmt= MEDIA_BUS_FMT_RGB888_1X24,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_YUV422,
+ .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X16,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_YUV444,
+ .bus_fmt= MEDIA_BUS_FMT_VUY8_1X24,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_P210,
+ .bus_fmt= MEDIA_BUS_FMT_UYVY10_1X20,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_101010,
+ },
+};
+
static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
{
return readl(disp->avbuf.base + reg);
@@ -883,16 +927,17 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer
*layer,
}
/**
- * zynqmp_disp_layer_drm_formats - Return the DRM formats supported by the
layer
+ * zynqmp_disp_layer_formats - Return DRM or media bus formats supported by
+ * the layer
* @layer: The layer
* @num_formats: Pointer to the returned number of formats
*
- * Return: A newly allocated u32 array that stores all the DRM formats
+ * Return: A newly allocated u32 array that stores all DRM or media bus formats
* supported by the layer. The number of formats in the array is returned
* through the num_formats argument.
*/
-u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
- unsigned int *num_formats)
+u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
+ unsigned int *num_formats)
{
unsigned int i;
u32 *formats;
@@ -903,7 +948,9 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer
*layer,
return NULL;
Re: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Set layer mode of operation (live or dma-based) during layer creation.
Each DPSUB layer mode of operation is defined by corresponding DT node port
connection, so it is possible to assign it during layer object creation.
Previously it was set in layer enable functions, although it is too late
as setting layer format depends on layer mode, and should be done before
given layer enabled.
Signed-off-by: Anatoliy Klymenko
Reviewed-by: Laurent Pinchart
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 20
drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +
drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8a39b3accce5..e6d26ef60e89 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -64,6 +64,16 @@
#define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
+/**
+ * enum zynqmp_dpsub_layer_mode - Layer mode
+ * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
+ * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
+ */
+enum zynqmp_dpsub_layer_mode {
+ ZYNQMP_DPSUB_LAYER_NONLIVE,
+ ZYNQMP_DPSUB_LAYER_LIVE,
+};
+
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
@@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct
zynqmp_disp_layer *layer,
/**
* zynqmp_disp_layer_enable - Enable a layer
* @layer: The layer
- * @mode: Operating mode of layer
*
* Enable the @layer in the audio/video buffer manager and the blender. DMA
* channels are started separately by zynqmp_disp_layer_update().
*/
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode)
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
{
- layer->mode = mode;
zynqmp_disp_avbuf_enable_video(layer->disp, layer);
zynqmp_disp_blend_layer_enable(layer->disp, layer);
}
@@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp
*disp)
layer->id = i;
layer->disp = disp;
layer->info = _info[i];
+ /* For now assume dpsub works in either live or non-live mode
for both layers.
+* Hybrid mode is not supported yet.
+*/
This comment style is not according to the style guide, and in fact you
fix it in the patch 4. So please fix it here instead.
Tomi
+ layer->mode = disp->dpsub->dma_enabled ?
ZYNQMP_DPSUB_LAYER_NONLIVE
+ :
ZYNQMP_DPSUB_LAYER_LIVE;
ret = zynqmp_disp_layer_request_dma(disp, layer);
if (ret)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
b/drivers/gpu/drm/xlnx/zynqmp_disp.h
index 123cffac08be..9b8b202224d9 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
@@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
ZYNQMP_DPSUB_LAYER_GFX,
};
-/**
- * enum zynqmp_dpsub_layer_mode - Layer mode
- * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
- * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
- */
-enum zynqmp_dpsub_layer_mode {
- ZYNQMP_DPSUB_LAYER_NONLIVE,
- ZYNQMP_DPSUB_LAYER_LIVE,
-};
-
void zynqmp_disp_enable(struct zynqmp_disp *disp);
void zynqmp_disp_disable(struct zynqmp_disp *disp);
int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
@@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp
*disp,
u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
unsigned int *num_formats);
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode);
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
const struct drm_format_info *info);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 1846c4971fd8..04b6bcac3b07 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
/* TODO: Make the format configurable. */
info = drm_format_info(DRM_FORMAT_YUV422);
zynqmp_disp_layer_set_format(layer, info);
- zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
+ zynqmp_disp_layer_enable(layer);
if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index
Re: [PATCH v3 3/9] drm: xlnx: zynqmp_dpsub: Add connected live layer helper
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Add a helper function capturing the first connected live display layer
discovery logic.
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_dp.c | 37 +++--
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 04b6bcac3b07..4faafdd76798 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1276,28 +1276,40 @@ static void zynqmp_dp_encoder_mode_set_stream(struct
zynqmp_dp *dp,
* DISP Configuration
*/
+/**
+ * zynqmp_dp_disp_connected_live_layer - Return the first connected live layer
+ * @dp: DisplayPort IP core structure
+ *
+ * Return: The first connected live display layer or NULL if none of the live
+ * layer is connected.
"layers"
Reviewed-by: Tomi Valkeinen
Tomi
+ */
+static struct zynqmp_disp_layer *
+zynqmp_dp_disp_connected_live_layer(struct zynqmp_dp *dp)
+{
+ if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
+ return dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_VID];
+ else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
+ return dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX];
+ else
+ return NULL;
+}
+
static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
struct drm_bridge_state *old_bridge_state)
{
- enum zynqmp_dpsub_layer_id layer_id;
struct zynqmp_disp_layer *layer;
const struct drm_format_info *info;
- if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
- layer_id = ZYNQMP_DPSUB_LAYER_VID;
- else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
- layer_id = ZYNQMP_DPSUB_LAYER_GFX;
- else
+ layer = zynqmp_dp_disp_connected_live_layer(dp);
+ if (!layer)
return;
- layer = dp->dpsub->layers[layer_id];
-
/* TODO: Make the format configurable. */
info = drm_format_info(DRM_FORMAT_YUV422);
zynqmp_disp_layer_set_format(layer, info);
zynqmp_disp_layer_enable(layer);
- if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
+ if (layer == dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX])
zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
else
zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, false, 0);
@@ -1310,11 +1322,8 @@ static void zynqmp_dp_disp_disable(struct zynqmp_dp *dp,
{
struct zynqmp_disp_layer *layer;
- if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
- layer = dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_VID];
- else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
- layer = dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX];
- else
+ layer = zynqmp_dp_disp_connected_live_layer(dp);
+ if (!layer)
return;
zynqmp_disp_disable(dp->dpsub->disp);
Re: [PATCH v3 2/9] drm: xlnx: zynqmp_dpsub: Update live format defines
On 21/03/2024 22:43, Anatoliy Klymenko wrote: Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register layout. I think this description needs a bit more. Mention that the defines are not currently used, so we can change them like this without any other change. Tomi Reviewed-by: Laurent Pinchart Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h index f92a006d5070..fa3935384834 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h @@ -165,10 +165,10 @@ #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 0x1 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 0x2 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8) #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400
Re: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Set layer mode of operation (live or dma-based) during layer creation.
Each DPSUB layer mode of operation is defined by corresponding DT node port
connection, so it is possible to assign it during layer object creation.
Previously it was set in layer enable functions, although it is too late
as setting layer format depends on layer mode, and should be done before
given layer enabled.
Signed-off-by: Anatoliy Klymenko
Reviewed-by: Laurent Pinchart
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 20
drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +
drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 19 insertions(+), 18 deletions(-)
Reviewed-by: Tomi Valkeinen
Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8a39b3accce5..e6d26ef60e89 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -64,6 +64,16 @@
#define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
+/**
+ * enum zynqmp_dpsub_layer_mode - Layer mode
+ * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
+ * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
+ */
+enum zynqmp_dpsub_layer_mode {
+ ZYNQMP_DPSUB_LAYER_NONLIVE,
+ ZYNQMP_DPSUB_LAYER_LIVE,
+};
+
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
@@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct
zynqmp_disp_layer *layer,
/**
* zynqmp_disp_layer_enable - Enable a layer
* @layer: The layer
- * @mode: Operating mode of layer
*
* Enable the @layer in the audio/video buffer manager and the blender. DMA
* channels are started separately by zynqmp_disp_layer_update().
*/
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode)
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
{
- layer->mode = mode;
zynqmp_disp_avbuf_enable_video(layer->disp, layer);
zynqmp_disp_blend_layer_enable(layer->disp, layer);
}
@@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp
*disp)
layer->id = i;
layer->disp = disp;
layer->info = _info[i];
+ /* For now assume dpsub works in either live or non-live mode
for both layers.
+* Hybrid mode is not supported yet.
+*/
+ layer->mode = disp->dpsub->dma_enabled ?
ZYNQMP_DPSUB_LAYER_NONLIVE
+ :
ZYNQMP_DPSUB_LAYER_LIVE;
ret = zynqmp_disp_layer_request_dma(disp, layer);
if (ret)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
b/drivers/gpu/drm/xlnx/zynqmp_disp.h
index 123cffac08be..9b8b202224d9 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
@@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
ZYNQMP_DPSUB_LAYER_GFX,
};
-/**
- * enum zynqmp_dpsub_layer_mode - Layer mode
- * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
- * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
- */
-enum zynqmp_dpsub_layer_mode {
- ZYNQMP_DPSUB_LAYER_NONLIVE,
- ZYNQMP_DPSUB_LAYER_LIVE,
-};
-
void zynqmp_disp_enable(struct zynqmp_disp *disp);
void zynqmp_disp_disable(struct zynqmp_disp *disp);
int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
@@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp
*disp,
u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
unsigned int *num_formats);
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode);
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
const struct drm_format_info *info);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 1846c4971fd8..04b6bcac3b07 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
/* TODO: Make the format configurable. */
info = drm_format_info(DRM_FORMAT_YUV422);
zynqmp_disp_layer_set_format(layer, info);
- zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
+ zynqmp_disp_layer_enable(layer);
if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index db3bb4afbfc4..43bf416b33d5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp
[PATCH] MAINTAINERS: Add myself as maintainer for Xilinx DRM drivers
Add myself as a co-maintainer for Xilinx DRM drivers to help Laurent. Signed-off-by: Tomi Valkeinen --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 1aabf1c15bb3..79ef5a6bf21b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7306,6 +7306,7 @@ F:drivers/gpu/drm/xen/ DRM DRIVERS FOR XILINX M: Laurent Pinchart +M: Tomi Valkeinen L: dri-devel@lists.freedesktop.org S: Maintained T: git git://anongit.freedesktop.org/drm/drm-misc --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240327-xilinx-maintainer-f6020f6cba4d Best regards, -- Tomi Valkeinen
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
On 22/03/2024 23:22, Sean Anderson wrote:
On 3/22/24 14:09, Tomi Valkeinen wrote:
On 22/03/2024 18:18, Sean Anderson wrote:
On 3/22/24 01:32, Tomi Valkeinen wrote:
On 21/03/2024 21:17, Sean Anderson wrote:
On 3/21/24 15:08, Tomi Valkeinen wrote:
On 21/03/2024 20:01, Sean Anderson wrote:
On 3/21/24 13:25, Tomi Valkeinen wrote:
On 21/03/2024 17:52, Sean Anderson wrote:
On 3/20/24 02:53, Tomi Valkeinen wrote:
On 20/03/2024 00:51, Sean Anderson wrote:
Do we need to handle interrupts while either delayed work is being done?
Probably not.
If we do need a delayed work, would just one work be enough which
handles both HPD_EVENT and HPD_IRQ, instead of two?
Maybe, but then we need to determine which pending events we need to
handle. I think since we have only two events it will be easier to just
have separate workqueues.
The less concurrency, the better...Which is why it would be nice to do it all
in the threaded irq.
Yeah, but we can use a mutex for this which means there is not too much
interesting going on.
Ok. Yep, if we get (hopefully) a single mutex with clearly defined fields that
it protects, I'm ok with workqueues.
I'd still prefer just one workqueue, though...
Yeah, but then we need a spinlock or something to tell the workqueue what it
should do.
Yep. We could also always look at the HPD (if we drop the big sleeps) in the
wq, and have a flag for the HPD IRQ, which would reduce the state to a single
bit.
How about something like
zynqmp_dp_irq_handler(...)
{
/* Read status and handle underflow/overflow/vblank */
status &= ZYNQMP_DP_INT_HPD_EVENT | ZYNQMP_DP_INT_HPD_IRQ;
if (status) {
atomic_or(status, >status);
return IRQ_WAKE_THREAD;
}
return IRQ_HANDLED;
}
zynqmp_dp_thread_handler(...)
{
status = atomic_xchg(>status, 0);
/* process HPD stuff */
}
which gets rid of the workqueue too.
I like it. We can't use IRQF_ONESHOT, as that would keep the irq masked while
the threaded handler is being ran. I don't think that's a problem, but just
something to keep in mind that both handlers can run concurrently.
Actually, I'm not sure we can do it like this. Imagine we have something
like
CPU 0 CPU 1
zynqmp_dp_thread_handler()
atomic_xchg()
__handle_irq_event_percpu
zynqmp_dp_irq_handler()
atomic_or()
return IRQ_WAIT_THREAD
__irq_wake_thread()
test_and_set_bit(IRQTF_RUNTHREAD, ...)
return
return IRQ_HANDLED
and whoops we now have bits set in dp->status but the thread isn't
running. I don't think there's a way to fix this without locking (or two
In your example above, the IRQTF_RUNTHREAD has been cleared by the
threaded-irq before calling zynqmp_dp_thread_handler. So the hard-irq
will set that flag before the zynqmp_dp_thread_handler() returns.
When zynqmp_dp_thread_handler() returns, the execution will go to
irq_wait_for_interrupt(). That function will notice the IRQTF_RUNTHREAD
flag (and clear it), and run the zynqmp_dp_thread_handler() again.
So if I'm not mistaken, when the hard-irq function returns
IRQ_WAKE_THREAD, it's always guaranteed that a "fresh" run of the
threaded handler will be ran.
I think that makes sense, as I'm not sure how threaded handlers without
IRQF_ONESHOT could be used if that were not the case. I hope I'm right
in my analysis =).
Tomi
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
On 22/03/2024 18:18, Sean Anderson wrote:
On 3/22/24 01:32, Tomi Valkeinen wrote:
On 21/03/2024 21:17, Sean Anderson wrote:
On 3/21/24 15:08, Tomi Valkeinen wrote:
On 21/03/2024 20:01, Sean Anderson wrote:
On 3/21/24 13:25, Tomi Valkeinen wrote:
On 21/03/2024 17:52, Sean Anderson wrote:
On 3/20/24 02:53, Tomi Valkeinen wrote:
On 20/03/2024 00:51, Sean Anderson wrote:
Do we need to handle interrupts while either delayed work is being done?
Probably not.
If we do need a delayed work, would just one work be enough which
handles both HPD_EVENT and HPD_IRQ, instead of two?
Maybe, but then we need to determine which pending events we need to
handle. I think since we have only two events it will be easier to just
have separate workqueues.
The less concurrency, the better...Which is why it would be nice to do it all
in the threaded irq.
Yeah, but we can use a mutex for this which means there is not too much
interesting going on.
Ok. Yep, if we get (hopefully) a single mutex with clearly defined fields that
it protects, I'm ok with workqueues.
I'd still prefer just one workqueue, though...
Yeah, but then we need a spinlock or something to tell the workqueue what it
should do.
Yep. We could also always look at the HPD (if we drop the big sleeps) in the
wq, and have a flag for the HPD IRQ, which would reduce the state to a single
bit.
How about something like
zynqmp_dp_irq_handler(...)
{
/* Read status and handle underflow/overflow/vblank */
status &= ZYNQMP_DP_INT_HPD_EVENT | ZYNQMP_DP_INT_HPD_IRQ;
if (status) {
atomic_or(status, >status);
return IRQ_WAKE_THREAD;
}
return IRQ_HANDLED;
}
zynqmp_dp_thread_handler(...)
{
status = atomic_xchg(>status, 0);
/* process HPD stuff */
}
which gets rid of the workqueue too.
I like it. We can't use IRQF_ONESHOT, as that would keep the irq masked
while the threaded handler is being ran. I don't think that's a problem,
but just something to keep in mind that both handlers can run concurrently.
Tomi
Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
Hi,
On 08/03/2024 22:47, Sean Anderson wrote:
We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
initialized. We do this before zynqmp_dpsub_drm_init since that calls
drm_bridge_attach. This fixes the following lockdep warning:
[ 19.217084] [ cut here ]
[ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582
__mutex_lock+0x4bc/0x550
[ 19.241696] Modules linked in:
[ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
[ 19.252046] Hardware name: xlnx,zynqmp (DT)
[ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
[ 19.261795] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 19.269104] pc : __mutex_lock+0x4bc/0x550
[ 19.273364] lr : __mutex_lock+0x4bc/0x550
[ 19.277592] sp : ffc085c5bbe0
[ 19.281066] x29: ffc085c5bbe0 x28: x27: ff88009417f8
[ 19.288624] x26: ff8800941788 x25: ff8800020008 x24: ffc082aa3000
[ 19.296227] x23: ffc080d90e3c x22: 0002 x21:
[ 19.303744] x20: x19: ff88002f5210 x18:
[ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
[ 19.318922] x14: x13: 284e4f5f4e524157 x12: 0001
[ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
[ 19.334003] x8 : 0001ff88003f3888 x7 : x6 :
[ 19.341537] x5 : x4 : 1668 x3 :
[ 19.349054] x2 : x1 : x0 : ff88003f3880
[ 19.356581] Call trace:
[ 19.359160] __mutex_lock+0x4bc/0x550
[ 19.363032] mutex_lock_nested+0x24/0x30
[ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
[ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
[ 19.376364] process_one_work+0x3ac/0x988
[ 19.380660] worker_thread+0x398/0x694
[ 19.384736] kthread+0x1bc/0x1c0
[ 19.388241] ret_from_fork+0x10/0x20
[ 19.392031] irq event stamp: 183
[ 19.395450] hardirqs last enabled at (183): []
finish_task_switch.isra.0+0xa8/0x2d4
[ 19.405140] hardirqs last disabled at (182): []
__schedule+0x714/0xd04
[ 19.413612] softirqs last enabled at (114): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.423128] softirqs last disabled at (110): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.432614] ---[ end trace ]---
Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
Signed-off-by: Sean Anderson
---
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..639fff2c693f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device
*pdev)
if (ret)
goto err_dp;
+ drm_bridge_add(dpsub->bridge);
if (dpsub->dma_enabled) {
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device
*pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
+ drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
I sent a similar patch:
https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03...@ideasonboard.com/
I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where
the bridge is set up, so it felt like a logical place. You add it later,
just before the bridge is used the first time.
I like mine a bit more as it has all the bridge code in the same place,
but I also wonder if there might be some risks in adding the bridge
early (before zynqmp_disp_probe()), although I can't see any issue right
away...
In any case, as this works for me too:
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc
On 21/03/2024 17:33, Sean Anderson wrote:
On 3/20/24 02:05, Randy Dunlap wrote:
On 3/19/24 22:42, Tomi Valkeinen wrote:
On 20/03/2024 00:51, Sean Anderson wrote:
Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
members.
Signed-off-by: Sean Anderson
---
Changes in v2:
- New
drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++---
drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
drivers/gpu/drm/xlnx/zynqmp_kms.h | 4 ++--
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..f79bf3fb8110 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
* struct zynqmp_disp - Display controller
* @dev: Device structure
* @dpsub: Display subsystem
- * @blend.base: Register I/O base address for the blender
- * @avbuf.base: Register I/O base address for the audio/video buffer manager
- * @audio.base: Registers I/O base address for the audio mixer
+ * @blend: Register I/O base address for the blender
+ * @avbuf: Register I/O base address for the audio/video buffer manager
+ * @audio: Registers I/O base address for the audio mixer
Afaics, the kernel doc guide:
https://docs.kernel.org/doc-guide/kernel-doc.html#nested-structs-unions
says that the current version is correct. Or is the issue that while, say,
'base' is documented, 'blend' was not?
Hi,
I would do it more like so:
---
drivers/gpu/drm/xlnx/zynqmp_disp.c |3 +++
1 file changed, 3 insertions(+)
diff -- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -128,8 +128,11 @@ struct zynqmp_disp_layer {
* struct zynqmp_disp - Display controller
* @dev: Device structure
* @dpsub: Display subsystem
+ * @blend: blender iomem info
* @blend.base: Register I/O base address for the blender
+ * @avbuf: audio/video buffer iomem info
* @avbuf.base: Register I/O base address for the audio/video buffer manager
+ * @audio: audio mixer iomem info
* @audio.base: Registers I/O base address for the audio mixer
* @layers: Layers (planes)
*/
but in my testing, Sean's way or my way result in no warning/errors.
The specific errors are:
../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
struct member 'blend' not described in 'zynqmp_disp'
../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
struct member 'avbuf' not described in 'zynqmp_disp'
../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
struct member 'audio' not described in 'zynqmp_disp'
I don't see the need to document a single-member struct twice. Actually,
But if only the struct is documented, then we're documenting the wrong
thing. A tool showing to the user what blend.base is would miss that
documentation.
maybe it would be better to just lift the .base member to live in
zynqmp_disp. But I think that would be better in another series.
Yes, there's not much point with the structs.
Tomi
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
On 21/03/2024 21:17, Sean Anderson wrote: On 3/21/24 15:08, Tomi Valkeinen wrote: On 21/03/2024 20:01, Sean Anderson wrote: On 3/21/24 13:25, Tomi Valkeinen wrote: On 21/03/2024 17:52, Sean Anderson wrote: On 3/20/24 02:53, Tomi Valkeinen wrote: On 20/03/2024 00:51, Sean Anderson wrote: Retraining the link can take a while, and might involve waiting for DPCD reads/writes to complete. This is inappropriate for an IRQ handler. Just schedule this work for later completion. This is racy, but will be fixed in the next commit. You should add the locks first, and use them here, rather than first adding a buggy commit and fixing it in the next one. I didn't think I could add the locks first since I only noticed the IRQ was threaded right before sending out this series. So yeah, we could add locking, add the workqueue, and then unthread the IRQ. Signed-off-by: Sean Anderson --- Actually, on second look this IRQ is threaded. So why do we have a workqueue for HPD events? Maybe we should make it unthreaded? Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded. We could move the queued work to be inside the threaded irq handler, but with a quick look, the HPD work has lines like "msleep(100)" (and that's inside a for loop...), which is probably not a good thing to do even in threaded irq handler. Although I'm not sure if that code is good to have anywhere. Why do we even have such code in the HPD work path... We already got the HPD interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" even mean... The documentation for this bit is | HPD_STATE 0 ro 0x0 Contains the raw state of the HPD pin on the DisplayPort connector. So I think the idea is to perform some debouncing. Hmm, it just looks a bit odd to me. It can sleep for a second. And the wording "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" makes it sound like some kind of a hack... The docs mention debounce once: https://docs.amd.com/r/en-US/pg299-v-dp-txss1/Hot-Plug-Detection Are you sure this is the right document? This seems to be documentation for [1]. Is that instantiated as a hard block on the ZynqMP? [1] https://www.xilinx.com/products/intellectual-property/ef-di-displayport.html You're right, wrong document. The registers and bitfield names I looked at just matched, so I didn't think it through... The right doc says even less: https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/Upon-HPD-Assertion But it's not immediately obvious what the SW must do and what's done by the HW. Debounce is not mentioned later, e.g. in the HPD Event Handling. But if debounce is needed, wouldn't it be perhaps in a few milliseconds, instead of hundreds of milliseconds... Well, the DP spec says | If the HPD is the result of a new device being connected, either | directly to the Source device (signaled by a long HPD), –or– downstream | of a Branch device (indicated by incrementing the DFP_COUNT field value | in the DOWN_STREAM_PORT_COUNT register (DPCD 7h[3:0]) and signaled | by an IRQ_HPD pulse), the Source device shall read the new DisplayID or | legacy EDID that has been made available to it to ensure that content | being transmitted over the link is able to be properly received and | rendered. | | Informative Note: If the HPD signal toggling (or bouncing) is the | result of the Hot Unplug followed by Hot Plug of a | cable-connector assembly, the HPD signal is likely | to remain unstable during the de-bouncing period, | which is in the order of tens of milliseconds. The | Source device may either check the HPD signal’s | stability before initiating an AUX read transaction, | –or– immediately initiate the AUX read transaction | after each HPD rising edge. So a 100 ms delay seems plausible for some monitors. I read the text above as "it may take tens of milliseconds for HPD to stabilize". So polling it for total of 100ms sounds fine, but we're polling it for 1000ms. And I think checking for stability is fine, but for detect() I think it goes overboard: if the cable is disconnected, every detect call spends a second checking for HPD, even if we haven't seen any sign of an HPD =). And if we're checking the HPD stability, wouldn't we, say, poll the HPD for some time, and see if it stays the same? At the moment the code proceeds right away if HPD is high, but keeps polling if HPD is low. That said, maybe we can just skip this and always read the DPCD. If the HPD is bouncing, is the AUX line also unstable? I don't mind a HPD stability check, I think it makes sense as (I think) the HW doesn't handle de-bouncing here. I think think it could be much much shorter than what it is now, and tha
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
On 21/03/2024 20:01, Sean Anderson wrote: On 3/21/24 13:25, Tomi Valkeinen wrote: On 21/03/2024 17:52, Sean Anderson wrote: On 3/20/24 02:53, Tomi Valkeinen wrote: On 20/03/2024 00:51, Sean Anderson wrote: Retraining the link can take a while, and might involve waiting for DPCD reads/writes to complete. This is inappropriate for an IRQ handler. Just schedule this work for later completion. This is racy, but will be fixed in the next commit. You should add the locks first, and use them here, rather than first adding a buggy commit and fixing it in the next one. I didn't think I could add the locks first since I only noticed the IRQ was threaded right before sending out this series. So yeah, we could add locking, add the workqueue, and then unthread the IRQ. Signed-off-by: Sean Anderson --- Actually, on second look this IRQ is threaded. So why do we have a workqueue for HPD events? Maybe we should make it unthreaded? Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded. We could move the queued work to be inside the threaded irq handler, but with a quick look, the HPD work has lines like "msleep(100)" (and that's inside a for loop...), which is probably not a good thing to do even in threaded irq handler. Although I'm not sure if that code is good to have anywhere. Why do we even have such code in the HPD work path... We already got the HPD interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" even mean... The documentation for this bit is | HPD_STATE0ro0x0Contains the raw state of the HPD pin on the DisplayPort connector. So I think the idea is to perform some debouncing. Hmm, it just looks a bit odd to me. It can sleep for a second. And the wording "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" makes it sound like some kind of a hack... The docs mention debounce once: https://docs.amd.com/r/en-US/pg299-v-dp-txss1/Hot-Plug-Detection Are you sure this is the right document? This seems to be documentation for [1]. Is that instantiated as a hard block on the ZynqMP? [1] https://www.xilinx.com/products/intellectual-property/ef-di-displayport.html You're right, wrong document. The registers and bitfield names I looked at just matched, so I didn't think it through... The right doc says even less: https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/Upon-HPD-Assertion But it's not immediately obvious what the SW must do and what's done by the HW. Debounce is not mentioned later, e.g. in the HPD Event Handling. But if debounce is needed, wouldn't it be perhaps in a few milliseconds, instead of hundreds of milliseconds... Well, the DP spec says | If the HPD is the result of a new device being connected, either | directly to the Source device (signaled by a long HPD), –or– downstream | of a Branch device (indicated by incrementing the DFP_COUNT field value | in the DOWN_STREAM_PORT_COUNT register (DPCD 7h[3:0]) and signaled | by an IRQ_HPD pulse), the Source device shall read the new DisplayID or | legacy EDID that has been made available to it to ensure that content | being transmitted over the link is able to be properly received and | rendered. | | Informative Note: If the HPD signal toggling (or bouncing) is the | result of the Hot Unplug followed by Hot Plug of a | cable-connector assembly, the HPD signal is likely | to remain unstable during the de-bouncing period, | which is in the order of tens of milliseconds. The | Source device may either check the HPD signal’s | stability before initiating an AUX read transaction, | –or– immediately initiate the AUX read transaction | after each HPD rising edge. So a 100 ms delay seems plausible for some monitors. I read the text above as "it may take tens of milliseconds for HPD to stabilize". So polling it for total of 100ms sounds fine, but we're polling it for 1000ms. And I think checking for stability is fine, but for detect() I think it goes overboard: if the cable is disconnected, every detect call spends a second checking for HPD, even if we haven't seen any sign of an HPD =). And if we're checking the HPD stability, wouldn't we, say, poll the HPD for some time, and see if it stays the same? At the moment the code proceeds right away if HPD is high, but keeps polling if HPD is low. That said, maybe we can just skip this and always read the DPCD. If the HPD is bouncing, is the AUX line also unstable? I don't mind a HPD stability check, I think it makes sense as (I think) the HW doesn't handle de-bouncing here. I think think it could be much much shorter than what it is now, and that it would make sense to observe the HPD for a period, instead of just waiting for th
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
On 21/03/2024 17:52, Sean Anderson wrote: On 3/20/24 02:53, Tomi Valkeinen wrote: On 20/03/2024 00:51, Sean Anderson wrote: Retraining the link can take a while, and might involve waiting for DPCD reads/writes to complete. This is inappropriate for an IRQ handler. Just schedule this work for later completion. This is racy, but will be fixed in the next commit. You should add the locks first, and use them here, rather than first adding a buggy commit and fixing it in the next one. I didn't think I could add the locks first since I only noticed the IRQ was threaded right before sending out this series. So yeah, we could add locking, add the workqueue, and then unthread the IRQ. Signed-off-by: Sean Anderson --- Actually, on second look this IRQ is threaded. So why do we have a workqueue for HPD events? Maybe we should make it unthreaded? Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded. We could move the queued work to be inside the threaded irq handler, but with a quick look, the HPD work has lines like "msleep(100)" (and that's inside a for loop...), which is probably not a good thing to do even in threaded irq handler. Although I'm not sure if that code is good to have anywhere. Why do we even have such code in the HPD work path... We already got the HPD interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" even mean... The documentation for this bit is | HPD_STATE 0 ro 0x0 Contains the raw state of the HPD pin on the DisplayPort connector. So I think the idea is to perform some debouncing. Hmm, it just looks a bit odd to me. It can sleep for a second. And the wording "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" makes it sound like some kind of a hack... The docs mention debounce once: https://docs.amd.com/r/en-US/pg299-v-dp-txss1/Hot-Plug-Detection But it's not immediately obvious what the SW must do and what's done by the HW. Debounce is not mentioned later, e.g. in the HPD Event Handling. But if debounce is needed, wouldn't it be perhaps in a few milliseconds, instead of hundreds of milliseconds... zynqmp_dp_bridge_detect() is used for drm_bridge_funcs.detect(), and if the cable is not connected, it'll sleep for 1 second (probably more) until returning not connected. It just doesn't sound correct to me. Well, it's not part of this patch as such, but related to the amount of time we spend in the interrupt handler (and also the detect()). Would it be possible to clean up the work funcs a bit (I haven't looked a the new work func yet), to remove the worst extra sleeps, and just do all that inside the threaded irq handler? Probably not, since a HPD IRQ results in link retraining, which can take a while. But is it any different if you have a workqueue? Isn't a threaded interrupt handler basically the same thing? Probably at least the zynqmp_dp_hpd_work_func() could be done in the threaded irq just fine, if the insane 1s sleep can be dropped. Do we need to handle interrupts while either delayed work is being done? Probably not. If we do need a delayed work, would just one work be enough which handles both HPD_EVENT and HPD_IRQ, instead of two? Maybe, but then we need to determine which pending events we need to handle. I think since we have only two events it will be easier to just have separate workqueues. The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq. Tomi
Re: [PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing
On 21/03/2024 18:08, Sean Anderson wrote: On 3/20/24 03:49, Tomi Valkeinen wrote: On 20/03/2024 00:51, Sean Anderson wrote: +/** + * enum test_pattern - Test patterns for test testing "for test testing"? =) @@ -1655,6 +2321,9 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work) u8 status[DP_LINK_STATUS_SIZE + 2]; int err; +if (READ_ONCE(dp->ignore_hpd)) +return; + mutex_lock(>lock); err = drm_dp_dpcd_read(>aux, DP_SINK_COUNT, status, DP_LINK_STATUS_SIZE + 2); Why do you need READ/WRITE_ONCE() for ignore_hpd? It's not protected by dp->lock so we don't have to take it for zynqmp_dp_hpd_work_func. Although maybe we should make a version of zynqmp_dp_bridge_detect which assumes we already hold the lock. Does using the macros solve some potential issue, or is it just for documenting that this variable is accessed without lock? Tomi
Re: [PATCH v2 2/4] dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs
On 20/03/2024 17:37, Rob Herring wrote: On Tue, Mar 19, 2024 at 10:22:37AM +0200, Tomi Valkeinen wrote: The DP subsystem for ZynqMP support audio via two channels, and the DP DMA has dma-engines for those channels. For some reason the DT binding has not specified those channels, even if the picture included in xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. New required entries is an ABI change. This message kind of indicates it was a mistake, but should be a lot more explicit. Are things broken without the entries? Need 'Fixes'? I'll improve the desc for the next version. So, yes, it's an ABI change, and as far as I can guess (I can't figure out any other reason), the audio DMAs were left out by mistake or misunderstanding. The Linux driver has not supported audio, so this has not been an issue and nothing is broken. Now that this series adds the audio support, I had to add the audio DMAs. I considered making the DMAs optional in the DT, but that doesn't sound right, even if that would keep the ABI compatibility (wouldn't it?). The driver I add in this series does consider the audio DMAs as optional, though. If they're not present, the driver will continue without audio support. So, strictly speaking I think this is a fix to the original commit that adds the DT node, but as the driver using the audio DMAs comes in only now, I think there's no need for the 'Fixes' and backporting. I'm happy to change the approach if you think some other way is better. Tomi Add the two audio DMAs to the binding. Signed-off-by: Tomi Valkeinen --- .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml| 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml index 554f9d5809d4..6b754d4f260e 100644 --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml @@ -100,12 +100,16 @@ properties: - description: Video layer, plane 1 (U/V or U) - description: Video layer, plane 2 (V) - description: Graphics layer + - description: Audio channel 0 + - description: Audio channel 1 dma-names: items: - const: vid0 - const: vid1 - const: vid2 - const: gfx0 + - const: aud0 + - const: aud1 phys: description: PHYs for the DP data lanes @@ -194,11 +198,13 @@ examples: power-domains = <_dp>; resets = < ZYNQMP_RESET_DP>; -dma-names = "vid0", "vid1", "vid2", "gfx0"; +dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; dmas = <_dpdma 0>, <_dpdma 1>, <_dpdma 2>, - <_dpdma 3>; + <_dpdma 3>, + <_dpdma 4>, + <_dpdma 5>; phys = < 1 PHY_TYPE_DP 0 3>, < 0 PHY_TYPE_DP 1 3>; -- 2.34.1
Re: [PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing
On 20/03/2024 00:51, Sean Anderson wrote: +/** + * enum test_pattern - Test patterns for test testing "for test testing"? =) @@ -1655,6 +2321,9 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work) u8 status[DP_LINK_STATUS_SIZE + 2]; int err; + if (READ_ONCE(dp->ignore_hpd)) + return; + mutex_lock(>lock); err = drm_dp_dpcd_read(>aux, DP_SINK_COUNT, status, DP_LINK_STATUS_SIZE + 2); Why do you need READ/WRITE_ONCE() for ignore_hpd? Tomi
Re: [PATCH v2 7/8] drm: zynqmp_dp: Split off several helper functions
On 20/03/2024 00:51, Sean Anderson wrote: In preparation for supporting compliance testing, split off several helper functions. No functional change intended. Signed-off-by: Sean Anderson Reviewed-by: Laurent Pinchart --- (no changes since v1) drivers/gpu/drm/xlnx/zynqmp_dp.c | 49 ++-- 1 file changed, 34 insertions(+), 15 deletions(-) Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
On 20/03/2024 00:51, Sean Anderson wrote: Retraining the link can take a while, and might involve waiting for DPCD reads/writes to complete. This is inappropriate for an IRQ handler. Just schedule this work for later completion. This is racy, but will be fixed in the next commit. You should add the locks first, and use them here, rather than first adding a buggy commit and fixing it in the next one. Signed-off-by: Sean Anderson --- Actually, on second look this IRQ is threaded. So why do we have a workqueue for HPD events? Maybe we should make it unthreaded? Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded. We could move the queued work to be inside the threaded irq handler, but with a quick look, the HPD work has lines like "msleep(100)" (and that's inside a for loop...), which is probably not a good thing to do even in threaded irq handler. Although I'm not sure if that code is good to have anywhere. Why do we even have such code in the HPD work path... We already got the HPD interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" even mean... Would it be possible to clean up the work funcs a bit (I haven't looked a the new work func yet), to remove the worst extra sleeps, and just do all that inside the threaded irq handler? Do we need to handle interrupts while either delayed work is being done? If we do need a delayed work, would just one work be enough which handles both HPD_EVENT and HPD_IRQ, instead of two? Tomi
Re: [PATCH v2 4/8] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding
On 20/03/2024 00:51, Sean Anderson wrote:
Sort the members of struct zynqmp_dp to reduce padding necessary for
alignment.
Signed-off-by: Sean Anderson
---
Changes in v2:
- New
drivers/gpu/drm/xlnx/zynqmp_dp.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 8635b5673386..f1834c8e3c02 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -255,10 +255,10 @@ struct zynqmp_dp_link_config {
* @fmt: format identifier string
*/
struct zynqmp_dp_mode {
- u8 bw_code;
- u8 lane_cnt;
- int pclock;
const char *fmt;
+ int pclock;
+ u8 bw_code;
+ u8 lane_cnt;
};
/**
@@ -295,27 +295,27 @@ struct zynqmp_dp_config {
* @train_set: set of training data
*/
struct zynqmp_dp {
+ struct drm_dp_aux aux;
+ struct drm_bridge bridge;
+ struct delayed_work hpd_work;
+
+ struct drm_bridge *next_bridge;
struct device *dev;
struct zynqmp_dpsub *dpsub;
void __iomem *iomem;
struct reset_control *reset;
- int irq;
-
- struct drm_bridge bridge;
- struct drm_bridge *next_bridge;
-
- struct zynqmp_dp_config config;
- struct drm_dp_aux aux;
struct phy *phy[ZYNQMP_DP_MAX_LANES];
- u8 num_lanes;
- struct delayed_work hpd_work;
+
enum drm_connector_status status;
+ int irq;
bool enabled;
- u8 dpcd[DP_RECEIVER_CAP_SIZE];
- struct zynqmp_dp_link_config link_config;
struct zynqmp_dp_mode mode;
+ struct zynqmp_dp_link_config link_config;
+ struct zynqmp_dp_config config;
+ u8 dpcd[DP_RECEIVER_CAP_SIZE];
u8 train_set[ZYNQMP_DP_MAX_LANES];
+ u8 num_lanes;
};
static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)
If you change the order of the fields, you should change the order in
the kernel doc accordingly.
To be honest, I'm not sure if I like this patch. We have usually one
instance of these structs allocated. How many bytes do we save?
I'm fine with getting easy savings by changing the field order in some
cases, but I think the "human" side of the order is important too:
usually the fields are grouped in some way, and ordered so that the more
base or generic ones are first, and fields for some specific feature are
later. And fields protected by a lock should be grouped together, with
their lock being first/last in that group.
Looking at the zynqmp_dp struct with this patch, I get an urge to start
moving things around: dev, dpsub, iomem, etc first, hpd somewhere later.
Base config fields like config, num_lanes, irq would be grouped
together. Etc.
Tomi
Re: [PATCH v2 3/8] drm: zynqmp_dp: Adjust training values per-lane
On 20/03/2024 00:51, Sean Anderson wrote:
The feedback we get from the DPRX is per-lane. Make changes using this
information, instead of picking the maximum values from all lanes. This
results in more-consistent training on marginal links.
Signed-off-by: Sean Anderson
---
(no changes since v1)
drivers/gpu/drm/xlnx/zynqmp_dp.c | 23 ---
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 98a32e6a0459..8635b5673386 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -605,28 +605,21 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
u8 link_status[DP_LINK_STATUS_SIZE])
{
u8 *train_set = dp->train_set;
- u8 voltage = 0, preemphasis = 0;
u8 i;
for (i = 0; i < dp->mode.lane_cnt; i++) {
- u8 v = drm_dp_get_adjust_request_voltage(link_status, i);
- u8 p = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
+ u8 voltage = drm_dp_get_adjust_request_voltage(link_status, i);
+ u8 preemphasis =
+ drm_dp_get_adjust_request_pre_emphasis(link_status, i);
- if (v > voltage)
- voltage = v;
+ if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
+ voltage |= DP_TRAIN_MAX_SWING_REACHED;
- if (p > preemphasis)
- preemphasis = p;
- }
+ if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
+ preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
- if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
- voltage |= DP_TRAIN_MAX_SWING_REACHED;
-
- if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
- preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
-
- for (i = 0; i < dp->mode.lane_cnt; i++)
train_set[i] = voltage | preemphasis;
+ }
}
/**
Looks fine to me, but a few cosmetic suggestions, feel free to ignore if
not to your liking:
1)
u8 voltage, preemphasis;
voltage = drm_dp_get_adjust_request_voltage(link_status, i);
preemphasis = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
2)
for (unsigned int i = 0; i < dp->mode.lane_cnt; i++)
3)
dp->train_set[i] = voltage | preemphasis;
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v2 2/8] drm: zynqmp_dp: Downgrade log level for aux retries message
On 20/03/2024 00:51, Sean Anderson wrote:
Enable this message for verbose debugging only as it is otherwise
printed after every AUX message, quickly filling the log buffer.
Signed-off-by: Sean Anderson
Reviewed-by: Laurent Pinchart
---
(no changes since v1)
drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index a0606fab0e22..98a32e6a0459 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1006,7 +1006,7 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct
drm_dp_aux_msg *msg)
msg->buffer, msg->size,
>reply);
if (!ret) {
- dev_dbg(dp->dev, "aux %d retries\n", i);
+ dev_vdbg(dp->dev, "aux %d retries\n", i);
return msg->size;
}
Yes, these are annoying... In my work branch I had added "if (i)" there,
so that this is only printed if there actually are retries.
But this is fine too (or even dropping the print totally), so:
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc
On 20/03/2024 00:51, Sean Anderson wrote:
Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
members.
Signed-off-by: Sean Anderson
---
Changes in v2:
- New
drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++---
drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
drivers/gpu/drm/xlnx/zynqmp_kms.h | 4 ++--
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..f79bf3fb8110 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
* struct zynqmp_disp - Display controller
* @dev: Device structure
* @dpsub: Display subsystem
- * @blend.base: Register I/O base address for the blender
- * @avbuf.base: Register I/O base address for the audio/video buffer manager
- * @audio.base: Registers I/O base address for the audio mixer
+ * @blend: Register I/O base address for the blender
+ * @avbuf: Register I/O base address for the audio/video buffer manager
+ * @audio: Registers I/O base address for the audio mixer
Afaics, the kernel doc guide:
https://docs.kernel.org/doc-guide/kernel-doc.html#nested-structs-unions
says that the current version is correct. Or is the issue that while,
say, 'base' is documented, 'blend' was not?
Tomi
[PATCH v2 4/4] drm: xlnx: zynqmp_dpsub: Add DP audio support
Add basic DisplayPort audio support.
Support non-live audio playback from two PCMs (DMA channels), and the
volume control in the audio mixer.
As older dtb files may not have the audio DMA channels defined, the
driver will just mark the audio support as disabled if the audio DMA is
missing, and will continue with only display support.
Note: Reset doesn't seem to work (ZYNQMP_DISP_AUD_SOFT_RESET). If we do
a reset, audio playback won't start again even if, afaics, we do set up
all the necessary registers. So, at the moment, resetting the audio
block in dp_dai_hw_free() is commented out.
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/xlnx/Kconfig| 9 +
drivers/gpu/drm/xlnx/Makefile | 1 +
drivers/gpu/drm/xlnx/zynqmp_disp.c | 50
drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 7 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c| 54 ++--
drivers/gpu/drm/xlnx/zynqmp_dp.h| 7 +
drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 39 +--
drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 15 +-
9 files changed, 540 insertions(+), 103 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
index 68ee897de9d7..d88cfbaf2863 100644
--- a/drivers/gpu/drm/xlnx/Kconfig
+++ b/drivers/gpu/drm/xlnx/Kconfig
@@ -15,3 +15,12 @@ config DRM_ZYNQMP_DPSUB
This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
this option if you have a Xilinx ZynqMP SoC with DisplayPort
subsystem.
+
+config DRM_ZYNQMP_DPSUB_AUDIO
+ bool "ZynqMP DisplayPort Audio Support"
+ depends on DRM_ZYNQMP_DPSUB
+ depends on SND && SND_SOC
+ select SND_SOC_GENERIC_DMAENGINE_PCM
+ help
+ Choose this option to enable DisplayPort audio support in the ZynqMP
+ DisplayPort driver.
diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
index ea1422a39502..ab6e2ffd7e8d 100644
--- a/drivers/gpu/drm/xlnx/Makefile
+++ b/drivers/gpu/drm/xlnx/Makefile
@@ -1,2 +1,3 @@
zynqmp-dpsub-y := zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o zynqmp_kms.o
+zynqmp-dpsub-$(CONFIG_DRM_ZYNQMP_DPSUB_AUDIO) += zynqmp_dp_audio.o
obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..d2bf0e2d0135 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -130,7 +130,6 @@ struct zynqmp_disp_layer {
* @dpsub: Display subsystem
* @blend.base: Register I/O base address for the blender
* @avbuf.base: Register I/O base address for the audio/video buffer manager
- * @audio.base: Registers I/O base address for the audio mixer
* @layers: Layers (planes)
*/
struct zynqmp_disp {
@@ -143,9 +142,6 @@ struct zynqmp_disp {
struct {
void __iomem *base;
} avbuf;
- struct {
- void __iomem *base;
- } audio;
struct zynqmp_disp_layer layers[ZYNQMP_DPSUB_NUM_LAYERS];
};
@@ -807,42 +803,6 @@ static void zynqmp_disp_blend_layer_disable(struct
zynqmp_disp *disp,
csc_zero_offsets);
}
-/*
-
- * Audio Mixer
- */
-
-static void zynqmp_disp_audio_write(struct zynqmp_disp *disp, int reg, u32 val)
-{
- writel(val, disp->audio.base + reg);
-}
-
-/**
- * zynqmp_disp_audio_enable - Enable the audio mixer
- * @disp: Display controller
- *
- * Enable the audio mixer by de-asserting the soft reset. The audio state is
set to
- * default values by the reset, set the default mixer volume explicitly.
- */
-static void zynqmp_disp_audio_enable(struct zynqmp_disp *disp)
-{
- /* Clear the audio soft reset register as it's an non-reset flop. */
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_SOFT_RESET, 0);
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_MIXER_VOLUME,
- ZYNQMP_DISP_AUD_MIXER_VOLUME_NO_SCALE);
-}
-
-/**
- * zynqmp_disp_audio_disable - Disable the audio mixer
- * @disp: Display controller
- *
- * Disable the audio mixer by asserting its soft reset.
- */
-static void zynqmp_disp_audio_disable(struct zynqmp_disp *disp)
-{
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_SOFT_RESET,
- ZYNQMP_DISP_AUD_SOFT_RESET_AUD_SRST);
-}
-
/*
-
* ZynqMP Display Layer & DRM Plane
*/
@@ -1169,8 +1129,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
true);
zynqmp_disp_avbuf_enable_channels(disp);
zynqmp_disp_avbuf_enable_audio(disp);
-
- zynqmp_disp_audio_enable(disp);
}
/**
@@ -1179,8 +1137,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
*/
void zynqmp_disp_disable
[PATCH v2 3/4] arm64: dts: zynqmp: Add DMA for DP audio
Add the two DMA channels used for the DisplayPort audio to the
zynqmp_dpsub node.
Signed-off-by: Tomi Valkeinen
---
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index eaba466804bc..811d80cbf4c5 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -1036,11 +1036,14 @@ zynqmp_dpsub: display@fd4a {
"dp_vtc_pixel_clk_in";
power-domains = <_firmware PD_DP>;
resets = <_reset ZYNQMP_RESET_DP>;
- dma-names = "vid0", "vid1", "vid2", "gfx0";
+ dma-names = "vid0", "vid1", "vid2", "gfx0",
+ "aud0", "aud1";
dmas = <_dpdma ZYNQMP_DPDMA_VIDEO0>,
<_dpdma ZYNQMP_DPDMA_VIDEO1>,
<_dpdma ZYNQMP_DPDMA_VIDEO2>,
- <_dpdma ZYNQMP_DPDMA_GRAPHICS>;
+ <_dpdma ZYNQMP_DPDMA_GRAPHICS>,
+ <_dpdma ZYNQMP_DPDMA_AUDIO0>,
+ <_dpdma ZYNQMP_DPDMA_AUDIO1>;
ports {
#address-cells = <1>;
--
2.34.1
[PATCH v2 2/4] dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs
The DP subsystem for ZynqMP support audio via two channels, and the DP DMA has dma-engines for those channels. For some reason the DT binding has not specified those channels, even if the picture included in xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. Add the two audio DMAs to the binding. Signed-off-by: Tomi Valkeinen --- .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml| 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml index 554f9d5809d4..6b754d4f260e 100644 --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml @@ -100,12 +100,16 @@ properties: - description: Video layer, plane 1 (U/V or U) - description: Video layer, plane 2 (V) - description: Graphics layer + - description: Audio channel 0 + - description: Audio channel 1 dma-names: items: - const: vid0 - const: vid1 - const: vid2 - const: gfx0 + - const: aud0 + - const: aud1 phys: description: PHYs for the DP data lanes @@ -194,11 +198,13 @@ examples: power-domains = <_dp>; resets = < ZYNQMP_RESET_DP>; -dma-names = "vid0", "vid1", "vid2", "gfx0"; +dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; dmas = <_dpdma 0>, <_dpdma 1>, <_dpdma 2>, - <_dpdma 3>; + <_dpdma 3>, + <_dpdma 4>, + <_dpdma 5>; phys = < 1 PHY_TYPE_DP 0 3>, < 0 PHY_TYPE_DP 1 3>; -- 2.34.1
[PATCH v2 1/4] ASoC: dmaengine_pcm: Allow passing component name via config
At the moment we cannot instantiate two dmaengine_pcms with the same
parent device, as the components will be named the same, leading to
conflicts.
Add 'name' field to the snd_dmaengine_pcm_config, and use that (if
defined) as the component name instead of deriving the component name
from the device.
Signed-off-by: Tomi Valkeinen
---
include/sound/dmaengine_pcm.h | 2 ++
sound/soc/soc-core.c | 8 +---
sound/soc/soc-generic-dmaengine-pcm.c | 3 +++
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index d70c55f17df7..c11aaf8079fb 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -118,6 +118,7 @@ int snd_dmaengine_pcm_refine_runtime_hwparams(
* which do not use devicetree.
* @process: Callback used to apply processing on samples transferred from/to
* user space.
+ * @name: Component name. If null, dev_name will be used.
* @compat_filter_fn: Will be used as the filter function when requesting a
* channel for platforms which do not use devicetree. The filter parameter
* will be the DAI's DMA data.
@@ -143,6 +144,7 @@ struct snd_dmaengine_pcm_config {
int (*process)(struct snd_pcm_substream *substream,
int channel, unsigned long hwoff,
unsigned long bytes);
+ const char *name;
dma_filter_fn compat_filter_fn;
struct device *dma_dev;
const char *chan_names[SNDRV_PCM_STREAM_LAST + 1];
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 516350533e73..772d67065611 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2792,10 +2792,12 @@ int snd_soc_component_initialize(struct
snd_soc_component *component,
INIT_LIST_HEAD(>list);
mutex_init(>io_mutex);
- component->name = fmt_single_name(dev, >id);
if (!component->name) {
- dev_err(dev, "ASoC: Failed to allocate name\n");
- return -ENOMEM;
+ component->name = fmt_single_name(dev, >id);
+ if (!component->name) {
+ dev_err(dev, "ASoC: Failed to allocate name\n");
+ return -ENOMEM;
+ }
}
component->dev = dev;
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c
b/sound/soc/soc-generic-dmaengine-pcm.c
index 092ca09f3631..83db1a83d8ba 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -441,6 +441,9 @@ int snd_dmaengine_pcm_register(struct device *dev,
pcm->config = config;
pcm->flags = flags;
+ if (config->name)
+ pcm->component.name = config->name;
+
ret = dmaengine_pcm_request_chan_of(pcm, dev, config);
if (ret)
goto err_free_dma;
--
2.34.1
[PATCH v2 0/4] drm: xlnx: zynqmp: Add DP audio support
Add DisplayPort audio support for Xilinx ZynqMP platforms. This depends on patch adding cyclic DMA mode for DPDMA driver: https://lore.kernel.org/all/20240228042124.3074044-3-vishal.sa...@amd.com/ If that patch is missing, starting an audio playback will fail with an ASoC error. The current DT is, for some reason, missing the DMA channels for the audio. This series adds that to the bindings and the dts file, but to support older dtb files without the audio DMA, the driver will not fail if the audio DMA is missing, but will just mark the audio support as disabled. The series also includes an improvement to the soc-generic-dmaengine-pcm.c, required to support two dmaengine_pcms. Signed-off-by: Tomi Valkeinen --- Changes in v2: - Fix a missing double-quote in the DT binding - Link to v1: https://lore.kernel.org/r/20240312-xilinx-dp-audio-v1-0-696c79fac...@ideasonboard.com --- Tomi Valkeinen (4): ASoC: dmaengine_pcm: Allow passing component name via config dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs arm64: dts: zynqmp: Add DMA for DP audio drm: xlnx: zynqmp_dpsub: Add DP audio support .../bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml | 10 +- arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +- drivers/gpu/drm/xlnx/Kconfig | 9 + drivers/gpu/drm/xlnx/Makefile | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 50 --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h| 7 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 54 ++- drivers/gpu/drm/xlnx/zynqmp_dp.h | 7 + drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461 + drivers/gpu/drm/xlnx/zynqmp_dpsub.c| 39 +- drivers/gpu/drm/xlnx/zynqmp_dpsub.h| 15 +- include/sound/dmaengine_pcm.h | 2 + sound/soc/soc-core.c | 8 +- sound/soc/soc-generic-dmaengine-pcm.c | 3 + 14 files changed, 563 insertions(+), 110 deletions(-) --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240312-xilinx-dp-audio-468ad12f9f64 Best regards, -- Tomi Valkeinen
Re: [PATCH v3 0/2] Fixes for omapdrm console
Hi, On 28/02/2024 08:35, Tony Lindgren wrote: Here are two fixes for omapdrm for missing drm_framebuffer_funcs.dirty that needs to be paired with omap_framebuffer_dirty(), and to add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS so things behave as earlier with drm_fb_helper_sys_write(). Without these fixes, the console won't update for the command mode displays. And likely mmap() using writes can miss updates as noted by Thomas. Regards, Tony Changes since v2: - Fix cache issue noted by Tomi using custom omap_fbdev_fb_mmap() as suggested by Thomas - Add FB_DMAMEM_HELPERS_DEFERRED Kconfig option and use it for omapdrm as noted by Thomas Changes since v1: - Add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to use with FB_DEFAULT_DEFERRED_OPS as suggested by Thomas Tony Lindgren (2): drm/omapdrm: Fix console by implementing fb_dirty drm/omapdrm: Fix console with deferred ops drivers/gpu/drm/omapdrm/Kconfig | 2 +- drivers/gpu/drm/omapdrm/omap_fbdev.c | 40 +++- drivers/video/fbdev/core/Kconfig | 6 + include/linux/fb.h | 4 +++ 4 files changed, 45 insertions(+), 7 deletions(-) Looks fine to me, I'll apply to drm-misc-next. Tomi
[PATCH 4/4] drm: xlnx: zynqmp_dpsub: Add DP audio support
Add basic DisplayPort audio support.
Support non-live audio playback from two PCMs (DMA channels), and the
volume control in the audio mixer.
As older dtb files may not have the audio DMA channels defined, the
driver will just mark the audio support as disabled if the audio DMA is
missing, and will continue with only display support.
Note: Reset doesn't seem to work (ZYNQMP_DISP_AUD_SOFT_RESET). If we do
a reset, audio playback won't start again even if, afaics, we do set up
all the necessary registers. So, at the moment, resetting the audio
block in dp_dai_hw_free() is commented out.
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/xlnx/Kconfig| 9 +
drivers/gpu/drm/xlnx/Makefile | 1 +
drivers/gpu/drm/xlnx/zynqmp_disp.c | 50
drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 7 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c| 54 ++--
drivers/gpu/drm/xlnx/zynqmp_dp.h| 7 +
drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 39 +--
drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 15 +-
9 files changed, 540 insertions(+), 103 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
index 68ee897de9d7..d88cfbaf2863 100644
--- a/drivers/gpu/drm/xlnx/Kconfig
+++ b/drivers/gpu/drm/xlnx/Kconfig
@@ -15,3 +15,12 @@ config DRM_ZYNQMP_DPSUB
This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
this option if you have a Xilinx ZynqMP SoC with DisplayPort
subsystem.
+
+config DRM_ZYNQMP_DPSUB_AUDIO
+ bool "ZynqMP DisplayPort Audio Support"
+ depends on DRM_ZYNQMP_DPSUB
+ depends on SND && SND_SOC
+ select SND_SOC_GENERIC_DMAENGINE_PCM
+ help
+ Choose this option to enable DisplayPort audio support in the ZynqMP
+ DisplayPort driver.
diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
index ea1422a39502..ab6e2ffd7e8d 100644
--- a/drivers/gpu/drm/xlnx/Makefile
+++ b/drivers/gpu/drm/xlnx/Makefile
@@ -1,2 +1,3 @@
zynqmp-dpsub-y := zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o zynqmp_kms.o
+zynqmp-dpsub-$(CONFIG_DRM_ZYNQMP_DPSUB_AUDIO) += zynqmp_dp_audio.o
obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..d2bf0e2d0135 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -130,7 +130,6 @@ struct zynqmp_disp_layer {
* @dpsub: Display subsystem
* @blend.base: Register I/O base address for the blender
* @avbuf.base: Register I/O base address for the audio/video buffer manager
- * @audio.base: Registers I/O base address for the audio mixer
* @layers: Layers (planes)
*/
struct zynqmp_disp {
@@ -143,9 +142,6 @@ struct zynqmp_disp {
struct {
void __iomem *base;
} avbuf;
- struct {
- void __iomem *base;
- } audio;
struct zynqmp_disp_layer layers[ZYNQMP_DPSUB_NUM_LAYERS];
};
@@ -807,42 +803,6 @@ static void zynqmp_disp_blend_layer_disable(struct
zynqmp_disp *disp,
csc_zero_offsets);
}
-/*
-
- * Audio Mixer
- */
-
-static void zynqmp_disp_audio_write(struct zynqmp_disp *disp, int reg, u32 val)
-{
- writel(val, disp->audio.base + reg);
-}
-
-/**
- * zynqmp_disp_audio_enable - Enable the audio mixer
- * @disp: Display controller
- *
- * Enable the audio mixer by de-asserting the soft reset. The audio state is
set to
- * default values by the reset, set the default mixer volume explicitly.
- */
-static void zynqmp_disp_audio_enable(struct zynqmp_disp *disp)
-{
- /* Clear the audio soft reset register as it's an non-reset flop. */
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_SOFT_RESET, 0);
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_MIXER_VOLUME,
- ZYNQMP_DISP_AUD_MIXER_VOLUME_NO_SCALE);
-}
-
-/**
- * zynqmp_disp_audio_disable - Disable the audio mixer
- * @disp: Display controller
- *
- * Disable the audio mixer by asserting its soft reset.
- */
-static void zynqmp_disp_audio_disable(struct zynqmp_disp *disp)
-{
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_SOFT_RESET,
- ZYNQMP_DISP_AUD_SOFT_RESET_AUD_SRST);
-}
-
/*
-
* ZynqMP Display Layer & DRM Plane
*/
@@ -1169,8 +1129,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
true);
zynqmp_disp_avbuf_enable_channels(disp);
zynqmp_disp_avbuf_enable_audio(disp);
-
- zynqmp_disp_audio_enable(disp);
}
/**
@@ -1179,8 +1137,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
*/
void zynqmp_disp_disable
[PATCH 3/4] arm64: dts: zynqmp: Add DMA for DP audio
Add the two DMA channels used for the DisplayPort audio to the
zynqmp_dpsub node.
Signed-off-by: Tomi Valkeinen
---
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index eaba466804bc..811d80cbf4c5 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -1036,11 +1036,14 @@ zynqmp_dpsub: display@fd4a {
"dp_vtc_pixel_clk_in";
power-domains = <_firmware PD_DP>;
resets = <_reset ZYNQMP_RESET_DP>;
- dma-names = "vid0", "vid1", "vid2", "gfx0";
+ dma-names = "vid0", "vid1", "vid2", "gfx0",
+ "aud0", "aud1";
dmas = <_dpdma ZYNQMP_DPDMA_VIDEO0>,
<_dpdma ZYNQMP_DPDMA_VIDEO1>,
<_dpdma ZYNQMP_DPDMA_VIDEO2>,
- <_dpdma ZYNQMP_DPDMA_GRAPHICS>;
+ <_dpdma ZYNQMP_DPDMA_GRAPHICS>,
+ <_dpdma ZYNQMP_DPDMA_AUDIO0>,
+ <_dpdma ZYNQMP_DPDMA_AUDIO1>;
ports {
#address-cells = <1>;
--
2.34.1
[PATCH 2/4] dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs
The DP subsystem for ZynqMP support audio via two channels, and the DP DMA has dma-engines for those channels. For some reason the DT binding has not specified those channels, even if the picture included in xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. Add the two audio DMAs to the binding. Signed-off-by: Tomi Valkeinen --- .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml| 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml index 554f9d5809d4..8a56ab923cca 100644 --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml @@ -100,12 +100,16 @@ properties: - description: Video layer, plane 1 (U/V or U) - description: Video layer, plane 2 (V) - description: Graphics layer + - description: Audio channel 0 + - description: Audio channel 1 dma-names: items: - const: vid0 - const: vid1 - const: vid2 - const: gfx0 + - const: aud0 + - const: aud1 phys: description: PHYs for the DP data lanes @@ -194,11 +198,13 @@ examples: power-domains = <_dp>; resets = < ZYNQMP_RESET_DP>; -dma-names = "vid0", "vid1", "vid2", "gfx0"; +dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1; dmas = <_dpdma 0>, <_dpdma 1>, <_dpdma 2>, - <_dpdma 3>; + <_dpdma 3>, + <_dpdma 4>, + <_dpdma 5>; phys = < 1 PHY_TYPE_DP 0 3>, < 0 PHY_TYPE_DP 1 3>; -- 2.34.1
[PATCH 1/4] ASoC: dmaengine_pcm: Allow passing component name via config
At the moment we cannot instantiate two dmaengine_pcms with the same
parent device, as the components will be named the same, leading to
conflicts.
Add 'name' field to the snd_dmaengine_pcm_config, and use that (if
defined) as the component name instead of deriving the component name
from the device.
Signed-off-by: Tomi Valkeinen
---
include/sound/dmaengine_pcm.h | 2 ++
sound/soc/soc-core.c | 8 +---
sound/soc/soc-generic-dmaengine-pcm.c | 3 +++
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index d70c55f17df7..c11aaf8079fb 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -118,6 +118,7 @@ int snd_dmaengine_pcm_refine_runtime_hwparams(
* which do not use devicetree.
* @process: Callback used to apply processing on samples transferred from/to
* user space.
+ * @name: Component name. If null, dev_name will be used.
* @compat_filter_fn: Will be used as the filter function when requesting a
* channel for platforms which do not use devicetree. The filter parameter
* will be the DAI's DMA data.
@@ -143,6 +144,7 @@ struct snd_dmaengine_pcm_config {
int (*process)(struct snd_pcm_substream *substream,
int channel, unsigned long hwoff,
unsigned long bytes);
+ const char *name;
dma_filter_fn compat_filter_fn;
struct device *dma_dev;
const char *chan_names[SNDRV_PCM_STREAM_LAST + 1];
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 516350533e73..772d67065611 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2792,10 +2792,12 @@ int snd_soc_component_initialize(struct
snd_soc_component *component,
INIT_LIST_HEAD(>list);
mutex_init(>io_mutex);
- component->name = fmt_single_name(dev, >id);
if (!component->name) {
- dev_err(dev, "ASoC: Failed to allocate name\n");
- return -ENOMEM;
+ component->name = fmt_single_name(dev, >id);
+ if (!component->name) {
+ dev_err(dev, "ASoC: Failed to allocate name\n");
+ return -ENOMEM;
+ }
}
component->dev = dev;
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c
b/sound/soc/soc-generic-dmaengine-pcm.c
index 092ca09f3631..83db1a83d8ba 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -441,6 +441,9 @@ int snd_dmaengine_pcm_register(struct device *dev,
pcm->config = config;
pcm->flags = flags;
+ if (config->name)
+ pcm->component.name = config->name;
+
ret = dmaengine_pcm_request_chan_of(pcm, dev, config);
if (ret)
goto err_free_dma;
--
2.34.1
[PATCH 0/4] drm: xlnx: zynqmp: Add DP audio support
Add DisplayPort audio support for Xilinx ZynqMP platforms. This depends on patch adding cyclic DMA mode for DPDMA driver: https://lore.kernel.org/all/20240228042124.3074044-3-vishal.sa...@amd.com/ If that patch is missing, starting an audio playback will fail with an ASoC error. The current DT is, for some reason, missing the DMA channels for the audio. This series adds that to the bindings and the dts file, but to support older dtb files without the audio DMA, the driver will not fail if the audio DMA is missing, but will just mark the audio support as disabled. The series also includes an improvement to the soc-generic-dmaengine-pcm.c, required to support two dmaengine_pcms. Signed-off-by: Tomi Valkeinen --- Tomi Valkeinen (4): ASoC: dmaengine_pcm: Allow passing component name via config dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs arm64: dts: zynqmp: Add DMA for DP audio drm: xlnx: zynqmp_dpsub: Add DP audio support .../bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml | 10 +- arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +- drivers/gpu/drm/xlnx/Kconfig | 9 + drivers/gpu/drm/xlnx/Makefile | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 50 --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h| 7 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 54 ++- drivers/gpu/drm/xlnx/zynqmp_dp.h | 7 + drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461 + drivers/gpu/drm/xlnx/zynqmp_dpsub.c| 39 +- drivers/gpu/drm/xlnx/zynqmp_dpsub.h| 15 +- include/sound/dmaengine_pcm.h | 2 + sound/soc/soc-core.c | 8 +- sound/soc/soc-generic-dmaengine-pcm.c | 3 + 14 files changed, 563 insertions(+), 110 deletions(-) --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240312-xilinx-dp-audio-468ad12f9f64 Best regards, -- Tomi Valkeinen
[PATCH] drm: xlnx: zynqmp_dpsub: Fix missing drm_bridge_add() call
The driver creates a bridge, but never calls drm_bridge_add() when
non-live input is used. This leaves the bridge's hpd_mutex
uninitialized, leading to:
WARNING: CPU: 0 PID: 9 at kernel/locking/mutex.c:582 __mutex_lock+0x708/0x840
Add the bridge add & remove calls so that the bridge gets managed
correctly.
Signed-off-by: Tomi Valkeinen
Fixes: 561671612394 ("drm: xlnx: zynqmp_dpsub: Add support for live video
input")
---
drivers/gpu/drm/xlnx/zynqmp_dp.c| 4
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 4
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index a0606fab0e22..9f750740dfb8 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1761,6 +1761,8 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
dpsub->dp = dp;
+ drm_bridge_add(dpsub->bridge);
+
dev_dbg(dp->dev, "ZynqMP DisplayPort Tx probed with %u lanes\n",
dp->num_lanes);
@@ -1789,4 +1791,6 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
zynqmp_dp_phy_exit(dp);
zynqmp_dp_reset(dp, true);
+
+ drm_bridge_remove(dpsub->bridge);
}
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..3933c4f1a44f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -260,8 +260,6 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,8 +286,6 @@ static void zynqmp_dpsub_remove(struct platform_device
*pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
---
base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
change-id: 20240312-xilinx-dp-lock-fix-cf68f43a7bab
Best regards,
--
Tomi Valkeinen
Re: [PATCH] drm: xlnx: dp: Reset DisplayPort IP
On 29/02/2024 11:05, Laurent Pinchart wrote: Tomi, could you push this through drm-misc ? On Wed, Feb 28, 2024 at 06:22:25PM +0200, Laurent Pinchart wrote: Hello Rohit, Thank you for the patch. On Fri, Feb 16, 2024 at 04:40:43AM -0800, Rohit Visavalia wrote: Assert DisplayPort reset signal before deasserting, it is to clear out any registers programmed before booting kernel. Signed-off-by: Rohit Visavalia --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 1846c4971fd8..5a40aa1d4283 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) goto err_free; } + ret = zynqmp_dp_reset(dp, true); + if (ret < 0) + return ret; + This looks fine to me, so Reviewed-by: Laurent Pinchart Thanks, tested and pushed to drm-misc-next. Tomi
Re: [PATCH v2 0/2] Fixes for omapdrm console
On 26/02/2024 10:26, Tomi Valkeinen wrote: Hi Tony, On 25/02/2024 08:46, Tony Lindgren wrote: Here are two fixes for omapdrm console. How is it broken? I don't usually use the console (or fbdev) but enabling it now, it seems to work fine for me, on DRA76 EVM with HDMI output. After applying your patches, I see a lot of cache-related artifacts on the screen when updating the fb. Tomi
Re: [PATCH v2 0/2] Fixes for omapdrm console
Hi Tony, On 25/02/2024 08:46, Tony Lindgren wrote: Here are two fixes for omapdrm console. How is it broken? I don't usually use the console (or fbdev) but enabling it now, it seems to work fine for me, on DRA76 EVM with HDMI output. Tomi Regards, Tony Changes since v1: - Add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to use with FB_DEFAULT_DEFERRED_OPS as suggested by Thomas Tony Lindgren (2): drm/omapdrm: Fix console by implementing fb_dirty drm/omapdrm: Fix console with deferred ops drivers/gpu/drm/omapdrm/omap_fbdev.c | 39 +++- include/linux/fb.h | 4 +++ 2 files changed, 31 insertions(+), 12 deletions(-)
Re: [PATCH v2 0/2] drm/bridge: tc358767: Fix DRM_BRIDGE_ATTACH_NO_CONNECTOR case
On 15/02/2024 11:03, Alexander Stein wrote: Hi everyone, Am Donnerstag, 15. Februar 2024, 09:53:54 CET schrieb Jan Kiszka: On 11.12.23 09:07, Aradhya Bhatia wrote: On 06/12/23 17:41, Tomi Valkeinen wrote: Hi, On 08/11/2023 14:45, Alexander Stein wrote: Hi Tomi, Am Mittwoch, 8. November 2023, 12:27:21 CET schrieb Tomi Valkeinen: These two patches are needed to make tc358767 work in the DRM_BRIDGE_ATTACH_NO_CONNECTOR case, at least when using a DP connector. I have tested this with TI AM654 EVM with a tc358767 add-on card connected to a DP monitor. Just a question regarding the usage of this DSI-DP bridge. What is the state of the DSI lanes after the DSI host has been initialized, but before calling atomic_pre_enable? AFAIK this bridge requires LP-11 on DSI at any time for accessing the AUX channel. + Marek Marek, Alexander, A quick grep tells me that you have added devicetree for tc358767 in DSI to (e)DP mode on other platforms. Could you please test these patches and report if you find any issue? Sorry, I can't provide any feedback here. I've yet to setup the DSI-DP correctly. Ok. Does anyone have a worry that these patches make the situation worse for the DSI case than it was before? Afaics, if the DSI lanes are not set up early enough by the DSI host, the driver would break with and without these patches. These do fix the driver for DRM_BRIDGE_ATTACH_NO_CONNECTOR and DPI, so I'd like to merge these unless these cause a regression with the DSI case. Tomi
Re: [PATCH 1/2] drm/tidss: Fix initial plane zpos values
Hi, On 13/02/2024 13:39, Daniel Stone wrote: Hi, On Tue, 13 Feb 2024 at 10:18, Marius Vlad wrote: On Tue, Feb 13, 2024 at 11:57:59AM +0200, Tomi Valkeinen wrote: I haven't. I'm quite unfamiliar with Weston, and Randolph from TI (cc'd) has been working on the Weston side of things. I also don't know if there's something TI specific here, as the use case is with non-mainline GPU drivers and non-mainline Mesa. I should have been a bit clearer in the patch description, as I didn't mean that upstream Weston has a bug (maybe it has, maybe it has not). Don't worry about it. We've had bugs in the past and I'm sure we'll have more. :) Either way, it's definitely better to have the kernel expose sensible behaviour rather than weird workarounds, unless they've been around for so long that they're basically baked into ABI. Yeah, that's always a worry. I do hope that no user of tidss expects the plane zpos values to be the current funny ones. But we'll probably find out when I merge this =). The issue seen is that when Weston decides to use DRM planes for composition, the plane zpositions are not configured correctly (or at all?). Afaics, this leads to e.g. weston showing a window with a DRM "overlay" plane that is behind the "primary" root plane, so the window is not visible. And as Weston thinks that the area supposedly covered by the overlay plane does not need to be rendered on the root plane, there are also artifacts on that area. Also, the Weston I used is a bit older one (10.0.1), as I needed to go back in my buildroot versions to get all that non-mainline GPU stuff compiled and working. A more recent Weston may behave differently. Right after Weston 10, we had a few minor changes related to the zpos-sorting list of planes and how we parse the plan list without having a temporary zpos ordered list to pick planes from. And there's another fix for missing out to set out the zpos for scanout to the minimum available - which seems like a good candidate to explain what happens in the issue described above. So if trying Weston again, please try with at least Weston 12, which should have those changes in. Specifically, you probably want commits 4cde507be6a1 and 58dde0e0c000. I think the window of breakage was small enough that - assuming either those commits or an upgrade to Weston 12/13 fixes it - we can just ask people to upgrade to a fixed Weston. Presuming this is not related to any TI specific code, I guess it's a regression in the sense that at some point Weston added the support to use planes for composition, so previously with only a single plane per display there was no issue. That point was 12 years ago, so not that novel. ;) Hmm, so do I understand it right, the plane code from 12 years back supposedly works ok, but somewhere around Weston 10 something broke, but was fixed with the commits you mention above? Tomi
Re: [PATCH v5 0/4] Add common1 region for AM62, AM62A & AM65x
On 16/02/2024 08:24, Devarsh Thakkar wrote: This adds DSS common1 region for respective SoCs supporting it. Changelog: V2 : Remove do-not-merge tag and add am62a dss common1 reion V3 : Add Fixes tag to each commit V4 : Add Reviewed-by tag and AM62A SoC TRM Link V5 : Split dts patch to separate patches for each SoC Devarsh Thakkar (4): dt-bindings: display: ti,am65x-dss: Add support for common1 region arm64: dts: ti: Add common1 register space for AM65x SoC arm64: dts: ti: Add common1 register space for AM62x SoC arm64: dts: ti: Add common1 register space for AM62A SoC .../devicetree/bindings/display/ti/ti,am65x-dss.yaml | 7 +-- arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 5 +++-- arch/arm64/boot/dts/ti/k3-am62a-main.dtsi | 5 +++-- arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++-- 4 files changed, 14 insertions(+), 8 deletions(-) For the series: Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 1/2] dt-bindings: display: ti,am65x-dss: Add support for common1 region
On 14/02/2024 11:10, Tomi Valkeinen wrote: Hi, On 15/01/2024 14:57, Devarsh Thakkar wrote: TI keystone display subsystem present in AM65 and other SoCs such as AM62 support two separate register spaces namely "common" and "common1" which can be used by two separate hosts to program the display controller as described in respective Technical Reference Manuals [1]. The common1 register space has similar set of configuration registers as supported in common register space except the global configuration registers which are exclusive to common region. This adds binding for "common1" register region too as supported by the hardware. [1]: AM62x TRM: https://www.ti.com/lit/pdf/spruiv7 (Section 14.8.9.1 DSS Registers) AM65x TRM: https://www.ti.com/lit/pdf/spruid7 (Section 12.6.5 DSS Registers) Signed-off-by: Devarsh Thakkar --- .../devicetree/bindings/display/ti/ti,am65x-dss.yaml | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml index b6767ef0d24d..55e3e490d0e6 100644 --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml @@ -37,6 +37,7 @@ properties: - description: OVR2 overlay manager for vp2 - description: VP1 video port 1 - description: VP2 video port 2 + - description: common1 DSS register area reg-names: items: @@ -47,6 +48,7 @@ properties: - const: ovr2 - const: vp1 - const: vp2 + - const: common1 clocks: items: @@ -147,9 +149,10 @@ examples: <0x04a07000 0x1000>, /* ovr1 */ <0x04a08000 0x1000>, /* ovr2 */ <0x04a0a000 0x1000>, /* vp1 */ - <0x04a0b000 0x1000>; /* vp2 */ + <0x04a0b000 0x1000>, /* vp2 */ + <0x04a01000 0x1000>; /* common1 */ reg-names = "common", "vidl1", "vid", - "ovr1", "ovr2", "vp1", "vp2"; + "ovr1", "ovr2", "vp1", "vp2", "common1"; ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>; power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>; clocks = <_clks 67 1>, Looks fine to me, I'll apply to drm-misc-next. Hmm, now thinking about this, doesn't this cause dtb checks to start failing, as the dtbs are missing one entry? Is it better to merge these kind of changes with the dts changes? Or does it matter? Tomi
Re: [PATCH 1/2] dt-bindings: display: ti,am65x-dss: Add support for common1 region
Hi, On 15/01/2024 14:57, Devarsh Thakkar wrote: TI keystone display subsystem present in AM65 and other SoCs such as AM62 support two separate register spaces namely "common" and "common1" which can be used by two separate hosts to program the display controller as described in respective Technical Reference Manuals [1]. The common1 register space has similar set of configuration registers as supported in common register space except the global configuration registers which are exclusive to common region. This adds binding for "common1" register region too as supported by the hardware. [1]: AM62x TRM: https://www.ti.com/lit/pdf/spruiv7 (Section 14.8.9.1 DSS Registers) AM65x TRM: https://www.ti.com/lit/pdf/spruid7 (Section 12.6.5 DSS Registers) Signed-off-by: Devarsh Thakkar --- .../devicetree/bindings/display/ti/ti,am65x-dss.yaml | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml index b6767ef0d24d..55e3e490d0e6 100644 --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml @@ -37,6 +37,7 @@ properties: - description: OVR2 overlay manager for vp2 - description: VP1 video port 1 - description: VP2 video port 2 + - description: common1 DSS register area reg-names: items: @@ -47,6 +48,7 @@ properties: - const: ovr2 - const: vp1 - const: vp2 + - const: common1 clocks: items: @@ -147,9 +149,10 @@ examples: <0x04a07000 0x1000>, /* ovr1 */ <0x04a08000 0x1000>, /* ovr2 */ <0x04a0a000 0x1000>, /* vp1 */ -<0x04a0b000 0x1000>; /* vp2 */ +<0x04a0b000 0x1000>, /* vp2 */ +<0x04a01000 0x1000>; /* common1 */ reg-names = "common", "vidl1", "vid", -"ovr1", "ovr2", "vp1", "vp2"; +"ovr1", "ovr2", "vp1", "vp2", "common1"; ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>; power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>; clocks =<_clks 67 1>, Looks fine to me, I'll apply to drm-misc-next. Tomi
