| Message ID | 20241217-drm-bridge-hdmi-connector-v7-0-cb9df2b6a515@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: add DRM HDMI Codec framework | expand |
On Tue Dec 17, 2024 at 1:40 AM CET, Dmitry Baryshkov wrote: > This series depends on the ELD mutex series [1] > > [1] https://lore.kernel.org/r/20241201-drm-connector-eld-mutex-v1-0-ba56a6545c03@linaro.org There's a v2 of that patch series here: https://lore.kernel.org/all/20241206-drm-connector-eld-mutex-v2-0-c9bce1ee8bea@linaro.org/ HTH, Diederik
On Tue, Dec 17, 2024 at 01:36:29PM +0100, Diederik de Haas wrote: > On Tue Dec 17, 2024 at 1:40 AM CET, Dmitry Baryshkov wrote: > > This series depends on the ELD mutex series [1] > > > > [1] https://lore.kernel.org/r/20241201-drm-connector-eld-mutex-v1-0-ba56a6545c03@linaro.org > > There's a v2 of that patch series here: > https://lore.kernel.org/all/20241206-drm-connector-eld-mutex-v2-0-c9bce1ee8bea@linaro.org/ ... and it's even merged to drm-misc-next. I dropped it from b4 deps, but I forgot to update the cover letter.
Hi, On Tue, Dec 17, 2024 at 02:40:22AM +0200, Dmitry Baryshkov wrote: > While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector > framework, I stumbled upon an issue while handling the Audio InfoFrames. > The HDMI codec callbacks weren't receiving the drm_atomic_state, so > there was no simple way to get the drm_connector that stayed at the end > of the bridge chain. At the same point the drm_hdmi_connector functions > expected to get drm_connector instance. > > While looking for a way to solve the issue, I stumbled upon several > deficiencies in existing hdmi_codec_ops implementations. Only few of the > implementations were able to handle codec's 'plugged' callback. One > third of the drivers didn't implement the get_eld() callback. > > Most of the issues can be solved if drm_connector handles > hdmi-audio-codec on its own, delegating functionality to the actual > implementation, be it a driver that implements drm_connector or > drm_bridge. > > Implement such high-level framework, adding proper support for Audio > InfoFrame generation to the LT9611 driver. > > Several design decisions to be kept in mind: > > - drm_connector_hdmi_codec is kept as simple as possible. It implements > generic functionality (ELD, hotplug, registration). > > - drm_hdmi_connector sets up HDMI codec device if the connector > is setup correspondingly (either I2S or S/PDIF is marked as > supported). > > - drm_bridge_connector provides a way to link HDMI audio codec > funcionality in the drm_bridge with the drm_connector_hdmi_codec > framework. > > - It might be worth reverting the no_i2s_capture / no_spdif_capture > bits. Only TDA889x driver sets them, while it's safe to assume that > most of HDMI / DP devices do not support ARC / capture. I think the > drivers should opt-in capture support rather than having to opt-out of > it. Sorry if this isn't clear to me and I'm quite late to the party, but did you test this on vc4 with both a pi3 and pi4, or was it just compile tested? Maxime
On Tue, 17 Dec 2024 at 19:21, Maxime Ripard <mripard@kernel.org> wrote: > > Hi, > > On Tue, Dec 17, 2024 at 02:40:22AM +0200, Dmitry Baryshkov wrote: > > While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector > > framework, I stumbled upon an issue while handling the Audio InfoFrames. > > The HDMI codec callbacks weren't receiving the drm_atomic_state, so > > there was no simple way to get the drm_connector that stayed at the end > > of the bridge chain. At the same point the drm_hdmi_connector functions > > expected to get drm_connector instance. > > > > While looking for a way to solve the issue, I stumbled upon several > > deficiencies in existing hdmi_codec_ops implementations. Only few of the > > implementations were able to handle codec's 'plugged' callback. One > > third of the drivers didn't implement the get_eld() callback. > > > > Most of the issues can be solved if drm_connector handles > > hdmi-audio-codec on its own, delegating functionality to the actual > > implementation, be it a driver that implements drm_connector or > > drm_bridge. > > > > Implement such high-level framework, adding proper support for Audio > > InfoFrame generation to the LT9611 driver. > > > > Several design decisions to be kept in mind: > > > > - drm_connector_hdmi_codec is kept as simple as possible. It implements > > generic functionality (ELD, hotplug, registration). > > > > - drm_hdmi_connector sets up HDMI codec device if the connector > > is setup correspondingly (either I2S or S/PDIF is marked as > > supported). > > > > - drm_bridge_connector provides a way to link HDMI audio codec > > funcionality in the drm_bridge with the drm_connector_hdmi_codec > > framework. > > > > - It might be worth reverting the no_i2s_capture / no_spdif_capture > > bits. Only TDA889x driver sets them, while it's safe to assume that > > most of HDMI / DP devices do not support ARC / capture. I think the > > drivers should opt-in capture support rather than having to opt-out of > > it. > > Sorry if this isn't clear to me and I'm quite late to the party, but did > you test this on vc4 with both a pi3 and pi4, or was it just compile > tested? LT9611 is actually tested, VC4 is only compile-tested. Should I put an RFT tag?
On Wed, Dec 18, 2024 at 07:24:23AM +0200, Dmitry Baryshkov wrote: > On Tue, 17 Dec 2024 at 19:21, Maxime Ripard <mripard@kernel.org> wrote: > > On Tue, Dec 17, 2024 at 02:40:22AM +0200, Dmitry Baryshkov wrote: > > > While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector > > > framework, I stumbled upon an issue while handling the Audio InfoFrames. > > > The HDMI codec callbacks weren't receiving the drm_atomic_state, so > > > there was no simple way to get the drm_connector that stayed at the end > > > of the bridge chain. At the same point the drm_hdmi_connector functions > > > expected to get drm_connector instance. > > > > > > While looking for a way to solve the issue, I stumbled upon several > > > deficiencies in existing hdmi_codec_ops implementations. Only few of the > > > implementations were able to handle codec's 'plugged' callback. One > > > third of the drivers didn't implement the get_eld() callback. > > > > > > Most of the issues can be solved if drm_connector handles > > > hdmi-audio-codec on its own, delegating functionality to the actual > > > implementation, be it a driver that implements drm_connector or > > > drm_bridge. > > > > > > Implement such high-level framework, adding proper support for Audio > > > InfoFrame generation to the LT9611 driver. > > > > > > Several design decisions to be kept in mind: > > > > > > - drm_connector_hdmi_codec is kept as simple as possible. It implements > > > generic functionality (ELD, hotplug, registration). > > > > > > - drm_hdmi_connector sets up HDMI codec device if the connector > > > is setup correspondingly (either I2S or S/PDIF is marked as > > > supported). > > > > > > - drm_bridge_connector provides a way to link HDMI audio codec > > > funcionality in the drm_bridge with the drm_connector_hdmi_codec > > > framework. > > > > > > - It might be worth reverting the no_i2s_capture / no_spdif_capture > > > bits. Only TDA889x driver sets them, while it's safe to assume that > > > most of HDMI / DP devices do not support ARC / capture. I think the > > > drivers should opt-in capture support rather than having to opt-out of > > > it. > > > > Sorry if this isn't clear to me and I'm quite late to the party, but did > > you test this on vc4 with both a pi3 and pi4, or was it just compile > > tested? > > LT9611 is actually tested, VC4 is only compile-tested. Should I put an RFT tag? Yeah, we definitely need to test it on the pi3 (polling-based) and the pi4 (irq-based) at least. Dave, Maira, could you give it a try? Maxime
Hi Maxime & Dmitry On Wed, 18 Dec 2024 at 07:59, Maxime Ripard <mripard@kernel.org> wrote: > > On Wed, Dec 18, 2024 at 07:24:23AM +0200, Dmitry Baryshkov wrote: > > On Tue, 17 Dec 2024 at 19:21, Maxime Ripard <mripard@kernel.org> wrote: > > > On Tue, Dec 17, 2024 at 02:40:22AM +0200, Dmitry Baryshkov wrote: > > > > While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector > > > > framework, I stumbled upon an issue while handling the Audio InfoFrames. > > > > The HDMI codec callbacks weren't receiving the drm_atomic_state, so > > > > there was no simple way to get the drm_connector that stayed at the end > > > > of the bridge chain. At the same point the drm_hdmi_connector functions > > > > expected to get drm_connector instance. > > > > > > > > While looking for a way to solve the issue, I stumbled upon several > > > > deficiencies in existing hdmi_codec_ops implementations. Only few of the > > > > implementations were able to handle codec's 'plugged' callback. One > > > > third of the drivers didn't implement the get_eld() callback. > > > > > > > > Most of the issues can be solved if drm_connector handles > > > > hdmi-audio-codec on its own, delegating functionality to the actual > > > > implementation, be it a driver that implements drm_connector or > > > > drm_bridge. > > > > > > > > Implement such high-level framework, adding proper support for Audio > > > > InfoFrame generation to the LT9611 driver. > > > > > > > > Several design decisions to be kept in mind: > > > > > > > > - drm_connector_hdmi_codec is kept as simple as possible. It implements > > > > generic functionality (ELD, hotplug, registration). > > > > > > > > - drm_hdmi_connector sets up HDMI codec device if the connector > > > > is setup correspondingly (either I2S or S/PDIF is marked as > > > > supported). > > > > > > > > - drm_bridge_connector provides a way to link HDMI audio codec > > > > funcionality in the drm_bridge with the drm_connector_hdmi_codec > > > > framework. > > > > > > > > - It might be worth reverting the no_i2s_capture / no_spdif_capture > > > > bits. Only TDA889x driver sets them, while it's safe to assume that > > > > most of HDMI / DP devices do not support ARC / capture. I think the > > > > drivers should opt-in capture support rather than having to opt-out of > > > > it. > > > > > > Sorry if this isn't clear to me and I'm quite late to the party, but did > > > you test this on vc4 with both a pi3 and pi4, or was it just compile > > > tested? > > > > LT9611 is actually tested, VC4 is only compile-tested. Should I put an RFT tag? > > Yeah, we definitely need to test it on the pi3 (polling-based) and the > pi4 (irq-based) at least. > > Dave, Maira, could you give it a try? I'm on it. Dave > Maxime
On Wed, 18 Dec 2024 at 14:52, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Maxime & Dmitry > > On Wed, 18 Dec 2024 at 07:59, Maxime Ripard <mripard@kernel.org> wrote: > > > > On Wed, Dec 18, 2024 at 07:24:23AM +0200, Dmitry Baryshkov wrote: > > > On Tue, 17 Dec 2024 at 19:21, Maxime Ripard <mripard@kernel.org> wrote: > > > > On Tue, Dec 17, 2024 at 02:40:22AM +0200, Dmitry Baryshkov wrote: > > > > > While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector > > > > > framework, I stumbled upon an issue while handling the Audio InfoFrames. > > > > > The HDMI codec callbacks weren't receiving the drm_atomic_state, so > > > > > there was no simple way to get the drm_connector that stayed at the end > > > > > of the bridge chain. At the same point the drm_hdmi_connector functions > > > > > expected to get drm_connector instance. > > > > > > > > > > While looking for a way to solve the issue, I stumbled upon several > > > > > deficiencies in existing hdmi_codec_ops implementations. Only few of the > > > > > implementations were able to handle codec's 'plugged' callback. One > > > > > third of the drivers didn't implement the get_eld() callback. > > > > > > > > > > Most of the issues can be solved if drm_connector handles > > > > > hdmi-audio-codec on its own, delegating functionality to the actual > > > > > implementation, be it a driver that implements drm_connector or > > > > > drm_bridge. > > > > > > > > > > Implement such high-level framework, adding proper support for Audio > > > > > InfoFrame generation to the LT9611 driver. > > > > > > > > > > Several design decisions to be kept in mind: > > > > > > > > > > - drm_connector_hdmi_codec is kept as simple as possible. It implements > > > > > generic functionality (ELD, hotplug, registration). > > > > > > > > > > - drm_hdmi_connector sets up HDMI codec device if the connector > > > > > is setup correspondingly (either I2S or S/PDIF is marked as > > > > > supported). > > > > > > > > > > - drm_bridge_connector provides a way to link HDMI audio codec > > > > > funcionality in the drm_bridge with the drm_connector_hdmi_codec > > > > > framework. > > > > > > > > > > - It might be worth reverting the no_i2s_capture / no_spdif_capture > > > > > bits. Only TDA889x driver sets them, while it's safe to assume that > > > > > most of HDMI / DP devices do not support ARC / capture. I think the > > > > > drivers should opt-in capture support rather than having to opt-out of > > > > > it. > > > > > > > > Sorry if this isn't clear to me and I'm quite late to the party, but did > > > > you test this on vc4 with both a pi3 and pi4, or was it just compile > > > > tested? > > > > > > LT9611 is actually tested, VC4 is only compile-tested. Should I put an RFT tag? > > > > Yeah, we definitely need to test it on the pi3 (polling-based) and the > > pi4 (irq-based) at least. > > > > Dave, Maira, could you give it a try? > > I'm on it. Basic checks look OK. Pi3 and Pi4 tested, including 4k60 on Pi4 (enables scrambling). Audio works, and all the EDID parsing seems to be correct. Ideally I would like to do a couple more tests though as I had a couple of spontaneous reboots on Pi4 whilst hotplugging. Most likely it was just grounding issues, as I can't think of an obvious way for the kernel to be triggering an automatic reboot. They seemed to go away when I had enabled drm.debug=0x14, but more likely I'd connected the serial port and therefore provided another ground. Please don't hold the patches waiting on me though - I don't know when I'll have time to do them. Also unrelated to this patch set, I have noted I'm getting [ 60.780897] WARNING: CPU: 3 PID: 501 at drivers/gpu/drm/vc4/vc4_hvs.c:743 __vc4_hvs_stop_channel+0x148/0x164 [vc4] ... [ 60.781192] __vc4_hvs_stop_channel+0x148/0x164 [vc4] (P) [ 60.781212] __vc4_hvs_stop_channel+0x40/0x164 [vc4] (L) [ 60.781230] vc4_hvs_stop_channel+0x30/0x3c [vc4] [ 60.781248] vc4_crtc_disable+0x140/0x1e8 [vc4] [ 60.781266] vc4_crtc_atomic_disable+0x98/0xb8 [vc4] [ 60.781284] disable_outputs+0x22c/0x33c [drm_kms_helper] [ 60.781326] drm_atomic_helper_commit_modeset_disables+0x1c/0x4c [drm_kms_helper] [ 60.781347] vc4_atomic_commit_tail+0x10c/0x8e4 [vc4] [ 60.781364] commit_tail+0xa0/0x188 [drm_kms_helper] [ 60.781385] drm_atomic_helper_commit+0x16c/0x180 [drm_kms_helper] [ 60.781406] drm_atomic_commit+0x88/0xc4 [drm] [ 60.781500] drm_client_modeset_commit_atomic+0x204/0x264 [drm] [ 60.781553] drm_client_modeset_commit_locked+0x5c/0x198 [drm] [ 60.781603] drm_client_modeset_commit+0x30/0x58 [drm] [ 60.781652] __drm_fb_helper_restore_fbdev_mode_unlocked+0xb4/0xe8 [drm_kms_helper] [ 60.781674] drm_fb_helper_hotplug_event+0x108/0x12c [drm_kms_helper] [ 60.781695] drm_fbdev_client_hotplug+0x28/0xd4 [drm_client_lib] [ 60.781700] drm_client_dev_hotplug+0xc8/0x12c [drm] [ 60.781750] drm_connector_helper_hpd_irq_event+0x70/0xb0 [drm_kms_helper] [ 60.781771] vc4_hdmi_hpd_irq_thread+0x2c/0x3c [vc4] [ 60.781790] irq_thread_fn+0x2c/0xa8 [ 60.781799] irq_thread+0x16c/0x2f4 [ 60.781805] kthread+0x118/0x11c [ 60.781813] ret_from_fork+0x10/0x20 On first plugging or unplugging a second monitor. I suspect that is spurious though and I'll look into it, but probably not this side of the Christmas break. Dave
On Thu, Dec 19, 2024 at 04:16:37PM +0000, Dave Stevenson wrote: > On Wed, 18 Dec 2024 at 14:52, Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Maxime & Dmitry > > > > On Wed, 18 Dec 2024 at 07:59, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > On Wed, Dec 18, 2024 at 07:24:23AM +0200, Dmitry Baryshkov wrote: > > > > On Tue, 17 Dec 2024 at 19:21, Maxime Ripard <mripard@kernel.org> wrote: > > > > > On Tue, Dec 17, 2024 at 02:40:22AM +0200, Dmitry Baryshkov wrote: > > > > > > While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector > > > > > > framework, I stumbled upon an issue while handling the Audio InfoFrames. > > > > > > The HDMI codec callbacks weren't receiving the drm_atomic_state, so > > > > > > there was no simple way to get the drm_connector that stayed at the end > > > > > > of the bridge chain. At the same point the drm_hdmi_connector functions > > > > > > expected to get drm_connector instance. > > > > > > > > > > > > While looking for a way to solve the issue, I stumbled upon several > > > > > > deficiencies in existing hdmi_codec_ops implementations. Only few of the > > > > > > implementations were able to handle codec's 'plugged' callback. One > > > > > > third of the drivers didn't implement the get_eld() callback. > > > > > > > > > > > > Most of the issues can be solved if drm_connector handles > > > > > > hdmi-audio-codec on its own, delegating functionality to the actual > > > > > > implementation, be it a driver that implements drm_connector or > > > > > > drm_bridge. > > > > > > > > > > > > Implement such high-level framework, adding proper support for Audio > > > > > > InfoFrame generation to the LT9611 driver. > > > > > > > > > > > > Several design decisions to be kept in mind: > > > > > > > > > > > > - drm_connector_hdmi_codec is kept as simple as possible. It implements > > > > > > generic functionality (ELD, hotplug, registration). > > > > > > > > > > > > - drm_hdmi_connector sets up HDMI codec device if the connector > > > > > > is setup correspondingly (either I2S or S/PDIF is marked as > > > > > > supported). > > > > > > > > > > > > - drm_bridge_connector provides a way to link HDMI audio codec > > > > > > funcionality in the drm_bridge with the drm_connector_hdmi_codec > > > > > > framework. > > > > > > > > > > > > - It might be worth reverting the no_i2s_capture / no_spdif_capture > > > > > > bits. Only TDA889x driver sets them, while it's safe to assume that > > > > > > most of HDMI / DP devices do not support ARC / capture. I think the > > > > > > drivers should opt-in capture support rather than having to opt-out of > > > > > > it. > > > > > > > > > > Sorry if this isn't clear to me and I'm quite late to the party, but did > > > > > you test this on vc4 with both a pi3 and pi4, or was it just compile > > > > > tested? > > > > > > > > LT9611 is actually tested, VC4 is only compile-tested. Should I put an RFT tag? > > > > > > Yeah, we definitely need to test it on the pi3 (polling-based) and the > > > pi4 (irq-based) at least. > > > > > > Dave, Maira, could you give it a try? > > > > I'm on it. > > Basic checks look OK. > Pi3 and Pi4 tested, including 4k60 on Pi4 (enables scrambling). Audio > works, and all the EDID parsing seems to be correct. > > Ideally I would like to do a couple more tests though as I had a > couple of spontaneous reboots on Pi4 whilst hotplugging. > Most likely it was just grounding issues, as I can't think of an > obvious way for the kernel to be triggering an automatic reboot. They > seemed to go away when I had enabled drm.debug=0x14, but more likely > I'd connected the serial port and therefore provided another ground. > Please don't hold the patches waiting on me though - I don't know when > I'll have time to do them. Tested-by? > > > Also unrelated to this patch set, I have noted I'm getting > [ 60.780897] WARNING: CPU: 3 PID: 501 at > drivers/gpu/drm/vc4/vc4_hvs.c:743 __vc4_hvs_stop_channel+0x148/0x164 > [vc4] > ... > [ 60.781192] __vc4_hvs_stop_channel+0x148/0x164 [vc4] (P) > [ 60.781212] __vc4_hvs_stop_channel+0x40/0x164 [vc4] (L) > [ 60.781230] vc4_hvs_stop_channel+0x30/0x3c [vc4] > [ 60.781248] vc4_crtc_disable+0x140/0x1e8 [vc4] > [ 60.781266] vc4_crtc_atomic_disable+0x98/0xb8 [vc4] > [ 60.781284] disable_outputs+0x22c/0x33c [drm_kms_helper] > [ 60.781326] drm_atomic_helper_commit_modeset_disables+0x1c/0x4c > [drm_kms_helper] > [ 60.781347] vc4_atomic_commit_tail+0x10c/0x8e4 [vc4] > [ 60.781364] commit_tail+0xa0/0x188 [drm_kms_helper] > [ 60.781385] drm_atomic_helper_commit+0x16c/0x180 [drm_kms_helper] > [ 60.781406] drm_atomic_commit+0x88/0xc4 [drm] > [ 60.781500] drm_client_modeset_commit_atomic+0x204/0x264 [drm] > [ 60.781553] drm_client_modeset_commit_locked+0x5c/0x198 [drm] > [ 60.781603] drm_client_modeset_commit+0x30/0x58 [drm] > [ 60.781652] __drm_fb_helper_restore_fbdev_mode_unlocked+0xb4/0xe8 > [drm_kms_helper] > [ 60.781674] drm_fb_helper_hotplug_event+0x108/0x12c [drm_kms_helper] > [ 60.781695] drm_fbdev_client_hotplug+0x28/0xd4 [drm_client_lib] > [ 60.781700] drm_client_dev_hotplug+0xc8/0x12c [drm] > [ 60.781750] drm_connector_helper_hpd_irq_event+0x70/0xb0 [drm_kms_helper] > [ 60.781771] vc4_hdmi_hpd_irq_thread+0x2c/0x3c [vc4] > [ 60.781790] irq_thread_fn+0x2c/0xa8 > [ 60.781799] irq_thread+0x16c/0x2f4 > [ 60.781805] kthread+0x118/0x11c > [ 60.781813] ret_from_fork+0x10/0x20 > On first plugging or unplugging a second monitor. I suspect that is > spurious though and I'll look into it, but probably not this side of > the Christmas break. > > Dave
On Fri, 20 Dec 2024 at 00:45, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, Dec 19, 2024 at 04:16:37PM +0000, Dave Stevenson wrote: > > On Wed, 18 Dec 2024 at 14:52, Dave Stevenson > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > Hi Maxime & Dmitry > > > > > > On Wed, 18 Dec 2024 at 07:59, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > On Wed, Dec 18, 2024 at 07:24:23AM +0200, Dmitry Baryshkov wrote: > > > > > On Tue, 17 Dec 2024 at 19:21, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > On Tue, Dec 17, 2024 at 02:40:22AM +0200, Dmitry Baryshkov wrote: > > > > > > > While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector > > > > > > > framework, I stumbled upon an issue while handling the Audio InfoFrames. > > > > > > > The HDMI codec callbacks weren't receiving the drm_atomic_state, so > > > > > > > there was no simple way to get the drm_connector that stayed at the end > > > > > > > of the bridge chain. At the same point the drm_hdmi_connector functions > > > > > > > expected to get drm_connector instance. > > > > > > > > > > > > > > While looking for a way to solve the issue, I stumbled upon several > > > > > > > deficiencies in existing hdmi_codec_ops implementations. Only few of the > > > > > > > implementations were able to handle codec's 'plugged' callback. One > > > > > > > third of the drivers didn't implement the get_eld() callback. > > > > > > > > > > > > > > Most of the issues can be solved if drm_connector handles > > > > > > > hdmi-audio-codec on its own, delegating functionality to the actual > > > > > > > implementation, be it a driver that implements drm_connector or > > > > > > > drm_bridge. > > > > > > > > > > > > > > Implement such high-level framework, adding proper support for Audio > > > > > > > InfoFrame generation to the LT9611 driver. > > > > > > > > > > > > > > Several design decisions to be kept in mind: > > > > > > > > > > > > > > - drm_connector_hdmi_codec is kept as simple as possible. It implements > > > > > > > generic functionality (ELD, hotplug, registration). > > > > > > > > > > > > > > - drm_hdmi_connector sets up HDMI codec device if the connector > > > > > > > is setup correspondingly (either I2S or S/PDIF is marked as > > > > > > > supported). > > > > > > > > > > > > > > - drm_bridge_connector provides a way to link HDMI audio codec > > > > > > > funcionality in the drm_bridge with the drm_connector_hdmi_codec > > > > > > > framework. > > > > > > > > > > > > > > - It might be worth reverting the no_i2s_capture / no_spdif_capture > > > > > > > bits. Only TDA889x driver sets them, while it's safe to assume that > > > > > > > most of HDMI / DP devices do not support ARC / capture. I think the > > > > > > > drivers should opt-in capture support rather than having to opt-out of > > > > > > > it. > > > > > > > > > > > > Sorry if this isn't clear to me and I'm quite late to the party, but did > > > > > > you test this on vc4 with both a pi3 and pi4, or was it just compile > > > > > > tested? > > > > > > > > > > LT9611 is actually tested, VC4 is only compile-tested. Should I put an RFT tag? > > > > > > > > Yeah, we definitely need to test it on the pi3 (polling-based) and the > > > > pi4 (irq-based) at least. > > > > > > > > Dave, Maira, could you give it a try? > > > > > > I'm on it. > > > > Basic checks look OK. > > Pi3 and Pi4 tested, including 4k60 on Pi4 (enables scrambling). Audio > > works, and all the EDID parsing seems to be correct. > > > > Ideally I would like to do a couple more tests though as I had a > > couple of spontaneous reboots on Pi4 whilst hotplugging. > > Most likely it was just grounding issues, as I can't think of an > > obvious way for the kernel to be triggering an automatic reboot. They > > seemed to go away when I had enabled drm.debug=0x14, but more likely > > I'd connected the serial port and therefore provided another ground. > > Please don't hold the patches waiting on me though - I don't know when > > I'll have time to do them. > > Tested-by? I'd held off hoping to get the extra testing done today, but that hasn't happened, and today is my last day before the Christmas break. So based on what I've done so far: Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > > Also unrelated to this patch set, I have noted I'm getting > > [ 60.780897] WARNING: CPU: 3 PID: 501 at > > drivers/gpu/drm/vc4/vc4_hvs.c:743 __vc4_hvs_stop_channel+0x148/0x164 > > [vc4] > > ... > > [ 60.781192] __vc4_hvs_stop_channel+0x148/0x164 [vc4] (P) > > [ 60.781212] __vc4_hvs_stop_channel+0x40/0x164 [vc4] (L) > > [ 60.781230] vc4_hvs_stop_channel+0x30/0x3c [vc4] > > [ 60.781248] vc4_crtc_disable+0x140/0x1e8 [vc4] > > [ 60.781266] vc4_crtc_atomic_disable+0x98/0xb8 [vc4] > > [ 60.781284] disable_outputs+0x22c/0x33c [drm_kms_helper] > > [ 60.781326] drm_atomic_helper_commit_modeset_disables+0x1c/0x4c > > [drm_kms_helper] > > [ 60.781347] vc4_atomic_commit_tail+0x10c/0x8e4 [vc4] > > [ 60.781364] commit_tail+0xa0/0x188 [drm_kms_helper] > > [ 60.781385] drm_atomic_helper_commit+0x16c/0x180 [drm_kms_helper] > > [ 60.781406] drm_atomic_commit+0x88/0xc4 [drm] > > [ 60.781500] drm_client_modeset_commit_atomic+0x204/0x264 [drm] > > [ 60.781553] drm_client_modeset_commit_locked+0x5c/0x198 [drm] > > [ 60.781603] drm_client_modeset_commit+0x30/0x58 [drm] > > [ 60.781652] __drm_fb_helper_restore_fbdev_mode_unlocked+0xb4/0xe8 > > [drm_kms_helper] > > [ 60.781674] drm_fb_helper_hotplug_event+0x108/0x12c [drm_kms_helper] > > [ 60.781695] drm_fbdev_client_hotplug+0x28/0xd4 [drm_client_lib] > > [ 60.781700] drm_client_dev_hotplug+0xc8/0x12c [drm] > > [ 60.781750] drm_connector_helper_hpd_irq_event+0x70/0xb0 [drm_kms_helper] > > [ 60.781771] vc4_hdmi_hpd_irq_thread+0x2c/0x3c [vc4] > > [ 60.781790] irq_thread_fn+0x2c/0xa8 > > [ 60.781799] irq_thread+0x16c/0x2f4 > > [ 60.781805] kthread+0x118/0x11c > > [ 60.781813] ret_from_fork+0x10/0x20 > > On first plugging or unplugging a second monitor. I suspect that is > > spurious though and I'll look into it, but probably not this side of > > the Christmas break. > > > > Dave > > -- > With best wishes > Dmitry
On Fri, Dec 20, 2024 at 03:00:47PM +0000, Dave Stevenson wrote: > On Fri, 20 Dec 2024 at 00:45, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Thu, Dec 19, 2024 at 04:16:37PM +0000, Dave Stevenson wrote: > > > On Wed, 18 Dec 2024 at 14:52, Dave Stevenson > > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > > > Hi Maxime & Dmitry > > > > > > > > On Wed, 18 Dec 2024 at 07:59, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > > > On Wed, Dec 18, 2024 at 07:24:23AM +0200, Dmitry Baryshkov wrote: > > > > > > On Tue, 17 Dec 2024 at 19:21, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > On Tue, Dec 17, 2024 at 02:40:22AM +0200, Dmitry Baryshkov wrote: > > > > > > > > While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector > > > > > > > > framework, I stumbled upon an issue while handling the Audio InfoFrames. > > > > > > > > The HDMI codec callbacks weren't receiving the drm_atomic_state, so > > > > > > > > there was no simple way to get the drm_connector that stayed at the end > > > > > > > > of the bridge chain. At the same point the drm_hdmi_connector functions > > > > > > > > expected to get drm_connector instance. > > > > > > > > > > > > > > > > While looking for a way to solve the issue, I stumbled upon several > > > > > > > > deficiencies in existing hdmi_codec_ops implementations. Only few of the > > > > > > > > implementations were able to handle codec's 'plugged' callback. One > > > > > > > > third of the drivers didn't implement the get_eld() callback. > > > > > > > > > > > > > > > > Most of the issues can be solved if drm_connector handles > > > > > > > > hdmi-audio-codec on its own, delegating functionality to the actual > > > > > > > > implementation, be it a driver that implements drm_connector or > > > > > > > > drm_bridge. > > > > > > > > > > > > > > > > Implement such high-level framework, adding proper support for Audio > > > > > > > > InfoFrame generation to the LT9611 driver. > > > > > > > > > > > > > > > > Several design decisions to be kept in mind: > > > > > > > > > > > > > > > > - drm_connector_hdmi_codec is kept as simple as possible. It implements > > > > > > > > generic functionality (ELD, hotplug, registration). > > > > > > > > > > > > > > > > - drm_hdmi_connector sets up HDMI codec device if the connector > > > > > > > > is setup correspondingly (either I2S or S/PDIF is marked as > > > > > > > > supported). > > > > > > > > > > > > > > > > - drm_bridge_connector provides a way to link HDMI audio codec > > > > > > > > funcionality in the drm_bridge with the drm_connector_hdmi_codec > > > > > > > > framework. > > > > > > > > > > > > > > > > - It might be worth reverting the no_i2s_capture / no_spdif_capture > > > > > > > > bits. Only TDA889x driver sets them, while it's safe to assume that > > > > > > > > most of HDMI / DP devices do not support ARC / capture. I think the > > > > > > > > drivers should opt-in capture support rather than having to opt-out of > > > > > > > > it. > > > > > > > > > > > > > > Sorry if this isn't clear to me and I'm quite late to the party, but did > > > > > > > you test this on vc4 with both a pi3 and pi4, or was it just compile > > > > > > > tested? > > > > > > > > > > > > LT9611 is actually tested, VC4 is only compile-tested. Should I put an RFT tag? > > > > > > > > > > Yeah, we definitely need to test it on the pi3 (polling-based) and the > > > > > pi4 (irq-based) at least. > > > > > > > > > > Dave, Maira, could you give it a try? > > > > > > > > I'm on it. > > > > > > Basic checks look OK. > > > Pi3 and Pi4 tested, including 4k60 on Pi4 (enables scrambling). Audio > > > works, and all the EDID parsing seems to be correct. > > > > > > Ideally I would like to do a couple more tests though as I had a > > > couple of spontaneous reboots on Pi4 whilst hotplugging. > > > Most likely it was just grounding issues, as I can't think of an > > > obvious way for the kernel to be triggering an automatic reboot. They > > > seemed to go away when I had enabled drm.debug=0x14, but more likely > > > I'd connected the serial port and therefore provided another ground. > > > Please don't hold the patches waiting on me though - I don't know when > > > I'll have time to do them. > > > > Tested-by? > > I'd held off hoping to get the extra testing done today, but that > hasn't happened, and today is my last day before the Christmas break. > So based on what I've done so far: > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Thank you! > > > > > > > > > > Also unrelated to this patch set, I have noted I'm getting > > > [ 60.780897] WARNING: CPU: 3 PID: 501 at > > > drivers/gpu/drm/vc4/vc4_hvs.c:743 __vc4_hvs_stop_channel+0x148/0x164 > > > [vc4] > > > ... > > > [ 60.781192] __vc4_hvs_stop_channel+0x148/0x164 [vc4] (P) > > > [ 60.781212] __vc4_hvs_stop_channel+0x40/0x164 [vc4] (L) > > > [ 60.781230] vc4_hvs_stop_channel+0x30/0x3c [vc4] > > > [ 60.781248] vc4_crtc_disable+0x140/0x1e8 [vc4] > > > [ 60.781266] vc4_crtc_atomic_disable+0x98/0xb8 [vc4] > > > [ 60.781284] disable_outputs+0x22c/0x33c [drm_kms_helper] > > > [ 60.781326] drm_atomic_helper_commit_modeset_disables+0x1c/0x4c > > > [drm_kms_helper] > > > [ 60.781347] vc4_atomic_commit_tail+0x10c/0x8e4 [vc4] > > > [ 60.781364] commit_tail+0xa0/0x188 [drm_kms_helper] > > > [ 60.781385] drm_atomic_helper_commit+0x16c/0x180 [drm_kms_helper] > > > [ 60.781406] drm_atomic_commit+0x88/0xc4 [drm] > > > [ 60.781500] drm_client_modeset_commit_atomic+0x204/0x264 [drm] > > > [ 60.781553] drm_client_modeset_commit_locked+0x5c/0x198 [drm] > > > [ 60.781603] drm_client_modeset_commit+0x30/0x58 [drm] > > > [ 60.781652] __drm_fb_helper_restore_fbdev_mode_unlocked+0xb4/0xe8 > > > [drm_kms_helper] > > > [ 60.781674] drm_fb_helper_hotplug_event+0x108/0x12c [drm_kms_helper] > > > [ 60.781695] drm_fbdev_client_hotplug+0x28/0xd4 [drm_client_lib] > > > [ 60.781700] drm_client_dev_hotplug+0xc8/0x12c [drm] > > > [ 60.781750] drm_connector_helper_hpd_irq_event+0x70/0xb0 [drm_kms_helper] > > > [ 60.781771] vc4_hdmi_hpd_irq_thread+0x2c/0x3c [vc4] > > > [ 60.781790] irq_thread_fn+0x2c/0xa8 > > > [ 60.781799] irq_thread+0x16c/0x2f4 > > > [ 60.781805] kthread+0x118/0x11c > > > [ 60.781813] ret_from_fork+0x10/0x20 > > > On first plugging or unplugging a second monitor. I suspect that is > > > spurious though and I'll look into it, but probably not this side of > > > the Christmas break. > > > > > > Dave > > > > -- > > With best wishes > > Dmitry
While porting lt9611 DSI-to-HDMI bridge driver to use HDMI Connector framework, I stumbled upon an issue while handling the Audio InfoFrames. The HDMI codec callbacks weren't receiving the drm_atomic_state, so there was no simple way to get the drm_connector that stayed at the end of the bridge chain. At the same point the drm_hdmi_connector functions expected to get drm_connector instance. While looking for a way to solve the issue, I stumbled upon several deficiencies in existing hdmi_codec_ops implementations. Only few of the implementations were able to handle codec's 'plugged' callback. One third of the drivers didn't implement the get_eld() callback. Most of the issues can be solved if drm_connector handles hdmi-audio-codec on its own, delegating functionality to the actual implementation, be it a driver that implements drm_connector or drm_bridge. Implement such high-level framework, adding proper support for Audio InfoFrame generation to the LT9611 driver. Several design decisions to be kept in mind: - drm_connector_hdmi_codec is kept as simple as possible. It implements generic functionality (ELD, hotplug, registration). - drm_hdmi_connector sets up HDMI codec device if the connector is setup correspondingly (either I2S or S/PDIF is marked as supported). - drm_bridge_connector provides a way to link HDMI audio codec funcionality in the drm_bridge with the drm_connector_hdmi_codec framework. - It might be worth reverting the no_i2s_capture / no_spdif_capture bits. Only TDA889x driver sets them, while it's safe to assume that most of HDMI / DP devices do not support ARC / capture. I think the drivers should opt-in capture support rather than having to opt-out of it. This series depends on the ELD mutex series [1] [1] https://lore.kernel.org/r/20241201-drm-connector-eld-mutex-v1-0-ba56a6545c03@linaro.org Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Changes in v7: - Renamed drm_connector_hdmi_codec_init() to drm_connector_hdmi_audio_init() (Maxime) - Added extra empty line in struct drm_connector_hdmi_codec_funcs (Maxime) - Dropped if/else from drm_bridge_connector_audio_startup() (Maxime) - Added optional .read_edid() callback and reworked drm_atomic_helper_connector_hdmi_hotplug() to use that callback instead of having an internal function which accepts EDID (Maxime) - Made VC4 and drm_bridge_connector use .force() in addition to .detect() and .detect_ctx(). - Moved HDMI codec functions out of struct drm_connector_hdmi_funcs. Assign them from drm_connector_hdmi_audio_init(). - Documented struct drm_connector_hdmi_codec and its fields. - Link to v6: https://lore.kernel.org/r/20241206-drm-bridge-hdmi-connector-v6-0-50dc145a9c06@linaro.org Changes in v6: - Dropped extra checks on the EDID (Jani) - Reworked drmm_connector_hdmi_init(), splitting the codec init to a separate optional function rather than passing arguments through drm_connector (Maxime) - Reworked EDID update functions (Maxime, Jani) - No longer refresh the EDID in vc4_hdmi_connector_get_modes(), it is redundant as vc4_hdmi_connector_detect_cxtx() already does that. - Link to v5: https://lore.kernel.org/r/20241201-drm-bridge-hdmi-connector-v5-0-b5316e82f61a@linaro.org Changes in v5: - Moved prototypes from drm_internal.h to drm_connector_hdmi_codec_internal.h (Jani) - Rebased on top of ELD mutex series, resolving the long-standing FIXME - Converted the VC4 driver (compile-tested only) - Link to v4: https://lore.kernel.org/r/20241122-drm-bridge-hdmi-connector-v4-0-b4d69d6e3bd9@linaro.org Changes in v4: - Added forward declaration of struct drm_edid (LKP) - Fixed kerneldoc for drm_atomic_helper_connector_hdmi_update_edid(). - Link to v3: https://lore.kernel.org/r/20241109-drm-bridge-hdmi-connector-v3-0-c15afdca5884@linaro.org Changes in v3: - Dropped RFC status - Fixed drm_connector_hdmi_codec_init() kerneldoc (LKP) - Dropped double underscore prefix from __drm_atomic_helper_connector_hdmi_update_edid() (Jani) - Moved drm_edid_free() from drm_atomic_helper_connector_hdmi_update_edid() to the caller's side (Jani) - Link to v2: https://lore.kernel.org/r/20241101-drm-bridge-hdmi-connector-v2-0-739ef9addf9e@linaro.org Changes in v2: - Use drm_atomic_get_old_connector_for_encoder in atomic_disable() to prevent it from crashing - Reworked HDMI codec init/exit, removing drmm_ calls (Maxime) - Drafted the helper to be called from .detect_ctx() that performs HDMI Connector maintenance duties (Maxime) - Moved no_capture_mute to struct hdmi_codec_pdata - Link to v1: https://lore.kernel.org/r/20240615-drm-bridge-hdmi-connector-v1-0-d59fc7865ab2@linaro.org --- Dmitry Baryshkov (10): ASoC: hdmi-codec: pass data to get_dai_id too ASoC: hdmi-codec: move no_capture_mute to struct hdmi_codec_pdata drm/connector: implement generic HDMI codec helpers drm/bridge: connector: add support for HDMI codec framework drm/bridge: lt9611: switch to using the DRM HDMI codec framework drm/display/hdmi: implement hotplug functions drm/bridge_connector: hook drm_atomic_helper_connector_hdmi_hotplug() drm/vc4: hdmi: switch to using generic HDMI Codec infrastructure drm/vc4: hdmi: stop rereading EDID in get_modes() drm/vc4: hdmi: use drm_atomic_helper_connector_hdmi_hotplug_edid() drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 3 +- drivers/gpu/drm/bridge/analogix/anx7625.c | 3 +- drivers/gpu/drm/bridge/ite-it66121.c | 2 +- drivers/gpu/drm/bridge/lontium-lt9611.c | 169 ++++++++---------- drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 3 +- drivers/gpu/drm/bridge/sii902x.c | 5 +- .../gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 3 +- drivers/gpu/drm/display/drm_bridge_connector.c | 137 ++++++++++++++- drivers/gpu/drm/display/drm_hdmi_state_helper.c | 56 ++++++ drivers/gpu/drm/drm_connector.c | 5 + drivers/gpu/drm/drm_connector_hdmi_codec.c | 189 +++++++++++++++++++++ drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/mediatek/mtk_dp.c | 2 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +- drivers/gpu/drm/rockchip/cdn-dp-core.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/vc4/vc4_hdmi.c | 99 +++-------- drivers/gpu/drm/vc4/vc4_hdmi.h | 2 - include/drm/display/drm_hdmi_state_helper.h | 5 + include/drm/drm_bridge.h | 38 +++++ include/drm/drm_connector.h | 141 +++++++++++++++ include/sound/hdmi-codec.h | 7 +- sound/soc/codecs/hdmi-codec.c | 4 +- 25 files changed, 678 insertions(+), 206 deletions(-) --- base-commit: 81a9a93b169a273ccc4a9a1ee56f17e9981d3f98 change-id: 20240530-drm-bridge-hdmi-connector-9b0f6725973e Best regards,