| Message ID | 20170901094629.6566-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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;
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.
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
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
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 >
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
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
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;
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(+)