| Message ID | 20210811235253.924867-1-robdclark@gmail.com |
|---|---|
| Headers | show |
| Series | drm/msm+ti-sn65dsi86: Fix NO_CONNECTOR fallout | expand |
Quoting Rob Clark (2021-08-11 16:52:47) > From: Rob Clark <robdclark@chromium.org> > > If we created our own connector because the driver does not support the > NO_CONNECTOR flag, we don't want the downstream bridge to *also* create > a connector. And if this driver did pass the NO_CONNECTOR flag (and we > supported that mode) this would change nothing. > > Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- Thanks for saving me the packaging effort. Reported-by: Stephen Boyd <swboyd@chromium.org> Tested-by: Stephen Boyd <swboyd@chromium.org>
Hi Rob, Thank you for the patch. On Wed, Aug 11, 2021 at 04:52:47PM -0700, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > If we created our own connector because the driver does not support the > NO_CONNECTOR flag, we don't want the downstream bridge to *also* create > a connector. And if this driver did pass the NO_CONNECTOR flag (and we > supported that mode) this would change nothing. > > Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge") > Signed-off-by: Rob Clark <robdclark@chromium.org> Makes complete sense. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 9bf889302bcc..5d3b30b2f547 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -736,6 +736,9 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, > } > pdata->dsi = dsi; > > + /* We never want the next bridge to *also* create a connector: */ > + flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR; > + > /* Attach the next bridge */ > ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge, > &pdata->bridge, flags);
Hi, On Wed, Aug 11, 2021 at 4:51 PM Rob Clark <robdclark@gmail.com> wrote: > > From: Rob Clark <robdclark@chromium.org> > > If we created our own connector because the driver does not support the > NO_CONNECTOR flag, we don't want the downstream bridge to *also* create > a connector. And if this driver did pass the NO_CONNECTOR flag (and we > supported that mode) this would change nothing. > > Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +++ > 1 file changed, 3 insertions(+) Reviewed-by: Douglas Anderson <dianders@chromium.org> Tested-by: Douglas Anderson <dianders@chromium.org> I'm going to apply this one to drm-misc/drm-misc-next and push since it's a fix and we're before -rc6, then I'll take a look at the later patches in the series. -Doug
On Thu, Aug 12, 2021 at 9:55 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Aug 11, 2021 at 4:51 PM Rob Clark <robdclark@gmail.com> wrote: > > > > From: Rob Clark <robdclark@chromium.org> > > > > If we created our own connector because the driver does not support the > > NO_CONNECTOR flag, we don't want the downstream bridge to *also* create > > a connector. And if this driver did pass the NO_CONNECTOR flag (and we > > supported that mode) this would change nothing. > > > > Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge") > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +++ > > 1 file changed, 3 insertions(+) > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Douglas Anderson <dianders@chromium.org> > > I'm going to apply this one to drm-misc/drm-misc-next and push since > it's a fix and we're before -rc6, then I'll take a look at the later > patches in the series. > Thanks.. this is the only one with some urgency, the rest can wait until next cycle. (And the bridge vs msm patches can land independently, I've tested the different possible combinations) BR, -R
On Thu, Aug 12, 2021 at 11:44 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Rob, > > Thank you for the patch. > > On Wed, Aug 11, 2021 at 04:52:49PM -0700, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > For the brave new world of bridges not creating their own connectors, we > > need to implement the max clock limitation via bridge->mode_valid() > > instead of connector->mode_valid(). > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index 5d3b30b2f547..38dcc49eccaf 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -595,6 +595,15 @@ static struct auxiliary_driver ti_sn_aux_driver = { > > .id_table = ti_sn_aux_id_table, > > }; > > > > +static enum drm_mode_status check_mode(const struct drm_display_mode *mode) > > +{ > > + /* maximum supported resolution is 4K at 60 fps */ > > + if (mode->clock > 594000) > > + return MODE_CLOCK_HIGH; > > + > > + return MODE_OK; > > +} > > + > > /* ----------------------------------------------------------------------------- > > * DRM Connector Operations > > */ > > @@ -616,11 +625,7 @@ static enum drm_mode_status > > ti_sn_bridge_connector_mode_valid(struct drm_connector *connector, > > struct drm_display_mode *mode) > > { > > - /* maximum supported resolution is 4K at 60 fps */ > > - if (mode->clock > 594000) > > - return MODE_CLOCK_HIGH; > > - > > - return MODE_OK; > > + return check_mode(mode); > > Do we need to implement the connector .mode_valid() operation, given > that the bridge is linked in the chain ? My understanding is that we need to keep it for display drivers that are not converted to NO_CONNECTOR.. But AFAIK snapdragon is the only upstream user of this bridge, so after the drm/msm/dsi patch lands we could probably garbage collect the connector support. BR, -R > > } > > > > static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = { > > @@ -763,6 +768,14 @@ static void ti_sn_bridge_detach(struct drm_bridge *bridge) > > drm_dp_aux_unregister(&bridge_to_ti_sn65dsi86(bridge)->aux); > > } > > > > +static enum drm_mode_status > > +ti_sn_bridge_mode_valid(struct drm_bridge *bridge, > > + const struct drm_display_info *info, > > + const struct drm_display_mode *mode) > > +{ > > + return check_mode(mode); > > +} > > + > > static void ti_sn_bridge_disable(struct drm_bridge *bridge) > > { > > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > @@ -1118,6 +1131,7 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > > static const struct drm_bridge_funcs ti_sn_bridge_funcs = { > > .attach = ti_sn_bridge_attach, > > .detach = ti_sn_bridge_detach, > > + .mode_valid = ti_sn_bridge_mode_valid, > > .pre_enable = ti_sn_bridge_pre_enable, > > .enable = ti_sn_bridge_enable, > > .disable = ti_sn_bridge_disable, > > -- > Regards, > > Laurent Pinchart
Hi Rob, On Thu, Aug 12, 2021 at 12:09:12PM -0700, Rob Clark wrote: > On Thu, Aug 12, 2021 at 11:44 AM Laurent Pinchart wrote: > > On Wed, Aug 11, 2021 at 04:52:49PM -0700, Rob Clark wrote: > > > From: Rob Clark <robdclark@chromium.org> > > > > > > For the brave new world of bridges not creating their own connectors, we > > > need to implement the max clock limitation via bridge->mode_valid() > > > instead of connector->mode_valid(). > > > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > > --- > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 24 +++++++++++++++++++----- > > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > index 5d3b30b2f547..38dcc49eccaf 100644 > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > @@ -595,6 +595,15 @@ static struct auxiliary_driver ti_sn_aux_driver = { > > > .id_table = ti_sn_aux_id_table, > > > }; > > > > > > +static enum drm_mode_status check_mode(const struct drm_display_mode *mode) > > > +{ > > > + /* maximum supported resolution is 4K at 60 fps */ > > > + if (mode->clock > 594000) > > > + return MODE_CLOCK_HIGH; > > > + > > > + return MODE_OK; > > > +} > > > + > > > /* ----------------------------------------------------------------------------- > > > * DRM Connector Operations > > > */ > > > @@ -616,11 +625,7 @@ static enum drm_mode_status > > > ti_sn_bridge_connector_mode_valid(struct drm_connector *connector, > > > struct drm_display_mode *mode) > > > { > > > - /* maximum supported resolution is 4K at 60 fps */ > > > - if (mode->clock > 594000) > > > - return MODE_CLOCK_HIGH; > > > - > > > - return MODE_OK; > > > + return check_mode(mode); > > > > Do we need to implement the connector .mode_valid() operation, given > > that the bridge is linked in the chain ? > > My understanding is that we need to keep it for display drivers that > are not converted to NO_CONNECTOR.. > > But AFAIK snapdragon is the only upstream user of this bridge, so > after the drm/msm/dsi patch lands we could probably garbage collect > the connector support. Even in the !NO_CONNECTOR case, the bridge will still be linked in the chain, so the atomic helpers should call the bridge .mode_valid() in addition to the connector .mode_valid(). I think the connector operation is redundant. > > > } > > > > > > static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = { > > > @@ -763,6 +768,14 @@ static void ti_sn_bridge_detach(struct drm_bridge *bridge) > > > drm_dp_aux_unregister(&bridge_to_ti_sn65dsi86(bridge)->aux); > > > } > > > > > > +static enum drm_mode_status > > > +ti_sn_bridge_mode_valid(struct drm_bridge *bridge, > > > + const struct drm_display_info *info, > > > + const struct drm_display_mode *mode) > > > +{ > > > + return check_mode(mode); > > > +} > > > + > > > static void ti_sn_bridge_disable(struct drm_bridge *bridge) > > > { > > > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > > @@ -1118,6 +1131,7 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > > > static const struct drm_bridge_funcs ti_sn_bridge_funcs = { > > > .attach = ti_sn_bridge_attach, > > > .detach = ti_sn_bridge_detach, > > > + .mode_valid = ti_sn_bridge_mode_valid, > > > .pre_enable = ti_sn_bridge_pre_enable, > > > .enable = ti_sn_bridge_enable, > > > .disable = ti_sn_bridge_disable, -- Regards, Laurent Pinchart
From: Rob Clark <robdclark@chromium.org> The first patch fixes breakage in drm-next for the ti eDP bridge (which is used on nearly all the snapdragon windows laptops and chromebooks). The second add drm/msm NO_CONNECTOR support, and the final two add NO_CONNECTOR support to the ti eDP bridge. Would be nice to get at least the first one into drm-next kinda soonish since at the moment a lot of things are broken. Rob Clark (4): drm/bridge: ti-sn65dsi86: Avoid creating multiple connectors drm/msm/dsi: Support NO_CONNECTOR bridges drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid() drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support drivers/gpu/drm/bridge/ti-sn65dsi86.c | 61 ++++++++++++++++++++++----- drivers/gpu/drm/msm/Kconfig | 2 + drivers/gpu/drm/msm/dsi/dsi_manager.c | 41 ++++++++++++------ 3 files changed, 81 insertions(+), 23 deletions(-)