| Message ID | 20240327225057.672304-1-shreeya.patel@collabora.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Synopsys DesignWare HDMI RX Controller | expand |
On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: > This series implements support for the Synopsys DesignWare > HDMI RX Controller, being compliant with standard HDMI 1.4b > and HDMI 2.0. > Hi Mauro and Hans, I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. Thanks, Shreeya Patel > Features that are currently supported by the HDMI RX driver > have been tested on rock5b board using a HDMI to micro-HDMI cable. > It is recommended to use a good quality cable as there were > multiple issues seen during testing the driver. > > Please note the below information :- > * While testing the driver on rock5b we noticed that the binary BL31 > from Rockchip contains some unknown code to get the HDMI-RX PHY > access working. With TF-A BL31, the HDMI-RX PHY doesn't work as > expected since there are no interrupts seen for rk_hdmirx-hdmi > leading to some failures in the driver [0]. > * We have tested the working of OBS studio with HDMIRX driver and > there were no issues seen. > * We also tested and verified the support for interlaced video. > > [0] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/trusted-firmware-a/-/issues/1 > > To test the HDMI RX Controller driver, following example commands can be used :- > > root@debian-rockchip-rock5b-rk3588:~# v4l2-ctl --verbose -d /dev/video0 \ > --set-fmt-video=width=1920,height=1080,pixelformat='BGR3' --stream-mmap=4 \ > --stream-skip=3 --stream-count=100 --stream-to=/home/hdmiin4k.raw --stream-poll > > root@debian-rockchip-rock5b-rk3588:~# ffmpeg -f rawvideo -vcodec rawvideo \ > -s 1920x1080 -r 60 -pix_fmt bgr24 -i /home/hdmiin4k.raw output.mkv > > > Following is the v4l2-compliance test result :- > > root@debian-rockchip-rock5b-rk3588:~# v4l2-compliance -d /dev/video0 > v4l2-compliance 1.27.0-5174, 64 bits, 64-bit time_t > v4l2-compliance SHA: d700deb14368 2024-01-18 12:19:05 > > Compliance test for snps_hdmirx device /dev/video0: > > Driver Info: > Driver name : snps_hdmirx > Card type : snps_hdmirx > Bus info : platform: snps_hdmirx > Driver version : 6.8.0 > Capabilities : 0x84201000 > Video Capture Multiplanar > Streaming > Extended Pix Format > Device Capabilities > Device Caps : 0x04201000 > Video Capture Multiplanar > Streaming > Extended Pix Format > > Required ioctls: > test VIDIOC_QUERYCAP: OK > test invalid ioctls: OK > > Allow for multiple opens: > test second /dev/video0 open: OK > test VIDIOC_QUERYCAP: OK > test VIDIOC_G/S_PRIORITY: OK > test for unlimited opens: OK > > Debug ioctls: > test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) > test VIDIOC_LOG_STATUS: OK > > Input ioctls: > test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) > test VIDIOC_ENUMAUDIO: OK (Not Supported) > test VIDIOC_G/S/ENUMINPUT: OK > test VIDIOC_G/S_AUDIO: OK (Not Supported) > Inputs: 1 Audio Inputs: 0 Tuners: 0 > > Output ioctls: > test VIDIOC_G/S_MODULATOR: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_ENUMAUDOUT: OK (Not Supported) > test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) > test VIDIOC_G/S_AUDOUT: OK (Not Supported) > Outputs: 0 Audio Outputs: 0 Modulators: 0 > > Input/Output configuration ioctls: > test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) > test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK > test VIDIOC_DV_TIMINGS_CAP: OK > test VIDIOC_G/S_EDID: OK > > Control ioctls (Input 0): > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK > test VIDIOC_QUERYCTRL: OK > test VIDIOC_G/S_CTRL: OK > test VIDIOC_G/S/TRY_EXT_CTRLS: OK > test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK > test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) > Standard Controls: 2 Private Controls: 0 > > Format ioctls (Input 0): > test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK > test VIDIOC_G/S_PARM: OK > test VIDIOC_G_FBUF: OK (Not Supported) > test VIDIOC_G_FMT: OK > test VIDIOC_TRY_FMT: OK > test VIDIOC_S_FMT: OK > test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) > test Cropping: OK (Not Supported) > test Composing: OK (Not Supported) > test Scaling: OK (Not Supported) > > Codec ioctls (Input 0): > test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) > test VIDIOC_G_ENC_INDEX: OK (Not Supported) > test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) > > Buffer ioctls (Input 0): > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK > test CREATE_BUFS maximum buffers: OK > test VIDIOC_EXPBUF: OK > test Requests: OK (Not Supported) > > Total for snps_hdmirx device /dev/video0: 46, Succeeded: 46, Failed: 0, Warnings: 0 > > Changes in v3 :- > - Use v4l2-common helpers in the HDMIRX driver > - Rename cma node and phandle names > - Elaborate the comment to explain 160MiB calculation > - Move &hdmi_receiver_cma to the rock5b dts file > - Add information about interlaced video testing in the > cover-letter > > Changes in v2 :- > - Fix checkpatch --strict warnings > - Move the dt-binding include file changes in a separate patch > - Add a description for the hardware in the dt-bindings file > - Rename resets, vo1 grf and HPD properties > - Add a proper description for grf and vo1-grf phandles in the > bindings > - Rename the HDMI RX node name to hdmi-receiver > - Include gpio header file in binding example to fix the > dt_binding_check failure > - Move hdmirx_cma node to the rk3588.dtsi file > - Add an entry to MAINTAINERS file for the HDMIRX driver > > Shreeya Patel (6): > dt-bindings: reset: Define reset id used for HDMI Receiver > clk: rockchip: rst-rk3588: Add reset line for HDMI Receiver > dt-bindings: media: Document HDMI RX Controller > arm64: dts: rockchip: Add device tree support for HDMI RX Controller > media: platform: synopsys: Add support for hdmi input driver > MAINTAINERS: Add entry for Synopsys DesignWare HDMI RX Driver > > .../bindings/media/snps,dw-hdmi-rx.yaml | 132 + > MAINTAINERS | 8 + > .../boot/dts/rockchip/rk3588-pinctrl.dtsi | 41 + > .../boot/dts/rockchip/rk3588-rock-5b.dts | 19 + > arch/arm64/boot/dts/rockchip/rk3588.dtsi | 56 + > drivers/clk/rockchip/rst-rk3588.c | 1 + > drivers/media/platform/Kconfig | 1 + > drivers/media/platform/Makefile | 1 + > drivers/media/platform/synopsys/Kconfig | 3 + > drivers/media/platform/synopsys/Makefile | 2 + > .../media/platform/synopsys/hdmirx/Kconfig | 18 + > .../media/platform/synopsys/hdmirx/Makefile | 4 + > .../platform/synopsys/hdmirx/snps_hdmirx.c | 2726 +++++++++++++++++ > .../platform/synopsys/hdmirx/snps_hdmirx.h | 394 +++ > .../synopsys/hdmirx/snps_hdmirx_cec.c | 289 ++ > .../synopsys/hdmirx/snps_hdmirx_cec.h | 46 + > .../dt-bindings/reset/rockchip,rk3588-cru.h | 2 + > 17 files changed, 3743 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml > create mode 100644 drivers/media/platform/synopsys/Kconfig > create mode 100644 drivers/media/platform/synopsys/Makefile > create mode 100644 drivers/media/platform/synopsys/hdmirx/Kconfig > create mode 100644 drivers/media/platform/synopsys/hdmirx/Makefile > create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c > create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h > create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.c > create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.h > > -- > 2.39.2 > > _______________________________________________ > Kernel mailing list -- kernel@mailman.collabora.com > To unsubscribe send an email to kernel-leave@mailman.collabora.com > This list is managed by https://mailman.collabora.com
On 03/04/2024 11:24, Shreeya Patel wrote: > On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: > >> This series implements support for the Synopsys DesignWare >> HDMI RX Controller, being compliant with standard HDMI 1.4b >> and HDMI 2.0. >> > > Hi Mauro and Hans, > > I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. Why did you put clk changes here? These go via different subsystem. That might be one of obstacles for your patchset. Also, you sent it just a week ago and you already ping. Please relax, and help out by reviewing other patches on the mailing lists in order to relieve the burden of maintainers and move your patches higher up the list. Best regards, Krzysztof
On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 03/04/2024 11:24, Shreeya Patel wrote: > > On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: > > > >> This series implements support for the Synopsys DesignWare > >> HDMI RX Controller, being compliant with standard HDMI 1.4b > >> and HDMI 2.0. > >> > > > > Hi Mauro and Hans, > > > > I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. > > Why did you put clk changes here? These go via different subsystem. That > might be one of obstacles for your patchset. > I added clock changes in this patch series because HDMIRX driver depends on it. I thought it is wrong to send the driver patches which don't even compile? Since you are a more experienced developer, can you help me understand what would be the right way to send patches in such scenarios? Thanks, Shreeya Patel > Also, you sent it just a week ago and you already ping. Please relax, > and help out by reviewing other patches on the mailing lists in order to > relieve the burden of maintainers and move your patches higher up the list. > > > Best regards, > Krzysztof >
On 03/04/2024 13:20, Shreeya Patel wrote: > On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 03/04/2024 11:24, Shreeya Patel wrote: >>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: >>> >>>> This series implements support for the Synopsys DesignWare >>>> HDMI RX Controller, being compliant with standard HDMI 1.4b >>>> and HDMI 2.0. >>>> >>> >>> Hi Mauro and Hans, >>> >>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. >> >> Why did you put clk changes here? These go via different subsystem. That >> might be one of obstacles for your patchset. >> > > I added clock changes in this patch series because HDMIRX driver depends on it. > I thought it is wrong to send the driver patches which don't even compile? Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. Please get it reviewed internally first. > > Since you are a more experienced developer, can you help me understand what would > be the right way to send patches in such scenarios? I am not the substitute for your Collabora engineers and peers. You do not get free work from the community. First, do the work and review internally, to solve all trivial things, like how to submit patches upstream or how to make your driver buildable, and then ask community for the review. Best regards, Krzysztof
On Wed, Apr 03, 2024 at 01:24:05PM +0200, Krzysztof Kozlowski wrote: > On 03/04/2024 13:20, Shreeya Patel wrote: > > On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >> On 03/04/2024 11:24, Shreeya Patel wrote: > >>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: > >>> > >>>> This series implements support for the Synopsys DesignWare > >>>> HDMI RX Controller, being compliant with standard HDMI 1.4b > >>>> and HDMI 2.0. > >>>> > >>> > >>> Hi Mauro and Hans, > >>> > >>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. > >> > >> Why did you put clk changes here? These go via different subsystem. That > >> might be one of obstacles for your patchset. > >> > > > > I added clock changes in this patch series because HDMIRX driver depends on it. > > I thought it is wrong to send the driver patches which don't even compile? > > Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. > Please get it reviewed internally first. > > > > > Since you are a more experienced developer, can you help me understand what would > > be the right way to send patches in such scenarios? > > I am not the substitute for your Collabora engineers and peers. You do > not get free work from the community. First, do the work and review > internally, to solve all trivial things, like how to submit patches > upstream or how to make your driver buildable, and then ask community > for the review. I don't think Shreeya was asking for "free" work from the community. Her question wasn't trivial or obvious since reasonable people seem to sometimes disagree about where to send a patch especially if it's needed to make a series compile. I heard the issue was already resolved but had to say something since this accusation seemed so unfair. > > Best regards, > Krzysztof > >
Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski: > On 03/04/2024 13:20, Shreeya Patel wrote: > > On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >> On 03/04/2024 11:24, Shreeya Patel wrote: > >>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: > >>> > >>>> This series implements support for the Synopsys DesignWare > >>>> HDMI RX Controller, being compliant with standard HDMI 1.4b > >>>> and HDMI 2.0. > >>>> > >>> > >>> Hi Mauro and Hans, > >>> > >>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. > >> > >> Why did you put clk changes here? These go via different subsystem. That > >> might be one of obstacles for your patchset. > >> > > > > I added clock changes in this patch series because HDMIRX driver depends on it. > > I thought it is wrong to send the driver patches which don't even compile? > > Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. > Please get it reviewed internally first. For the change in question, the clock controller on the soc also handles the reset controls (hence its name CRU, clock-and-reset-unit) . There are at least 660 reset lines in the unit and it seems the hdmi-rx one was overlooked on the initial submission, hence patches 1+2 add the reset-line. Of course, here only the "arm64: dts:" patch depends on the clock change, is it references the new reset-id. Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski: > Please do not engage multiple subsystems in one patchset, if not > necessary. Especially do not mix DTS into media or USB subsystems. And > do not put DTS in the middle! picking up your reply from patch 4/6, there seem to be different "schools of thought" for this. Some maintainers might want to really only see patches that are explicitly for their subsystem - I guess networking might be a prime example for that, who will essentially apply whole series' if nobody protests in time (including dts patches) On the other hand I also remember seeing requests for "the full picture" and individual maintainers then just picking and applying the patches meant for their subsystem. The series as it stands right now is nice in that it allows (random) developers to just pick it up, apply it to a tree and test the actual driver without needing to hunt for multiple dependant series. Of course you're right, the "arm64: dts:" patch should be the last in the series and not be in the middle of it. Regards Heiko
On 04/04/2024 00:48, Heiko Stübner wrote: > Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski: >> On 03/04/2024 13:20, Shreeya Patel wrote: >>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 03/04/2024 11:24, Shreeya Patel wrote: >>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: >>>>> >>>>>> This series implements support for the Synopsys DesignWare >>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b >>>>>> and HDMI 2.0. >>>>>> >>>>> >>>>> Hi Mauro and Hans, >>>>> >>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. >>>> >>>> Why did you put clk changes here? These go via different subsystem. That >>>> might be one of obstacles for your patchset. >>>> >>> >>> I added clock changes in this patch series because HDMIRX driver depends on it. >>> I thought it is wrong to send the driver patches which don't even compile? >> >> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. >> Please get it reviewed internally first. > > For the change in question, the clock controller on the soc also handles > the reset controls (hence its name CRU, clock-and-reset-unit) . > > There are at least 660 reset lines in the unit and it seems the hdmi-rx one > was overlooked on the initial submission, hence patches 1+2 add the > reset-line. > > Of course, here only the "arm64: dts:" patch depends on the clock > change, is it references the new reset-id. Wait, that's expected, but it is not what was written. Claim was HDMIRX driver depends *build time* ("don't even compile"). > > > Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski: >> Please do not engage multiple subsystems in one patchset, if not >> necessary. Especially do not mix DTS into media or USB subsystems. And >> do not put DTS in the middle! > > picking up your reply from patch 4/6, there seem to be different "schools > of thought" for this. Some maintainers might want to really only see > patches that are explicitly for their subsystem - I guess networking > might be a prime example for that, who will essentially apply whole series' > if nobody protests in time (including dts patches) There is no school saying DTS is allowed to be in the middle. Other schools are indeed saying that seeing DTS is good and recommendation is to post it separate and provide a link. That's way you avoid DTS being pulled by Greg, media or networking. > > On the other hand I also remember seeing requests for "the full picture" > and individual maintainers then just picking and applying the patches > meant for their subsystem. > > The series as it stands right now is nice in that it allows (random) > developers to just pick it up, apply it to a tree and test the actual driver > without needing to hunt for multiple dependant series. > > > Of course you're right, the "arm64: dts:" patch should be the last in the > series and not be in the middle of it. Best regards, Krzysztof
On 03/04/2024 23:13, Deborah Brouwer wrote: > On Wed, Apr 03, 2024 at 01:24:05PM +0200, Krzysztof Kozlowski wrote: >> On 03/04/2024 13:20, Shreeya Patel wrote: >>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 03/04/2024 11:24, Shreeya Patel wrote: >>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: >>>>> >>>>>> This series implements support for the Synopsys DesignWare >>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b >>>>>> and HDMI 2.0. >>>>>> >>>>> >>>>> Hi Mauro and Hans, >>>>> >>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. >>>> >>>> Why did you put clk changes here? These go via different subsystem. That >>>> might be one of obstacles for your patchset. >>>> >>> >>> I added clock changes in this patch series because HDMIRX driver depends on it. >>> I thought it is wrong to send the driver patches which don't even compile? >> >> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. >> Please get it reviewed internally first. >> >>> >>> Since you are a more experienced developer, can you help me understand what would >>> be the right way to send patches in such scenarios? >> >> I am not the substitute for your Collabora engineers and peers. You do >> not get free work from the community. First, do the work and review >> internally, to solve all trivial things, like how to submit patches >> upstream or how to make your driver buildable, and then ask community >> for the review. > > I don't think Shreeya was asking for "free" work from the community. > Her question wasn't trivial or obvious since reasonable people seem to sometimes > disagree about where to send a patch especially if it's needed to make a series compile. > I heard the issue was already resolved but had to say something since this accusation > seemed so unfair. If HDMI driver does not build because of clock driver, something is really wrong at the basics level. Therefore I am sure my statement was fair,. based on Shreeya statement of build failure. I am sorry, but independence of drivers and independence of DTS is a basic thing, so to solve such you can easily get help internally from your experienced folks (which you have). Best regards, Krzysztof
On Thursday, April 04, 2024 11:45 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 04/04/2024 00:48, Heiko Stübner wrote: > > Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski: > >> On 03/04/2024 13:20, Shreeya Patel wrote: > >>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>> > >>>> On 03/04/2024 11:24, Shreeya Patel wrote: > >>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: > >>>>> > >>>>>> This series implements support for the Synopsys DesignWare > >>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b > >>>>>> and HDMI 2.0. > >>>>>> > >>>>> > >>>>> Hi Mauro and Hans, > >>>>> > >>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. > >>>> > >>>> Why did you put clk changes here? These go via different subsystem. That > >>>> might be one of obstacles for your patchset. > >>>> > >>> > >>> I added clock changes in this patch series because HDMIRX driver depends on it. > >>> I thought it is wrong to send the driver patches which don't even compile? > >> > >> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. > >> Please get it reviewed internally first. > > > > For the change in question, the clock controller on the soc also handles > > the reset controls (hence its name CRU, clock-and-reset-unit) . > > > > There are at least 660 reset lines in the unit and it seems the hdmi-rx one > > was overlooked on the initial submission, hence patches 1+2 add the > > reset-line. > > > > Of course, here only the "arm64: dts:" patch depends on the clock > > change, is it references the new reset-id. > > Wait, that's expected, but it is not what was written. Claim was HDMIRX > driver depends *build time* ("don't even compile"). > For some context, when I was testing the downstream driver against the device tree, I saw some failures because of the missing reset ( which I am trying to add in the first two patches for this series ) ... hdmirx_dev->rst_biu = devm_reset_control_get(hdmirx_dev->dev, "rst_biu"); if (IS_ERR(hdmirx_dev->rst_biu)) { dev_err(dev, "failed to get rst_biu control\n"); return PTR_ERR(hdmirx_dev->rst_biu); } shreeya@shreeya:~/collabora/rd/rockchip/linux$ make dtbs DTC arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb Error: arch/arm64/boot/dts/rockchip/rk3588.dtsi:187.23-24 syntax error FATAL ERROR: Unable to parse input tree make[3]: *** [scripts/Makefile.lib:419: arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb] Error 1 make[2]: *** [scripts/Makefile.build:481: arch/arm64/boot/dts/rockchip] Error 2 make[1]: *** [/home/shreeya/collabora/rd/rockchip/linux/Makefile:1392: dtbs] Error 2 make: *** [Makefile:240: __sub-make] Error 2 Sorry for referring this as a driver build failure but I am sure you would also have not been okay with the above issues. Ofcourse I can simply remove this dependency from the driver but maybe that will affect the functionality and I didn't want to send a buggy patch series. My intention here was just like Heiko said, to keep all the dependent patches together. I didn't know that would be a trouble for Maintainers. FWIW, HDMIRX patch series was reviewed multiple times and that is why you see a Reviewed-by tag from a Collabora Engineer. Sometimes the things that look problematic to one might not look the same to others and that is why I asked you to provide some more details about the problem that you were seeing. https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/21 https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/17 > > > > > > Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski: > >> Please do not engage multiple subsystems in one patchset, if not > >> necessary. Especially do not mix DTS into media or USB subsystems. And > >> do not put DTS in the middle! > > > > picking up your reply from patch 4/6, there seem to be different "schools > > of thought" for this. Some maintainers might want to really only see > > patches that are explicitly for their subsystem - I guess networking > > might be a prime example for that, who will essentially apply whole series' > > if nobody protests in time (including dts patches) > > There is no school saying DTS is allowed to be in the middle. > > Other schools are indeed saying that seeing DTS is good and > recommendation is to post it separate and provide a link. That's way you > avoid DTS being pulled by Greg, media or networking. > This was my mistake and I simply forgot to remove the DTS when I was testing the driver for the last time before sending the v3 upstream. Adding it in the middle was incorrect, I should have added it as a separate patch but honestly this has always been very confusing and the expectation differs from maintainers to maintainers. > > > > On the other hand I also remember seeing requests for "the full picture" > > and individual maintainers then just picking and applying the patches > > meant for their subsystem. > > > > The series as it stands right now is nice in that it allows (random) > > developers to just pick it up, apply it to a tree and test the actual driver > > without needing to hunt for multiple dependant series. > > > > > > Of course you're right, the "arm64: dts:" patch should be the last in the > > series and not be in the middle of it. > I will make sure to correct this in my v4. Thanks, Shreeya Patel > > Best regards, > Krzysztof >
On 04/04/2024 10:07, Shreeya Patel wrote: > On Thursday, April 04, 2024 11:45 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 04/04/2024 00:48, Heiko Stübner wrote: >>> Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski: >>>> On 03/04/2024 13:20, Shreeya Patel wrote: >>>>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>>> >>>>>> On 03/04/2024 11:24, Shreeya Patel wrote: >>>>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: >>>>>>> >>>>>>>> This series implements support for the Synopsys DesignWare >>>>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b >>>>>>>> and HDMI 2.0. >>>>>>>> >>>>>>> >>>>>>> Hi Mauro and Hans, >>>>>>> >>>>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. >>>>>> >>>>>> Why did you put clk changes here? These go via different subsystem. That >>>>>> might be one of obstacles for your patchset. >>>>>> >>>>> >>>>> I added clock changes in this patch series because HDMIRX driver depends on it. >>>>> I thought it is wrong to send the driver patches which don't even compile? >>>> >>>> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. >>>> Please get it reviewed internally first. >>> >>> For the change in question, the clock controller on the soc also handles >>> the reset controls (hence its name CRU, clock-and-reset-unit) . >>> >>> There are at least 660 reset lines in the unit and it seems the hdmi-rx one >>> was overlooked on the initial submission, hence patches 1+2 add the >>> reset-line. >>> >>> Of course, here only the "arm64: dts:" patch depends on the clock >>> change, is it references the new reset-id. >> >> Wait, that's expected, but it is not what was written. Claim was HDMIRX >> driver depends *build time* ("don't even compile"). >> > > For some context, when I was testing the downstream driver against the > device tree, I saw some failures because of the missing reset ( which I am trying > to add in the first two patches for this series ) > > ... > hdmirx_dev->rst_biu = devm_reset_control_get(hdmirx_dev->dev, "rst_biu"); > if (IS_ERR(hdmirx_dev->rst_biu)) { > dev_err(dev, "failed to get rst_biu control\n"); > return PTR_ERR(hdmirx_dev->rst_biu); > } That's a driver.... > shreeya@shreeya:~/collabora/rd/rockchip/linux$ make dtbs > DTC arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb and that's a DTS. > Error: arch/arm64/boot/dts/rockchip/rk3588.dtsi:187.23-24 syntax error > FATAL ERROR: Unable to parse input tree > make[3]: *** [scripts/Makefile.lib:419: arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb] Error 1 > make[2]: *** [scripts/Makefile.build:481: arch/arm64/boot/dts/rockchip] Error 2 > make[1]: *** [/home/shreeya/collabora/rd/rockchip/linux/Makefile:1392: dtbs] Error 2 > make: *** [Makefile:240: __sub-make] Error 2 They are not related anyhow. Look above which Makefile target produced error. Which file failed to build. This is a expressed in make[3] line. Directory is expressed in other places. > > Sorry for referring this as a driver build failure but I am sure you would > also have not been okay with the above issues. > Ofcourse I can simply remove this dependency from the driver but maybe > that will affect the functionality and I didn't want to send a buggy patch series. What dependency? It seems you did not understand anything from Heiko's message, so please reach to your colleagues for explanation where is the dependency. > > My intention here was just like Heiko said, to keep all the dependent patches > together. I didn't know that would be a trouble for Maintainers. They are not dependent. > > FWIW, HDMIRX patch series was reviewed multiple times and that is why you > see a Reviewed-by tag from a Collabora Engineer. Sometimes the things that look > problematic to one might not look the same to others and that is why I asked you > to provide some more details about the problem that you were seeing. > > https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/21 > https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/17 Sorry, that's some third party gitlab... I don't know what does it prove. > > >>> >>> >>> Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski: >>>> Please do not engage multiple subsystems in one patchset, if not >>>> necessary. Especially do not mix DTS into media or USB subsystems. And >>>> do not put DTS in the middle! >>> >>> picking up your reply from patch 4/6, there seem to be different "schools >>> of thought" for this. Some maintainers might want to really only see >>> patches that are explicitly for their subsystem - I guess networking >>> might be a prime example for that, who will essentially apply whole series' >>> if nobody protests in time (including dts patches) >> >> There is no school saying DTS is allowed to be in the middle. >> >> Other schools are indeed saying that seeing DTS is good and >> recommendation is to post it separate and provide a link. That's way you >> avoid DTS being pulled by Greg, media or networking. >> > > This was my mistake and I simply forgot to remove the DTS when I was > testing the driver for the last time before sending the v3 upstream. > Adding it in the middle was incorrect, I should have added it as a separate > patch but honestly this has always been very confusing and the expectation > differs from maintainers to maintainers. There were guidelines - presented in the conferences, mailing list and even SoC maintainer profile explains how patches eventually end up. I agree that it still might be confusing, but these are the basics of submitting patches to anything touching SoC. Anyone working with SoC will need to know them, so how about Collabora creates some internal guideline explaining this, so such confusions could be avoided? I know that such guidelines exist in other companies... Best regards, Krzysztof
Am Donnerstag, 4. April 2024, 08:15:50 CEST schrieb Krzysztof Kozlowski: > On 04/04/2024 00:48, Heiko Stübner wrote: > > Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski: > >> On 03/04/2024 13:20, Shreeya Patel wrote: > >>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>> > >>>> On 03/04/2024 11:24, Shreeya Patel wrote: > >>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: > >>>>> > >>>>>> This series implements support for the Synopsys DesignWare > >>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b > >>>>>> and HDMI 2.0. > >>>>>> > >>>>> > >>>>> Hi Mauro and Hans, > >>>>> > >>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. > >>>> > >>>> Why did you put clk changes here? These go via different subsystem. That > >>>> might be one of obstacles for your patchset. > >>>> > >>> > >>> I added clock changes in this patch series because HDMIRX driver depends on it. > >>> I thought it is wrong to send the driver patches which don't even compile? > >> > >> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. > >> Please get it reviewed internally first. > > > > For the change in question, the clock controller on the soc also handles > > the reset controls (hence its name CRU, clock-and-reset-unit) . > > > > There are at least 660 reset lines in the unit and it seems the hdmi-rx one > > was overlooked on the initial submission, hence patches 1+2 add the > > reset-line. > > > > Of course, here only the "arm64: dts:" patch depends on the clock > > change, is it references the new reset-id. > > Wait, that's expected, but it is not what was written. Claim was HDMIRX > driver depends *build time* ("don't even compile"). Trying to do a full build (kernel + dts) will fail, because the the device-tree patch references the newly added reset-id . So you end up with a dtc error. Same with the binding. I think in the past to uncouple this we did reference the id by number first: + resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>, + <&cru SRST_HDMIRX_REF>, <&cru 660>; Had the id-addition separately and then a later series for kernel x+1 to convert from 660 to SRST_A_HDMIRX_BIU . > > Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski: > >> Please do not engage multiple subsystems in one patchset, if not > >> necessary. Especially do not mix DTS into media or USB subsystems. And > >> do not put DTS in the middle! > > > > picking up your reply from patch 4/6, there seem to be different "schools > > of thought" for this. Some maintainers might want to really only see > > patches that are explicitly for their subsystem - I guess networking > > might be a prime example for that, who will essentially apply whole series' > > if nobody protests in time (including dts patches) > > There is no school saying DTS is allowed to be in the middle. I think I wrote exactly that in my original reply :-) Am Donnerstag, 4. April 2024, 00:48:38 CEST schrieb Heiko Stübner: > Of course you're right, the "arm64: dts:" patch should be the last in the > series and not be in the middle of it. Heiko
On 04/04/2024 10:19, Heiko Stübner wrote: > Am Donnerstag, 4. April 2024, 08:15:50 CEST schrieb Krzysztof Kozlowski: >> On 04/04/2024 00:48, Heiko Stübner wrote: >>> Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski: >>>> On 03/04/2024 13:20, Shreeya Patel wrote: >>>>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>>> >>>>>> On 03/04/2024 11:24, Shreeya Patel wrote: >>>>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote: >>>>>>> >>>>>>>> This series implements support for the Synopsys DesignWare >>>>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b >>>>>>>> and HDMI 2.0. >>>>>>>> >>>>>>> >>>>>>> Hi Mauro and Hans, >>>>>>> >>>>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series. >>>>>> >>>>>> Why did you put clk changes here? These go via different subsystem. That >>>>>> might be one of obstacles for your patchset. >>>>>> >>>>> >>>>> I added clock changes in this patch series because HDMIRX driver depends on it. >>>>> I thought it is wrong to send the driver patches which don't even compile? >>>> >>>> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong. >>>> Please get it reviewed internally first. >>> >>> For the change in question, the clock controller on the soc also handles >>> the reset controls (hence its name CRU, clock-and-reset-unit) . >>> >>> There are at least 660 reset lines in the unit and it seems the hdmi-rx one >>> was overlooked on the initial submission, hence patches 1+2 add the >>> reset-line. >>> >>> Of course, here only the "arm64: dts:" patch depends on the clock >>> change, is it references the new reset-id. >> >> Wait, that's expected, but it is not what was written. Claim was HDMIRX >> driver depends *build time* ("don't even compile"). > > Trying to do a full build (kernel + dts) will fail, because the the > device-tree patch references the newly added reset-id . > > So you end up with a dtc error. Same with the binding. Which is quite expected, nothing special, most patchsets have exactly the same dependency. It's not a HDMIRX driver dependency. It's DTS and clock provider on the binding header, not clock consumer. We solved it many times and different SoC subsystems have their own guidelines. Putting here media is not the right approach and not justified. Best regards, Krzysztof
On Thu, 28 Mar 2024 04:20:51 +0530, Shreeya Patel wrote: > This series implements support for the Synopsys DesignWare > HDMI RX Controller, being compliant with standard HDMI 1.4b > and HDMI 2.0. > > Features that are currently supported by the HDMI RX driver > have been tested on rock5b board using a HDMI to micro-HDMI cable. > It is recommended to use a good quality cable as there were > multiple issues seen during testing the driver. > > [...] Applied, thanks! [1/6] dt-bindings: reset: Define reset id used for HDMI Receiver commit: ca151fd56b5736a7adbdba5675b9d87d70f20b23 [2/6] clk: rockchip: rst-rk3588: Add reset line for HDMI Receiver commit: 7af67019cd78d028ef377df689ac103d51905518 Best regards,
This series implements support for the Synopsys DesignWare HDMI RX Controller, being compliant with standard HDMI 1.4b and HDMI 2.0. Features that are currently supported by the HDMI RX driver have been tested on rock5b board using a HDMI to micro-HDMI cable. It is recommended to use a good quality cable as there were multiple issues seen during testing the driver. Please note the below information :- * While testing the driver on rock5b we noticed that the binary BL31 from Rockchip contains some unknown code to get the HDMI-RX PHY access working. With TF-A BL31, the HDMI-RX PHY doesn't work as expected since there are no interrupts seen for rk_hdmirx-hdmi leading to some failures in the driver [0]. * We have tested the working of OBS studio with HDMIRX driver and there were no issues seen. * We also tested and verified the support for interlaced video. [0] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/trusted-firmware-a/-/issues/1 To test the HDMI RX Controller driver, following example commands can be used :- root@debian-rockchip-rock5b-rk3588:~# v4l2-ctl --verbose -d /dev/video0 \ --set-fmt-video=width=1920,height=1080,pixelformat='BGR3' --stream-mmap=4 \ --stream-skip=3 --stream-count=100 --stream-to=/home/hdmiin4k.raw --stream-poll root@debian-rockchip-rock5b-rk3588:~# ffmpeg -f rawvideo -vcodec rawvideo \ -s 1920x1080 -r 60 -pix_fmt bgr24 -i /home/hdmiin4k.raw output.mkv Following is the v4l2-compliance test result :- root@debian-rockchip-rock5b-rk3588:~# v4l2-compliance -d /dev/video0 v4l2-compliance 1.27.0-5174, 64 bits, 64-bit time_t v4l2-compliance SHA: d700deb14368 2024-01-18 12:19:05 Compliance test for snps_hdmirx device /dev/video0: Driver Info: Driver name : snps_hdmirx Card type : snps_hdmirx Bus info : platform: snps_hdmirx Driver version : 6.8.0 Capabilities : 0x84201000 Video Capture Multiplanar Streaming Extended Pix Format Device Capabilities Device Caps : 0x04201000 Video Capture Multiplanar Streaming Extended Pix Format Required ioctls: test VIDIOC_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/video0 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK test VIDIOC_DV_TIMINGS_CAP: OK test VIDIOC_G/S_EDID: OK Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 2 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test CREATE_BUFS maximum buffers: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Total for snps_hdmirx device /dev/video0: 46, Succeeded: 46, Failed: 0, Warnings: 0 Changes in v3 :- - Use v4l2-common helpers in the HDMIRX driver - Rename cma node and phandle names - Elaborate the comment to explain 160MiB calculation - Move &hdmi_receiver_cma to the rock5b dts file - Add information about interlaced video testing in the cover-letter Changes in v2 :- - Fix checkpatch --strict warnings - Move the dt-binding include file changes in a separate patch - Add a description for the hardware in the dt-bindings file - Rename resets, vo1 grf and HPD properties - Add a proper description for grf and vo1-grf phandles in the bindings - Rename the HDMI RX node name to hdmi-receiver - Include gpio header file in binding example to fix the dt_binding_check failure - Move hdmirx_cma node to the rk3588.dtsi file - Add an entry to MAINTAINERS file for the HDMIRX driver Shreeya Patel (6): dt-bindings: reset: Define reset id used for HDMI Receiver clk: rockchip: rst-rk3588: Add reset line for HDMI Receiver dt-bindings: media: Document HDMI RX Controller arm64: dts: rockchip: Add device tree support for HDMI RX Controller media: platform: synopsys: Add support for hdmi input driver MAINTAINERS: Add entry for Synopsys DesignWare HDMI RX Driver .../bindings/media/snps,dw-hdmi-rx.yaml | 132 + MAINTAINERS | 8 + .../boot/dts/rockchip/rk3588-pinctrl.dtsi | 41 + .../boot/dts/rockchip/rk3588-rock-5b.dts | 19 + arch/arm64/boot/dts/rockchip/rk3588.dtsi | 56 + drivers/clk/rockchip/rst-rk3588.c | 1 + drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/synopsys/Kconfig | 3 + drivers/media/platform/synopsys/Makefile | 2 + .../media/platform/synopsys/hdmirx/Kconfig | 18 + .../media/platform/synopsys/hdmirx/Makefile | 4 + .../platform/synopsys/hdmirx/snps_hdmirx.c | 2726 +++++++++++++++++ .../platform/synopsys/hdmirx/snps_hdmirx.h | 394 +++ .../synopsys/hdmirx/snps_hdmirx_cec.c | 289 ++ .../synopsys/hdmirx/snps_hdmirx_cec.h | 46 + .../dt-bindings/reset/rockchip,rk3588-cru.h | 2 + 17 files changed, 3743 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml create mode 100644 drivers/media/platform/synopsys/Kconfig create mode 100644 drivers/media/platform/synopsys/Makefile create mode 100644 drivers/media/platform/synopsys/hdmirx/Kconfig create mode 100644 drivers/media/platform/synopsys/hdmirx/Makefile create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.c create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.h