Hi Neil,
On Mon, Jan 4, 2021 at 2:29 PM Neil Armstrong narmstrong@baylibre.com wrote:
Hi,
Sorry for the delay...
On 31/12/2020 00:24, Martin Blumenstingl wrote:
Hi Neil and all interested people,
in the past there were concerns about how some of the components are coupled in our Meson DRM driver(s). With this discussion I would like to achieve four things:
- understand the current issues that we have> 2. come up with a TODO list of things that need to be tackled as well
as recommendations how to solve it (for example: "driver ABC function ABC uses the recommended way - take that as reference") 3. one by one work on the items on the TODO list 4. add support for the 32-bit SoCs to the Meson VPU DRM driver (without adding more "not recommended" code)
Disclaimer: I am not familiar with the DRM subsystem - so apologies if the terminology is not correct.
drivers/gpu/drm/meson/meson_dw_hdmi.c currently serves four purposes:
- manage the TOP (glue) registers for the dw-hdmi IP
This is Amlogic specific and consists of things like HPD filtering, some internal resets, etc. In my opinion this part is supposed to stay in this driver
Yep, it's tightly coupled to the DW-HDMI core
- load the driver for the dw-hdmi IP by calling dw_hdmi_probe()
I read somewhere that this is not recommended anymore and should be replaced. Is my understanding correct and what is the recommended replacement?
Yeah in fact the dw-hdmi glue should be a pure bridge, not a component anymore.
This means it should probe by itself entirely, should not use the component stuff.
OK, I see
This also means all the VPU related part (mainly encoder and clock) should be moved out of this file as a bridge and built with the meson_drm driver, then find the "next bridge" like other drivers.
is that linkage automatically set up with the endpoint definitions inside the .dts?
- it manages the HDMI PHY registers in the HHI register area
For the 32-bit SoCs I will not follow this pattern and will create a separate PHY instead. As a long-term goal I think we should also move this into a dedicated PHY driver.
I looked at it, and ... it's complex. For the 32-bit socs it's easy because you only have a single PHY setup, for the new gens you have to deal with the 4k modes and co. This could be handle by adding a new parameters set to the phy_configure union, but what should we add in it to be super generic ?
you are right, on the 32-bit SoCs it's pretty easy actually. it's only "4k" and "everything else".
I think using the phy_configure approach is the right way but to be honest: I have not thought about which parameters to add to that union (for the 64-bit SoCs) to make it "generic" enough also I think this is a TODO "for later", so no action needed now - but it's great to see that you had the same idea in mind :)
- call back into VPU/VENC functions to set up these registers
This is a blocker for 32-bit SoC support as I would have to duplicate this code if we don't make any changes. This includes things like calculating (and setting) clock frequencies, calling meson_venc_hdmi_mode_set for setting up the DVI pixel encoder, etc. My understanding is that this part should not be part of the meson_dw_hdmi driver, but "some other" driver. I don't understand which driver that's supposed to be though and how things would be wired up in the end.
Yep it should be a bridge. You can chain bridges, it's designed for such use case.
We will have internal bridges for encoders, ENCL+ENCP grouped for HDMI and ENCL.
I see. adding to my question above: would this mean that we have then more "endpoints" defined in our .dts - one for the ENCI+ENCP (HDMI) output, another one for the ENCL (DSI), ...?
CVBS can be handled separately without bridges.
indeed, let's postpone CVBS for now as it's easy to adapt the current code for the 32-bit SoCs. in a perfect world I think the CVBS encoder/bridge (whatever the correct type is) would be it's own driver as it's part of the HHI registers
I can have a try to move stuff if you can review/test on your side. Would it be a good start ?
that would be awesome if there's any way I can help (you add FIXMEs/TODOs to your code which you want me to solve, testing, etc.) then please let me know!
In addition to HDMI my understanding is that for adding MIPI DSI support you would a) either have to follow the pattern from the meson_dw_hdmi driver or b) also require some better way to achieve this
With the cut I described before, we'll need a add a simple ENCL bridge in meson_drm and a standalone bridge for dw-mipi-dsi.
that sounds like we don't need to duplicate any code which would be great
The biggest question marks for me are #2 and #4 from the list above. Also is there anything I have missed? Any input, feedback and questions are welcome!
PS: an additional topic on the TODO list will be "use the common clock framework" for clock setup. it's currently not clear to me if that's possible on the 64-bit SoCs in all cases.
It's the same issue for 4k & co, the high freqs needs special PLL settings, not sure how this would be easily doable in the PLL driver. We may need to add a gx/g12 HDMI specific pll driver.
the PLL settings are unfortunately also not very easy for the 32-bit SoCs. but let's have this discussion at another time, I think that changing the drm driver structure can be separate from this.
Best regards, Martin