diff mbox series

[v2,2/4] drm/bridge: ti-sn65dsi86: Make connector creation optional

Message ID 20220307175955.363057-3-kieran.bingham+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors | expand

Commit Message

Kieran Bingham March 7, 2022, 5:59 p.m. UTC 
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Now that the driver supports the connector-related bridge operations,
make the connector creation optional. This enables usage of the
sn65dsi86 with the DRM bridge connector helper.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

---
Changes since v1:
 - None

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

Doug Anderson March 7, 2022, 11:21 p.m. UTC | #1 
Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Now that the driver supports the connector-related bridge operations,
> make the connector creation optional. This enables usage of the
> sn65dsi86 with the DRM bridge connector helper.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> ---
> Changes since v1:
>  - None
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)

Can you please CC me on this series next time you send it out? I was
pretty involved in previous reviews here. Luckily I got CCed on one of
the patches so I knew to look, but since that one is (probably)
getting dropped it'd be good to make sure I was on the others. It's
also pretty important to include +Sam Ravnborg since there's a lot of
overlap with his series [1]:


> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ffb6c04f6c46..29f5f7123ed9 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -745,11 +745,6 @@ static int  (struct drm_bridge *bridge,
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>         int ret;
>
> -       if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -               DRM_ERROR("Fix bridge driver to make connector optional!");
> -               return -EINVAL;
> -       }

That won't work without some other fixes, sorry!

The problem is that you'll break ti_sn_bridge_get_bpp(). That
absolutely relies on having our own connector so you need to fix that
_before_ making the connector optional.

Rob Clark made an attempt on this [2] but Laurent didn't like the
solution he came up with. Sam's series that I mentioned [1] also made
an attempt at this, specifically in patch 5 ("drm/bridge:
ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series.
Unfortunately, it didn't work because the "output_bus_cfg.format"
wasn't set to anything. Personally I wouldn't mind Rob's solution as a
stopgap if Laurent removes his NAK. Then we're not stuck while someone
tries to find time to fix this correctly.

One last problem here is that your patch doesn't actually apply to
drm-misc/drm-misc-next, which is likely where it would land. I believe
it conflicts with my recent commit e283820cbf80 ("drm/bridge:
ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your
series on mainline, but you should really be targeting drm-misc-next.

-Doug

[1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/
[2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/
Kieran Bingham March 8, 2022, 2:07 p.m. UTC | #2 
Hi Doug,

Quoting Doug Anderson (2022-03-07 23:21:28)
> Hi,
> 
> On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Now that the driver supports the connector-related bridge operations,
> > make the connector creation optional. This enables usage of the
> > sn65dsi86 with the DRM bridge connector helper.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> > ---
> > Changes since v1:
> >  - None
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> Can you please CC me on this series next time you send it out? I was
> pretty involved in previous reviews here. Luckily I got CCed on one of
> the patches so I knew to look, but since that one is (probably)
> getting dropped it'd be good to make sure I was on the others. It's
> also pretty important to include +Sam Ravnborg since there's a lot of
> overlap with his series [1]:

Absolutely - I was assuming you were the main target for reviews. I'm
sorry - I also assumed get_maintainer.pl had / would add you - and I
didn't check getting the patches out last night.

I'll be sure to manually add you.

> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index ffb6c04f6c46..29f5f7123ed9 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -745,11 +745,6 @@ static int  (struct drm_bridge *bridge,
> >         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >         int ret;
> >
> > -       if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -               DRM_ERROR("Fix bridge driver to make connector optional!");
> > -               return -EINVAL;
> > -       }
> 
> That won't work without some other fixes, sorry!

Ok ;-)

To be clear, my goal here has been to get the IRQ HPD working - so
my main focus is there. I guess I now have to handle custodianship of
these dependencies somehow too though.


> The problem is that you'll break ti_sn_bridge_get_bpp(). That
> absolutely relies on having our own connector so you need to fix that
> _before_ making the connector optional.
> 
> Rob Clark made an attempt on this [2] but Laurent didn't like the
> solution he came up with. Sam's series that I mentioned [1] also made
> an attempt at this, specifically in patch 5 ("drm/bridge:
> ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series.
> Unfortunately, it didn't work because the "output_bus_cfg.format"
> wasn't set to anything. Personally I wouldn't mind Rob's solution as a
> stopgap if Laurent removes his NAK. Then we're not stuck while someone
> tries to find time to fix this correctly.

Ok - all of that is going to take some time for me to review and
process.
 
> One last problem here is that your patch doesn't actually apply to
> drm-misc/drm-misc-next, which is likely where it would land. I believe
> it conflicts with my recent commit e283820cbf80 ("drm/bridge:
> ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your
> series on mainline, but you should really be targeting drm-misc-next.

Ah sorry - I thought I had merged in drm-misc-next, but it was a week
ago or so so I guess I'm already outdated.

I'll cleanup for a new base now.


> -Doug
> 
> [1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/
> [2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ffb6c04f6c46..29f5f7123ed9 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -745,11 +745,6 @@  static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	int ret;
 
-	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) {
@@ -757,12 +752,14 @@  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;
 
-	/* We never want the next bridge to *also* create a connector: */
-	flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+		/* We never want the next bridge to *also* create a connector: */
+		flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+	}
 
 	/* Attach the next bridge */
 	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,