Re: [PATCH v2 6/6] drm/vs: Add hdmi driver

[Maxime Ripard]

Hi,

On Sun, Oct 29, 2023 at 06:52:24PM +0200, Dmitry Baryshkov wrote:
> On Thu, 26 Oct 2023 at 14:53, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> >
> > On Thu, Oct 26, 2023 at 11:57:22AM +0300, Dmitry Baryshkov wrote:
> > > On Thu, 26 Oct 2023 at 11:07, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Oct 26, 2023 at 01:23:53AM +0300, Dmitry Baryshkov wrote:
> > > > > > +static int starfive_hdmi_register(struct drm_device *drm, struct starfive_hdmi *hdmi)
> > > > > > +{
> > > > > > +   struct drm_encoder *encoder = &hdmi->encoder;
> > > > > > +   struct device *dev = hdmi->dev;
> > > > > > +
> > > > > > +   encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
> > > > > > +
> > > > > > +   /*
> > > > > > +    * If we failed to find the CRTC(s) which this encoder is
> > > > > > +    * supposed to be connected to, it's because the CRTC has
> > > > > > +    * not been registered yet.  Defer probing, and hope that
> > > > > > +    * the required CRTC is added later.
> > > > > > +    */
> > > > > > +   if (encoder->possible_crtcs == 0)
> > > > > > +           return -EPROBE_DEFER;
> > > > > > +
> > > > > > +   drm_encoder_helper_add(encoder, &starfive_hdmi_encoder_helper_funcs);
> > > > > > +
> > > > > > +   hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > > > > > +
> > > > > > +   drm_connector_helper_add(&hdmi->connector,
> > > > > > +                            &starfive_hdmi_connector_helper_funcs);
> > > > > > +   drmm_connector_init(drm, &hdmi->connector,
> > > > > > +                       &starfive_hdmi_connector_funcs,
> > > > > > +                       DRM_MODE_CONNECTOR_HDMIA,
> > > > >
> > > > > On an embedded device one can not be so sure. There can be MHL or HDMI
> > > > > Alternative Mode. Usually we use drm_bridge here and drm_bridge_connector.
> > > >
> > > > On an HDMI driver, it's far from being a requirement, especially given
> > > > the limitations bridges have.
> > >
> > > It's a blessing that things like MHL / HDMI-in-USB-C / HDMI-to-MyDP
> > > are not widely used in the wild and are mostly non-existing except
> > > several phones that preate wide DP usage.
> >
> > And those can be supported without relying on bridges.
> 
> Yes, they likely can, in the way that nouveau handles I2C TV encoders.
> But I don't think this can scale. We can have different devices
> attached to the DSI, LVDS, HDMI and even DP image sources. I don't see
> a scalable solution for either of them. E.g. by switching drm/msm to
> use panel bridges for DSI panels we were able to significantly unify
> and simplify code paths.

I'm glad it worked fine for drm/msm, but what I don't really like is the
current dogma that *everything* should be a bridge, and that's just a
poor guideline.

> > > Using drm_connector directly prevents one from handling possible
> > > modifications on the board level. For example, with the DRM connector
> > > in place, handling a separate HPD GPIO will result in code duplication
> > > from the hdmi-connector driver. Handling any other variations in the
> > > board design (which are pretty common in the embedded world) will also
> > > require changing the driver itself. drm_bridge / drm_bridge_connector
> > > save us from those issues.
> >
> > And we have other solutions there too. Like, EDIDs are pretty much in
> > the same spot with a lot of device variations, but it also works without
> > a common driver. I'd really wish we were having less bridges and more
> > helpers, but here we are.
> >
> > > BTW: what are the limitations of the drm_bridge wrt. HDMI output? I'm
> > > asking because we heavily depend on the bridge infrastructure for HDMI
> > > output. Maybe we are missing something there, which went unnoticed to
> > > me and my colleagues.
> >
> > A bridge cannot extend the connector state or use properties, for
> > example. It works for basic stuff but falls apart as soon as you're
> > trying to do something slightly advanced.
> 
> Ack. I agree, we didn't have a necessity to implement properties up to
> now. But that sounds like an interesting topic for DSI-to-HDMI bridges
> and HDCP support. I'll need to check if any of the RB3/RB5/Dragonboard
> bridges are programmed with the HDCP keys.

Aside from HDCP, the current color management work will also require to
expose properties on the connectors.

Maxime

Attachment: signature.asc
Description: PGP signature