Mailing List Archive

[PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
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;
--
2.31.1
Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support [ In reply to ]
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
Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support [ In reply to ]
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
Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support [ In reply to ]
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;

--
Regards,

Laurent Pinchart
Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support [ In reply to ]
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
Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support [ In reply to ]
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
Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support [ In reply to ]
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;

--
Regards,

Laurent Pinchart
Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support [ In reply to ]
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
Re: [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support [ In reply to ]
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