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

[Martin Blumenstingl]

Hi Neil,

On Mon, Jan 4, 2021 at 2:29 PM Neil Armstrong <narmstrong@xxxxxxxxxxxx> 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:
> > 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.
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?

> > 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 ?
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 :)

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