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(-)
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(+)
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);
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
From: Rob Clark robdclark@chromium.org
For now, since we have a mix of bridges which support this flag, which which do *not* support this flag, or work both ways, try it once with NO_CONNECTOR and then fall back to the old way if that doesn't work. Eventually we can drop the fallback path.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/msm/Kconfig | 2 ++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 41 ++++++++++++++++++--------- 2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig index e9c6af78b1d7..36e5ba3ccc28 100644 --- a/drivers/gpu/drm/msm/Kconfig +++ b/drivers/gpu/drm/msm/Kconfig @@ -14,6 +14,8 @@ config DRM_MSM select REGULATOR select DRM_KMS_HELPER select DRM_PANEL + select DRM_BRIDGE + select DRM_PANEL_BRIDGE select DRM_SCHED select SHMEM select TMPFS diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index c41d39f5b7cf..1fd1cf93abbf 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -3,6 +3,8 @@ * Copyright (c) 2015, The Linux Foundation. All rights reserved. */
+#include "drm/drm_bridge_connector.h" + #include "msm_kms.h" #include "dsi.h"
@@ -690,8 +692,7 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id) struct drm_device *dev = msm_dsi->dev; struct drm_encoder *encoder; struct drm_bridge *int_bridge, *ext_bridge; - struct drm_connector *connector; - struct list_head *connector_list; + int ret;
int_bridge = msm_dsi->bridge; ext_bridge = msm_dsi->external_bridge = @@ -699,22 +700,36 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
encoder = msm_dsi->encoder;
- /* link the internal dsi bridge to the external bridge */ - drm_bridge_attach(encoder, ext_bridge, int_bridge, 0); - /* - * we need the drm_connector created by the external bridge - * driver (or someone else) to feed it to our driver's - * priv->connector[] list, mainly for msm_fbdev_init() + * Try first to create the bridge without it creating it's own + * connector.. currently some bridges support this, and others + * do not (and some support both modes) */ - connector_list = &dev->mode_config.connector_list; + ret = drm_bridge_attach(encoder, ext_bridge, int_bridge, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (ret == -EINVAL) { + struct drm_connector *connector; + struct list_head *connector_list; + + /* link the internal dsi bridge to the external bridge */ + drm_bridge_attach(encoder, ext_bridge, int_bridge, 0); + + /* + * we need the drm_connector created by the external bridge + * driver (or someone else) to feed it to our driver's + * priv->connector[] list, mainly for msm_fbdev_init() + */ + connector_list = &dev->mode_config.connector_list; + + list_for_each_entry(connector, connector_list, head) { + if (drm_connector_has_possible_encoder(connector, encoder)) + return connector; + }
- list_for_each_entry(connector, connector_list, head) { - if (drm_connector_has_possible_encoder(connector, encoder)) - return connector; + return ERR_PTR(-ENODEV); }
- return ERR_PTR(-ENODEV); + return drm_bridge_connector_init(dev, encoder); }
void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
Hi Rob
Thank you for the patch.
On Wed, Aug 11, 2021 at 04:52:48PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
For now, since we have a mix of bridges which support this flag, which which do *not* support this flag, or work both ways, try it once with NO_CONNECTOR and then fall back to the old way if that doesn't work. Eventually we can drop the fallback path.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/msm/Kconfig | 2 ++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 41 ++++++++++++++++++--------- 2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig index e9c6af78b1d7..36e5ba3ccc28 100644 --- a/drivers/gpu/drm/msm/Kconfig +++ b/drivers/gpu/drm/msm/Kconfig @@ -14,6 +14,8 @@ config DRM_MSM select REGULATOR select DRM_KMS_HELPER select DRM_PANEL
- select DRM_BRIDGE
- select DRM_PANEL_BRIDGE select DRM_SCHED select SHMEM select TMPFS
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index c41d39f5b7cf..1fd1cf93abbf 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -3,6 +3,8 @@
- Copyright (c) 2015, The Linux Foundation. All rights reserved.
*/
+#include "drm/drm_bridge_connector.h"
#include "msm_kms.h" #include "dsi.h"
@@ -690,8 +692,7 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id) struct drm_device *dev = msm_dsi->dev; struct drm_encoder *encoder; struct drm_bridge *int_bridge, *ext_bridge;
- struct drm_connector *connector;
- struct list_head *connector_list;
int ret;
int_bridge = msm_dsi->bridge; ext_bridge = msm_dsi->external_bridge =
@@ -699,22 +700,36 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
encoder = msm_dsi->encoder;
- /* link the internal dsi bridge to the external bridge */
- drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
- /*
* we need the drm_connector created by the external bridge* driver (or someone else) to feed it to our driver's* priv->connector[] list, mainly for msm_fbdev_init()
* Try first to create the bridge without it creating it's own
s/it's/its/
* connector.. currently some bridges support this, and others* do not (and some support both modes)
- connector_list = &dev->mode_config.connector_list;
- ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
Should all this be moved one layer up, to the code that attaches to the mem_dsi->bridge ? I suppose we can start here, but as part of a global move to bridges and DRM_BRIDGE_ATTACH_NO_CONNECTOR, I think the top-level would make more sense in the long term.
If you want to start here,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- if (ret == -EINVAL) {
struct drm_connector *connector;struct list_head *connector_list;/* link the internal dsi bridge to the external bridge */drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);/** we need the drm_connector created by the external bridge* driver (or someone else) to feed it to our driver's* priv->connector[] list, mainly for msm_fbdev_init()*/connector_list = &dev->mode_config.connector_list;list_for_each_entry(connector, connector_list, head) {if (drm_connector_has_possible_encoder(connector, encoder))return connector;}
- list_for_each_entry(connector, connector_list, head) {
if (drm_connector_has_possible_encoder(connector, encoder))return connector;
return ERR_PTR(-ENODEV);
- return ERR_PTR(-ENODEV);
- return drm_bridge_connector_init(dev, encoder);
}
void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
Hi Rob,
On Wed, Aug 11, 2021 at 04:52:48PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
For now, since we have a mix of bridges which support this flag, which which do *not* support this flag, or work both ways, try it once with NO_CONNECTOR and then fall back to the old way if that doesn't work. Eventually we can drop the fallback path.
Which bridges are missing support for the NO_CONNECTOR flags that you are using?
Background for the question is that if you are using one of the bridges missing a conversion then we could do the conversion and maybe you could help with some tests.
We in the above sentence is likely me, so it may take some weeks, but known there is a chance for testing is a good motivator.
Sam
On Thu, Aug 12, 2021 at 10:31 AM Sam Ravnborg sam@ravnborg.org wrote:
Hi Rob,
On Wed, Aug 11, 2021 at 04:52:48PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
For now, since we have a mix of bridges which support this flag, which which do *not* support this flag, or work both ways, try it once with NO_CONNECTOR and then fall back to the old way if that doesn't work. Eventually we can drop the fallback path.
Which bridges are missing support for the NO_CONNECTOR flags that you are using?
Currently (as far as things that I am aware of) it is just ti-sn65dsi86, which the last two patches in this series convert.
But who knows what someone somewhere decides to build ;-)
BR, -R
Background for the question is that if you are using one of the bridges missing a conversion then we could do the conversion and maybe you could help with some tests.
We in the above sentence is likely me, so it may take some weeks, but known there is a chance for testing is a good motivator.
Sam
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); }
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,
Hi,
On Wed, Aug 11, 2021 at 4:51 PM Rob Clark robdclark@gmail.com 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(-)
This looks good to me. I'll plan to land this together with the next patch into drm-misc-next sometime next week unless someone beats me to it.
Reviewed-by: Douglas Anderson dianders@chromium.org
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 ?
}
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,
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,
From: Rob Clark robdclark@chromium.org
Slightly awkward to fish out the display_info when we aren't creating own connector. But I don't see an obvious better way.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 34 +++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 38dcc49eccaf..dc8112bab3d3 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -693,9 +693,11 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, return ret; }
- ret = ti_sn_bridge_connector_init(pdata); - if (ret < 0) - goto err_conn_init; + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { + ret = ti_sn_bridge_connector_init(pdata); + if (ret < 0) + goto err_conn_init; + }
/* * TODO: ideally finding host resource and dsi dev registration needs @@ -757,7 +759,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, err_dsi_attach: mipi_dsi_device_unregister(dsi); err_dsi_host: - drm_connector_cleanup(&pdata->connector); + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) + drm_connector_cleanup(&pdata->connector); err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; @@ -806,9 +809,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); }
+/* + * Find the connector and fish out the bpc from display_info. It would + * be nice if we could get this instead from drm_bridge_state, but that + * doesn't yet appear to be the case. + */ static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) { - if (pdata->connector.display_info.bpc <= 6) + struct drm_bridge *bridge = &pdata->bridge; + struct drm_connector_list_iter conn_iter; + struct drm_connector *connector; + unsigned bpc = 0; + + drm_connector_list_iter_begin(bridge->dev, &conn_iter); + drm_for_each_connector_iter(connector, &conn_iter) { + if (drm_connector_has_possible_encoder(connector, bridge->encoder)) { + bpc = connector->display_info.bpc; + break; + } + } + drm_connector_list_iter_end(&conn_iter); + + WARN_ON(bpc == 0); + + if (bpc <= 6) return 18; else return 24;
Hi,
On Wed, Aug 11, 2021 at 4:51 PM Rob Clark robdclark@gmail.com wrote:
From: Rob Clark robdclark@chromium.org
Slightly awkward to fish out the display_info when we aren't creating own connector. But I don't see an obvious better way.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 34 +++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 38dcc49eccaf..dc8112bab3d3 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -693,9 +693,11 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, return ret; }
ret = ti_sn_bridge_connector_init(pdata);if (ret < 0)goto err_conn_init;
if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {ret = ti_sn_bridge_connector_init(pdata);if (ret < 0)goto err_conn_init;} /* * TODO: ideally finding host resource and dsi dev registration needs@@ -757,7 +759,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, err_dsi_attach: mipi_dsi_device_unregister(dsi); err_dsi_host:
drm_connector_cleanup(&pdata->connector);
if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))drm_connector_cleanup(&pdata->connector);err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; @@ -806,9 +809,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); }
+/*
- Find the connector and fish out the bpc from display_info. It would
- be nice if we could get this instead from drm_bridge_state, but that
- doesn't yet appear to be the case.
- */
static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) {
if (pdata->connector.display_info.bpc <= 6)
struct drm_bridge *bridge = &pdata->bridge;struct drm_connector_list_iter conn_iter;struct drm_connector *connector;unsigned bpc = 0;drm_connector_list_iter_begin(bridge->dev, &conn_iter);drm_for_each_connector_iter(connector, &conn_iter) {if (drm_connector_has_possible_encoder(connector, bridge->encoder)) {bpc = connector->display_info.bpc;break;}}drm_connector_list_iter_end(&conn_iter);
This looks reasonable to me. I'll plan to apply it to drm-misc-next sometime next week to give Laurent a chance to comment on whether this causes any problems with his planned support for full DP using this bridge chip. IIUC that means it'll hit mainline 1 rev later, but as per IRC comments this should be fine.
Reviewed-by: Douglas Anderson dianders@chromium.org
-Doug
Hi Rob,
Thank you for the patch.
On Wed, Aug 11, 2021 at 04:52:50PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Slightly awkward to fish out the display_info when we aren't creating own connector. But I don't see an obvious better way.
We need a bit more than this, to support the NO_CONNECTOR case, the bridge has to implement a few extra operations, and set the bridge .ops field. I've submitted two patches to do so a while ago:
- [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations ([1]) - [RFC PATCH 09/11] drm/bridge: ti-sn65dsi86: Make connector creation optional ([2])
The second patch is similar to the first half of this patch, but misses the cleanup code. I'll try to rebase this and resubmit, but it may take a bit of time.
[1] https://lore.kernel.org/dri-devel/20210322030128.2283-9-laurent.pinchart+ren... [2] https://lore.kernel.org/dri-devel/20210322030128.2283-10-laurent.pinchart+re...
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 34 +++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 38dcc49eccaf..dc8112bab3d3 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -693,9 +693,11 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, return ret; }
- ret = ti_sn_bridge_connector_init(pdata);
- if (ret < 0)
goto err_conn_init;
if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
ret = ti_sn_bridge_connector_init(pdata);if (ret < 0)goto err_conn_init;}
/*
- TODO: ideally finding host resource and dsi dev registration needs
@@ -757,7 +759,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, err_dsi_attach: mipi_dsi_device_unregister(dsi); err_dsi_host:
- drm_connector_cleanup(&pdata->connector);
- if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
drm_connector_cleanup(&pdata->connector);err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; @@ -806,9 +809,30 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); }
+/*
- Find the connector and fish out the bpc from display_info. It would
- be nice if we could get this instead from drm_bridge_state, but that
- doesn't yet appear to be the case.
- */
This should be done with
struct drm_atomic_state *state = old_bridge_state->base.state; struct drm_connector *connector;
connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) {
- if (pdata->connector.display_info.bpc <= 6)
- struct drm_bridge *bridge = &pdata->bridge;
- struct drm_connector_list_iter conn_iter;
- struct drm_connector *connector;
- unsigned bpc = 0;
- drm_connector_list_iter_begin(bridge->dev, &conn_iter);
- drm_for_each_connector_iter(connector, &conn_iter) {
if (drm_connector_has_possible_encoder(connector, bridge->encoder)) {bpc = connector->display_info.bpc;break;}- }
- drm_connector_list_iter_end(&conn_iter);
- WARN_ON(bpc == 0);
- if (bpc <= 6) return 18; else return 24;
Laurent,
On Thu, Aug 12, 2021 at 12:26 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
Thank you for the patch.
On Wed, Aug 11, 2021 at 04:52:50PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Slightly awkward to fish out the display_info when we aren't creating own connector. But I don't see an obvious better way.
We need a bit more than this, to support the NO_CONNECTOR case, the bridge has to implement a few extra operations, and set the bridge .ops field. I've submitted two patches to do so a while ago:
- [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations ([1])
Rob asked me about this over IRC, so if he left it out and it's needed then it's my fault. However, I don't believe it's needed until your series making this bridge chip support full DP. For the the eDP case the bridge chip driver in ToT no longer queries the EDID itself. It simply provides an AUX bus to the panel driver and the panel driver queries the EDID. I think that means we don't need to add DRM_BRIDGE_OP_EDID, right?
I was also wondering if in the full DP case we should actually model the physical DP jack as a drm_bridge and have it work the same way. It would get probed via the DP AUX bus just like a panel. I seem to remember Stephen Boyd was talking about modeling the DP connector as a drm_bridge because it would allow us to handle the fact that some TCPC chips could only support HBR2 whereas others could support HBR3. Maybe it would end up being a fairly elegant solution?
- [RFC PATCH 09/11] drm/bridge: ti-sn65dsi86: Make connector creation optional ([2])
The second patch is similar to the first half of this patch, but misses the cleanup code. I'll try to rebase this and resubmit, but it may take a bit of time.
Whoops! You're right that Rob's patch won't work at all because we'll just hit the "Fix bridge driver to make connector optional!" case. I should have noticed that. :(
-Doug
On Thu, Aug 12, 2021 at 1:08 PM Doug Anderson dianders@chromium.org wrote:
Laurent,
On Thu, Aug 12, 2021 at 12:26 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
Thank you for the patch.
On Wed, Aug 11, 2021 at 04:52:50PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Slightly awkward to fish out the display_info when we aren't creating own connector. But I don't see an obvious better way.
We need a bit more than this, to support the NO_CONNECTOR case, the bridge has to implement a few extra operations, and set the bridge .ops field. I've submitted two patches to do so a while ago:
- [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations ([1])
Rob asked me about this over IRC, so if he left it out and it's needed then it's my fault. However, I don't believe it's needed until your series making this bridge chip support full DP. For the the eDP case the bridge chip driver in ToT no longer queries the EDID itself. It simply provides an AUX bus to the panel driver and the panel driver queries the EDID. I think that means we don't need to add DRM_BRIDGE_OP_EDID, right?
I was also wondering if in the full DP case we should actually model the physical DP jack as a drm_bridge and have it work the same way. It would get probed via the DP AUX bus just like a panel. I seem to remember Stephen Boyd was talking about modeling the DP connector as a drm_bridge because it would allow us to handle the fact that some TCPC chips could only support HBR2 whereas others could support HBR3. Maybe it would end up being a fairly elegant solution?
- [RFC PATCH 09/11] drm/bridge: ti-sn65dsi86: Make connector creation optional ([2])
The second patch is similar to the first half of this patch, but misses the cleanup code. I'll try to rebase this and resubmit, but it may take a bit of time.
Whoops! You're right that Rob's patch won't work at all because we'll just hit the "Fix bridge driver to make connector optional!" case. I should have noticed that. :(
Yes, indeed.. once I fix that, I get no display..
Not sure if Laurent is still working on his series, otherwise I can try to figure out what bridge ops are missing
BR, -R
Hi Rob and Doug,
On Mon, Sep 20, 2021 at 11:32:02AM -0700, Rob Clark wrote:
On Thu, Aug 12, 2021 at 1:08 PM Doug Anderson wrote:
On Thu, Aug 12, 2021 at 12:26 PM Laurent Pinchart wrote:
On Wed, Aug 11, 2021 at 04:52:50PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Slightly awkward to fish out the display_info when we aren't creating own connector. But I don't see an obvious better way.
We need a bit more than this, to support the NO_CONNECTOR case, the bridge has to implement a few extra operations, and set the bridge .ops field. I've submitted two patches to do so a while ago:
- [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations ([1])
Rob asked me about this over IRC, so if he left it out and it's needed then it's my fault. However, I don't believe it's needed until your series making this bridge chip support full DP. For the the eDP case the bridge chip driver in ToT no longer queries the EDID itself. It simply provides an AUX bus to the panel driver and the panel driver queries the EDID. I think that means we don't need to add DRM_BRIDGE_OP_EDID, right?
That's right.
I was also wondering if in the full DP case we should actually model the physical DP jack as a drm_bridge and have it work the same way. It would get probed via the DP AUX bus just like a panel. I seem to remember Stephen Boyd was talking about modeling the DP connector as a drm_bridge because it would allow us to handle the fact that some TCPC chips could only support HBR2 whereas others could support HBR3. Maybe it would end up being a fairly elegant solution?
Physical connectors are already handled as bridges, see drivers/gpu/drm/bridge/display-connector.c. I however don't think it should handle EDID retrieval, because that's really not an operation implemented by the connector itself.
- [RFC PATCH 09/11] drm/bridge: ti-sn65dsi86: Make connector creation optional ([2])
The second patch is similar to the first half of this patch, but misses the cleanup code. I'll try to rebase this and resubmit, but it may take a bit of time.
Whoops! You're right that Rob's patch won't work at all because we'll just hit the "Fix bridge driver to make connector optional!" case. I should have noticed that. :(
Yes, indeed.. once I fix that, I get no display..
Not sure if Laurent is still working on his series, otherwise I can try to figure out what bridge ops are missing
I am, but too slowly. I don't mind fast-tracking the changes you need though.
dri-devel@lists.freedesktop.org
-
Doug Anderson -
Laurent Pinchart -
Rob Clark -
Sam Ravnborg -
Stephen Boyd