From: Rob Clark <[email protected]>
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(-)
--
2.31.1
From: Rob Clark <[email protected]>
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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)
--
2.31.1
From: Rob Clark <[email protected]>
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 <[email protected]>
---
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;
--
2.31.1
From: Rob Clark <[email protected]>
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 <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
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,
--
2.31.1
Hi,
On Mon, Sep 20, 2021 at 3:53 PM Rob Clark <[email protected]> wrote:
>
> From: Rob Clark <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> 1 file changed, 29 insertions(+), 10 deletions(-)
This seems fine to me:
Reviewed-by: Douglas Anderson <[email protected]>
...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 <[email protected]> wrote:
>
> Hi,
>
> On Mon, Sep 20, 2021 at 3:53 PM Rob Clark <[email protected]> wrote:
> >
> > From: Rob Clark <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> > 1 file changed, 29 insertions(+), 10 deletions(-)
>
> This seems fine to me:
>
> Reviewed-by: Douglas Anderson <[email protected]>
>
> ...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 Rob,
Thank you for the patch.
On Mon, Sep 20, 2021 at 03:57:59PM -0700, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> 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,
--
Regards,
Laurent Pinchart
Hi Rob,
Thank you for the patch.
On Mon, Sep 20, 2021 at 03:58:00PM -0700, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
--
Regards,
Laurent Pinchart
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 <[email protected]> wrote:
> >
> > Hi,
> >
> > On Mon, Sep 20, 2021 at 3:53 PM Rob Clark <[email protected]> wrote:
> > >
> > > From: Rob Clark <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 ++++++++++++++++++++-------
> > > 1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > This seems fine to me:
> >
> > Reviewed-by: Douglas Anderson <[email protected]>
> >
> > ...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
On Wed, Sep 22, 2021 at 5:44 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 03:58:00PM -0700, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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;
--
Regards,
Laurent Pinchart
On 21/09/2021 01:57, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
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)
>
--
With best wishes
Dmitry
On Fri, Oct 1, 2021 at 10:28 AM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 21/09/2021 01:57, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > 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 <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> 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_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)
> >
>
>
> --
> With best wishes
> Dmitry
Hi,
On Thu, Sep 23, 2021 at 7:26 PM Laurent Pinchart
<[email protected]> 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,
On Wed, Sep 22, 2021 at 5:31 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 03:57:59PM -0700, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > 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 <[email protected]>
> > Reviewed-by: Douglas Anderson <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
>
> > ---
> > 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
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.
--
Regards,
Laurent Pinchart