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 was thinking about this some more, can we leverage struct hdmi_colorspace for this? Chances are it's already used by any driver that supports YCbCr output to setup the infoframes.
Maxime