fw_devlinks preventing a panel driver from probing
Hi, We have an issue where two devices have dependencies to each other, according to drivers/base/core.c's fw_devlinks, and this prevents them from probing. I've been adding debugging to the core.c, but so far I don't quite grasp the issue, so I thought to ask. Maybe someone can instantly say that this just won't work... So, we have two devices, DSS (display subsystem) and an LVDS panel. The DSS normally outputs parallel video from its video ports (VP), but it has an integrated LVDS block (OLDI, Open LVDS Display Interface). The OLDI block takes input from DSS's parallel outputs. The OLDI is not modeled as a separate device (neither in the DT nor in the Linux device model) as it has no register space, and is controlled fully by the DSS. To support dual-link LVDS, the DSS has two OLDI instances. They both take their input from the same parallel video port, but each OLDI sends alternate lines forward. So for a dual-link setup the connections would be like this: +-+-+ +---+ +--+ | | | | | | | | | VP1 ++--->| OLDI0 +>| | | | ||| | | | | DSS +-+|+---+ | Panel | | | ||| | | | | | VP2 |+--->| OLDI1 +>| | | | | | | | | +-+-+ +---+ +--+ As the OLDI is not a separate device, it also does not have an independent device tree node, but rather it's inside DSS's node. The DSS parallel outputs are under a normal "ports" node, but OLDI ports are under "oldi-txes/ports" (see below for dts to clarify this). And I think (guess...) this is the root of the issue we're seeing, as it means the following, one or both of which might be the reason for this issue: - OLDI fwnodes don't have an associated struct device *. I think the reason is that the OLDI media graph ports are one level too deep in the hierarchy. So while the DSS ports are associated with the DSS device, OLDI ports are not. - The VP ports inside the DSS point to OLDI ports, which are also inside DSS. So ports from a device point to ports in the same device (and back). If I understand the fw_devlink code correctly, in a normal case the links formed with media graphs are marked as a cycle (FWLINK_FLAG_CYCLE), and then ignored as far as probing goes. What we see here is that when using a single-link OLDI panel, the panel driver's probe never gets called, as it depends on the OLDI, and the link between the panel and the OLDI is not a cycle. The DSS driver probes, but the probe fails as it requires all the panel devices to have been probed (and thus registered to the DRM framework) before it can finish its setup. With dual-link, probing does happen and the drivers work. But I believe this is essentially an accident, in the sense that the first link between the panel and the OLDI still blocks the probing, but the second links allows the driver core to traverse the devlinks further, causing it to mark the links to the panel as FWLINK_FLAG_CYCLE (or maybe it only marks one of those links, and that's enough). If I set fw_devlink=off as a kernel parameter, the probing proceeds successfully in both single- and dual-link cases. Now, my questions is, is this a bug in the driver core, a bug in the DT bindings, or something in between (DT is fine-ish, but the structure is something that won't be supported by the driver core). And a follow-up question, regardless of the answer to the first one: which direction should I go from here =). The device tree data (simplified) for this is as follows, first the dual-link case, then the single-link case: /* Dual-link */ dss: dss@3020 { compatible = "ti,am625-dss"; oldi-txes { oldi0: oldi@0 { oldi0_ports: ports { port@0 { oldi_0_in: endpoint { remote-endpoint = <&dpi0_out0>; }; }; port@1 { oldi_0_out: endpoint { remote-endpoint = <&lcd_in0>; }; }; }; }; oldi1: oldi@1 { oldi1_ports: ports { port@0 { oldi_1_in: endpoint { remote-endpoint = <&dpi0_out1>; }; }; port@1 { oldi_1_out: endpoint {
Re: [PATCH] drm/tidss: Add MIT license along with GPL-2.0
On 12/09/2024 21:08, Maxime Ripard wrote: On Thu, Sep 12, 2024 at 06:04:11PM GMT, Maxime Ripard wrote: On Thu, 12 Sep 2024 22:41:42 +0530, Devarsh Thakkar wrote: Modify license to include dual licensing as GPL-2.0-only OR MIT license for tidss display driver. This allows other operating system ecosystems such as Zephyr and also the commercial firmwares to refer and derive code from this display driver in a more permissive manner. [ ... ] Acked-by: Maxime Ripard Also, we need the ack of all contributors to that driver, so my ack isn't enough to merge that patch. IANAL, maybe a silly question: if someone from company XYZ has sent a patch for tidss, don't we then need an ack from someone in XYZ who's high enough in XYZ to allow changing the license for their code? Tomi
[PATCH v3 2/3] 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 b1b31dcf6291..673ca8422e6b 100644 --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi @@ -1207,11 +1207,14 @@ zynqmp_dpsub: display@fd4a { "dp_vtc_pixel_clk_in"; power-domains = <&zynqmp_firmware PD_DP>; resets = <&zynqmp_reset ZYNQMP_RESET_DP>; - dma-names = "vid0", "vid1", "vid2", "gfx0"; + dma-names = "vid0", "vid1", "vid2", "gfx0", + "aud0", "aud1"; dmas = <&zynqmp_dpdma ZYNQMP_DPDMA_VIDEO0>, <&zynqmp_dpdma ZYNQMP_DPDMA_VIDEO1>, <&zynqmp_dpdma ZYNQMP_DPDMA_VIDEO2>, - <&zynqmp_dpdma ZYNQMP_DPDMA_GRAPHICS>; + <&zynqmp_dpdma ZYNQMP_DPDMA_GRAPHICS>, + <&zynqmp_dpdma ZYNQMP_DPDMA_AUDIO0>, + <&zynqmp_dpdma ZYNQMP_DPDMA_AUDIO1>; ports { #address-cells = <1>; -- 2.43.0
[PATCH v3 3/3] 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. Tested-by: Anatoliy Klymenko Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/xlnx/Kconfig| 9 + drivers/gpu/drm/xlnx/Makefile | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 48 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(+), 101 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 9368acf56eaf..77dc485c0887 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -143,7 +143,6 @@ struct zynqmp_disp_layer { * @dpsub: Display subsystem * @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 { @@ -152,7 +151,6 @@ struct zynqmp_disp { void __iomem *blend; void __iomem *avbuf; - void __iomem *audio; struct zynqmp_disp_layer layers[ZYNQMP_DPSUB_NUM_LAYERS]; }; @@ -865,42 +863,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 + 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 */ @@ -1338,8 +1300,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp) disp->dpsub->vid_clk_from_ps); zynqmp_disp_avbuf_enable_channels(disp); zynqmp_disp_avbuf_enable_audio(disp); - - zynqmp_disp_audio_enable(disp); } /** @@ -1348,8 +1308,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp) */ void zynqmp_disp_disable(struct zynqmp_disp *d
[PATCH v3 1/3] dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs
The DP subsystem for ZynqMP supports 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. This hasn't caused any issues as the drivers have not supported audio, and has thus gone unnoticed. To make it possible to add the audio support to the driver, add the two audio DMAs to the binding. While strictly speaking this is an ABI break, there should be no regressions caused by this as we're adding new entries at the end of the dmas list, and, after the audio support has been added in "arm64: dts: zynqmp: Add DMA for DP audio", the driver will treat the audio DMAs as optional to also support the old bindings. 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 = <&pd_dp>; resets = <&reset ZYNQMP_RESET_DP>; -dma-names = "vid0", "vid1", "vid2", "gfx0"; +dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; dmas = <&xlnx_dpdma 0>, <&xlnx_dpdma 1>, <&xlnx_dpdma 2>, - <&xlnx_dpdma 3>; + <&xlnx_dpdma 3>, + <&xlnx_dpdma 4>, + <&xlnx_dpdma 5>; phys = <&psgtr 1 PHY_TYPE_DP 0 3>, <&psgtr 0 PHY_TYPE_DP 1 3>; -- 2.43.0
[PATCH v3 0/3] 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 cyclic DMA patch has recently been accepted to the DMA tree. 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. To: Lars-Peter Clausen To: Jaroslav Kysela To: Takashi Iwai To: Liam Girdwood To: Mark Brown To: Laurent Pinchart To: Maarten Lankhorst To: Maxime Ripard To: Thomas Zimmermann To: David Airlie To: Daniel Vetter To: Rob Herring To: Krzysztof Kozlowski To: Conor Dooley To: Michal Simek To: Krzysztof Kozlowski Cc: linux-so...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: devicet...@vger.kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: Vishal Sagar Cc: Anatoliy Klymenko Cc: Péter Ujfalusi Signed-off-by: Tomi Valkeinen Changes in v3: - Expand the description in "dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs" to be more clear about the DT binding change. - Rebased on top of current upstream - Link to v2: https://lore.kernel.org/r/20240319-xilinx-dp-audio-v2-0-92d6d3a7c...@ideasonboard.com 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 (3): 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 | 48 --- 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 +- 11 files changed, 553 insertions(+), 105 deletions(-) --- base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6 change-id: 20240312-xilinx-dp-audio-468ad12f9f64 Best regards, -- Tomi Valkeinen
Re: [PATCH v3 0/4] drm/tidss: Add OLDI bridge support
On 09/09/2024 12:31, Aradhya Bhatia wrote: Hi, Thank you, Francesco and Max, for testing and reporting this! On 09/09/24 13:45, Tomi Valkeinen wrote: Hi, On 06/09/2024 14:43, Francesco Dolcini wrote: +Max Hello Aradhya, On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote: The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need for some major changes for a full feature experience. 1. The OF graph needs to be updated to accurately show the data flow. 2. The tidss and OLDI drivers now need to support the dual-link and the cloned single-link OLDI video signals. 3. The drivers also need to support the case where 2 OLDI TXes are connected to 2 different VPs - thereby creating 2 independent streams of single-link OLDI outputs. Have you considered/tested the use case in which only single link is used? You do not mention it here and to me this is a relevant use case. There is a workaround for this (use option 2, cloned, even if nothing is connected to the second link), but this seems not correct. I agree. The whole idea behind the series is to be able to accurately describe the hardware. Putting the devicetree in clone mode in order to get the single-link mode working is far from ideal. Btw, with the fw_devlink=off hack, and removing the second link from k3-am625-sk-microtips-mf101hie-panel.dtso, is still not enough, as the k3-am62-main.dtsi contains the ti,companion-oldi property which makes the driver think it's a cloning case. So, I think, either the ti,companion-oldi and ti,secondary-oldi should only be set in the overlay when setting up cloning/dual-link, or the companion-oldi property shouldn't actually make a difference, and the selection between clone and single-link should be done via some other means. We (Max in Cc here) noticed that this specific use case is broken on your downstream v6.6 TI branch. Yes, it was been brought to my attention that the single-link usecase is not working over the downstream ti-6.6 kernel. As I have since discovered, it's not working on this series either. For some reason, the supplier-consumer dependency between the OLDI and the panel devicetree nodes is not getting flagged as `FWLINK_FLAG_CYCLE` in cases of single-link configuration. This flag allows the panel driver to continue to probe without waiting for the OLDI driver (panel's supplier). Absence of the flag getting set is causing these drivers to keep deferring probe in an endless cycle. Even with the flag, the OLDI (and tidss) cannot complete probe until the panel driver is probed and ready. That is because the OLDI (and tidss) need the panel to continue with the bridge-chain creation. However, over with the dual-lvds configuration (and as Francesco has now mentioned the clone configuration as well), the flag gets set by default, and everything works. This is what the debug has led to, so far. Yes, I came to the same conclusion with my debug. What if you set "fw_devlink=off" kernel boot parameter? Yes! I haven't tested it, but it seems that this will bypass the supplier check and let the panel probe continue. Tomi, any idea, why is this issue happening only for single-link in the first place? It seems as if having 2 ports inside the panel devicetree lets the probe mechanism recognize the circular dependency and ignore the supplier OLDIs? I have to say I have no idea... I don't really understand the devlink code here, but I'm guessing that the "cycle" part comes from the fact that with a media graph we have links (remote-endpoint) both ways in the DT data. So it's not possible to say which side is the consumer, which one is the supplier. Thus, it's marked as a cycle and, I think, basically ignored for the probing purpose. But why not here? I can see the links being created both ways: /display Linked as a fwnode consumer to /bus@f/dss@3020/oldi-txes/oldi@0 /bus@f/dss@3020/oldi-txes/oldi@0 Linked as a fwnode consumer to /display but it's never marked as a cycle. This is the function where the difference comes down to, by the way - __fw_devlink_relax_cycles(), per my knowledge. I am still on my way to understand what exactly it is doing and why is it not relaxing the single-link case. Yep. The answer is probably somewhere there =). Tomi
Re: [PATCH v3 0/4] drm/tidss: Add OLDI bridge support
Hi, On 06/09/2024 14:43, Francesco Dolcini wrote: +Max Hello Aradhya, On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote: The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need for some major changes for a full feature experience. 1. The OF graph needs to be updated to accurately show the data flow. 2. The tidss and OLDI drivers now need to support the dual-link and the cloned single-link OLDI video signals. 3. The drivers also need to support the case where 2 OLDI TXes are connected to 2 different VPs - thereby creating 2 independent streams of single-link OLDI outputs. Have you considered/tested the use case in which only single link is used? You do not mention it here and to me this is a relevant use case. There is a workaround for this (use option 2, cloned, even if nothing is connected to the second link), but this seems not correct. We (Max in Cc here) noticed that this specific use case is broken on your downstream v6.6 TI branch. What if you set "fw_devlink=off" kernel boot parameter? Tomi
Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
Hi, On 03/09/2024 14:56, Maxime Ripard wrote: On Mon, Sep 02, 2024 at 03:31:28PM GMT, Tomi Valkeinen wrote: Hi, On 02/09/2024 13:50, Daniel Vetter wrote: On Mon, Sep 02, 2024 at 11:26:11AM +0200, Maxime Ripard wrote: Hi, On Wed, Aug 07, 2024 at 03:19:23PM GMT, Tomi Valkeinen wrote: On 25/07/2024 14:28, Maxime Ripard wrote: On Mon, Jul 15, 2024 at 11:32:34AM GMT, Tomi Valkeinen wrote: 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. FTR, I'm all for supporting module unloading. The discussion above was about what is broken exactly
Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
On 03/09/2024 10:40, Simona Vetter wrote: On Mon, Sep 02, 2024 at 03:31:28PM +0300, Tomi Valkeinen wrote: Hi, On 02/09/2024 13:50, Daniel Vetter wrote: On Mon, Sep 02, 2024 at 11:26:11AM +0200, Maxime Ripard wrote: Hi, On Wed, Aug 07, 2024 at 03:19:23PM GMT, Tomi Valkeinen wrote: On 25/07/2024 14:28, Maxime Ripard wrote: On Mon, Jul 15, 2024 at 11:32:34AM GMT, Tomi Valkeinen wrote: 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. FTR, I'm all for supporting module unloading. The discussion above was about what is broken exactly, s
Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
Hi, On 02/09/2024 13:50, Daniel Vetter wrote: On Mon, Sep 02, 2024 at 11:26:11AM +0200, Maxime Ripard wrote: Hi, On Wed, Aug 07, 2024 at 03:19:23PM GMT, Tomi Valkeinen wrote: On 25/07/2024 14:28, Maxime Ripard wrote: On Mon, Jul 15, 2024 at 11:32:34AM GMT, Tomi Valkeinen wrote: 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. FTR, I'm all for supporting module unloading. The discussion above was about what is broken exactly, so we can come up with a good solution. Does that mean that you're ok with the patch, or that someth
Re: [PATCH RFC 2/2] pmdomain: ti-sci: Support retaining PD boot time state
Hi Ulf, 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. Do you know if there's been any recent work related to this? I tried to look around on the lists, but nothing caught my eye. Tomi
Re: [PATCHv2 0/6] drm/omap: hdmi: improve hdmi4 CEC, add CEC for hdmi5
Hi, On 25/08/2024 23:31, H. Nikolaus Schaller wrote: Hi, CEC features are useful to e.g. control HDMI monitor standby. But I wonder what happened to this series? I could find some reviewed-by: and acked-by: in [1] but it wasn't merged upstream for unidentifiable reasons. We apparently had merged this series some years ago into our LetuxOS distro kernel and now we found it to be broken (NULL dereference) at least for omap5uevm (and likely Pyra Handheld) after rebasing to v6.11-rc (it was already broken since v6.9-rc1). Fixes were not difficult, but it would be better if it were part of upstream. There was a v3: 20210428132545.1205162-1-hverkuil-ci...@xs4all.nl I see there was a concern from Laurent in: yljmzix71mcqn...@pendragon.ideasonboard.com And we need an ack from the bridge maintainers for the drm_bridge parts. But the series is three years old, so I think someone would have to rebase on top of mainline and re-test and re-send first. Tomi BR and thanks, Nikolaus [1] https://lore.kernel.org/r/all/20210302162403.983585-4-hverkuil-ci...@xs4all.nl/T/ Am 02.03.2021 um 17:23 schrieb Hans Verkuil : This series improves the drm_bridge support for CEC by introducing two new bridge ops in the first patch, and using those in the second patch. This makes it possible to call cec_s_conn_info() and set CEC_CAP_CONNECTOR_INFO for the CEC adapter, so userspace can associate the CEC adapter with the corresponding DRM connector. The third patch simplifies CEC physical address handling by using the cec_s_phys_addr_from_edid helper function that didn't exist when this code was originally written. The fourth patch adds the cec clock to ti,omap5-dss.txt. The fifth patch the missing cec clock to the dra7 and omap5 device tree, and the last patch adds CEC support to the OMAP5 driver. Tested with a Pandaboard and a Beagle X15 board. Regards, Hans Changes since v1: - as per suggestion from Laurent, changed cec_init/exit to connector_attach/_detach which are just called for all bridges. The DRM_BRIDGE_OP_CEC was dropped. - added patch to add the cec clock to ti,omap5-dss.txt - swapped the order of the last two patches - incorporated Tomi's suggestions for the hdmi5 CEC support. Hans Verkuil (6): drm: drm_bridge: add connector_attach/detach bridge ops drm/omapdrm/dss/hdmi4: switch to the connector bridge ops drm/omapdrm/dss/hdmi4: simplify CEC Phys Addr handling dt-bindings: display: ti: ti,omap5-dss.txt: add cec clock dra7.dtsi/omap5.dtsi: add cec clock drm/omapdrm/dss/hdmi5: add CEC support .../bindings/display/ti/ti,omap5-dss.txt | 4 +- arch/arm/boot/dts/dra7.dtsi | 5 +- arch/arm/boot/dts/omap5.dtsi | 5 +- drivers/gpu/drm/drm_bridge_connector.c| 9 + drivers/gpu/drm/omapdrm/Kconfig | 8 + drivers/gpu/drm/omapdrm/Makefile | 1 + drivers/gpu/drm/omapdrm/dss/hdmi.h| 1 + drivers/gpu/drm/omapdrm/dss/hdmi4.c | 40 ++-- drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 13 +- drivers/gpu/drm/omapdrm/dss/hdmi4_cec.h | 12 +- drivers/gpu/drm/omapdrm/dss/hdmi5.c | 63 +- drivers/gpu/drm/omapdrm/dss/hdmi5_cec.c | 209 ++ drivers/gpu/drm/omapdrm/dss/hdmi5_cec.h | 42 drivers/gpu/drm/omapdrm/dss/hdmi5_core.c | 35 ++- drivers/gpu/drm/omapdrm/dss/hdmi5_core.h | 33 ++- include/drm/drm_bridge.h | 27 +++ 16 files changed, 453 insertions(+), 54 deletions(-) create mode 100644 drivers/gpu/drm/omapdrm/dss/hdmi5_cec.c create mode 100644 drivers/gpu/drm/omapdrm/dss/hdmi5_cec.h -- 2.30.1
Re: [PATCH v2 0/2] drm/bridge: tc358767: Fix DRM_BRIDGE_ATTACH_NO_CONNECTOR case
On 26/08/2024 22:35, Jan Kiszka wrote: On 24.06.24 12:17, Dmitry Baryshkov wrote: On Mon, 24 Jun 2024 at 13:07, Alexander Stein wrote: Hi, Am Montag, 24. Juni 2024, 11:49:04 CEST schrieb Dmitry Baryshkov: On Mon, Jun 24, 2024 at 03:07:10PM GMT, Aradhya Bhatia wrote: On 22/06/24 17:49, Dmitry Baryshkov wrote: On Sat, Jun 22, 2024 at 05:16:58PM GMT, Aradhya Bhatia wrote: On 17-Jun-24 13:41, Dmitry Baryshkov wrote: On Mon, Jun 17, 2024 at 07:40:32AM GMT, Jan Kiszka wrote: On 16.02.24 15:57, Marek Vasut wrote: On 2/16/24 10:10, Tomi Valkeinen wrote: 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. 1/2 looks good to me, go ahead and apply . Isn't there any way for the second patch to move forward as well though? The bridge device (under DPI to (e)DP mode) cannot really work without it, and the patches have been pending idle for a long time. =) My local patches still apply on top of 6.10-rc4, so I don't think this ever happened. What's still holding up this long-pending fix (at least for our devices)? Neither of the patches contains Fixes tags. If the first patch fixes an issue in previous kernels, please consider following the stable process. If we are unsure about the second patch, please send the first patch separately, adding proper tags. Thanks Dmitry! I can send the patches again with the required fixes tags (or just patch-1 if we cannot do anything about patch-2). The problem with the second patch is that it get mixed reviews. I can ack the first patch, but for the second one I'd need a confirmation from somebody else. I'll go on and apply the first patch later today. Thanks Dmitry! However, would it be okay if I instead add another patch that makes 2 versions of the "tc_edp_bridge_funcs", say "tc_dpi_edp_bridge_funcs" and "tc_dsi_edp_bridge_funcs", that have all the same function hooks except for the .edid_read()? The dsi edid_read() will remain the same, and Tomi's patch - patch 2/2 - will only fix the dpi version of the edid_read()? The bridge already has the capability to distinguish a DSI input from a DPI input. This can be leveraged to decide which set of functions need to be used without any major changes. I'd prefer if somebody with the DSI setup can ack / test the second patch. Now that my DSI-DP setup works apparently without issue I could test patch 2. Since my setup does not use DRM_BRIDGE_ATTACH_NO_CONNECTOR (running on drivers/gpu/drm/mxsfb/lcdif_drv.c), I can only say there is no regression. Let me send a (non-tested) patch, switching to drm_bridge_connector, then you can probably test both of them I suppose [1] was that follow-up, just not leading to success, right? Now, what's next? I'd love to see the regression we have for the IOT2050 devices finally fixed, even if it now only requires a short downstream patch. Jan [1] https://lore.kernel.org/dri-devel/20240624-mxc-lcdif-bridge-attach-v1-1-37e8c5d5d...@linaro.org/ I have to say I don't remember the details for this anymore, but half a year ago I said: Afaics, if the DSI lanes are not set up early enough by the DSI host, the driver would break with and without these patches. I'm not sure if that is correct or not. But if it is, then, afaiu, this (the second patch): - Fixes the issue for the DPI-DP use case - Doesn't cause issues for the DSI-DP use case without DRM_BRIDGE_ATTACH_NO_CONNECTOR (as per Alexander's test) - Shouldn't cause (new) issues for the DSI-DP use case with DRM_BRIDGE_ATTACH_NO_CONNECTOR (as per my code review and guessing...) The third point is somewhat concerning, of course, but do we have any setup with DSI-DP and DRM_BRIDGE_ATTACH_NO_CONNECTOR that works now? If not, maybe we can just ignore the possible issues, as this fixes problems on a setup we do have. Tomi
Re: [PATCH v4 0/4] Add support for RZ/G2UL Display Unit
Hi, On 23/08/2024 18:00, Laurent Pinchart wrote: Hi Biju, On Fri, Aug 23, 2024 at 01:52:14PM +, Biju Das wrote: On Friday, August 23, 2024 2:15 PM, Laurent Pinchart wrote: On Thu, Aug 22, 2024 at 05:23:13PM +0100, Biju Das wrote: This patch series aims to add support for RZ/G2UL DU. The LCD controller is composed of Frame Compression Processor (FCPVD), Video Signal Processor (VSPD), and Display Unit (DU). The output of LCDC is connected display parallel interface (DPI) and supports a maximum resolution of WXGA along with 2 RPFs to support the blending of two picture layers and raster operations (ROPs) It is similar to LCDC IP on RZ/G2L SoCs, but does not have DSI interface. v3->v4: * Restored the ports property for RZ/G2UL and described port@0 for the DPI interface in bindings patch. * Restored tags from Geert and Conor as the change is trivial (Replaced port 1->0 from v2). * Used "&" instead of "==" in rzg2l_du_start_stop() for scalability. * Restored port variable in struct rzg2l_du_output_routing * Updated rzg2l_du_encoders_init() to handle port based on hardware indices. * Restored ports property in du node and used port@0 for connected DPI interface. v2->v3: * Split patch series based on subsystem from DU patch series [1]. * Replaced ports->port property for RZ/G2UL as it supports only DPI and retained ports property for RZ/{G2L,V2L} as it supports both DSI and DPI output interface. * Added missing blank line before example. * Dropped tags from Conor and Geert as there are new changes in bindings * Avoided the line break in rzg2l_du_start_stop() for rstate. * Replaced port->du_output in struct rzg2l_du_output_routing and dropped using the port number to indicate the output type in rzg2l_du_encoders_init(). * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info. [1] https://lore.kernel.org/all/20240709135152.185042-1-biju.das...@bp.renesas.com/ v1->v2: * Updated cover letter header "DU IP->Display Unit". * Updated commit description related to non ABI breakage for patch#3. * Added Ack from Conor for binding patches. Biju Das (4): dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings drm: renesas: rz-du: Add RZ/G2UL DU Support The first two patches look good to me. Do you have access to drm-misc, will you push them yourself, or do you expect a maintainer to pick them up ? I don’t have access to drm-misc. I expect a maintainer to pick it up(Maybe via rcar-du tree or drm-misc tree), or else if it is ok, what is the procedure to get access for drm-misc tree?? You can find instructions to request drm-misc commit access at https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html Tomi, to avoid delays, could you push the first two patches to drm-misc ? I have pushed the first two patches. Tomi
Re: [PATCH] drm: renesas: Move RZ/G2L MIPI DSI driver to rz-du
Hi, On 23/08/2024 18:00, Laurent Pinchart wrote: On Fri, Aug 23, 2024 at 02:33:49PM +0100, Lad, Prabhakar wrote: On Wed, Jun 26, 2024 at 6:51 AM Laurent Pinchart wrote: On Tue, Jun 25, 2024 at 01:32:44PM +0100, Prabhakar wrote: From: Lad Prabhakar All the RZ/G2L DU specific components are located under the rz-du folder, so it makes sense to move the RZ/G2L MIPI DSI driver there instead of keeping it in the rcar-du folder. This change improves the organization and modularity of the driver configuration by grouping related settings together. I was thinking the same the other day. Thanks for beating me at sending a patch :-) Reviewed-by: Laurent Pinchart Do you or Biju has committer rights to drm-misc to push this patch ? We dont, can you please queue this patch via your tree? I don't have other pending patches for DRM at the moment. Tomi, could you push this to drm-misc ? I have pushed this. Should the DSI driver depend on the DU driver in the kconfig? It compiles fine without the DU, but can it ever be used alone? Tomi
Re: [PATCH 9/9] drm/tilcdc: Use backlight power constants
Hi, On 15/08/2024 10:59, Thomas Zimmermann wrote: Ping. This patch still needs an ack. Am 31.07.24 um 14:17 schrieb Thomas Zimmermann: Replace FB_BLANK_ constants with their counterparts from the backlight subsystem. The values are identical, so there's no change in functionality or semantics. Signed-off-by: Thomas Zimmermann Cc: Jyri Sarha Cc: Tomi Valkeinen --- drivers/gpu/drm/tilcdc/tilcdc_panel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/ tilcdc/tilcdc_panel.c index 68093d6b6b16..5f2d1b6f9ee9 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c @@ -49,7 +49,7 @@ static void panel_encoder_dpms(struct drm_encoder *encoder, int mode) if (backlight) { backlight->props.power = mode == DRM_MODE_DPMS_ON ? - FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN; + BACKLIGHT_POWER_ON : BACKLIGHT_POWER_OFF; backlight_update_status(backlight); } Jyri acked it a few weeks ago. But in addition: Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 1/9] of: property: add of_graph_get_next_port()
Hi, On 09/08/2024 05:10, Kuninori Morimoto wrote: Hi Tomi Thank you for your review +/** + * of_graph_get_next_ports() - get next ports node. + * @parent: pointer to the parent device node + * @ports: current ports node, or NULL to get first + * + * Return: A 'ports' node pointer with refcount incremented. Refcount + * of the passed @prev node is decremented. No "prev" argument in the code. The of_graph_get_next_endpoint() function uses "previous" as the argument name (well, the function declaration uses "previous", the implementation uses "prev"...), and I would use the same naming here. Also of_graph_get_next_endpoint() talks about "previous endpoint node", whereas here it's "current ports node". I'd use the same style here, so "previous ports node". The same comments for the of_graph_get_next_port(). OK, thanks. Will fix in v2 +struct device_node *of_graph_get_next_ports(struct device_node *parent, + struct device_node *ports) +{ + if (!parent) + return NULL; + + if (!ports) { + ports = of_get_child_by_name(parent, "ports"); + + /* use parent as its ports of this device if it not exist */ I think this needs to be described in the kernel doc. I understand the need for this, but it's somewhat counter-intuitive that this returns the parent node if there are no ports nodes, so it must be highlighted in the documentation. I wonder if a bit more complexity here would be good... I think here we could: - If there are no 'ports' nodes in the parent, but there is a 'port' node in the parent, return the parent node - If there are no 'ports' nor 'port' nodes in the parent, return NULL Thanks, but unfortunately, get_next_ports() checks only "ports" and doesn't check whether it has "port" node or not. Yes, my point was if the check should be added. My reasoning is: If we have this, of_graph_get_next_ports() returns the ports@0, and that makes sense: parent { ports@0 { port@0 { }; }; }; If we have this, of_graph_get_next_ports() returns the parent, and that's a bit surprising, but I can see the need, and it just needs to be documented: parent { port { }; }; But if we have this, does it make sense that of_graph_get_next_ports() returns the parent, or should it return NULL: parent { /* No ports or port */ }; So correct comment here is maybe... If "parent" doesn't have "ports", it returns "parent" itself as "ports" I will add it on v2 +/** + * of_graph_get_next_port() - get next port node. + * @parent: pointer to the parent device node + * @port: current port node, or NULL to get first + * + * Return: A 'port' node pointer with refcount incremented. Refcount + * of the passed @prev node is decremented. + */ +struct device_node *of_graph_get_next_port(struct device_node *parent, + struct device_node *port) +{ + if (!parent) + return NULL; + + if (!port) { + struct device_node *ports __free(device_node) = + of_graph_get_next_ports(parent, NULL); + + return of_get_child_by_name(ports, "port"); + } + + do { + port = of_get_next_child(parent, port); + if (!port) + break; + } while (!of_node_name_eq(port, "port")); + + return port; +} Hmm... So if I call this with of_graph_get_next_port(dev_node, NULL) (dev_node being the device node of the device), it'll give me the first port in the first ports node, or the first port in the dev_node if there are no ports nodes? Yes And if I then continue iterating with of_graph_get_next_port(dev_node, prev_port)... The call will return NULL if the dev_node contains "ports" node (because the dev_node does not contain any "port" nodes)? So if I understand right, of_graph_get_next_port() must always be called with a parent that contains port nodes. Sometimes that's the device's node (if there's just one port) and sometimes that's ports node. If it's called with a parent that contains ports node, it will not work correctly. If the above is right, then should this just return "of_get_child_by_name(parent, "port")" if !port, instead of calling of_graph_get_next_ports()? Hmm ? Do you mean you want access to ports@1 memeber port after ports@0 ? I have tested below in my test environment parent { (X) ports@0 { (A) port@0 { }; (B) port@1 { }; }; (Y) ports@1 { (C) port@0 { }; }; }; In this case, if you call get_next_port() with parent, you can get ports@0 member port. /* use "paramet" and use "ports@0" are same result */ // use parent port = of_graph_get_next_port(parent, NULL); // (A) port = of_graph_get_next_port(parent, port); // (B) p
Re: [PATCH v2 2/4] dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs
Hi Rob, On 21/03/2024 07:52, Tomi Valkeinen wrote: 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. Do you have any feedback on the above? Should I mark the audio DMAs as optional in the binding? Tomi 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 = <&pd_dp>; resets = <&reset ZYNQMP_RESET_DP>; - dma-names = "vid0", "vid1", "vid2", "gfx0"; + dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; dmas = <&xlnx_dpdma 0>, <&xlnx_dpdma 1>, <&xlnx_dpdma 2>, - <&xlnx_dpdma 3>; + <&xlnx_dpdma 3>, + <&xlnx_dpdma 4>, + <&xlnx_dpdma 5>; phys = <&psgtr 1 PHY_TYPE_DP 0 3>, <&psgtr 0 PHY_TYPE_DP 1 3>; -- 2.34.1
Re: [PATCH v5 00/10] drm: zynqmp_dp: IRQ cleanups and debugfs support
Hi Sean, On 17/06/2024 17:48, Sean Anderson wrote: On 6/17/24 03:47, Tomi Valkeinen wrote: 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. Are you running into [1]? --Sean [1] https://lore.kernel.org/dri-devel/4d8f4c9b-2efb-4774-9a37-2f257f79b...@linux.dev/ No. Afaics, it breaks because the irq handler is requested with IRQF_SHARED, and that means the handler can be called at any time. The handler reads DP registers, but the DP IP could already be powered off. You'll probably see it easily if you enable CONFIG_DEBUG_SHIRQ, and unload the module or unbind the device. Tomi
Re: [PATCH] drm: omapdrm: Add missing check for alloc_ordered_workqueue
On 08/08/2024 09:13, Ma Ke wrote: As it may return NULL pointer and cause NULL pointer dereference. Add check for the return value of alloc_ordered_workqueue. Cc: sta...@vger.kernel.org Fixes: 2f95bc6d324a ("drm: omapdrm: Perform initialization/cleanup at probe/remove time") Signed-off-by: Ma Ke --- drivers/gpu/drm/omapdrm/omap_drv.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 6598c9c08ba1..94a57f0d1c08 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -695,6 +695,10 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) soc = soc_device_match(omapdrm_soc_devices); priv->omaprev = soc ? (uintptr_t)soc->data : 0; priv->wq = alloc_ordered_workqueue("omapdrm", 0); + if (!priv->wq) { + ret = -ENOMEM; + goto err_alloc_workqueue; + } mutex_init(&priv->list_lock); INIT_LIST_HEAD(&priv->obj_list); @@ -753,6 +757,7 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) drm_mode_config_cleanup(ddev); omap_gem_deinit(ddev); destroy_workqueue(priv->wq); +err_alloc_workqueue: omap_disconnect_pipelines(ddev); drm_dev_put(ddev); return ret; Thanks, I'll pick this up to drm-misc. Tomi
Re: [PATCH 9/9] media: xilinx-tpg: use new of_graph functions
On 06/08/2024 07:58, Kuninori Morimoto wrote: Now we can use new port related functions for port parsing. Use it. Signed-off-by: Kuninori Morimoto --- drivers/media/platform/xilinx/xilinx-tpg.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/media/platform/xilinx/xilinx-tpg.c b/drivers/media/platform/xilinx/xilinx-tpg.c index e05e528ffc6f..a25f216b2513 100644 --- a/drivers/media/platform/xilinx/xilinx-tpg.c +++ b/drivers/media/platform/xilinx/xilinx-tpg.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -744,7 +745,7 @@ static int xtpg_parse_of(struct xtpg_device *xtpg) } if (nports == 0) { - endpoint = of_get_next_child(port, NULL); + endpoint = of_graph_get_next_port_endpoint(port, NULL); if (endpoint) has_endpoint = true; of_node_put(endpoint); Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 7/9] gpu: drm: omapdrm: use new of_graph functions
On 06/08/2024 07:58, Kuninori Morimoto wrote: Now we can use new port related functions for port parsing. Use it. Signed-off-by: Kuninori Morimoto --- drivers/gpu/drm/omapdrm/dss/dpi.c | 3 ++- drivers/gpu/drm/omapdrm/dss/sdi.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/dss/dpi.c b/drivers/gpu/drm/omapdrm/dss/dpi.c index 030f997eccd0..b17e77f700dd 100644 --- a/drivers/gpu/drm/omapdrm/dss/dpi.c +++ b/drivers/gpu/drm/omapdrm/dss/dpi.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -709,7 +710,7 @@ int dpi_init_port(struct dss_device *dss, struct platform_device *pdev, if (!dpi) return -ENOMEM; - ep = of_get_next_child(port, NULL); + ep = of_graph_get_next_port_endpoint(port, NULL); if (!ep) return 0; diff --git a/drivers/gpu/drm/omapdrm/dss/sdi.c b/drivers/gpu/drm/omapdrm/dss/sdi.c index 91eaae3b9481..f9ae358e8e52 100644 --- a/drivers/gpu/drm/omapdrm/dss/sdi.c +++ b/drivers/gpu/drm/omapdrm/dss/sdi.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -346,7 +347,7 @@ int sdi_init_port(struct dss_device *dss, struct platform_device *pdev, if (!sdi) return -ENOMEM; - ep = of_get_next_child(port, NULL); + ep = of_graph_get_next_port_endpoint(port, NULL); if (!ep) { r = 0; goto err_free; Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 2/9] of: property: add of_graph_get_next_port_endpoint()
On 06/08/2024 07:58, Kuninori Morimoto wrote: We already have of_graph_get_next_endpoint(), but it is not intuitive to use. (X) node { (Y) ports { port@0 { endpoint { remote-endpoint = ...; };}; (A1)port@1 { endpoint { remote-endpoint = ...; }; (A2) endpoint { remote-endpoint = ...; };}; (B) port@2 { endpoint { remote-endpoint = ...; };}; }; }; For example, if I want to handle port@1's 2 endpoints (= A1, A2), I want to use like below A1 = of_graph_get_next_endpoint(port1, NULL); A2 = of_graph_get_next_endpoint(port1, A1); But 1st one will be error, because of_graph_get_next_endpoint() requested "parent" means "node" (X) or "ports" (Y), not "port". Below are OK /* These will be node/ports/port@0/endpoint */ of_graph_get_next_endpoint(node, NULL); of_graph_get_next_endpoint(ports, NULL); In other words, we can't handle A1/A2 directly via of_graph_get_next_endpoint() so far. There is another non intuitive behavior on of_graph_get_next_endpoint(). In case of if I could get A1 pointer for some way, and if I want to handle port@1 things, I would like use it like below /* * "ep" is now A1, and handle port1 things here, * but we don't know how many endpoints port1 has. * * Because "ep" is non NULL, we can use port1 * as of_graph_get_next_endpoint(port1, xxx) */ do { /* do something for port1 specific things here */ } while (ep = of_graph_get_next_endpoint(port1, ep)) But it also not worked as I expected. I expect it will be A1 -> A2 -> NULL, but it will be A1 -> A2 -> B,because of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port". It is not useful on generic driver like Generic Sound Card. It uses of_get_next_child() instead for now, but it is not intuitive. And it doesn't check node name (= "endpoint"). To handle endpoint more intuitive, create of_graph_get_next_port_endpoint() of_graph_get_next_port_endpoint(port1, NULL); // A1 of_graph_get_next_port_endpoint(port1, A1); // A2 of_graph_get_next_port_endpoint(port1, A2); // NULL Signed-off-by: Kuninori Morimoto --- drivers/of/property.c| 22 ++ include/linux/of_graph.h | 20 2 files changed, 42 insertions(+) diff --git a/drivers/of/property.c b/drivers/of/property.c index 3b2d09c0376a..de56795a7362 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -692,6 +692,28 @@ struct device_node *of_graph_get_next_port(struct device_node *parent, } EXPORT_SYMBOL(of_graph_get_next_port); +/** + * of_graph_get_next_port_endpoint() - get next endpoint node in port. + * If it reached to end of the port, it will return NULL. + * @port: pointer to the target port node + * @endpoint: current endpoint node, or NULL to get first + * + * Return: An 'endpoint' node pointer with refcount incremented. Refcount + * of the passed @prev node is decremented. + */ Same issues here too. No "prev" parameter, and I suggest using "previous", not "current", to be consistent with of_graph_get_next_endpoint(). (or alternatively, change of_graph_get_next_endpoint()). Oh, the declaration of the function uses "prev", but the implementation "endpoint". Please make the naming same. +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port, + struct device_node *endpoint) +{ + do { + endpoint = of_get_next_child(port, endpoint); + if (!endpoint) + break; + } while (!of_node_name_eq(endpoint, "endpoint")); + + return endpoint; +} +EXPORT_SYMBOL(of_graph_get_next_port_endpoint); + /** * of_graph_get_next_endpoint() - get next endpoint node * diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h index 30169b50b042..8b4777938c5e 100644 --- a/include/linux/of_graph.h +++ b/include/linux/of_graph.h @@ -59,6 +59,17 @@ struct of_endpoint { for (child = of_graph_get_next_port(parent, NULL); child != NULL; \ child = of_graph_get_next_port(parent, child)) +/** + * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node + * @parent: parent device or ports node Hmm, shouldn't the parent be a port node? + * @child: loop variable pointing to the current endpoint node + * + * When breaking out of the loop, of_node_put(child) has to be called manually. + */ +#define for_each_of_graph_port_endpoint(parent, child) \ + for (child = of_graph_get_next_port_endpoint(parent, NULL); child != NULL; \ +child = of_graph_get_next_port_endpoint(parent, child)) + #ifdef CONFIG_OF bool of_graph_is_present(const struct device_node *node); int of_graph_parse
Re: [PATCH 1/9] of: property: add of_graph_get_next_port()
On 06/08/2024 07:58, Kuninori Morimoto wrote: We have endpoint base functions - of_graph_get_next_device_endpoint() - of_graph_get_device_endpoint_count() - for_each_of_graph_device_endpoint() Here, for_each_of_graph_device_endpoint() loop finds each endpoints ports { port@0 { (1) endpoint {...}; }; port@1 { (2) endpoint {...}; }; ... }; In above case, it finds endpoint as (1) -> (2) -> ... Basically, user/driver knows which port is used for what, but not in all cases. For example on flexible/generic driver case, how many ports are used is not fixed. For example Sound Generic Card driver which is used from many venders can't know how many ports are used. Because the driver is very flexible/generic, it is impossible to know how many ports are used, it depends on each vender SoC and/or its used board. And more, the port can have multi endpoints. For example Generic Sound Card case, it supports many type of connection between CPU / Codec, and some of them uses multi endpoint in one port. Then, Generic Sound Card want to handle each connection via "port" instead of "endpoint". But, it is very difficult to handle each "port" via for_each_of_graph_device_endpoint(). Getting "port" by using of_get_parent() from "endpoint" doesn't work. see below. ports { port@0 { (1) endpoint@0 {...}; (2) endpoint@1 {...}; }; port@1 { (3) endpoint {...}; }; ... }; In the same time, same reason, we want to handle "ports" same as "port". node { => ports@0 { port@0 { endpoint@0 {...}; endpoint@1 {...}; ... }; port@1 { endpoint@0 {...}; endpoint@1 {...}; ... }; ... }; => ports@1 { ... }; }; Add "ports" / "port" base functions. For above case, we can use for_each_of_graph_ports(node, ports) { for_each_of_graph_port(ports, port) { ... } } This loop works in case of "node" doesn't have "ports" also. Signed-off-by: Kuninori Morimoto --- drivers/of/property.c| 86 include/linux/of_graph.h | 47 ++ 2 files changed, 133 insertions(+) diff --git a/drivers/of/property.c b/drivers/of/property.c index 164d77cb9445..3b2d09c0376a 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -625,8 +625,76 @@ struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id) } EXPORT_SYMBOL(of_graph_get_port_by_id); +/** + * of_graph_get_next_ports() - get next ports node. + * @parent: pointer to the parent device node + * @ports: current ports node, or NULL to get first + * + * Return: A 'ports' node pointer with refcount incremented. Refcount + * of the passed @prev node is decremented. No "prev" argument in the code. The of_graph_get_next_endpoint() function uses "previous" as the argument name (well, the function declaration uses "previous", the implementation uses "prev"...), and I would use the same naming here. Also of_graph_get_next_endpoint() talks about "previous endpoint node", whereas here it's "current ports node". I'd use the same style here, so "previous ports node". The same comments for the of_graph_get_next_port(). + */ +struct device_node *of_graph_get_next_ports(struct device_node *parent, + struct device_node *ports) +{ + if (!parent) + return NULL; + + if (!ports) { + ports = of_get_child_by_name(parent, "ports"); + + /* use parent as its ports of this device if it not exist */ I think this needs to be described in the kernel doc. I understand the need for this, but it's somewhat counter-intuitive that this returns the parent node if there are no ports nodes, so it must be highlighted in the documentation. I wonder if a bit more complexity here would be good... I think here we could: - If there are no 'ports' nodes in the parent, but there is a 'port' node in the parent, return the parent node - If there are no 'ports' nor 'port' nodes in the parent, return NULL + if (!ports) { + ports = parent; + of_node_get(ports); + } You could just do "ports = of_node_get(parent);" + + return ports; + } + + do { + por
Re: [PATCH 3/9] ASoC: test-component: use new of_graph functions
On 06/08/2024 07:58, Kuninori Morimoto wrote: Current test-component.c is using for_each_endpoint_of_node() for parsing, but it should use "port" base loop instead of "endpoint", because properties are "port" base, instead of "endpoint". The subject of the patch makes this sound like it's just using new helper functions, but the description makes me think this is a bug fix. I think it would be good to mention what are the symptoms of the bug. Signed-off-by: Kuninori Morimoto --- sound/soc/generic/test-component.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/generic/test-component.c b/sound/soc/generic/test-component.c index e9e5e235a8a6..6f75da9f86e6 100644 --- a/sound/soc/generic/test-component.c +++ b/sound/soc/generic/test-component.c @@ -600,7 +600,7 @@ static int test_driver_probe(struct platform_device *pdev) } i = 0; - for_each_endpoint_of_node(node, ep) { + for_each_of_graph_port(node, ep) { "ep" is endpoint, so the variable name should also be changed (even if it's not used). snprintf(dname[i].name, TEST_NAME_LEN, "%s.%d", node->name, i); ddriv[i].name = dname[i].name; Tomi
Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
Hi, On 25/07/2024 14:28, Maxime Ripard wrote: On Mon, Jul 15, 2024 at 11:32:34AM GMT, Tomi Valkeinen wrote: 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. FTR, I'm all for supporting module unloading. The discussion above was about what is broken exactly, so we can come up with a good solution. Does that mean that you're ok with the patch, or that something should be improved? Tomi
Re: [PATCH] gpu: drm: use for_each_endpoint_of_node()
On 30/07/2024 03:34, Kuninori Morimoto wrote: We already have for_each_endpoint_of_node(), don't use of_graph_get_next_endpoint() directly. Replace it. Signed-off-by: Kuninori Morimoto Acked-by: Dmitry Baryshkov Reviewed-by: Laurent Pinchart --- drivers/gpu/drm/omapdrm/dss/base.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c index 050ca7eafac58..5f8002f6bb7a5 100644 --- a/drivers/gpu/drm/omapdrm/dss/base.c +++ b/drivers/gpu/drm/omapdrm/dss/base.c @@ -242,8 +242,7 @@ static void omapdss_walk_device(struct device *dev, struct device_node *node, of_node_put(n); - n = NULL; - while ((n = of_graph_get_next_endpoint(node, n)) != NULL) { + for_each_endpoint_of_node(node, n) { struct device_node *pn = of_graph_get_remote_port_parent(n); if (!pn) Thanks, pushed to drm-misc-next. Tomi
[PATCH 3/3] drm/omap: Fix locking in omap_gem_new_dmabuf()
omap_gem_new_dmabuf() creates the new gem object, and then takes and holds the omap_obj->lock for the rest of the function. This has two issues: - omap_gem_free_object(), which is called in the error paths, also takes the same lock, leading to deadlock - Even if the above wouldn't happen, in the error cases omap_gem_new_dmabuf() still unlocks omap_obj->lock, even after the omap_obj has already been freed. Furthermore, I don't think there's any reason to take the lock at all, as the object was just created and not yet shared with anyone else. To fix all this, drop taking the lock. Fixes: 3cbd0c587b12 ("drm/omap: gem: Replace struct_mutex usage with omap_obj private lock") Reported-by: Dan Carpenter Closes: https://lore.kernel.org/all/511b99d7-aade-4f92-bd3e-63163a13d617@stanley.mountain/ Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/omapdrm/omap_gem.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 9ea0c64c26b5..ebbe80c1c0ef 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1402,8 +1402,6 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, omap_obj = to_omap_bo(obj); - mutex_lock(&omap_obj->lock); - omap_obj->sgt = sgt; if (omap_gem_sgt_is_contiguous(sgt, size)) { @@ -1418,21 +1416,17 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL); if (!pages) { omap_gem_free_object(obj); - obj = ERR_PTR(-ENOMEM); - goto done; + return ERR_PTR(-ENOMEM); } omap_obj->pages = pages; ret = drm_prime_sg_to_page_array(sgt, pages, npages); if (ret) { omap_gem_free_object(obj); - obj = ERR_PTR(-ENOMEM); - goto done; + return ERR_PTR(-ENOMEM); } } -done: - mutex_unlock(&omap_obj->lock); return obj; } -- 2.43.0
[PATCH 2/3] drm/omap: Hide sparse warnings
sparse reports: drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:122:16: warning: incorrect type in argument 1 (different address spaces) drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:122:16:expected void const volatile [noderef] __iomem *addr drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:122:16:got unsigned int [usertype] *wa_dma_data drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:130:9: warning: incorrect type in argument 2 (different address spaces) drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:130:9:expected void volatile [noderef] __iomem *addr drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:130:9:got unsigned int [usertype] *wa_dma_data drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:414:9: warning: incorrect type in argument 1 (different address spaces) drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:414:9:expected void const volatile [noderef] __iomem *addr drivers/gpu/drm/omapdrm/omap_dmm_tiler.c:414:9:got unsigned int * These come from pieces of code which do essentially: p = dma_alloc_coherent() dma_transfer_to_p() readl(p) writel(x, p) dma_transfer_from_p() I think we would do just fine without readl() and writel(), accessing the memory without any extras, but ensuring that the necessary barriers are in place. But this code is for a legacy platform, has been working for ages, and it's doing work-arounds for hardware issues, and those hardware issues are very difficult to trigger... So I would just rather leave the code be as it is now. However, the warnings are not nice. Hide the warnings by a (__iomem void *) typecast. Signed-off-by: Tomi Valkeinen Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202407311737.vsj0sr1w-...@intel.com/ Cc: Ville Syrjälä --- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index 1aca3060333e..fcd600024136 100644 --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c @@ -119,7 +119,7 @@ static u32 dmm_read_wa(struct dmm *dmm, u32 reg) * earlier than the DMA finished writing the value to memory. */ rmb(); - return readl(dmm->wa_dma_data); + return readl((__iomem void *)dmm->wa_dma_data); } static void dmm_write_wa(struct dmm *dmm, u32 val, u32 reg) @@ -127,7 +127,7 @@ static void dmm_write_wa(struct dmm *dmm, u32 val, u32 reg) dma_addr_t src, dst; int r; - writel(val, dmm->wa_dma_data); + writel(val, (__iomem void *)dmm->wa_dma_data); /* * As per i878 workaround, the DMA is used to access the DMM registers. * Make sure that the writel is not moved by the compiler or the CPU, so @@ -411,7 +411,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait) */ /* read back to ensure the data is in RAM */ - readl(&txn->last_pat->next_pa); + readl((__iomem void *)&txn->last_pat->next_pa); /* write to PAT_DESCR to clear out any pending transaction */ dmm_write(dmm, 0x0, reg[PAT_DESCR][engine->id]); -- 2.43.0
[PATCH 1/3] drm/omap: Fix possible NULL dereference
smatch reports: drivers/gpu/drm/omapdrm/dss/base.c:176 omapdss_device_disconnect() error: we previously assumed 'src' could be null (see line 169) This code is mostly from a time when omapdrm had its own display device model. I can't honestly remember the details, and I don't think it's worth digging in deeply into that for a legacy driver. However, it looks like we only call omapdss_device_disconnect() and omapdss_device_connect() with NULL as the src parameter. We can thus drop the src parameter from both functions, and fix the smatch warning. I don't think omapdss_device_disconnect() ever gets NULL for the dst parameter (if it did, we'd crash soon after returning from the function), but I have kept the !dst check, just in case, but I added a WARN_ON() there. Also, if the dst parameter can be NULL, we can't always get the struct dss_device pointer from dst->dss (which is only used for a debug print). To make sure we can't hit that issue, do it similarly to the omapdss_device_connect() function: add 'struct dss_device *dss' as the first parameter, so that we always have it regardless of the dst. Fixes: 79107f274b2f ("drm/omap: Add support for drm_bridge") Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/omapdrm/dss/base.c| 25 ++--- drivers/gpu/drm/omapdrm/dss/omapdss.h | 3 +-- drivers/gpu/drm/omapdrm/omap_drv.c| 4 ++-- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c index 050ca7eafac5..556e0f9026be 100644 --- a/drivers/gpu/drm/omapdrm/dss/base.c +++ b/drivers/gpu/drm/omapdrm/dss/base.c @@ -139,21 +139,13 @@ static bool omapdss_device_is_connected(struct omap_dss_device *dssdev) } int omapdss_device_connect(struct dss_device *dss, - struct omap_dss_device *src, struct omap_dss_device *dst) { - dev_dbg(&dss->pdev->dev, "connect(%s, %s)\n", - src ? dev_name(src->dev) : "NULL", + dev_dbg(&dss->pdev->dev, "connect(%s)\n", dst ? dev_name(dst->dev) : "NULL"); - if (!dst) { - /* -* The destination is NULL when the source is connected to a -* bridge instead of a DSS device. Stop here, we will attach -* the bridge later when we will have a DRM encoder. -*/ - return src && src->bridge ? 0 : -EINVAL; - } + if (!dst) + return -EINVAL; if (omapdss_device_is_connected(dst)) return -EBUSY; @@ -163,19 +155,14 @@ int omapdss_device_connect(struct dss_device *dss, return 0; } -void omapdss_device_disconnect(struct omap_dss_device *src, +void omapdss_device_disconnect(struct dss_device *dss, struct omap_dss_device *dst) { - struct dss_device *dss = src ? src->dss : dst->dss; - - dev_dbg(&dss->pdev->dev, "disconnect(%s, %s)\n", - src ? dev_name(src->dev) : "NULL", + dev_dbg(&dss->pdev->dev, "disconnect(%s)\n", dst ? dev_name(dst->dev) : "NULL"); - if (!dst) { - WARN_ON(!src->bridge); + if (WARN_ON(!dst)) return; - } if (!dst->id && !omapdss_device_is_connected(dst)) { WARN_ON(1); diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 040d5a3e33d6..4c22c09c93d5 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -242,9 +242,8 @@ struct omap_dss_device *omapdss_device_get(struct omap_dss_device *dssdev); void omapdss_device_put(struct omap_dss_device *dssdev); struct omap_dss_device *omapdss_find_device_by_node(struct device_node *node); int omapdss_device_connect(struct dss_device *dss, - struct omap_dss_device *src, struct omap_dss_device *dst); -void omapdss_device_disconnect(struct omap_dss_device *src, +void omapdss_device_disconnect(struct dss_device *dss, struct omap_dss_device *dst); int omap_dss_get_num_overlay_managers(void); diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 6598c9c08ba1..d80d24fce79f 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -307,7 +307,7 @@ static void omap_disconnect_pipelines(struct drm_device *ddev) for (i = 0; i < priv->num_pipes; i++) { struct omap_drm_pipeline *pipe = &priv->pipes[i]; - omapdss_device_disconnect(NULL, pipe->output); + omapdss_device_disconnect(priv->dss, pipe-
[PATCH 0/3] drm/omap: Minor fixes
A few minor fixes to omapdrm, mostly to remove sparse or other checker warnings. Tomi Signed-off-by: Tomi Valkeinen --- Tomi Valkeinen (3): drm/omap: Fix possible NULL dereference drm/omap: Hide sparse warnings drm/omap: Fix locking in omap_gem_new_dmabuf() drivers/gpu/drm/omapdrm/dss/base.c | 25 ++--- drivers/gpu/drm/omapdrm/dss/omapdss.h| 3 +-- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 +++--- drivers/gpu/drm/omapdrm/omap_drv.c | 4 ++-- drivers/gpu/drm/omapdrm/omap_gem.c | 10 ++ 5 files changed, 14 insertions(+), 34 deletions(-) --- base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd change-id: 20240806-omapdrm-misc-fixes-2ea920193dde Best regards, -- Tomi Valkeinen
Re: [bug report] drm/omap: gem: Replace struct_mutex usage with omap_obj private lock
On 31/07/2024 15:26, Laurent Pinchart wrote: Hi Dan, (CC'ing Tomi) Thank for the report. It indeed seems that something is wrong. Tomi, could you handle this and send a fix ? On Tue, Jul 30, 2024 at 05:01:35PM -0500, Dan Carpenter wrote: Hello Laurent Pinchart, Commit 3cbd0c587b12 ("drm/omap: gem: Replace struct_mutex usage with omap_obj private lock") from May 26, 2018 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/omapdrm/omap_gem.c:1435 omap_gem_new_dmabuf() warn: 'omap_obj' was already freed. (line 1434) drivers/gpu/drm/omapdrm/omap_gem.c 1386 struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, 1387struct sg_table *sgt) 1388 { 1389 struct omap_drm_private *priv = dev->dev_private; 1390 struct omap_gem_object *omap_obj; 1391 struct drm_gem_object *obj; 1392 union omap_gem_size gsize; 1393 1394 /* Without a DMM only physically contiguous buffers can be supported. */ 1395 if (!omap_gem_sgt_is_contiguous(sgt, size) && !priv->has_dmm) 1396 return ERR_PTR(-EINVAL); 1397 1398 gsize.bytes = PAGE_ALIGN(size); 1399 obj = omap_gem_new(dev, gsize, OMAP_BO_MEM_DMABUF | OMAP_BO_WC); 1400 if (!obj) 1401 return ERR_PTR(-ENOMEM); 1402 1403 omap_obj = to_omap_bo(obj); ^^ This is omap_obj 1404 1405 mutex_lock(&omap_obj->lock); I wonder why we even lock it. We just allocated it above, no one else can be using it. Not only that, but omap_gem_free_object(), which is called in the error paths, _also_ takes the same lock. I think we can just drop the locking from this function. But first I need to figure out how to run this piece of code to test it... Tomi 1406 1407 omap_obj->sgt = sgt; 1408 1409 if (omap_gem_sgt_is_contiguous(sgt, size)) { 1410 omap_obj->dma_addr = sg_dma_address(sgt->sgl); 1411 } else { 1412 /* Create pages list from sgt */ 1413 struct page **pages; 1414 unsigned int npages; 1415 unsigned int ret; 1416 1417 npages = DIV_ROUND_UP(size, PAGE_SIZE); 1418 pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL); 1419 if (!pages) { 1420 omap_gem_free_object(obj); ^^^ It gets free inside this function 1421 obj = ERR_PTR(-ENOMEM); 1422 goto done; 1423 } 1424 1425 omap_obj->pages = pages; 1426 ret = drm_prime_sg_to_page_array(sgt, pages, npages); 1427 if (ret) { 1428 omap_gem_free_object(obj); ^^^ Same 1429 obj = ERR_PTR(-ENOMEM); 1430 goto done; So I think we can just return directly instead of unlocking. 1431 } 1432 } 1433 1434 done: --> 1435 mutex_unlock(&omap_obj->lock); 1436 return obj; 1437 }
Re: [PATCH] drm/omap: add CONFIG_MMU dependency
Hi, On 19/07/2024 12:59, Arnd Bergmann wrote: From: Arnd Bergmann Compile-testing with CONFIG_MMU disabled causes a link error in omapdrm: arm-linux-gnueabi-ld: drivers/gpu/drm/omapdrm/omap_gem.o: in function `omap_gem_fault_2d': omap_gem.c:(.text+0x36e): undefined reference to `vmf_insert_mixed' arm-linux-gnueabi-ld: drivers/gpu/drm/omapdrm/omap_gem.o: in function `omap_gem_fault': omap_gem.c:(.text+0x74a): undefined reference to `vmf_insert_mixed' Avoid this by adding a Kconfig dependency. Fixes: dc6fcaaba5a5 ("drm/omap: Allow build with COMPILE_TEST=y") Signed-off-by: Arnd Bergmann Thanks, applying to drm-misc-fixes. Tomi --- drivers/gpu/drm/omapdrm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig index 3f7139e211d2..64e440a2649b 100644 --- a/drivers/gpu/drm/omapdrm/Kconfig +++ b/drivers/gpu/drm/omapdrm/Kconfig @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only config DRM_OMAP tristate "OMAP DRM" + depends on MMU depends on DRM && OF depends on ARCH_OMAP2PLUS || (COMPILE_TEST && PAGE_SIZE_LESS_THAN_64KB) select DRM_KMS_HELPER
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 = <&k3_clks 186 0>; +clock-names = "s_clk"; +ti,oldi-io-ctrl = <&dss_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 &dss_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 &dss_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
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 = <&dss_oldi_io_ctrl>; -power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>; -clocks =<&k3_clks 67 1>, -<&k3_clks 216 1>, -<&k3_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 = <&lcd_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 = <&dss_oldi_io_ctrl>; +power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>; +clocks =<&k3_clks 67 1>, +<&k3_clks 216 1>, +<&k3_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 = <&lcd_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(&tidss->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 b/drivers/
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 = &dsi->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 &struct 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; +
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 = &dsi->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 + * &drm_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(&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 = &output->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, &dsi_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, &output->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 = &dsi->output; struct cdns_dsi_input *input = &dsi->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 = &dsi->output; struct cdns_dsi_input *input = &dsi->input; drm_bridge_remove(&input->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, &val); + ret = regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val); 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, &val); + ret = regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val); 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(&dsi->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(&dsi->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:
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
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 = &pdev->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(&pdev->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(&dp->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(&dp->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(&dp->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(&dp->hpd_irq_work); + if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY) + complete(&dp->aux_done); + + if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT) + complete(&dp->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(&dp->lock); + init_completion(&dp->aux_done); INIT_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func); INIT_WORK(&dp->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(&pdev->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(&dp->hpd_work); + cancel_work(&dp->hpd_work); zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0); drm_dp_dpcd_writeb(&dp->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(&dp->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(&dp->hpd_work, 0); + schedule_work(&dp->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(&dp->hpd_work, zynqmp_dp_hpd_work_func); + INIT_WORK(&dp->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(&dp->hpd_work); + cancel_work_sync(&dp->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 = &pdev->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 &layer->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
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
[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, &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 = &pdev->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->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
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", &panel_module_ops); - pinctrl = devm_pinctrl_get_select_default(&pdev->dev); - if (IS_ERR(pinctrl)) - dev_warn(&pdev->dev, "pins are not configured\n"); - panel_mod->timings = of_get_display_timings(node); if (!panel_mod->timings) { dev_err(&pdev->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 = &omap_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 &layer->info->formats[i]; + disp_format = &layer->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
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 = &layer->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 = &layer->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 = &layer_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 db3
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 = &layer_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/zy
[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, &dp->status); return IRQ_WAKE_THREAD; } return IRQ_HANDLED; } zynqmp_dp_thread_handler(...) { status = atomic_xchg(&dp->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, &dp->status); return IRQ_WAKE_THREAD; } return IRQ_HANDLED; } zynqmp_dp_thread_handler(...) { status = atomic_xchg(&dp->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