discussion: re-structuring of the Amlogic Meson VPU DRM driver

[Neil Armstrong]

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:
> 1. 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:
> 1. 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

> 
> 2. 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.

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.

> 
> 3. 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 ?

> 
> 4. 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.
CVBS can be handled separately without bridges.

I can have a try to move stuff if you can review/test on your side.
Would it be a good start ?

> 
> 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.

> 
> 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.

> I will start a separate discussion for that topic after this one.
> 
> 
> Best regards,
> Martin
> 

Thanks for starting the thread !

Neil