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
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?
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.
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.
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
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.
I will start a separate discussion for that topic after this one.
Best regards,
Martin
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
Hi Neil,
On Mon, Jan 4, 2021 at 2:29 PM Neil Armstrong <[email protected]> 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