From: Rob Clark robdclark@chromium.org
Respin of https://www.spinics.net/lists/linux-arm-msm/msg92182.html with the remaining 3 patches that are not yet merged.
At the end of this series, but drm/msm and ti-sn65dsi86 work in both combinations, so the two bridge patches can be merged indepdendently of the msm/dsi patch.
The last patch has some conficts with https://www.spinics.net/lists/linux-arm-msm/msg93731.html but I already have a rebased variant of it depending on which order patches land.
Rob Clark (3): 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 | 64 ++++++++++++++++++--------- drivers/gpu/drm/msm/Kconfig | 2 + drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 +++++++++++++++------ 3 files changed, 81 insertions(+), 35 deletions(-)
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.
v2: Add missing drm_connector_attach_encoder() so display actually comes up when the bridge properly handles the NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/msm/Kconfig | 2 ++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ++++++++++++++++++++------- 2 files changed, 39 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..e25877073d31 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"
@@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id) { struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); struct drm_device *dev = msm_dsi->dev; + struct drm_connector *connector; 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 +701,44 @@ 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 its 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); + } + + connector = drm_bridge_connector_init(dev, encoder); + if (IS_ERR(connector)) { + DRM_ERROR("Unable to create bridge connector\n"); + return ERR_CAST(connector); }
- return ERR_PTR(-ENODEV); + drm_connector_attach_encoder(connector, encoder); + + return connector; }
void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
On 21/09/2021 01:57, 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.
v2: Add missing drm_connector_attach_encoder() so display actually comes up when the bridge properly handles the NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
I think this patch can go through the drm/msm, while two other patches would need to through the drm-misc. Is it correct?
drivers/gpu/drm/msm/Kconfig | 2 ++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ++++++++++++++++++++------- 2 files changed, 39 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..e25877073d31 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"
@@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id) { struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); struct drm_device *dev = msm_dsi->dev;
- struct drm_connector *connector; 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 +701,44 @@ 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 its 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);- }
- connector = drm_bridge_connector_init(dev, encoder);
- if (IS_ERR(connector)) {
DRM_ERROR("Unable to create bridge connector\n");return ERR_CAST(connector);
- return ERR_PTR(-ENODEV);
drm_connector_attach_encoder(connector, encoder);
return connector; }
void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
On Fri, Oct 1, 2021 at 10:28 AM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 21/09/2021 01:57, 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.
v2: Add missing drm_connector_attach_encoder() so display actually comes up when the bridge properly handles the NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
I think this patch can go through the drm/msm, while two other patches would need to through the drm-misc. Is it correct?
Correct, I made sure things worked in either order (ie. with msm patch but without bridge patches, and visa versa), so they can land through different trees
BR, -R
drivers/gpu/drm/msm/Kconfig | 2 ++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ++++++++++++++++++++------- 2 files changed, 39 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_BRIDGEselect DRM_PANEL_BRIDGE select DRM_SCHED select SHMEM select TMPFSdiff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index c41d39f5b7cf..e25877073d31 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"
@@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id) { struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); struct drm_device *dev = msm_dsi->dev;
struct drm_connector *connector; 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 +701,44 @@ 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 its 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);}connector = drm_bridge_connector_init(dev, encoder);if (IS_ERR(connector)) {DRM_ERROR("Unable to create bridge connector\n");return ERR_CAST(connector); }
return ERR_PTR(-ENODEV);
drm_connector_attach_encoder(connector, encoder);return connector;}
void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
-- With best wishes Dmitry
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().
v2: Drop unneeded connector->mode_valid()
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 41d48a393e7f..6154bed0af5b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -615,20 +615,8 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) return drm_bridge_get_modes(pdata->next_bridge, connector); }
-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; -} - static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = { .get_modes = ti_sn_bridge_connector_get_modes, - .mode_valid = ti_sn_bridge_connector_mode_valid, };
static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = { @@ -766,6 +754,18 @@ 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) +{ + /* maximum supported resolution is 4K at 60 fps */ + if (mode->clock > 594000) + return MODE_CLOCK_HIGH; + + return MODE_OK; +} + static void ti_sn_bridge_disable(struct drm_bridge *bridge) { struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); @@ -1127,6 +1127,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 Rob,
Thank you for the patch.
On Mon, Sep 20, 2021 at 03:57:59PM -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().
v2: Drop unneeded connector->mode_valid()
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 41d48a393e7f..6154bed0af5b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -615,20 +615,8 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) return drm_bridge_get_modes(pdata->next_bridge, connector); }
-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;
-}
static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = { .get_modes = ti_sn_bridge_connector_get_modes,
- .mode_valid = ti_sn_bridge_connector_mode_valid,
};
static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = { @@ -766,6 +754,18 @@ 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)+{
- /* maximum supported resolution is 4K at 60 fps */
- if (mode->clock > 594000)
return MODE_CLOCK_HIGH;- return MODE_OK;
+}
static void ti_sn_bridge_disable(struct drm_bridge *bridge) { struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); @@ -1127,6 +1127,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, Sep 22, 2021 at 5:31 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
Thank you for the patch.
On Mon, Sep 20, 2021 at 03:57:59PM -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().
v2: Drop unneeded connector->mode_valid()
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
There's no reason to wait on this patch. Landed to drm-misc-next.
77d40e0176a5 drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()
-Doug
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.
v2: Remove error return with NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 6154bed0af5b..94c94cc8a4d8 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, .node = NULL, };
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - pdata->aux.drm_dev = bridge->dev; ret = drm_dp_aux_register(&pdata->aux); if (ret < 0) { @@ -679,9 +674,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 @@ -743,7 +740,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; @@ -792,9 +790,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 Mon, Sep 20, 2021 at 3:53 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.
v2: Remove error return with NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)
This seems fine to me:
Reviewed-by: Douglas Anderson dianders@chromium.org
...if you would like me to apply patch #2 / #3 to drm-misc-next then please yell.
-Doug
On Tue, Sep 21, 2021 at 3:20 PM Doug Anderson dianders@chromium.org wrote:
Hi,
On Mon, Sep 20, 2021 at 3:53 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.
v2: Remove error return with NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)
This seems fine to me:
Reviewed-by: Douglas Anderson dianders@chromium.org
...if you would like me to apply patch #2 / #3 to drm-misc-next then please yell.
Thanks.. I think we can give it a few days for Laurent to have a look.
This will conflict some with Maxime's bridge vs dsi-host ordering series.. not sure how close that one is to the finish line, but I can rebase either patch on top of the other depending on which order they are applied
BR, -R
Hi,
On Tue, Sep 21, 2021 at 03:42:05PM -0700, Rob Clark wrote:
On Tue, Sep 21, 2021 at 3:20 PM Doug Anderson dianders@chromium.org wrote:
Hi,
On Mon, Sep 20, 2021 at 3:53 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.
v2: Remove error return with NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)
This seems fine to me:
Reviewed-by: Douglas Anderson dianders@chromium.org
...if you would like me to apply patch #2 / #3 to drm-misc-next then please yell.
Thanks.. I think we can give it a few days for Laurent to have a look.
This will conflict some with Maxime's bridge vs dsi-host ordering series.. not sure how close that one is to the finish line, but I can rebase either patch on top of the other depending on which order they are applied
It's probably going to take a bit of time to get merged, so don't worry about this series and just go ahead, I'll rebase it on top of yours if needed.
Maxime
Hi Rob,
Thank you for the patch.
On Mon, Sep 20, 2021 at 03:58:00PM -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.
v2: Remove error return with NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 6154bed0af5b..94c94cc8a4d8 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, .node = NULL, };
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
DRM_ERROR("Fix bridge driver to make connector optional!");return -EINVAL;- }
- pdata->aux.drm_dev = bridge->dev; ret = drm_dp_aux_register(&pdata->aux); if (ret < 0) {
@@ -679,9 +674,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
@@ -743,7 +740,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);
I wonder if we actually need this. The connector gets attached to the encoder, won't it be destroyed by the DRM core in the error path ?
err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; @@ -792,9 +790,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.
You already have a bus format in the bridge state, from which you can derive the bpp. Could you give it a try ?
- */
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;
On Wed, Sep 22, 2021 at 5:44 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
Thank you for the patch.
On Mon, Sep 20, 2021 at 03:58:00PM -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.
v2: Remove error return with NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 6154bed0af5b..94c94cc8a4d8 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, .node = NULL, };
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {DRM_ERROR("Fix bridge driver to make connector optional!");return -EINVAL;}pdata->aux.drm_dev = bridge->dev; ret = drm_dp_aux_register(&pdata->aux); if (ret < 0) {@@ -679,9 +674,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@@ -743,7 +740,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);I wonder if we actually need this. The connector gets attached to the encoder, won't it be destroyed by the DRM core in the error path ?
This does not appear to be the case, we leak the connector if I remove this (and add a hack to trigger the error path)
err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; @@ -792,9 +790,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.
You already have a bus format in the bridge state, from which you can derive the bpp. Could you give it a try ?
Possibly the bridge should be converted to ->atomic_enable(), etc.. I'll leave that for another time
BR, -R
- */
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;-- Regards,
Laurent Pinchart
Hi Rob,
On Thu, Sep 23, 2021 at 10:31:52AM -0700, Rob Clark wrote:
On Wed, Sep 22, 2021 at 5:44 PM Laurent Pinchart wrote:
On Mon, Sep 20, 2021 at 03:58:00PM -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.
v2: Remove error return with NO_CONNECTOR flag
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 6154bed0af5b..94c94cc8a4d8 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -667,11 +667,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, .node = NULL, };
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {DRM_ERROR("Fix bridge driver to make connector optional!");return -EINVAL;}pdata->aux.drm_dev = bridge->dev; ret = drm_dp_aux_register(&pdata->aux); if (ret < 0) {@@ -679,9 +674,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@@ -743,7 +740,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);I wonder if we actually need this. The connector gets attached to the encoder, won't it be destroyed by the DRM core in the error path ?
This does not appear to be the case, we leak the connector if I remove this (and add a hack to trigger the error path)
OK.
err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; @@ -792,9 +790,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.
You already have a bus format in the bridge state, from which you can derive the bpp. Could you give it a try ?
Possibly the bridge should be converted to ->atomic_enable(), etc.. I'll leave that for another time
It should be fairly straightforward, and would avoid the hack below.
- */
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 Thu, Sep 23, 2021 at 7:26 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; @@ -792,9 +790,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.
You already have a bus format in the bridge state, from which you can derive the bpp. Could you give it a try ?
Possibly the bridge should be converted to ->atomic_enable(), etc.. I'll leave that for another time
It should be fairly straightforward, and would avoid the hack below.
Given this point of controversy, my inclination is to wait and not apply this patch now. I don't think there's anything urgent here, right? Worst case eventually Laurent might pick it up in his patch series? At least we know it will work with the MSM driver once patch #1 lands. :-)
-Doug
Hi Doug,
On Fri, Oct 01, 2021 at 11:02:54AM -0700, Doug Anderson wrote:
On Thu, Sep 23, 2021 at 7:26 PM Laurent Pinchart wrote:
err_conn_init: drm_dp_aux_unregister(&pdata->aux); return ret; @@ -792,9 +790,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.
You already have a bus format in the bridge state, from which you can derive the bpp. Could you give it a try ?
Possibly the bridge should be converted to ->atomic_enable(), etc.. I'll leave that for another time
It should be fairly straightforward, and would avoid the hack below.
Given this point of controversy, my inclination is to wait and not apply this patch now. I don't think there's anything urgent here, right? Worst case eventually Laurent might pick it up in his patch series? At least we know it will work with the MSM driver once patch #1 lands. :-)
I've recorded the task for my upcoming work on the ti-sn65dsi86 driver.
dri-devel@lists.freedesktop.org
-
Dmitry Baryshkov -
Doug Anderson -
Laurent Pinchart -
Maxime Ripard -
Rob Clark