* [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback
2017-09-04 7:43 ` Daniel Vetter
@ 2017-09-04 9:51 ` Laurent Pinchart
2017-09-07 13:26 ` Linus Walleij
2017-09-05 19:43 ` Eric Anholt
2017-09-07 12:59 ` Linus Walleij
2 siblings, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2017-09-04 9:51 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Monday, 4 September 2017 10:43:55 EEST Daniel Vetter wrote:
> On Fri, Sep 01, 2017 at 11:46:29AM +0200, Linus Walleij wrote:
> > If we cannot find a panel, assume that the output from the
> > PL111 is connected directly to a "dumb" VGA connector,
> > so look up the connector from that bridge.
> >
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > This is how the new API is used in the PL111.
> > ---
> >
> > drivers/gpu/drm/pl111/Kconfig | 1 +
> > drivers/gpu/drm/pl111/pl111_drv.c | 3 +++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/pl111/Kconfig b/drivers/gpu/drm/pl111/Kconfig
> > index e5e2abd66491..82cb3e60ddc8 100644
> > --- a/drivers/gpu/drm/pl111/Kconfig
> > +++ b/drivers/gpu/drm/pl111/Kconfig
> > @@ -8,6 +8,7 @@ config DRM_PL111
> >
> > select DRM_GEM_CMA_HELPER
> > select DRM_BRIDGE
> > select DRM_PANEL_BRIDGE
> >
> > + select DRM_DUMB_VGA_DAC
> >
> > select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
> > help
> >
> > Choose this option for DRM support for the PL111 CLCD controller.
> >
> > diff --git a/drivers/gpu/drm/pl111/pl111_drv.c
> > b/drivers/gpu/drm/pl111/pl111_drv.c index f5bc6f160e60..6db423bbd84e
> > 100644
> > --- a/drivers/gpu/drm/pl111/pl111_drv.c
> > +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> > @@ -67,6 +67,7 @@
> >
> > #include <drm/drm_of.h>
> > #include <drm/drm_bridge.h>
> > #include <drm/drm_panel.h>
> >
> > +#include <drm/dumb_vga_dac.h>
> >
> > #include "pl111_drm.h"
> > #include "pl111_versatile.h"
> >
> > @@ -128,6 +129,8 @@ static int pl111_modeset_init(struct drm_device *dev)
> >
> > if (panel) {
> >
> > priv->panel = panel;
> > priv->connector = panel->connector;
> >
> > + } else {
> > + priv->connector = drm_dumb_vga_get_connector(bridge);
>
> Why do you need to set this? The only code I could find tries to set
> polarity values from bus_flags, and for VGA I'd assume you want to instead
> key this off the mode?
>
> Wrt the more general problem: They way we solve this in the i915 atomic
> framework is that the various ->fixup calls fill out relevant fields in
> the drm_crtc_state. I guess we could add an ->atomic_check to bridges, add
> a bus_flags to drm_crtc_state (or maybe just let everyone patch up
> adjusted_mode->flags) instead?
Furthermore, if we needed to have access to the connector in the display
controller driver, this would call for creating the connector there, not in
the bridge driver. I've advocated this for a long time, and still haven't
given up all hopes of fixing it.
> > }
> > priv->bridge = bridge;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback
2017-09-04 9:51 ` Laurent Pinchart
@ 2017-09-07 13:26 ` Linus Walleij
0 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2017-09-07 13:26 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Sep 4, 2017 at 11:51 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday, 4 September 2017 10:43:55 EEST Daniel Vetter wrote:
>> On Fri, Sep 01, 2017 at 11:46:29AM +0200, Linus Walleij wrote:
>> Wrt the more general problem: They way we solve this in the i915 atomic
>> framework is that the various ->fixup calls fill out relevant fields in
>> the drm_crtc_state. I guess we could add an ->atomic_check to bridges, add
>> a bus_flags to drm_crtc_state (or maybe just let everyone patch up
>> adjusted_mode->flags) instead?
>
> Furthermore, if we needed to have access to the connector in the display
> controller driver, this would call for creating the connector there, not in
> the bridge driver. I've advocated this for a long time, and still haven't
> given up all hopes of fixing it.
Hm as it is I am trying to adopt to circumstances, so I was just asked
to replace the custom connector with the generic panel bridge.
(bridge/panel.c)
Then we get the connector out of the generic panel bridge by just
dereferencing panel->connector, this is what the PL111 driver
is already doing for panels since its inception.
Maybe that is wrong, I don't know :/
But like Eric says, the core problem is that the display driver
say enable function needs to know the flags of what it should
output. This driver looks at
connector->display_info.bus_flags & DRM_BUS_FLAG_DE_LOW
connector->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE
So the bus flags really need to be available somehow.
I am doing it by keeping a local reference to the connector
in the driver state container.
I tried to dereference the connector from from the pipe at one
point, passing NULL to but it didn't work out, it seems like
struct drm_simple_display_pipe_funcs
.enable() is called before attaching the connector or something
but I guess I should look closer.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback
2017-09-04 7:43 ` Daniel Vetter
2017-09-04 9:51 ` Laurent Pinchart
@ 2017-09-05 19:43 ` Eric Anholt
2017-09-07 18:46 ` Daniel Vetter
2017-09-07 12:59 ` Linus Walleij
2 siblings, 1 reply; 9+ messages in thread
From: Eric Anholt @ 2017-09-05 19:43 UTC (permalink / raw)
To: linux-arm-kernel
Daniel Vetter <daniel@ffwll.ch> writes:
> On Fri, Sep 01, 2017 at 11:46:29AM +0200, Linus Walleij wrote:
>> If we cannot find a panel, assume that the output from the
>> PL111 is connected directly to a "dumb" VGA connector,
>> so look up the connector from that bridge.
>>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> This is how the new API is used in the PL111.
>> ---
>> drivers/gpu/drm/pl111/Kconfig | 1 +
>> drivers/gpu/drm/pl111/pl111_drv.c | 3 +++
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/pl111/Kconfig b/drivers/gpu/drm/pl111/Kconfig
>> index e5e2abd66491..82cb3e60ddc8 100644
>> --- a/drivers/gpu/drm/pl111/Kconfig
>> +++ b/drivers/gpu/drm/pl111/Kconfig
>> @@ -8,6 +8,7 @@ config DRM_PL111
>> select DRM_GEM_CMA_HELPER
>> select DRM_BRIDGE
>> select DRM_PANEL_BRIDGE
>> + select DRM_DUMB_VGA_DAC
>> select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
>> help
>> Choose this option for DRM support for the PL111 CLCD controller.
>> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
>> index f5bc6f160e60..6db423bbd84e 100644
>> --- a/drivers/gpu/drm/pl111/pl111_drv.c
>> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
>> @@ -67,6 +67,7 @@
>> #include <drm/drm_of.h>
>> #include <drm/drm_bridge.h>
>> #include <drm/drm_panel.h>
>> +#include <drm/dumb_vga_dac.h>
>>
>> #include "pl111_drm.h"
>> #include "pl111_versatile.h"
>> @@ -128,6 +129,8 @@ static int pl111_modeset_init(struct drm_device *dev)
>> if (panel) {
>> priv->panel = panel;
>> priv->connector = panel->connector;
>> + } else {
>> + priv->connector = drm_dumb_vga_get_connector(bridge);
>
> Why do you need to set this? The only code I could find tries to set
> polarity values from bus_flags, and for VGA I'd assume you want to instead
> key this off the mode?
>
> Wrt the more general problem: They way we solve this in the i915 atomic
> framework is that the various ->fixup calls fill out relevant fields in
> the drm_crtc_state. I guess we could add an ->atomic_check to bridges, add
> a bus_flags to drm_crtc_state (or maybe just let everyone patch up
> adjusted_mode->flags) instead?
It seems to me like the bus_flags/format should be specific to each link
in the display output chain, rather than global to it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170905/a7121552/attachment-0001.sig>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback
2017-09-05 19:43 ` Eric Anholt
@ 2017-09-07 18:46 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-09-07 18:46 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 5, 2017 at 9:43 PM, Eric Anholt <eric@anholt.net> wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
>
>> On Fri, Sep 01, 2017 at 11:46:29AM +0200, Linus Walleij wrote:
>>> If we cannot find a panel, assume that the output from the
>>> PL111 is connected directly to a "dumb" VGA connector,
>>> so look up the connector from that bridge.
>>>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>> This is how the new API is used in the PL111.
>>> ---
>>> drivers/gpu/drm/pl111/Kconfig | 1 +
>>> drivers/gpu/drm/pl111/pl111_drv.c | 3 +++
>>> 2 files changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/pl111/Kconfig b/drivers/gpu/drm/pl111/Kconfig
>>> index e5e2abd66491..82cb3e60ddc8 100644
>>> --- a/drivers/gpu/drm/pl111/Kconfig
>>> +++ b/drivers/gpu/drm/pl111/Kconfig
>>> @@ -8,6 +8,7 @@ config DRM_PL111
>>> select DRM_GEM_CMA_HELPER
>>> select DRM_BRIDGE
>>> select DRM_PANEL_BRIDGE
>>> + select DRM_DUMB_VGA_DAC
>>> select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
>>> help
>>> Choose this option for DRM support for the PL111 CLCD controller.
>>> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
>>> index f5bc6f160e60..6db423bbd84e 100644
>>> --- a/drivers/gpu/drm/pl111/pl111_drv.c
>>> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
>>> @@ -67,6 +67,7 @@
>>> #include <drm/drm_of.h>
>>> #include <drm/drm_bridge.h>
>>> #include <drm/drm_panel.h>
>>> +#include <drm/dumb_vga_dac.h>
>>>
>>> #include "pl111_drm.h"
>>> #include "pl111_versatile.h"
>>> @@ -128,6 +129,8 @@ static int pl111_modeset_init(struct drm_device *dev)
>>> if (panel) {
>>> priv->panel = panel;
>>> priv->connector = panel->connector;
>>> + } else {
>>> + priv->connector = drm_dumb_vga_get_connector(bridge);
>>
>> Why do you need to set this? The only code I could find tries to set
>> polarity values from bus_flags, and for VGA I'd assume you want to instead
>> key this off the mode?
>>
>> Wrt the more general problem: They way we solve this in the i915 atomic
>> framework is that the various ->fixup calls fill out relevant fields in
>> the drm_crtc_state. I guess we could add an ->atomic_check to bridges, add
>> a bus_flags to drm_crtc_state (or maybe just let everyone patch up
>> adjusted_mode->flags) instead?
>
> It seems to me like the bus_flags/format should be specific to each link
> in the display output chain, rather than global to it.
Yeah that might be the even more general option, to have a
drm_bridge_state for this stuff. Thanks to the driver private state
support recently added you can add state pointers for any void * (and
with a bit of glue even set up type-safe duplicate/get/for_each
wrappers). But I'm not clear whether we really need that much
generality or not. Currently it's used for the intel shared dpll stuff
and the dp mst state, in case someone needs to dig for examples.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback
2017-09-04 7:43 ` Daniel Vetter
2017-09-04 9:51 ` Laurent Pinchart
2017-09-05 19:43 ` Eric Anholt
@ 2017-09-07 12:59 ` Linus Walleij
2017-09-07 16:12 ` Noralf Trønnes
2 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2017-09-07 12:59 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Sep 4, 2017 at 9:43 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Sep 01, 2017 at 11:46:29AM +0200, Linus Walleij wrote:
>> If we cannot find a panel, assume that the output from the
>> PL111 is connected directly to a "dumb" VGA connector,
>> so look up the connector from that bridge.
>>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> if (panel) {
>> priv->panel = panel;
>> priv->connector = panel->connector;
>> + } else {
>> + priv->connector = drm_dumb_vga_get_connector(bridge);
>
> Why do you need to set this?
Essentially because drm_simple_display_pipe_init() require that you
specify a connector in the last argument, so I have to get it from
somewhere.
> The only code I could find tries to set
> polarity values from bus_flags, and for VGA I'd assume you want to instead
> key this off the mode?
The usecase is that either a panel or a dumb VGA bridge is used
(not both, luckily).
On *some* dumb VGA bridges, the polarity of the clock signal is
inverted, i.e. it expects data to be clocked out on the negative
edge rather than the positive edge. See:
https://lists.freedesktop.org/archives/dri-devel/2017-September/151603.html
And the PL111 can control this in software, that is why it is inspecting
the flags on the connector to figure out when to clock things out.
It seems logical to check the connector to figure out what clock edge
it wants?
> Wrt the more general problem: They way we solve this in the i915 atomic
> framework is that the various ->fixup calls fill out relevant fields in
> the drm_crtc_state. I guess we could add an ->atomic_check to bridges, add
> a bus_flags to drm_crtc_state (or maybe just let everyone patch up
> adjusted_mode->flags) instead?
Not quite following... I do need fixups to mask some modes off the
PL111 but that would be on the driver side.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback
2017-09-07 12:59 ` Linus Walleij
@ 2017-09-07 16:12 ` Noralf Trønnes
2017-09-07 18:44 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Noralf Trønnes @ 2017-09-07 16:12 UTC (permalink / raw)
To: linux-arm-kernel
Den 07.09.2017 14.59, skrev Linus Walleij:
> On Mon, Sep 4, 2017 at 9:43 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Sep 01, 2017 at 11:46:29AM +0200, Linus Walleij wrote:
>>> If we cannot find a panel, assume that the output from the
>>> PL111 is connected directly to a "dumb" VGA connector,
>>> so look up the connector from that bridge.
>>>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>> if (panel) {
>>> priv->panel = panel;
>>> priv->connector = panel->connector;
>>> + } else {
>>> + priv->connector = drm_dumb_vga_get_connector(bridge);
>> Why do you need to set this?
> Essentially because drm_simple_display_pipe_init() require that you
> specify a connector in the last argument, so I have to get it from
> somewhere.
It's optional particularly for the bridge case. From docs:
?* If a connector is supplied, the pipe will be coupled with the provided
?* connector. You may supply a NULL connector when using drm bridges, that
?* handle connectors themselves (see
drm_simple_display_pipe_attach_bridge()).
Noralf.
>> The only code I could find tries to set
>> polarity values from bus_flags, and for VGA I'd assume you want to instead
>> key this off the mode?
> The usecase is that either a panel or a dumb VGA bridge is used
> (not both, luckily).
>
> On *some* dumb VGA bridges, the polarity of the clock signal is
> inverted, i.e. it expects data to be clocked out on the negative
> edge rather than the positive edge. See:
> https://lists.freedesktop.org/archives/dri-devel/2017-September/151603.html
>
> And the PL111 can control this in software, that is why it is inspecting
> the flags on the connector to figure out when to clock things out.
>
> It seems logical to check the connector to figure out what clock edge
> it wants?
>
>> Wrt the more general problem: They way we solve this in the i915 atomic
>> framework is that the various ->fixup calls fill out relevant fields in
>> the drm_crtc_state. I guess we could add an ->atomic_check to bridges, add
>> a bus_flags to drm_crtc_state (or maybe just let everyone patch up
>> adjusted_mode->flags) instead?
> Not quite following... I do need fixups to mask some modes off the
> PL111 but that would be on the driver side.
>
> Yours,
> Linus Walleij
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback
2017-09-07 16:12 ` Noralf Trønnes
@ 2017-09-07 18:44 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-09-07 18:44 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Sep 7, 2017 at 6:12 PM, Noralf Tr?nnes <noralf@tronnes.org> wrote:
>
> Den 07.09.2017 14.59, skrev Linus Walleij:
>>
>> On Mon, Sep 4, 2017 at 9:43 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>
>>> On Fri, Sep 01, 2017 at 11:46:29AM +0200, Linus Walleij wrote:
>>>>
>>>> If we cannot find a panel, assume that the output from the
>>>> PL111 is connected directly to a "dumb" VGA connector,
>>>> so look up the connector from that bridge.
>>>>
>>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>>> if (panel) {
>>>> priv->panel = panel;
>>>> priv->connector = panel->connector;
>>>> + } else {
>>>> + priv->connector = drm_dumb_vga_get_connector(bridge);
>>>
>>> Why do you need to set this?
>>
>> Essentially because drm_simple_display_pipe_init() require that you
>> specify a connector in the last argument, so I have to get it from
>> somewhere.
>
>
> It's optional particularly for the bridge case. From docs:
>
> * If a connector is supplied, the pipe will be coupled with the provided
> * connector. You may supply a NULL connector when using drm bridges, that
> * handle connectors themselves (see
> drm_simple_display_pipe_attach_bridge()).
Yup, this was the flip side of the work to make bridges useful for the
simple pipe helpers. The other one is the helper to set up the bridge
I mentioned earlier in this thread.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread