[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Linus Walleij]

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);
 	}
 	priv->bridge = bridge;
 
-- 
2.13.5

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Linus Walleij]

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);
 	}
 	priv->bridge = bridge;
 
-- 
2.13.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Daniel Vetter]

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?

Cheers, Daniel
>  	}
>  	priv->bridge = bridge;
>  
> -- 
> 2.13.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Daniel Vetter]

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?

Cheers, Daniel
>  	}
>  	priv->bridge = bridge;
>  
> -- 
> 2.13.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Laurent Pinchart]

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

Re: [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Laurent Pinchart]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Eric Anholt]

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>

Re: [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2371 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

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

> 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

Re: [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

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

> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Linus Walleij]

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

Re: [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Linus Walleij]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Noralf Trønnes]

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
>

Re: [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Noralf Trønnes]

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Daniel Vetter]

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

Re: [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Daniel Vetter]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Daniel Vetter]

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

Re: [PATCH 2/2] RFC: drm/pl111: Support using the VGA bridge as fallback

[Daniel Vetter]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel