Re: [PATCH v4 02/10] drm/bridge: add common api for inno hdmi

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Keith,

On Wed, May 22, 2024 at 05:58:00AM +0000, Keith Zhao wrote:
> Hi Alex, Laurent:
> 
> I want to make as few changes as possible on the current basis, and add bridge_fun, 
> 
> On 2024年5月21日 23:42, Laurent Pinchart wrote:
> > On Tue, May 21, 2024 at 05:36:43PM +0200, Alex Bee wrote:
> > > Hi Keith,
> > >
> > > thanks a lot for working on this. See some general remarks below
> > >
> > > Am 21.05.24 um 12:58 schrieb keith:
> > > > Add INNO common api so that it can be used by vendor drivers which
> > > > implement vendor specific extensions to Innosilicon HDMI.
> > > >
> > > > Signed-off-by: keith <keith.zhao@starfivetech.com>
> > > > ---
> > > >   MAINTAINERS                                   |   2 +
> > > >   drivers/gpu/drm/bridge/Kconfig                |   2 +
> > > >   drivers/gpu/drm/bridge/Makefile               |   1 +
> > > >   drivers/gpu/drm/bridge/innosilicon/Kconfig    |   6 +
> > > >   drivers/gpu/drm/bridge/innosilicon/Makefile   |   2 +
> > > >   .../gpu/drm/bridge/innosilicon/inno-hdmi.c    | 587 ++++++++++++++++++
> > > >   .../gpu/drm/bridge/innosilicon/inno-hdmi.h    |  97 +++
> > > >   include/drm/bridge/inno_hdmi.h                |  69 ++
> > > >   8 files changed, 766 insertions(+)
> > > >   create mode 100644 drivers/gpu/drm/bridge/innosilicon/Kconfig
> > > >   create mode 100644 drivers/gpu/drm/bridge/innosilicon/Makefile
> > > >   create mode 100644 drivers/gpu/drm/bridge/innosilicon/inno-hdmi.c
> > > >   create mode 100644 drivers/gpu/drm/bridge/innosilicon/inno-hdmi.h
> > > >   create mode 100644 include/drm/bridge/inno_hdmi.h
> > > >
> > > ....
> > >
> > > > +	drm_encoder_helper_add(encoder, pdata->helper_private);
> > > > +
> > > > +	hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > > > +
> > > > +	drm_connector_helper_add(&hdmi->connector,
> > > > +				 &inno_hdmi_connector_helper_funcs);
> > > > +
> > > > +	drmm_connector_init(drm, &hdmi->connector,
> > > > +			    &inno_hdmi_connector_funcs,
> > > > +			    DRM_MODE_CONNECTOR_HDMIA,
> > > > +			    hdmi->ddc);
> > > > +
> > >
> > > I really don't want to anticipate bridge maintainer's feedback, but
> > > new bridge drivers must not contain connector creation. That must
> > > happen somewhere else.
> > 
> > You're absolutely right :-) Connector creation should be handled by the
> > drm_bridge_connector helper. The HDMI bridge driver should focus on the
> > HDMI bridge itself.
> 
> static int inno_bridge_attach(struct drm_bridge *bridge,
> 				 enum drm_bridge_attach_flags flags)
> {
> 	struct inno_hdmi *hdmi = bridge_to_inno(bridge);
> 
> 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> 		DRM_ERROR("Fix bridge driver to make connector optional!");
> 		return -EINVAL;
> 	}

This kind of code was added to existing bridge drivers when we
transitioned to the new model where bridges don't create the connectors,
because we couldn't fix all the existing bridges in one go. New bridges
must do the opposite. See imx8qxp-pixel-link.c for instance, it has this
code instead in its attach function:

	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
		DRM_DEV_ERROR(pl->dev,
			      "do not support creating a drm_connector\n");
		return -EINVAL;
	}

(I would personally drop the DRM_DEV_ERROR message)

> 	hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
> 
> 	drm_connector_helper_add(&hdmi->connector,
> 				 &inno_hdmi_connector_helper_funcs);
> 
> 	drmm_connector_init(drm, &hdmi->connector,
> 			    &inno_hdmi_connector_funcs,
> 			    DRM_MODE_CONNECTOR_HDMIA,
> 			    hdmi->ddc);
> 
> 	drm_connector_attach_encoder(&hdmi->connector, encoder);
> }
> 
> static const struct drm_bridge_funcs inno_bridge_attach = {
> 	.attach = inno_bridge_attach,
> };
> 
> Connector creation is handled by the drm_bridge_funcs ->attach.
> Is it ok?

No, the connector should be created by the display controller driver by
calling drm_brige_connector_init(). It should not be created by the
bridge driver. The bridge driver should provide the bridge functions
(drm_bridge_funcs), but not create any connector.

> > > Also I'm neither seeing any drm_brige struct nor drm_bridge_funcs,
> > > which are both essential for a bridge driver. I don't think moving a
> > > part of a driver to .../drm/bridge/ makes it a bridge driver.
> > >
> > > > +	drm_connector_attach_encoder(&hdmi->connector, encoder);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > ....
> > >

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel