| Message ID | 20250225183058.607047-7-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add Synopsys DesignWare HDMI RX Controller | expand |
On 25/02/2025 19:30, Dmitry Osipenko wrote: > From: Sebastian Reichel <sebastian.reichel@collabora.com> > > The Rockchip RK3588 has a built-in HDMI receiver block from > Synopsys. Let's enable the driver for it. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > arch/arm64/configs/defconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index cb7da4415599..3dccc9e1c4aa 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -859,6 +859,8 @@ CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m > CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m > CONFIG_VIDEO_SAMSUNG_S5P_MFC=m > CONFIG_VIDEO_SUN6I_CSI=m > +CONFIG_VIDEO_SYNOPSYS_HDMIRX=m > +CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID=y I do not believe it is a good idea to default to y for this option. The EDID depends on the specific device you make, and you should think carefully about whether the default EDID fits the needs of the device. So if you want the default EDID, then you should manually select it and not have it autoselected. Regards, Hans > CONFIG_VIDEO_TI_J721E_CSI2RX=m > CONFIG_VIDEO_HANTRO=m > CONFIG_VIDEO_IMX219=m
On 2/26/25 11:31, Hans Verkuil wrote: > On 25/02/2025 19:30, Dmitry Osipenko wrote: >> From: Sebastian Reichel <sebastian.reichel@collabora.com> >> >> The Rockchip RK3588 has a built-in HDMI receiver block from >> Synopsys. Let's enable the driver for it. >> >> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> arch/arm64/configs/defconfig | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >> index cb7da4415599..3dccc9e1c4aa 100644 >> --- a/arch/arm64/configs/defconfig >> +++ b/arch/arm64/configs/defconfig >> @@ -859,6 +859,8 @@ CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m >> CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m >> CONFIG_VIDEO_SAMSUNG_S5P_MFC=m >> CONFIG_VIDEO_SUN6I_CSI=m >> +CONFIG_VIDEO_SYNOPSYS_HDMIRX=m >> +CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID=y > > I do not believe it is a good idea to default to y for this option. > > The EDID depends on the specific device you make, and you should > think carefully about whether the default EDID fits the needs of the > device. > > So if you want the default EDID, then you should manually select it > and not have it autoselected. Ack
Hi Hans, Le mercredi 26 février 2025 à 09:31 +0100, Hans Verkuil a écrit : > On 25/02/2025 19:30, Dmitry Osipenko wrote: > > From: Sebastian Reichel <sebastian.reichel@collabora.com> > > > > The Rockchip RK3588 has a built-in HDMI receiver block from > > Synopsys. Let's enable the driver for it. > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > --- > > arch/arm64/configs/defconfig | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/arm64/configs/defconfig > > b/arch/arm64/configs/defconfig > > index cb7da4415599..3dccc9e1c4aa 100644 > > --- a/arch/arm64/configs/defconfig > > +++ b/arch/arm64/configs/defconfig > > @@ -859,6 +859,8 @@ CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m > > CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m > > CONFIG_VIDEO_SAMSUNG_S5P_MFC=m > > CONFIG_VIDEO_SUN6I_CSI=m > > +CONFIG_VIDEO_SYNOPSYS_HDMIRX=m > > +CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID=y > > I do not believe it is a good idea to default to y for this option. > > The EDID depends on the specific device you make, and you should > think carefully about whether the default EDID fits the needs of the > device. > > So if you want the default EDID, then you should manually select it > and not have it autoselected. Following up here, from the device maker perspective sure, but I'm not sure this is the best choice for generic Linux distribution. As of today, pretty much no userspace capture software knows about this, meaning the device will not work out of the box in OBS, GStreamer, Ffmpeg, Web Browsers. In comparison, if you pick any UVC HDMI capture, it just work, with a default EDID that covers the range of capabilities, which in this case are defined by the SoC. Whatever we make as a choice, will always be the wrong choice for someone, though I expect this to be a lot easier to change for "specialized" implementation. Now, since there is no reason you can't do a specilized implementation on generic Linux distro, and because you might disagree with me, perhaps a compromise is to also add a module parameter, so that there is a run-time override to this. Nicolas > > Regards, > > Hans > > > CONFIG_VIDEO_TI_J721E_CSI2RX=m > > CONFIG_VIDEO_HANTRO=m > > CONFIG_VIDEO_IMX219=m > >
On 28/02/2025 04:51, Nicolas Dufresne wrote: > Hi Hans, > > Le mercredi 26 février 2025 à 09:31 +0100, Hans Verkuil a écrit : >> On 25/02/2025 19:30, Dmitry Osipenko wrote: >>> From: Sebastian Reichel <sebastian.reichel@collabora.com> >>> >>> The Rockchip RK3588 has a built-in HDMI receiver block from >>> Synopsys. Let's enable the driver for it. >>> >>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>> --- >>> arch/arm64/configs/defconfig | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/arch/arm64/configs/defconfig >>> b/arch/arm64/configs/defconfig >>> index cb7da4415599..3dccc9e1c4aa 100644 >>> --- a/arch/arm64/configs/defconfig >>> +++ b/arch/arm64/configs/defconfig >>> @@ -859,6 +859,8 @@ CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m >>> CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m >>> CONFIG_VIDEO_SAMSUNG_S5P_MFC=m >>> CONFIG_VIDEO_SUN6I_CSI=m >>> +CONFIG_VIDEO_SYNOPSYS_HDMIRX=m >>> +CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID=y >> >> I do not believe it is a good idea to default to y for this option. >> >> The EDID depends on the specific device you make, and you should >> think carefully about whether the default EDID fits the needs of the >> device. >> >> So if you want the default EDID, then you should manually select it >> and not have it autoselected. > > Following up here, from the device maker perspective sure, but I'm not > sure this is the best choice for generic Linux distribution. As of > today, pretty much no userspace capture software knows about this, > meaning the device will not work out of the box in OBS, GStreamer, > Ffmpeg, Web Browsers. In comparison, if you pick any UVC HDMI capture, > it just work, with a default EDID that covers the range of > capabilities, which in this case are defined by the SoC. A UVC HDMI capture device is not a good comparison: that has it's own EDID that is configured for the specific hardware and USB bandwidth limitations. EDID handling is all internal to that device, nothing to do with the UVC driver. That said, this device is a bit different compared to most other HDMI receivers in that it also has a DMA engine. Usually HDMI receivers are i2c devices that connect to an SoC. In this case the hardware is inside the SoC. So I am OK with making LOAD_DEFAULT_EDID=y in the defconfig. So: Acked-by: Hans Verkuil <hverkuil@xs4all.nl> Note that I will merge the v13 driver (patches 1-3), since it looks good. But v11-v13 of this defconfig patch dropped the "CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID=y" line, so you might want to post a v14 of just patches 4-6, restoring that line, once I merged patches 1-3. Regards, Hans > > Whatever we make as a choice, will always be the wrong choice for > someone, though I expect this to be a lot easier to change for > "specialized" implementation. Now, since there is no reason you can't > do a specilized implementation on generic Linux distro, and because you > might disagree with me, perhaps a compromise is to also add a module > parameter, so that there is a run-time override to this. > > Nicolas > >> >> Regards, >> >> Hans >> >>> CONFIG_VIDEO_TI_J721E_CSI2RX=m >>> CONFIG_VIDEO_HANTRO=m >>> CONFIG_VIDEO_IMX219=m >> >> >
On 3/4/25 19:17, Hans Verkuil wrote: > On 28/02/2025 04:51, Nicolas Dufresne wrote: >> Hi Hans, >> >> Le mercredi 26 février 2025 à 09:31 +0100, Hans Verkuil a écrit : >>> On 25/02/2025 19:30, Dmitry Osipenko wrote: >>>> From: Sebastian Reichel <sebastian.reichel@collabora.com> >>>> >>>> The Rockchip RK3588 has a built-in HDMI receiver block from >>>> Synopsys. Let's enable the driver for it. >>>> >>>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> --- >>>> arch/arm64/configs/defconfig | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/arch/arm64/configs/defconfig >>>> b/arch/arm64/configs/defconfig >>>> index cb7da4415599..3dccc9e1c4aa 100644 >>>> --- a/arch/arm64/configs/defconfig >>>> +++ b/arch/arm64/configs/defconfig >>>> @@ -859,6 +859,8 @@ CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m >>>> CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m >>>> CONFIG_VIDEO_SAMSUNG_S5P_MFC=m >>>> CONFIG_VIDEO_SUN6I_CSI=m >>>> +CONFIG_VIDEO_SYNOPSYS_HDMIRX=m >>>> +CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID=y >>> >>> I do not believe it is a good idea to default to y for this option. >>> >>> The EDID depends on the specific device you make, and you should >>> think carefully about whether the default EDID fits the needs of the >>> device. >>> >>> So if you want the default EDID, then you should manually select it >>> and not have it autoselected. >> >> Following up here, from the device maker perspective sure, but I'm not >> sure this is the best choice for generic Linux distribution. As of >> today, pretty much no userspace capture software knows about this, >> meaning the device will not work out of the box in OBS, GStreamer, >> Ffmpeg, Web Browsers. In comparison, if you pick any UVC HDMI capture, >> it just work, with a default EDID that covers the range of >> capabilities, which in this case are defined by the SoC. > > A UVC HDMI capture device is not a good comparison: that has it's own > EDID that is configured for the specific hardware and USB bandwidth > limitations. EDID handling is all internal to that device, nothing to > do with the UVC driver. > > That said, this device is a bit different compared to most other > HDMI receivers in that it also has a DMA engine. Usually HDMI receivers > are i2c devices that connect to an SoC. In this case the hardware is > inside the SoC. So I am OK with making LOAD_DEFAULT_EDID=y in the defconfig. > > So: > > Acked-by: Hans Verkuil <hverkuil@xs4all.nl> > > Note that I will merge the v13 driver (patches 1-3), since it looks good. > > But v11-v13 of this defconfig patch dropped the "CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID=y" > line, so you might want to post a v14 of just patches 4-6, restoring that > line, once I merged patches 1-3. Will post v14, thanks!
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index cb7da4415599..3dccc9e1c4aa 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -859,6 +859,8 @@ CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m CONFIG_VIDEO_SAMSUNG_S5P_MFC=m CONFIG_VIDEO_SUN6I_CSI=m +CONFIG_VIDEO_SYNOPSYS_HDMIRX=m +CONFIG_VIDEO_SYNOPSYS_HDMIRX_LOAD_DEFAULT_EDID=y CONFIG_VIDEO_TI_J721E_CSI2RX=m CONFIG_VIDEO_HANTRO=m CONFIG_VIDEO_IMX219=m