| Message ID | 20190820011721.30136-14-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: Replace custom display drivers with drm_bridge and drm_panel | expand |
Hi Laurent. On Tue, Aug 20, 2019 at 04:16:44AM +0300, Laurent Pinchart wrote: > Implement the newly added bridge connector operations, allowing the > usage of drm_bridge_panel with drm_bridge_connector. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/bridge/panel.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index f5b8e55301ac..1c7f5b648f05 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -60,7 +60,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge, > int ret; > > if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > - return -EINVAL; > + return 0; > > if (!bridge->encoder) { > DRM_ERROR("Missing encoder\n"); > @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) > drm_panel_unprepare(panel_bridge->panel); > } > > +static int panel_bridge_get_modes(struct drm_bridge *bridge, > + struct drm_connector *connector) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + /* > + * FIXME: drm_panel_get_modes() should take the connector as an > + * argument. > + */ Noted, I have patches to fix this. Needs a little testing/polishing before I post them. > + return drm_panel_get_modes(panel_bridge->panel); > +} > + > static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > .attach = panel_bridge_attach, > .detach = panel_bridge_detach, > @@ -130,6 +142,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > .enable = panel_bridge_enable, > .disable = panel_bridge_disable, > .post_disable = panel_bridge_post_disable, > + .get_modes = panel_bridge_get_modes, > }; > > /** > @@ -175,6 +188,9 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > #ifdef CONFIG_OF > panel_bridge->bridge.of_node = panel->dev->of_node; > #endif > + panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; > + /* FIXME: The panel should report its type. */ > + panel_bridge->bridge.type = DRM_MODE_CONNECTOR_DPI; Confused. We move the connector to the display controller. So the panel does not know the type. In others words - please put a few more words on this FIXME. Sam > > drm_bridge_add(&panel_bridge->bridge); > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Sam, On Tue, Aug 20, 2019 at 12:37:06PM +0200, Sam Ravnborg wrote: > On Tue, Aug 20, 2019 at 04:16:44AM +0300, Laurent Pinchart wrote: > > Implement the newly added bridge connector operations, allowing the > > usage of drm_bridge_panel with drm_bridge_connector. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/gpu/drm/bridge/panel.c | 18 +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > > index f5b8e55301ac..1c7f5b648f05 100644 > > --- a/drivers/gpu/drm/bridge/panel.c > > +++ b/drivers/gpu/drm/bridge/panel.c > > @@ -60,7 +60,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge, > > int ret; > > > > if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > > - return -EINVAL; > > + return 0; > > > > if (!bridge->encoder) { > > DRM_ERROR("Missing encoder\n"); > > @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) > > drm_panel_unprepare(panel_bridge->panel); > > } > > > > +static int panel_bridge_get_modes(struct drm_bridge *bridge, > > + struct drm_connector *connector) > > +{ > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > + > > + /* > > + * FIXME: drm_panel_get_modes() should take the connector as an > > + * argument. > > + */ > > Noted, I have patches to fix this. Needs a little testing/polishing > before I post them. Take your time. Thank you for addressing this issue :-) > > + return drm_panel_get_modes(panel_bridge->panel); > > +} > > + > > static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > > .attach = panel_bridge_attach, > > .detach = panel_bridge_detach, > > @@ -130,6 +142,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > > .enable = panel_bridge_enable, > > .disable = panel_bridge_disable, > > .post_disable = panel_bridge_post_disable, > > + .get_modes = panel_bridge_get_modes, > > }; > > > > /** > > @@ -175,6 +188,9 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > > #ifdef CONFIG_OF > > panel_bridge->bridge.of_node = panel->dev->of_node; > > #endif > > + panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; > > + /* FIXME: The panel should report its type. */ > > + panel_bridge->bridge.type = DRM_MODE_CONNECTOR_DPI; > > Confused. > We move the connector to the display controller. > So the panel does not know the type. > > In others words - please put a few more words on this FIXME. I mean the panel should report if it's a DPI, LVDS, or other type of panel, so that the display controller will know what to initialise the connector type to. I think the drm_panel structure should get a type field, similar to the bridge type field. Does that make sense to you ? > > > > drm_bridge_add(&panel_bridge->bridge); > >
On Tue, 20 Aug 2019 04:16:44 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Implement the newly added bridge connector operations, allowing the > usage of drm_bridge_panel with drm_bridge_connector. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/bridge/panel.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index f5b8e55301ac..1c7f5b648f05 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -60,7 +60,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge, > int ret; > > if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > - return -EINVAL; > + return 0; > > if (!bridge->encoder) { > DRM_ERROR("Missing encoder\n"); > @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) > drm_panel_unprepare(panel_bridge->panel); > } > > +static int panel_bridge_get_modes(struct drm_bridge *bridge, > + struct drm_connector *connector) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + /* > + * FIXME: drm_panel_get_modes() should take the connector as an > + * argument. > + */ > + return drm_panel_get_modes(panel_bridge->panel); > +} > + > static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > .attach = panel_bridge_attach, > .detach = panel_bridge_detach, > @@ -130,6 +142,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > .enable = panel_bridge_enable, > .disable = panel_bridge_disable, > .post_disable = panel_bridge_post_disable, > + .get_modes = panel_bridge_get_modes, > }; > > /** > @@ -175,6 +188,9 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > #ifdef CONFIG_OF > panel_bridge->bridge.of_node = panel->dev->of_node; > #endif > + panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; > + /* FIXME: The panel should report its type. */ > + panel_bridge->bridge.type = DRM_MODE_CONNECTOR_DPI; Shouldn't we patch all panel drivers to expose this type before doing this change? I mean, the connector type is exposed to userspace, and I wouldn't be surprised if some userspace apps/libs decided to base their output selection logic on this field. > > drm_bridge_add(&panel_bridge->bridge); >
Hi Boris, On Thu, Aug 22, 2019 at 06:29:09PM +0200, Boris Brezillon wrote: > On Tue, 20 Aug 2019 04:16:44 +0300 Laurent Pinchart wrote: > > > Implement the newly added bridge connector operations, allowing the > > usage of drm_bridge_panel with drm_bridge_connector. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/gpu/drm/bridge/panel.c | 18 +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > > index f5b8e55301ac..1c7f5b648f05 100644 > > --- a/drivers/gpu/drm/bridge/panel.c > > +++ b/drivers/gpu/drm/bridge/panel.c > > @@ -60,7 +60,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge, > > int ret; > > > > if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > > - return -EINVAL; > > + return 0; > > > > if (!bridge->encoder) { > > DRM_ERROR("Missing encoder\n"); > > @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) > > drm_panel_unprepare(panel_bridge->panel); > > } > > > > +static int panel_bridge_get_modes(struct drm_bridge *bridge, > > + struct drm_connector *connector) > > +{ > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > + > > + /* > > + * FIXME: drm_panel_get_modes() should take the connector as an > > + * argument. > > + */ > > + return drm_panel_get_modes(panel_bridge->panel); > > +} > > + > > static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > > .attach = panel_bridge_attach, > > .detach = panel_bridge_detach, > > @@ -130,6 +142,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > > .enable = panel_bridge_enable, > > .disable = panel_bridge_disable, > > .post_disable = panel_bridge_post_disable, > > + .get_modes = panel_bridge_get_modes, > > }; > > > > /** > > @@ -175,6 +188,9 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > > #ifdef CONFIG_OF > > panel_bridge->bridge.of_node = panel->dev->of_node; > > #endif > > + panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; > > + /* FIXME: The panel should report its type. */ > > + panel_bridge->bridge.type = DRM_MODE_CONNECTOR_DPI; > > Shouldn't we patch all panel drivers to expose this type before doing > this change? I mean, the connector type is exposed to userspace, and I > wouldn't be surprised if some userspace apps/libs decided to base their > output selection logic on this field. Note that this type will only make it to userspace for drivers that use the bridge->type field, likely through the drm bridge connector helper. I do agree that panel drivers should be updated, but given the number of panels in panel-simple and the fact that the information would need to be researched for most of them, this will be significant work. Can't this be done when converting display controller drivers on a need basis ? Or maybe we could, as an interim measure, derive the type from the bus formats reported by the panel if the panel type is not set ? If the panel reports MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, MEDIA_BUS_FMT_RGB666_1X7X4_SPWG or MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA then we can set the type to LVDS, otherwise we set it to DPI. I can submit a patch to add a type field to the panel structure and implement this logic. > > drm_bridge_add(&panel_bridge->bridge);
On Thu, 22 Aug 2019 19:35:24 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > On Thu, Aug 22, 2019 at 06:29:09PM +0200, Boris Brezillon wrote: > > On Tue, 20 Aug 2019 04:16:44 +0300 Laurent Pinchart wrote: > > > > > Implement the newly added bridge connector operations, allowing the > > > usage of drm_bridge_panel with drm_bridge_connector. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > drivers/gpu/drm/bridge/panel.c | 18 +++++++++++++++++- > > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > > > index f5b8e55301ac..1c7f5b648f05 100644 > > > --- a/drivers/gpu/drm/bridge/panel.c > > > +++ b/drivers/gpu/drm/bridge/panel.c > > > @@ -60,7 +60,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge, > > > int ret; > > > > > > if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > > > - return -EINVAL; > > > + return 0; > > > > > > if (!bridge->encoder) { > > > DRM_ERROR("Missing encoder\n"); > > > @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) > > > drm_panel_unprepare(panel_bridge->panel); > > > } > > > > > > +static int panel_bridge_get_modes(struct drm_bridge *bridge, > > > + struct drm_connector *connector) > > > +{ > > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > > + > > > + /* > > > + * FIXME: drm_panel_get_modes() should take the connector as an > > > + * argument. > > > + */ > > > + return drm_panel_get_modes(panel_bridge->panel); > > > +} > > > + > > > static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > > > .attach = panel_bridge_attach, > > > .detach = panel_bridge_detach, > > > @@ -130,6 +142,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > > > .enable = panel_bridge_enable, > > > .disable = panel_bridge_disable, > > > .post_disable = panel_bridge_post_disable, > > > + .get_modes = panel_bridge_get_modes, > > > }; > > > > > > /** > > > @@ -175,6 +188,9 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > > > #ifdef CONFIG_OF > > > panel_bridge->bridge.of_node = panel->dev->of_node; > > > #endif > > > + panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; > > > + /* FIXME: The panel should report its type. */ > > > + panel_bridge->bridge.type = DRM_MODE_CONNECTOR_DPI; > > > > Shouldn't we patch all panel drivers to expose this type before doing > > this change? I mean, the connector type is exposed to userspace, and I > > wouldn't be surprised if some userspace apps/libs decided to base their > > output selection logic on this field. > > Note that this type will only make it to userspace for drivers that use > the bridge->type field, likely through the drm bridge connector helper. > I do agree that panel drivers should be updated, but given the number of > panels in panel-simple and the fact that the information would need to > be researched for most of them, this will be significant work. Can't > this be done when converting display controller drivers on a need basis > ? I think setting a default value and fixing things on a need basis is okay, but that doesn't prevent you from adding the necessary infrastructure to let panel drivers pass this type (we can fallback to a default type in panel drivers instead of here). I'm also not sure why 'DPI' is chosen as the default, shouldn't we use 'Unknown' instead? > > Or maybe we could, as an interim measure, derive the type from the bus > formats reported by the panel if the panel type is not set ? If the > panel reports MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, > MEDIA_BUS_FMT_RGB666_1X7X4_SPWG or MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA then > we can set the type to LVDS, otherwise we set it to DPI. Hm, aren't we better off patching panel descs exposing these bus formats to also explicitly set desc->type to LVDS, leaving others to Unknown (Unknown is 0, so you don't have to patch all panel_desc definitions)? > I can submit a > patch to add a type field to the panel structure and implement this > logic.
Hi Boris, On Thu, Aug 22, 2019 at 08:02:47PM +0200, Boris Brezillon wrote: > On Thu, 22 Aug 2019 19:35:24 +0300 Laurent Pinchart wrote: > > On Thu, Aug 22, 2019 at 06:29:09PM +0200, Boris Brezillon wrote: > >> On Tue, 20 Aug 2019 04:16:44 +0300 Laurent Pinchart wrote: > >> > >>> Implement the newly added bridge connector operations, allowing the > >>> usage of drm_bridge_panel with drm_bridge_connector. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> drivers/gpu/drm/bridge/panel.c | 18 +++++++++++++++++- > >>> 1 file changed, 17 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > >>> index f5b8e55301ac..1c7f5b648f05 100644 > >>> --- a/drivers/gpu/drm/bridge/panel.c > >>> +++ b/drivers/gpu/drm/bridge/panel.c > >>> @@ -60,7 +60,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge, > >>> int ret; > >>> > >>> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > >>> - return -EINVAL; > >>> + return 0; > >>> > >>> if (!bridge->encoder) { > >>> DRM_ERROR("Missing encoder\n"); > >>> @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) > >>> drm_panel_unprepare(panel_bridge->panel); > >>> } > >>> > >>> +static int panel_bridge_get_modes(struct drm_bridge *bridge, > >>> + struct drm_connector *connector) > >>> +{ > >>> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > >>> + > >>> + /* > >>> + * FIXME: drm_panel_get_modes() should take the connector as an > >>> + * argument. > >>> + */ > >>> + return drm_panel_get_modes(panel_bridge->panel); > >>> +} > >>> + > >>> static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > >>> .attach = panel_bridge_attach, > >>> .detach = panel_bridge_detach, > >>> @@ -130,6 +142,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > >>> .enable = panel_bridge_enable, > >>> .disable = panel_bridge_disable, > >>> .post_disable = panel_bridge_post_disable, > >>> + .get_modes = panel_bridge_get_modes, > >>> }; > >>> > >>> /** > >>> @@ -175,6 +188,9 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > >>> #ifdef CONFIG_OF > >>> panel_bridge->bridge.of_node = panel->dev->of_node; > >>> #endif > >>> + panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; > >>> + /* FIXME: The panel should report its type. */ > >>> + panel_bridge->bridge.type = DRM_MODE_CONNECTOR_DPI; > >> > >> Shouldn't we patch all panel drivers to expose this type before doing > >> this change? I mean, the connector type is exposed to userspace, and I > >> wouldn't be surprised if some userspace apps/libs decided to base their > >> output selection logic on this field. > > > > Note that this type will only make it to userspace for drivers that use > > the bridge->type field, likely through the drm bridge connector helper. > > I do agree that panel drivers should be updated, but given the number of > > panels in panel-simple and the fact that the information would need to > > be researched for most of them, this will be significant work. Can't > > this be done when converting display controller drivers on a need basis > > ? > > I think setting a default value and fixing things on a need basis is > okay, but that doesn't prevent you from adding the necessary > infrastructure to let panel drivers pass this type (we can fallback to a > default type in panel drivers instead of here). I'll add the infrastructure/ > I'm also not sure why 'DPI' is chosen as the default, shouldn't we use > 'Unknown' instead? Mostly to avoid breaking userspace, as most panels supported by drm_panel use DPI. > > Or maybe we could, as an interim measure, derive the type from the bus > > formats reported by the panel if the panel type is not set ? If the > > panel reports MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, > > MEDIA_BUS_FMT_RGB666_1X7X4_SPWG or MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA then > > we can set the type to LVDS, otherwise we set it to DPI. > > Hm, aren't we better off patching panel descs exposing these bus formats > to also explicitly set desc->type to LVDS, leaving others to Unknown > (Unknown is 0, so you don't have to patch all panel_desc definitions)? I was thinking about adding this logic to drivers/gpu/drm/bridge/panel.c, which would avoid patching lots of panel drivers as a prerequisite. With such a logic there, plus a default to DPI, I thought we would be good enough for an initial version. It would leave DSI unhandled, so maybe not the best :-S > > I can submit a > > patch to add a type field to the panel structure and implement this > > logic.
Hi Sam, On 20/08/2019 13:37, Sam Ravnborg wrote: >> @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) >> drm_panel_unprepare(panel_bridge->panel); >> } >> >> +static int panel_bridge_get_modes(struct drm_bridge *bridge, >> + struct drm_connector *connector) >> +{ >> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> + >> + /* >> + * FIXME: drm_panel_get_modes() should take the connector as an >> + * argument. >> + */ > Noted, I have patches to fix this. Needs a little testing/polishing > before I post them. Do you have any testable patches for this? I was testing this series with a Toshiba DPI-2-DSI bridge and a DSI panel, and was hitting a crash as simple-panel couldn't get the connector. Laurent commented: > panel_bridge_attach() should be modified to call drm_panel_attach() even > when flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR, with the connctor parameter > set to NULL. That's easy, and drm_panel_attach() won't complain and > happily set panel->connector to NULL. The trouble is that the panel > drivers rely on panel->connector in their get_modes() implementation. > That function needs to be modified to take the connector pointer instead > of retrieving it from panel->connector (which can then be removed). Tomi
Hi Tomi On Thu, Oct 03, 2019 at 08:56:15AM +0300, Tomi Valkeinen wrote: > Hi Sam, > > On 20/08/2019 13:37, Sam Ravnborg wrote: > > > > @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) > > > drm_panel_unprepare(panel_bridge->panel); > > > } > > > +static int panel_bridge_get_modes(struct drm_bridge *bridge, > > > + struct drm_connector *connector) > > > +{ > > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > > + > > > + /* > > > + * FIXME: drm_panel_get_modes() should take the connector as an > > > + * argument. > > > + */ > > Noted, I have patches to fix this. Needs a little testing/polishing > > before I post them. > > Do you have any testable patches for this? > > I was testing this series with a Toshiba DPI-2-DSI bridge and a DSI panel, > and was hitting a crash as simple-panel couldn't get the connector. If time permits I will refresh the patches this weekend and send them out. But right now cannot promise much as I am awful behind. Sam
Hi Sam, On Wed, Oct 09, 2019 at 09:24:47PM +0200, Sam Ravnborg wrote: > On Thu, Oct 03, 2019 at 08:56:15AM +0300, Tomi Valkeinen wrote: > > On 20/08/2019 13:37, Sam Ravnborg wrote: > > > > > > @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) > > > > drm_panel_unprepare(panel_bridge->panel); > > > > } > > > > +static int panel_bridge_get_modes(struct drm_bridge *bridge, > > > > + struct drm_connector *connector) > > > > +{ > > > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > > > + > > > > + /* > > > > + * FIXME: drm_panel_get_modes() should take the connector as an > > > > + * argument. > > > > + */ > > > Noted, I have patches to fix this. Needs a little testing/polishing > > > before I post them. > > > > Do you have any testable patches for this? > > > > I was testing this series with a Toshiba DPI-2-DSI bridge and a DSI panel, > > and was hitting a crash as simple-panel couldn't get the connector. > > If time permits I will refresh the patches this weekend and send them > out. > But right now cannot promise much as I am awful behind. No worries at all. Is there any way I can help you, maybe continuing in-progress work ?
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index f5b8e55301ac..1c7f5b648f05 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -60,7 +60,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge, int ret; if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) - return -EINVAL; + return 0; if (!bridge->encoder) { DRM_ERROR("Missing encoder\n"); @@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) drm_panel_unprepare(panel_bridge->panel); } +static int panel_bridge_get_modes(struct drm_bridge *bridge, + struct drm_connector *connector) +{ + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + + /* + * FIXME: drm_panel_get_modes() should take the connector as an + * argument. + */ + return drm_panel_get_modes(panel_bridge->panel); +} + static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { .attach = panel_bridge_attach, .detach = panel_bridge_detach, @@ -130,6 +142,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { .enable = panel_bridge_enable, .disable = panel_bridge_disable, .post_disable = panel_bridge_post_disable, + .get_modes = panel_bridge_get_modes, }; /** @@ -175,6 +188,9 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, #ifdef CONFIG_OF panel_bridge->bridge.of_node = panel->dev->of_node; #endif + panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; + /* FIXME: The panel should report its type. */ + panel_bridge->bridge.type = DRM_MODE_CONNECTOR_DPI; drm_bridge_add(&panel_bridge->bridge);
Implement the newly added bridge connector operations, allowing the usage of drm_bridge_panel with drm_bridge_connector. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/bridge/panel.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)