On Mon, Nov 08, 2021 at 07:55:00PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 08, 2021 at 04:58:34PM +0100, Maxime Ripard wrote:
> > On Fri, Nov 05, 2021 at 08:14:04PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > > > > --- a/include/drm/drm_modes.h
> > > > > > +++ b/include/drm/drm_modes.h
> > > > > > @@ -424,6 +424,21 @@ static inline bool drm_mode_is_stereo(const struct drm_display_mode *mode)
> > > > > >  	return mode->flags & DRM_MODE_FLAG_3D_MASK;
> > > > > >  }
> > > > > >  
> > > > > > +/**
> > > > > > + * drm_mode_hdmi_requires_scrambling - Checks if a mode requires HDMI Scrambling
> > > > > > + * @mode: DRM display mode
> > > > > > + *
> > > > > > + * Checks if a given display mode requires the scrambling to be enabled.
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + * A boolean stating whether it's required or not.
> > > > > > + */
> > > > > > +static inline bool
> > > > > > +drm_mode_hdmi_requires_scrambling(const struct drm_display_mode *mode)
> > > > > > +{
> > > > > > +	return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > > > > +}
> > > > > 
> > > > > That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> > > > > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > > > > magic for the actually transmitted TMDS clock).
> > > > 
> > > > Yeah we might need to add the bus format for the cable here too, to make
> > > > this complete.
> > > 
> > > I don't think we have a usable thing for that on the drm level, so
> > > would need to invent something. Oh, and the mode->clock vs. 
> > > mode->crtc_clock funny business also limits the usability of this
> > > approach. So probably just easiest to pass in the the driver calculated
> > > TMDS clock instead.
> > 
> > If we look at all (I think?) the existing users of scrambling in KMS as
> > of 5.15, only i915 seems to use crtc_clock (which, in retrospect, seems
> > to be the right thing to do), and only i915 and dw-hdmi use an output
> > format, i915 rolling its own, and dw-hdmi using the mbus formats.
> > 
> > I think using the mbus format here makes the most sense: i915 already is
> > rolling a whole bunch of other code, and we use the mbus defines for the
> > bridge format enumeration as well which is probably going to have some
> > interaction.
> > 
> > I'm not quite sure what to do next though. The whole point of that
> > series is to streamline as much as possible the scrambling and TMDS
> > ratio setup to avoid the duplication we already have in the drivers that
> > support it, every one using the mostly-the-same-but-slightly-different
> > logic to configure it.
> > 
> > The mode is fortunately stored in generic structures so it's easy to
> > make that decision. Having to take into account the output format
> > however makes it mandatory to move the bus format in the
> > drm_connector_state(?) structure too.
> 
> I think involving state objects and the like is just going to make
> it harder to share code between all drivers, if that is the goal.
> Just a few tiny helpers I think is what would allow the broadest 
> reuse. I guess you could build additional midlayer on top of those
> for some drivers if you wish.
> 
> As for the bus_format stuff, that looks at the same time overkill,
> and insufficiently documented. I guess its main purpose is to exactly
> defines how some digtal bus works, which makes sense when you're
> chaining a bunch of random chips together. But looks overly complicated
> to me for defining what to output from a HDMI encoder. Looking at the
> defines I wouldn't even know what to use for HDMI actually. All we
> really want is RGB 4:4:4 vs. YCbCr 4:4:4 vs. YCbCr 4:2:2 vs. YCbCr 4:2:0.
> Beyond that level of detail we don't care how each bit gets transferred
> etc. Hence enum intel_output_format in i915.

I have the same feeling about the mbus formats.

I tried to start a discussion about this some time back, without much
success:
https://lore.kernel.org/all/20210317154352.732095-1-maxime@cerno.tech/

The main issue for our current series is that it's tied to the bridges,
while it should work for any HDMI connector, backed by a bridge or not.

However, the main point of this series is to streamline as much as
possible the scrambling setup, including dealing with hotplug properly
just like i915 is doing.

A flag in the connector state to enable the scrambling and high tmds
ratio allows for the helper to perform the disable/enable cycle when we
detected the hotplug. If we wouldn't have it, it wouldn't know what to
do in the first place, and we would end up disabling/enabling the
display every time (which isn't great).

This also allows for less duplication since everyone is using a variant
of the same algorithm to do so, without proper consideration for corner
cases (like enabling scrambling for displays that supports it for rates
< 340MHz)

So we really need something generic there. Or maybe an intermediate
hdmi_connector_state?

Maxime