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

[Doug Anderson]

Hi,

On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham
<kieran.bingham+renesas at ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart+renesas at 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 at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas at 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/