by Maxime Ripard

[permalink] [raw]

Subject: Re: (subset) [PATCH v5 18/44] drm/vc4: hdmi: Create destroy state implementation

On Thu, 07 Dec 2023 16:49:41 +0100, Maxime Ripard wrote:
> Even though we were rolling our own custom state for the vc4 HDMI
> controller driver, we were still using the generic helper to destroy
> that state.
>
> It was mostly working since the underlying state is the first member of
> our state so the pointers are probably equal in all relevant cases, but
> it's still fragile so let's fix this properly.
>
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

2023-12-14 08:48:16

Hi,

On 2023/12/7 23:50, Maxime Ripard wrote:
> atomic_check and mode_valid do not check for the same things which can
> lead to surprising result if the userspace commits a mode that didn't go
> through mode_valid. Let's merge the two implementations into a function
> called by both.
>
> Signed-off-by: Maxime Ripard <[email protected]>

Acked-by: Sui Jingfeng <[email protected]>

2024-01-12 13:59:27

Attachments:

(No filename) (9.78 kB)
signature.asc (235.00 B)
Download all attachments

2024-01-15 15:30:16

On Thu, Dec 14, 2023 at 03:10:43PM +0000, Dave Stevenson wrote:
> > +static bool
> > +sink_supports_format_bpc(const struct drm_connector *connector,
> > + const struct drm_display_info *info,
> > + const struct drm_display_mode *mode,
> > + unsigned int format, unsigned int bpc)
> > +{
> > + struct drm_device *dev = connector->dev;
> > + u8 vic = drm_match_cea_mode(mode);
> > +
> > + if (vic == 1 && bpc != 8) {
> > + drm_dbg(dev, "VIC1 requires a bpc of 8, got %u\n", bpc);
> > + return false;
> > + }
> > +
> > + if (!info->is_hdmi &&
> > + (format != HDMI_COLORSPACE_RGB || bpc != 8)) {
> > + drm_dbg(dev, "DVI Monitors require an RGB output at 8 bpc\n");
> > + return false;
> > + }
> > +
> > + if (!(connector->hdmi.supported_formats & BIT(format))) {
> > + drm_dbg(dev, "%s format unsupported by the connector.\n",
> > + drm_hdmi_connector_get_output_format_name(format));
> > + return false;
> > + }
> > +
> > + switch (format) {
> > + case HDMI_COLORSPACE_RGB:
> > + drm_dbg(dev, "RGB Format, checking the constraints.\n");
> > +
> > + if (!(info->color_formats & DRM_COLOR_FORMAT_RGB444))
> > + return false;
>
> We've dropped this check from vc4 in our downstream kernel as it stops
> you using the prebaked EDIDs (eg drm.edid_firmware=edid/1024x768.bin),
> or any other EDID that is defined as an analog monitor.
> The EDID parsing bombs out at [1], so info->color_formats gets left at 0.

Right, but it only does so if the display isn't defined as a digital display...

> RGB is mandatory for both DVI and HDMI, so rejecting it seems overly fussy.

... which is required for both DVI and HDMI.

And sure enough, if we decode that EDID:

edid-decode (hex):

00 ff ff ff ff ff ff 00 31 d8 00 00 00 00 00 00
05 16 01 03 6d 23 1a 78 ea 5e c0 a4 59 4a 98 25
20 50 54 00 08 00 61 40 01 01 01 01 01 01 01 01
01 01 01 01 01 01 64 19 00 40 41 00 26 30 08 90
36 00 63 0a 11 00 00 18 00 00 00 ff 00 4c 69 6e
75 78 20 23 30 0a 20 20 20 20 00 00 00 fd 00 3b
3d 2f 31 07 00 0a 20 20 20 20 20 20 00 00 00 fc
00 4c 69 6e 75 78 20 58 47 41 0a 20 20 20 00 55

----------------

Block 0, Base EDID:
EDID Structure Version & Revision: 1.3
Vendor & Product Identification:
Manufacturer: LNX
Model: 0
Made in: week 5 of 2012
Basic Display Parameters & Features:
Analog display
Signal Level Standard: 0.700 : 0.000 : 0.700 V p-p
Blank level equals black level
Sync: Separate Composite Serration
Maximum image size: 35 cm x 26 cm
Gamma: 2.20
DPMS levels: Standby Suspend Off
RGB color display
First detailed timing is the preferred timing
Color Characteristics:
Red : 0.6416, 0.3486
Green: 0.2919, 0.5957
Blue : 0.1474, 0.1250
White: 0.3125, 0.3281
Established Timings I & II:
DMT 0x10: 1024x768 60.003840 Hz 4:3 48.363 kHz 65.000000 MHz
Standard Timings:
DMT 0x10: 1024x768 60.003840 Hz 4:3 48.363 kHz 65.000000 MHz
Detailed Timing Descriptors:
DTD 1: 1024x768 60.003840 Hz 4:3 48.363 kHz 65.000000 MHz (355 mm x 266 mm)
Hfront 8 Hsync 144 Hback 168 Hpol N
Vfront 3 Vsync 6 Vback 29 Vpol N
Display Product Serial Number: 'Linux #0'
Display Range Limits:
Monitor ranges (GTF): 59-61 Hz V, 47-49 kHz H, max dotclock 70 MHz
Display Product Name: 'Linux XGA'
Checksum: 0x55

----------------

Warnings:

Block 0, Base EDID:
Detailed Timing Descriptor #1: DTD is similar but not identical to DMT 0x10.

EDID conformity: PASS

So, if anything, it's the EDID that needs to be updated, not the code there.
Maxime

Attachments:

(No filename) (3.90 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-01 15:34:00

by Dave Stevenson

[permalink] [raw]

Subject: Re: Re: [PATCH v5 15/44] drm/connector: hdmi: Compute bpc and format automatically

Hi Maxime

On Thu, 1 Feb 2024 at 12:51, Maxime Ripard <[email protected]> wrote:
>
> On Thu, Dec 14, 2023 at 03:10:43PM +0000, Dave Stevenson wrote:
> > > +static bool
> > > +sink_supports_format_bpc(const struct drm_connector *connector,
> > > + const struct drm_display_info *info,
> > > + const struct drm_display_mode *mode,
> > > + unsigned int format, unsigned int bpc)
> > > +{
> > > + struct drm_device *dev = connector->dev;
> > > + u8 vic = drm_match_cea_mode(mode);
> > > +
> > > + if (vic == 1 && bpc != 8) {
> > > + drm_dbg(dev, "VIC1 requires a bpc of 8, got %u\n", bpc);
> > > + return false;
> > > + }
> > > +
> > > + if (!info->is_hdmi &&
> > > + (format != HDMI_COLORSPACE_RGB || bpc != 8)) {
> > > + drm_dbg(dev, "DVI Monitors require an RGB output at 8 bpc\n");
> > > + return false;
> > > + }
> > > +
> > > + if (!(connector->hdmi.supported_formats & BIT(format))) {
> > > + drm_dbg(dev, "%s format unsupported by the connector.\n",
> > > + drm_hdmi_connector_get_output_format_name(format));
> > > + return false;
> > > + }
> > > +
> > > + switch (format) {
> > > + case HDMI_COLORSPACE_RGB:
> > > + drm_dbg(dev, "RGB Format, checking the constraints.\n");
> > > +
> > > + if (!(info->color_formats & DRM_COLOR_FORMAT_RGB444))
> > > + return false;
> >
> > We've dropped this check from vc4 in our downstream kernel as it stops
> > you using the prebaked EDIDs (eg drm.edid_firmware=edid/1024x768.bin),
> > or any other EDID that is defined as an analog monitor.
> > The EDID parsing bombs out at [1], so info->color_formats gets left at 0.
>
> Right, but it only does so if the display isn't defined as a digital display...
>
> > RGB is mandatory for both DVI and HDMI, so rejecting it seems overly fussy.
>
> ... which is required for both DVI and HDMI.
>
> And sure enough, if we decode that EDID:
>
> edid-decode (hex):
>
> 00 ff ff ff ff ff ff 00 31 d8 00 00 00 00 00 00
> 05 16 01 03 6d 23 1a 78 ea 5e c0 a4 59 4a 98 25
> 20 50 54 00 08 00 61 40 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 64 19 00 40 41 00 26 30 08 90
> 36 00 63 0a 11 00 00 18 00 00 00 ff 00 4c 69 6e
> 75 78 20 23 30 0a 20 20 20 20 00 00 00 fd 00 3b
> 3d 2f 31 07 00 0a 20 20 20 20 20 20 00 00 00 fc
> 00 4c 69 6e 75 78 20 58 47 41 0a 20 20 20 00 55
>
> ----------------
>
> Block 0, Base EDID:
> EDID Structure Version & Revision: 1.3
> Vendor & Product Identification:
> Manufacturer: LNX
> Model: 0
> Made in: week 5 of 2012
> Basic Display Parameters & Features:
> Analog display
> Signal Level Standard: 0.700 : 0.000 : 0.700 V p-p
> Blank level equals black level
> Sync: Separate Composite Serration
> Maximum image size: 35 cm x 26 cm
> Gamma: 2.20
> DPMS levels: Standby Suspend Off
> RGB color display
> First detailed timing is the preferred timing
> Color Characteristics:
> Red : 0.6416, 0.3486
> Green: 0.2919, 0.5957
> Blue : 0.1474, 0.1250
> White: 0.3125, 0.3281
> Established Timings I & II:
> DMT 0x10: 1024x768 60.003840 Hz 4:3 48.363 kHz 65.000000 MHz
> Standard Timings:
> DMT 0x10: 1024x768 60.003840 Hz 4:3 48.363 kHz 65.000000 MHz
> Detailed Timing Descriptors:
> DTD 1: 1024x768 60.003840 Hz 4:3 48.363 kHz 65.000000 MHz (355 mm x 266 mm)
> Hfront 8 Hsync 144 Hback 168 Hpol N
> Vfront 3 Vsync 6 Vback 29 Vpol N
> Display Product Serial Number: 'Linux #0'
> Display Range Limits:
> Monitor ranges (GTF): 59-61 Hz V, 47-49 kHz H, max dotclock 70 MHz
> Display Product Name: 'Linux XGA'
> Checksum: 0x55
>
> ----------------
>
> Warnings:
>
> Block 0, Base EDID:
> Detailed Timing Descriptor #1: DTD is similar but not identical to DMT 0x10.
>
> EDID conformity: PASS
>
> So, if anything, it's the EDID that needs to be updated, not the code there.

So are these EDIDs only valid for VGA outputs and another set needs to
be added for HDMI monitors?

Having drm.edid_firmware=edid/1024x768.bin works on an HDMI connector
prior to this patch, so presumably drm_edid_loader needs to
automatically switch between the existing (analog) and new (digital)
EDIDs based on the connector type? Or are you requiring users to
change the strings they use?

Cheers.
Dave

2024-02-01 16:55:21

Hi,

On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > /**
> > > * DOC: HDMI connector properties
> > > *
> > > + * Broadcast RGB
> > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > + * Infoframes will be generated according to that value.
> > > + *
> > > + * The value of this property can be one of the following:
> > > + *
> > > + * Automatic:
> > > + * RGB Range is selected automatically based on the mode
> > > + * according to the HDMI specifications.
> > > + *
> > > + * Full:
> > > + * Full RGB Range is forced.
> > > + *
> > > + * Limited 16:235:
> > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > + * this works for any number of bits-per-component.
> > > + *
> > > + * Drivers can set up this property by calling
> > > + * drm_connector_attach_broadcast_rgb_property().
> > > + *
> >
> > This is a good time to document this in more detail. There might be two
> > different things being affected:
> >
> > 1. The signalling (InfoFrame/SDP/...)
> > 2. The color pipeline processing
> >
> > All values of Broadcast RGB always affect the color pipeline processing
> > such that a full-range input to the CRTC is converted to either full- or
> > limited-range, depending on what the monitor is supposed to accept.
> >
> > When automatic is selected, does that mean that there is no signalling,
> > or that the signalling matches what the monitor is supposed to accept
> > according to the spec? Also, is this really HDMI specific?
> >
> > When full or limited is selected and the monitor doesn't support the
> > signalling, what happens?
>
> Forgot to mention: user-space still has no control over RGB vs YCbCr on
> the cable, so is this only affecting RGB? If not, how does it affect
> YCbCr?

So I dug a bit into both the i915 and vc4 drivers, and it looks like if
we're using a YCbCr format, i915 will always use a limited range while
vc4 will follow the value of the property.

I guess the best way to reconcile that would be to state that it also
controls the YCbCr range, and i915 can choose to reject/adjust the
configurations it can't support.

Does that make sense?

Maxime

Attachments:

(No filename) (2.28 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-02 13:59:06

by Jani Nikula

[permalink] [raw]

Subject: Re: Re: Re: [PATCH v5 15/44] drm/connector: hdmi: Compute bpc and format automatically

On Thu, 01 Feb 2024, Maxime Ripard <[email protected]> wrote:
> We've discussed that on IRC today. I'm not sure there was a conclusion
> other than "well this doesn't seem right". I think we should at least
> provide different EDIDs depending on the connector type indeed, but
> there was also a few discussions that arose:
>
> - Is it useful to have embedded EDIDs in the kernel at all, and could
> we just get rid of it?
>
> - Should we expose those EDIDs to userspace, and what happens to the
> compositor when we do?
>
> - The current way to generate those EDIDs isn't... optimal? Should we
> get rid of that as well?
>
> Anyway, all of those issues have been here for a while so I don't really
> expect this series to fix that.

IMO the direction should be towards deprecating and removing the builtin
firmware EDIDs from the kernel instead of adding more or expanding on
them. They were only ever meant to be the immediate aid to get something
on screen so the user could provide a proper EDID via userspace.

BR,
Jani.

--
Jani Nikula, Intel

2024-02-02 15:49:19

by Maxime Ripard

[permalink] [raw]

Subject: Re: Re: [PATCH v5 08/44] drm/connector: hdmi: Add Broadcast RGB property

Hi Sebastian,

On Mon, Jan 15, 2024 at 03:33:08PM +0100, Sebastian Wick wrote:
> > /**
> > * DOC: HDMI connector properties
> > *
> > + * Broadcast RGB
> > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > + * Infoframes will be generated according to that value.
> > + *
> > + * The value of this property can be one of the following:
> > + *
> > + * Automatic:
> > + * RGB Range is selected automatically based on the mode
> > + * according to the HDMI specifications.
> > + *
> > + * Full:
> > + * Full RGB Range is forced.
> > + *
> > + * Limited 16:235:
> > + * Limited RGB Range is forced. Unlike the name suggests,
> > + * this works for any number of bits-per-component.
> > + *
> > + * Drivers can set up this property by calling
> > + * drm_connector_attach_broadcast_rgb_property().
> > + *
>
> This is a good time to document this in more detail. There might be two
> different things being affected:
>
> 1. The signalling (InfoFrame/SDP/...)
> 2. The color pipeline processing
>
> All values of Broadcast RGB always affect the color pipeline processing
> such that a full-range input to the CRTC is converted to either full- or
> limited-range, depending on what the monitor is supposed to accept.
>
> When automatic is selected, does that mean that there is no signalling,
> or that the signalling matches what the monitor is supposed to accept
> according to the spec? Also, is this really HDMI specific?
>
> When full or limited is selected and the monitor doesn't support the
> signalling, what happens?

Leaving the YCbCr vs RGB discussion aside, would this be better ?

* Broadcast RGB (HDMI specific)
* Indicates the Quantization Range (Full vs Limited) used. The color
* processing pipeline will be adjusted to match the value of the
* property, and the Infoframes will be generated and sent accordingly.
*
* The value of this property can be one of the following:
*
* Automatic:
* The quantization range is selected automatically based on the
* mode according to the HDMI specifications (HDMI 1.4b - Section
* 6.6 - Video Quantization Ranges).
*
* Full:
* Full quantization range is forced.
*
* Limited 16:235:
* Limited quantization range is forced. Unlike the name suggests,
* this works for any number of bits-per-component.
*
* Property values other than Automatic can result in colors being off (if
* limited is selected but the display expects full), or a black screen
* (if full is selected but the display expects limited).
*
* Drivers can set up this property by calling
* drm_connector_attach_broadcast_rgb_property().

Thanks!
Maxime

Attachments:

(No filename) (2.87 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-02 16:06:38

On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrj?l? wrote:
> On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote:
> > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrj?l? wrote:
> > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote:
> > > > Hi,
> > > >
> > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > > > > > /**
> > > > > > > * DOC: HDMI connector properties
> > > > > > > *
> > > > > > > + * Broadcast RGB
> > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > > > > + * Infoframes will be generated according to that value.
> > > > > > > + *
> > > > > > > + * The value of this property can be one of the following:
> > > > > > > + *
> > > > > > > + * Automatic:
> > > > > > > + * RGB Range is selected automatically based on the mode
> > > > > > > + * according to the HDMI specifications.
> > > > > > > + *
> > > > > > > + * Full:
> > > > > > > + * Full RGB Range is forced.
> > > > > > > + *
> > > > > > > + * Limited 16:235:
> > > > > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > > > > + * this works for any number of bits-per-component.
> > > > > > > + *
> > > > > > > + * Drivers can set up this property by calling
> > > > > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > > > > + *
> > > > > >
> > > > > > This is a good time to document this in more detail. There might be two
> > > > > > different things being affected:
> > > > > >
> > > > > > 1. The signalling (InfoFrame/SDP/...)
> > > > > > 2. The color pipeline processing
> > > > > >
> > > > > > All values of Broadcast RGB always affect the color pipeline processing
> > > > > > such that a full-range input to the CRTC is converted to either full- or
> > > > > > limited-range, depending on what the monitor is supposed to accept.
> > > > > >
> > > > > > When automatic is selected, does that mean that there is no signalling,
> > > > > > or that the signalling matches what the monitor is supposed to accept
> > > > > > according to the spec? Also, is this really HDMI specific?
> > > > > >
> > > > > > When full or limited is selected and the monitor doesn't support the
> > > > > > signalling, what happens?
> > > > >
> > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on
> > > > > the cable, so is this only affecting RGB? If not, how does it affect
> > > > > YCbCr?
> > > >
> > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if
> > > > we're using a YCbCr format, i915 will always use a limited range while
> > > > vc4 will follow the value of the property.
> > >
> > > The property is literally called "Broadcast *RGB*".
> > > That should explain why it's only affecting RGB.
> >
> > Right. And the limited range option is called "Limited 16:235" despite
> > being usable on bpc > 8 bits. Naming errors occurs, and history happens
> > to make names inconsistent too, that's fine and not an argument in
> > itself.
> >
> > > Full range YCbCr is a much rarer beast so we've never bothered
> > > to enable it.
> >
> > vc4 supports it.
>
> Someone implemented it incorrectly then.

Incorrectly according to what documentation / specification? I'm sorry,
but I find it super ironic that i915 gets to do its own thing, not
document any of it, and when people try to clean things up they get told
that we got it all wrong.

> > > Eg. with DP it only became possible with the introduction of the VSC
> > > SDP (and I don't recall if there's additional capability checks that
> > > are also required). With DP MSA signalling full range YCbCr is not
> > > possible at all.
> >
> > This is for HDMI only.
> >
> > > I don't recall right now what the HDMI requirements are.
> >
> > HDMI has supported it for a while, and it's defined (for example) in the
> > HDMI 1.4 spec in Section 6.6 - Video Quantization Ranges. It supports
> > limited and full range on both RGB and YCbCr, as long as the EDIDs state
> > so and the Infoframes signal it.
>
> I think a good reason for not using a simple boolean like this
> YCbCr is that it doesn't cover the color encoding part at all,
> which is probably more important than the quantization range.
> So we need a new property anyway.

This isn't what is being discussed here, and as I've shown you, is
completely orthogonal as far as HDMI is concerned.

Maxime

Attachments:

(No filename) (4.47 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-09 20:31:03

by Sebastian Wick

[permalink] [raw]

Subject: Re: Re: [PATCH v5 08/44] drm/connector: hdmi: Add Broadcast RGB property

On Fri, Feb 02, 2024 at 04:49:04PM +0100, Maxime Ripard wrote:
> Hi Sebastian,
>
> On Mon, Jan 15, 2024 at 03:33:08PM +0100, Sebastian Wick wrote:
> > > /**
> > > * DOC: HDMI connector properties
> > > *
> > > + * Broadcast RGB
> > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > + * Infoframes will be generated according to that value.
> > > + *
> > > + * The value of this property can be one of the following:
> > > + *
> > > + * Automatic:
> > > + * RGB Range is selected automatically based on the mode
> > > + * according to the HDMI specifications.
> > > + *
> > > + * Full:
> > > + * Full RGB Range is forced.
> > > + *
> > > + * Limited 16:235:
> > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > + * this works for any number of bits-per-component.
> > > + *
> > > + * Drivers can set up this property by calling
> > > + * drm_connector_attach_broadcast_rgb_property().
> > > + *
> >
> > This is a good time to document this in more detail. There might be two
> > different things being affected:
> >
> > 1. The signalling (InfoFrame/SDP/...)
> > 2. The color pipeline processing
> >
> > All values of Broadcast RGB always affect the color pipeline processing
> > such that a full-range input to the CRTC is converted to either full- or
> > limited-range, depending on what the monitor is supposed to accept.
> >
> > When automatic is selected, does that mean that there is no signalling,
> > or that the signalling matches what the monitor is supposed to accept
> > according to the spec? Also, is this really HDMI specific?
> >
> > When full or limited is selected and the monitor doesn't support the
> > signalling, what happens?
>
> Leaving the YCbCr vs RGB discussion aside, would this be better ?

Yes, it is. Thanks.

We do have to resolve the YCbCr vs RGB issue though.

> * Broadcast RGB (HDMI specific)
> * Indicates the Quantization Range (Full vs Limited) used. The color
> * processing pipeline will be adjusted to match the value of the

Ah, another thing no note here is that the CRTC as configured by user
space must always produce full range pixels.

> * property, and the Infoframes will be generated and sent accordingly.
> *
> * The value of this property can be one of the following:
> *
> * Automatic:
> * The quantization range is selected automatically based on the
> * mode according to the HDMI specifications (HDMI 1.4b - Section
> * 6.6 - Video Quantization Ranges).
> *
> * Full:
> * Full quantization range is forced.
> *
> * Limited 16:235:
> * Limited quantization range is forced. Unlike the name suggests,
> * this works for any number of bits-per-component.
> *
> * Property values other than Automatic can result in colors being off (if
> * limited is selected but the display expects full), or a black screen
> * (if full is selected but the display expects limited).
> *
> * Drivers can set up this property by calling
> * drm_connector_attach_broadcast_rgb_property().
>
> Thanks!
> Maxime

2024-02-09 20:54:26

by Sebastian Wick

[permalink] [raw]

Subject: Re: Re: Re: Re: [PATCH v5 08/44] drm/connector: hdmi: Add Broadcast RGB property

On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote:
> On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrj?l? wrote:
> > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote:
> > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrj?l? wrote:
> > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > > > > > > /**
> > > > > > > > * DOC: HDMI connector properties
> > > > > > > > *
> > > > > > > > + * Broadcast RGB
> > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > > > > > + * Infoframes will be generated according to that value.
> > > > > > > > + *
> > > > > > > > + * The value of this property can be one of the following:
> > > > > > > > + *
> > > > > > > > + * Automatic:
> > > > > > > > + * RGB Range is selected automatically based on the mode
> > > > > > > > + * according to the HDMI specifications.
> > > > > > > > + *
> > > > > > > > + * Full:
> > > > > > > > + * Full RGB Range is forced.
> > > > > > > > + *
> > > > > > > > + * Limited 16:235:
> > > > > > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > > > > > + * this works for any number of bits-per-component.
> > > > > > > > + *
> > > > > > > > + * Drivers can set up this property by calling
> > > > > > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > > > > > + *
> > > > > > >
> > > > > > > This is a good time to document this in more detail. There might be two
> > > > > > > different things being affected:
> > > > > > >
> > > > > > > 1. The signalling (InfoFrame/SDP/...)
> > > > > > > 2. The color pipeline processing
> > > > > > >
> > > > > > > All values of Broadcast RGB always affect the color pipeline processing
> > > > > > > such that a full-range input to the CRTC is converted to either full- or
> > > > > > > limited-range, depending on what the monitor is supposed to accept.
> > > > > > >
> > > > > > > When automatic is selected, does that mean that there is no signalling,
> > > > > > > or that the signalling matches what the monitor is supposed to accept
> > > > > > > according to the spec? Also, is this really HDMI specific?
> > > > > > >
> > > > > > > When full or limited is selected and the monitor doesn't support the
> > > > > > > signalling, what happens?
> > > > > >
> > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on
> > > > > > the cable, so is this only affecting RGB? If not, how does it affect
> > > > > > YCbCr?
> > > > >
> > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if
> > > > > we're using a YCbCr format, i915 will always use a limited range while
> > > > > vc4 will follow the value of the property.
> > > >
> > > > The property is literally called "Broadcast *RGB*".
> > > > That should explain why it's only affecting RGB.
> > >
> > > Right. And the limited range option is called "Limited 16:235" despite
> > > being usable on bpc > 8 bits. Naming errors occurs, and history happens
> > > to make names inconsistent too, that's fine and not an argument in
> > > itself.
> > >
> > > > Full range YCbCr is a much rarer beast so we've never bothered
> > > > to enable it.
> > >
> > > vc4 supports it.
> >
> > Someone implemented it incorrectly then.
>
> Incorrectly according to what documentation / specification? I'm sorry,
> but I find it super ironic that i915 gets to do its own thing, not
> document any of it, and when people try to clean things up they get told
> that we got it all wrong.

FWIW, this was an i915 property and if another driver uses the same
property name it must have the same behavior. Yes, it isn't standardized
and yes, it's not documented (hence this effort here) but it's still on
vc4 to make the property compatible.

Trying to make the property handle YCbCr is very much in the "let's try
to fix the property" territory that I want to avoid, so I'm in favor of
adjusting vc4.

> > > > Eg. with DP it only became possible with the introduction of the VSC
> > > > SDP (and I don't recall if there's additional capability checks that
> > > > are also required). With DP MSA signalling full range YCbCr is not
> > > > possible at all.
> > >
> > > This is for HDMI only.
> > >
> > > > I don't recall right now what the HDMI requirements are.
> > >
> > > HDMI has supported it for a while, and it's defined (for example) in the
> > > HDMI 1.4 spec in Section 6.6 - Video Quantization Ranges. It supports
> > > limited and full range on both RGB and YCbCr, as long as the EDIDs state
> > > so and the Infoframes signal it.
> >
> > I think a good reason for not using a simple boolean like this
> > YCbCr is that it doesn't cover the color encoding part at all,
> > which is probably more important than the quantization range.
> > So we need a new property anyway.
>
> This isn't what is being discussed here, and as I've shown you, is
> completely orthogonal as far as HDMI is concerned.
>
> Maxime

2024-02-12 09:56:12

by Maxime Ripard

[permalink] [raw]

Subject: Re: Re: Re: [PATCH v5 08/44] drm/connector: hdmi: Add Broadcast RGB property

On Fri, Feb 09, 2024 at 09:30:46PM +0100, Sebastian Wick wrote:
> On Fri, Feb 02, 2024 at 04:49:04PM +0100, Maxime Ripard wrote:
> > Hi Sebastian,
> >
> > On Mon, Jan 15, 2024 at 03:33:08PM +0100, Sebastian Wick wrote:
> > > > /**
> > > > * DOC: HDMI connector properties
> > > > *
> > > > + * Broadcast RGB
> > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > + * Infoframes will be generated according to that value.
> > > > + *
> > > > + * The value of this property can be one of the following:
> > > > + *
> > > > + * Automatic:
> > > > + * RGB Range is selected automatically based on the mode
> > > > + * according to the HDMI specifications.
> > > > + *
> > > > + * Full:
> > > > + * Full RGB Range is forced.
> > > > + *
> > > > + * Limited 16:235:
> > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > + * this works for any number of bits-per-component.
> > > > + *
> > > > + * Drivers can set up this property by calling
> > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > + *
> > >
> > > This is a good time to document this in more detail. There might be two
> > > different things being affected:
> > >
> > > 1. The signalling (InfoFrame/SDP/...)
> > > 2. The color pipeline processing
> > >
> > > All values of Broadcast RGB always affect the color pipeline processing
> > > such that a full-range input to the CRTC is converted to either full- or
> > > limited-range, depending on what the monitor is supposed to accept.
> > >
> > > When automatic is selected, does that mean that there is no signalling,
> > > or that the signalling matches what the monitor is supposed to accept
> > > according to the spec? Also, is this really HDMI specific?
> > >
> > > When full or limited is selected and the monitor doesn't support the
> > > signalling, what happens?
> >
> > Leaving the YCbCr vs RGB discussion aside, would this be better ?
>
> Yes, it is. Thanks.

Great, it'll be in the next version.

> We do have to resolve the YCbCr vs RGB issue though.
>
> > * Broadcast RGB (HDMI specific)
> > * Indicates the Quantization Range (Full vs Limited) used. The color
> > * processing pipeline will be adjusted to match the value of the
>
> Ah, another thing no note here is that the CRTC as configured by user
> space must always produce full range pixels.

Added too

Thanks

Attachments:

(No filename) (2.49 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-12 10:08:17

On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrj?l? wrote:
> On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote:
> > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote:
> > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote:
> > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrj?l? wrote:
> > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote:
> > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrj?l? wrote:
> > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > > > > > > > > > /**
> > > > > > > > > > > * DOC: HDMI connector properties
> > > > > > > > > > > *
> > > > > > > > > > > + * Broadcast RGB
> > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > > > > > > > > + * Infoframes will be generated according to that value.
> > > > > > > > > > > + *
> > > > > > > > > > > + * The value of this property can be one of the following:
> > > > > > > > > > > + *
> > > > > > > > > > > + * Automatic:
> > > > > > > > > > > + * RGB Range is selected automatically based on the mode
> > > > > > > > > > > + * according to the HDMI specifications.
> > > > > > > > > > > + *
> > > > > > > > > > > + * Full:
> > > > > > > > > > > + * Full RGB Range is forced.
> > > > > > > > > > > + *
> > > > > > > > > > > + * Limited 16:235:
> > > > > > > > > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > > > > > > > > + * this works for any number of bits-per-component.
> > > > > > > > > > > + *
> > > > > > > > > > > + * Drivers can set up this property by calling
> > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > > > > > > > > + *
> > > > > > > > > >
> > > > > > > > > > This is a good time to document this in more detail. There might be two
> > > > > > > > > > different things being affected:
> > > > > > > > > >
> > > > > > > > > > 1. The signalling (InfoFrame/SDP/...)
> > > > > > > > > > 2. The color pipeline processing
> > > > > > > > > >
> > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing
> > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or
> > > > > > > > > > limited-range, depending on what the monitor is supposed to accept.
> > > > > > > > > >
> > > > > > > > > > When automatic is selected, does that mean that there is no signalling,
> > > > > > > > > > or that the signalling matches what the monitor is supposed to accept
> > > > > > > > > > according to the spec? Also, is this really HDMI specific?
> > > > > > > > > >
> > > > > > > > > > When full or limited is selected and the monitor doesn't support the
> > > > > > > > > > signalling, what happens?
> > > > > > > > >
> > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on
> > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect
> > > > > > > > > YCbCr?
> > > > > > > >
> > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if
> > > > > > > > we're using a YCbCr format, i915 will always use a limited range while
> > > > > > > > vc4 will follow the value of the property.
> > > > > > >
> > > > > > > The property is literally called "Broadcast *RGB*".
> > > > > > > That should explain why it's only affecting RGB.
> > > > > >
> > > > > > Right. And the limited range option is called "Limited 16:235" despite
> > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens
> > > > > > to make names inconsistent too, that's fine and not an argument in
> > > > > > itself.
> > > > > >
> > > > > > > Full range YCbCr is a much rarer beast so we've never bothered
> > > > > > > to enable it.
> > > > > >
> > > > > > vc4 supports it.
> > > > >
> > > > > Someone implemented it incorrectly then.
> > > >
> > > > Incorrectly according to what documentation / specification? I'm sorry,
> > > > but I find it super ironic that i915 gets to do its own thing, not
> > > > document any of it, and when people try to clean things up they get told
> > > > that we got it all wrong.
> > >
> > > FWIW, this was an i915 property and if another driver uses the same
> > > property name it must have the same behavior. Yes, it isn't standardized
> > > and yes, it's not documented (hence this effort here) but it's still on
> > > vc4 to make the property compatible.
> >
> > How is it not compatible? It's a superset of what i915 provides, but
> > it's strictly compatible with it.
>
> No it is not.

The property is compatible with i915 interpretation of it, whether you
like it or not. And that's what Sebastian was referring to.

> Eg. what happens if you set the thing to full range for RGB (which you
> must on many broken monitors), and then the kernel automagically
> switches to YCbCr (for whatever reason) but the monitor doesn't
> support full range YCbCr? Answer: you get crap output.

And that part is just moving goalposts.

Maxime

Attachments:

(No filename) (5.21 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-12 17:21:54

On Mon, Feb 12, 2024 at 06:06:18PM +0100, Sebastian Wick wrote:
> On Mon, Feb 12, 2024 at 05:53:48PM +0100, Maxime Ripard wrote:
> > On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrj?l? wrote:
> > > On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote:
> > > > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote:
> > > > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote:
> > > > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrj?l? wrote:
> > > > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote:
> > > > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote:
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > > > > > > > > > > > /**
> > > > > > > > > > > > > * DOC: HDMI connector properties
> > > > > > > > > > > > > *
> > > > > > > > > > > > > + * Broadcast RGB
> > > > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > > > > > > > > > > + * Infoframes will be generated according to that value.
> > > > > > > > > > > > > + *
> > > > > > > > > > > > > + * The value of this property can be one of the following:
> > > > > > > > > > > > > + *
> > > > > > > > > > > > > + * Automatic:
> > > > > > > > > > > > > + * RGB Range is selected automatically based on the mode
> > > > > > > > > > > > > + * according to the HDMI specifications.
> > > > > > > > > > > > > + *
> > > > > > > > > > > > > + * Full:
> > > > > > > > > > > > > + * Full RGB Range is forced.
> > > > > > > > > > > > > + *
> > > > > > > > > > > > > + * Limited 16:235:
> > > > > > > > > > > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > > > > > > > > > > + * this works for any number of bits-per-component.
> > > > > > > > > > > > > + *
> > > > > > > > > > > > > + * Drivers can set up this property by calling
> > > > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > > > > > > > > > > + *
> > > > > > > > > > > >
> > > > > > > > > > > > This is a good time to document this in more detail. There might be two
> > > > > > > > > > > > different things being affected:
> > > > > > > > > > > >
> > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...)
> > > > > > > > > > > > 2. The color pipeline processing
> > > > > > > > > > > >
> > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing
> > > > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or
> > > > > > > > > > > > limited-range, depending on what the monitor is supposed to accept.
> > > > > > > > > > > >
> > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling,
> > > > > > > > > > > > or that the signalling matches what the monitor is supposed to accept
> > > > > > > > > > > > according to the spec? Also, is this really HDMI specific?
> > > > > > > > > > > >
> > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the
> > > > > > > > > > > > signalling, what happens?
> > > > > > > > > > >
> > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on
> > > > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect
> > > > > > > > > > > YCbCr?
> > > > > > > > > >
> > > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if
> > > > > > > > > > we're using a YCbCr format, i915 will always use a limited range while
> > > > > > > > > > vc4 will follow the value of the property.
> > > > > > > > >
> > > > > > > > > The property is literally called "Broadcast *RGB*".
> > > > > > > > > That should explain why it's only affecting RGB.
> > > > > > > >
> > > > > > > > Right. And the limited range option is called "Limited 16:235" despite
> > > > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens
> > > > > > > > to make names inconsistent too, that's fine and not an argument in
> > > > > > > > itself.
> > > > > > > >
> > > > > > > > > Full range YCbCr is a much rarer beast so we've never bothered
> > > > > > > > > to enable it.
> > > > > > > >
> > > > > > > > vc4 supports it.
> > > > > > >
> > > > > > > Someone implemented it incorrectly then.
> > > > > >
> > > > > > Incorrectly according to what documentation / specification? I'm sorry,
> > > > > > but I find it super ironic that i915 gets to do its own thing, not
> > > > > > document any of it, and when people try to clean things up they get told
> > > > > > that we got it all wrong.
> > > > >
> > > > > FWIW, this was an i915 property and if another driver uses the same
> > > > > property name it must have the same behavior. Yes, it isn't standardized
> > > > > and yes, it's not documented (hence this effort here) but it's still on
> > > > > vc4 to make the property compatible.
> > > >
> > > > How is it not compatible? It's a superset of what i915 provides, but
> > > > it's strictly compatible with it.
> > >
> > > No it is not.
> >
> > The property is compatible with i915 interpretation of it, whether you
> > like it or not. And that's what Sebastian was referring to.
> >
> > > Eg. what happens if you set the thing to full range for RGB (which you
> > > must on many broken monitors), and then the kernel automagically
> > > switches to YCbCr (for whatever reason) but the monitor doesn't
> > > support full range YCbCr? Answer: you get crap output.
> >
> > And that part is just moving goalposts.
>
> But it's really not.

It really is. This whole discussion started by "well it would be nice if
we made that property handled by the core", and we're now at the "we
need to deal with broken YCbCr displays and i915 opinion about them"
stage. After creating documentation, unit tests, etc. It's the textbook
definition of moving goalposts. And while i915 won't be affected by all
that work.

That series has been stuck for multiple iterations on pointless and
mundane debates while the biggest part and whole point of it is not
getting any review. So yeah, sorry, it's frustrating.

> The Broadcast RGB property kind of works from a user space perspective
> because it's a workaround for broken sinks. If a sink expects limited
> range we can force full range. If this however affects YCbCr modes as
> well, then this isn't a workaround for broken RGB range anymore
> because it now breaks YCbCr.

Or, you know, it's a workaround for broken YCbCr display.

> Sorry, but vc4 just has to change.
>
> And again: let's please stop trying to improve the property.

I'm not. I'm super close to just dropping that patch entirely and keep
the current situation that seems to work fine for everyone.

Maxime

Attachments:

(No filename) (6.92 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-15 12:07:02

On Mon, Feb 19, 2024 at 03:01:44PM +0100, Sebastian Wick wrote:
> On Thu, Feb 15, 2024 at 12:00:01PM +0100, Maxime Ripard wrote:
> > On Mon, Feb 12, 2024 at 06:06:18PM +0100, Sebastian Wick wrote:
> > > On Mon, Feb 12, 2024 at 05:53:48PM +0100, Maxime Ripard wrote:
> > > > On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrj?l? wrote:
> > > > > On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote:
> > > > > > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote:
> > > > > > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote:
> > > > > > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote:
> > > > > > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > * DOC: HDMI connector properties
> > > > > > > > > > > > > > > *
> > > > > > > > > > > > > > > + * Broadcast RGB
> > > > > > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > > > > > > > > > > > > + * Infoframes will be generated according to that value.
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * The value of this property can be one of the following:
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * Automatic:
> > > > > > > > > > > > > > > + * RGB Range is selected automatically based on the mode
> > > > > > > > > > > > > > > + * according to the HDMI specifications.
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * Full:
> > > > > > > > > > > > > > > + * Full RGB Range is forced.
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * Limited 16:235:
> > > > > > > > > > > > > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > > > > > > > > > > > > + * this works for any number of bits-per-component.
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > + * Drivers can set up this property by calling
> > > > > > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This is a good time to document this in more detail. There might be two
> > > > > > > > > > > > > > different things being affected:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...)
> > > > > > > > > > > > > > 2. The color pipeline processing
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing
> > > > > > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or
> > > > > > > > > > > > > > limited-range, depending on what the monitor is supposed to accept.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling,
> > > > > > > > > > > > > > or that the signalling matches what the monitor is supposed to accept
> > > > > > > > > > > > > > according to the spec? Also, is this really HDMI specific?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the
> > > > > > > > > > > > > > signalling, what happens?
> > > > > > > > > > > > >
> > > > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on
> > > > > > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect
> > > > > > > > > > > > > YCbCr?
> > > > > > > > > > > >
> > > > > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if
> > > > > > > > > > > > we're using a YCbCr format, i915 will always use a limited range while
> > > > > > > > > > > > vc4 will follow the value of the property.
> > > > > > > > > > >
> > > > > > > > > > > The property is literally called "Broadcast *RGB*".
> > > > > > > > > > > That should explain why it's only affecting RGB.
> > > > > > > > > >
> > > > > > > > > > Right. And the limited range option is called "Limited 16:235" despite
> > > > > > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens
> > > > > > > > > > to make names inconsistent too, that's fine and not an argument in
> > > > > > > > > > itself.
> > > > > > > > > >
> > > > > > > > > > > Full range YCbCr is a much rarer beast so we've never bothered
> > > > > > > > > > > to enable it.
> > > > > > > > > >
> > > > > > > > > > vc4 supports it.
> > > > > > > > >
> > > > > > > > > Someone implemented it incorrectly then.
> > > > > > > >
> > > > > > > > Incorrectly according to what documentation / specification? I'm sorry,
> > > > > > > > but I find it super ironic that i915 gets to do its own thing, not
> > > > > > > > document any of it, and when people try to clean things up they get told
> > > > > > > > that we got it all wrong.
> > > > > > >
> > > > > > > FWIW, this was an i915 property and if another driver uses the same
> > > > > > > property name it must have the same behavior. Yes, it isn't standardized
> > > > > > > and yes, it's not documented (hence this effort here) but it's still on
> > > > > > > vc4 to make the property compatible.
> > > > > >
> > > > > > How is it not compatible? It's a superset of what i915 provides, but
> > > > > > it's strictly compatible with it.
> > > > >
> > > > > No it is not.
> > > >
> > > > The property is compatible with i915 interpretation of it, whether you
> > > > like it or not. And that's what Sebastian was referring to.
> > > >
> > > > > Eg. what happens if you set the thing to full range for RGB (which you
> > > > > must on many broken monitors), and then the kernel automagically
> > > > > switches to YCbCr (for whatever reason) but the monitor doesn't
> > > > > support full range YCbCr? Answer: you get crap output.
> > > >
> > > > And that part is just moving goalposts.
> > >
> > > But it's really not.
> >
> > It really is. This whole discussion started by "well it would be nice if
> > we made that property handled by the core", and we're now at the "we
> > need to deal with broken YCbCr displays and i915 opinion about them"
> > stage. After creating documentation, unit tests, etc. It's the textbook
> > definition of moving goalposts. And while i915 won't be affected by all
> > that work.
>
> Sorry, but what you're saying is just not true.
>
> The Broadcast RGB property is an Intel specific property.

No, it's not. vc4 has been using it for a year now.

> It lacked documentation but the user space contract exists and it
> based on how i915 implemented it. By changing the semantics you're
> breaking user space. The documentation has to document the current
> contract between i915 and user space, not whatever you want the
> property to be like.
>
> I get that you're frustrated that you have to do work while i915 doesn't
> but none of that is relevant for what the property is and how user space
> expects it to work.

That's not it, really. I don't mind doing the work. I do mind losing
functionalities on something that was working fine. And getting the
blame for something that is, at best, just as much of an documentation
issue on i915 devs.

> > That series has been stuck for multiple iterations on pointless and
> > mundane debates while the biggest part and whole point of it is not
> > getting any review. So yeah, sorry, it's frustrating.
>
> I'm reviewing the parts that I can, and that's the uAPI. I find it
> really offensive that you're saying that this is pointless and mundate.

I'm sorry I offended you, but I was talking about the whole debate
itself, not the uAPI. The uAPI itself exists. It's already there, it's
used in the wild on several drivers, and several user-space components.

What that patch does is trying to document it, and test it. It's a net
benefit. Is it perfect? Probably not.

It's a net benefit nonetheless. The part where I mostly disagree with
you I guess (and what we've actually been arguing obut) is trying to get
something perfect (to the best of our knowledge) out of it.

Anyway, I'll just shut up and to do the work I guess.

> The uAPI is your end product, if it can't be used, everything you do in
> your driver is utterly pointless.
>
> > > The Broadcast RGB property kind of works from a user space perspective
> > > because it's a workaround for broken sinks. If a sink expects limited
> > > range we can force full range. If this however affects YCbCr modes as
> > > well, then this isn't a workaround for broken RGB range anymore
> > > because it now breaks YCbCr.
> >
> > Or, you know, it's a workaround for broken YCbCr display.
>
> Displays can accept both RGB and YCbCr signals, drivers can chose
> whichever they want, and user space can not influence or even know which
> one is being used.
>
> The automatic selection of the range is very different between RGB and
> YCbCr. If user space forces the range to a specific value and the driver
> for whatever reason switches from RGB to YCbCr or the other way around,
> this forcing of the range will most likely be incorrect.
>
> This is what we're talking about when we say that the semantics of the
> vc4 Broadcast RGB property is broken. User space literally cannot use it
> consistenly. By restricting it to RGB signals, user space can user it
> consistently and fix monitors that do not follow the automatic
> quantization range algorithm correctly. Yes, if there is an issue with
> the quantization range of a YCbCr signal then this property doesn't
> help, but it never tried to help those cases.

Ack, thanks

Maxime

Attachments:

(No filename) (9.92 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-22 13:13:02

by Sebastian Wick

[permalink] [raw]

Subject: Re: [PATCH v5 08/44] drm/connector: hdmi: Add Broadcast RGB property

On Thu, Feb 22, 2024 at 02:58:45PM +0200, Ville Syrj?l? wrote:
> On Thu, Feb 22, 2024 at 11:54:04AM +0100, Maxime Ripard wrote:
> > On Mon, Feb 19, 2024 at 03:01:44PM +0100, Sebastian Wick wrote:
> > > On Thu, Feb 15, 2024 at 12:00:01PM +0100, Maxime Ripard wrote:
> > > > On Mon, Feb 12, 2024 at 06:06:18PM +0100, Sebastian Wick wrote:
> > > > > On Mon, Feb 12, 2024 at 05:53:48PM +0100, Maxime Ripard wrote:
> > > > > > On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrj?l? wrote:
> > > > > > > On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote:
> > > > > > > > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote:
> > > > > > > > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote:
> > > > > > > > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote:
> > > > > > > > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote:
> > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > > > * DOC: HDMI connector properties
> > > > > > > > > > > > > > > > > *
> > > > > > > > > > > > > > > > > + * Broadcast RGB
> > > > > > > > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > > > > > > > > > > > > > > + * Infoframes will be generated according to that value.
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * The value of this property can be one of the following:
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * Automatic:
> > > > > > > > > > > > > > > > > + * RGB Range is selected automatically based on the mode
> > > > > > > > > > > > > > > > > + * according to the HDMI specifications.
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * Full:
> > > > > > > > > > > > > > > > > + * Full RGB Range is forced.
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * Limited 16:235:
> > > > > > > > > > > > > > > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > > > > > > > > > > > > > > + * this works for any number of bits-per-component.
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * Drivers can set up this property by calling
> > > > > > > > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > This is a good time to document this in more detail. There might be two
> > > > > > > > > > > > > > > > different things being affected:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...)
> > > > > > > > > > > > > > > > 2. The color pipeline processing
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing
> > > > > > > > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or
> > > > > > > > > > > > > > > > limited-range, depending on what the monitor is supposed to accept.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling,
> > > > > > > > > > > > > > > > or that the signalling matches what the monitor is supposed to accept
> > > > > > > > > > > > > > > > according to the spec? Also, is this really HDMI specific?
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the
> > > > > > > > > > > > > > > > signalling, what happens?
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on
> > > > > > > > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect
> > > > > > > > > > > > > > > YCbCr?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if
> > > > > > > > > > > > > > we're using a YCbCr format, i915 will always use a limited range while
> > > > > > > > > > > > > > vc4 will follow the value of the property.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The property is literally called "Broadcast *RGB*".
> > > > > > > > > > > > > That should explain why it's only affecting RGB.
> > > > > > > > > > > >
> > > > > > > > > > > > Right. And the limited range option is called "Limited 16:235" despite
> > > > > > > > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens
> > > > > > > > > > > > to make names inconsistent too, that's fine and not an argument in
> > > > > > > > > > > > itself.
> > > > > > > > > > > >
> > > > > > > > > > > > > Full range YCbCr is a much rarer beast so we've never bothered
> > > > > > > > > > > > > to enable it.
> > > > > > > > > > > >
> > > > > > > > > > > > vc4 supports it.
> > > > > > > > > > >
> > > > > > > > > > > Someone implemented it incorrectly then.
> > > > > > > > > >
> > > > > > > > > > Incorrectly according to what documentation / specification? I'm sorry,
> > > > > > > > > > but I find it super ironic that i915 gets to do its own thing, not
> > > > > > > > > > document any of it, and when people try to clean things up they get told
> > > > > > > > > > that we got it all wrong.
> > > > > > > > >
> > > > > > > > > FWIW, this was an i915 property and if another driver uses the same
> > > > > > > > > property name it must have the same behavior. Yes, it isn't standardized
> > > > > > > > > and yes, it's not documented (hence this effort here) but it's still on
> > > > > > > > > vc4 to make the property compatible.
> > > > > > > >
> > > > > > > > How is it not compatible? It's a superset of what i915 provides, but
> > > > > > > > it's strictly compatible with it.
> > > > > > >
> > > > > > > No it is not.
> > > > > >
> > > > > > The property is compatible with i915 interpretation of it, whether you
> > > > > > like it or not. And that's what Sebastian was referring to.
> > > > > >
> > > > > > > Eg. what happens if you set the thing to full range for RGB (which you
> > > > > > > must on many broken monitors), and then the kernel automagically
> > > > > > > switches to YCbCr (for whatever reason) but the monitor doesn't
> > > > > > > support full range YCbCr? Answer: you get crap output.
> > > > > >
> > > > > > And that part is just moving goalposts.
> > > > >
> > > > > But it's really not.
> > > >
> > > > It really is. This whole discussion started by "well it would be nice if
> > > > we made that property handled by the core", and we're now at the "we
> > > > need to deal with broken YCbCr displays and i915 opinion about them"
> > > > stage. After creating documentation, unit tests, etc. It's the textbook
> > > > definition of moving goalposts. And while i915 won't be affected by all
> > > > that work.
> > >
> > > Sorry, but what you're saying is just not true.
> > >
> > > The Broadcast RGB property is an Intel specific property.
> >
> > No, it's not. vc4 has been using it for a year now.
> >
> > > It lacked documentation but the user space contract exists and it
> > > based on how i915 implemented it. By changing the semantics you're
> > > breaking user space. The documentation has to document the current
> > > contract between i915 and user space, not whatever you want the
> > > property to be like.
> > >
> > > I get that you're frustrated that you have to do work while i915 doesn't
> > > but none of that is relevant for what the property is and how user space
> > > expects it to work.
> >
> > That's not it, really. I don't mind doing the work. I do mind losing
> > functionalities on something that was working fine. And getting the
> > blame for something that is, at best, just as much of an documentation
> > issue on i915 devs.
>
> We've had a couple of these cases recently where people have taken
> some old property implemented by i915 and implemented it differently
> in some other driver. Dunno if the reason was that people didn't try
> to understand what i915 is doing and why, or they misundestood it,
> or they understood it but decided to ignore it anyway.
>
> Unfortunately having undocumented corners in the uapi is simply
> a fact of life when dealing with a >15 year old legacy codebase.
> Also there were basically no rules regarding properties in the
> past, so everyone just added random properties whenever they
> felt like it.
>
> I think going forward we should probably lay down some extra
> ground rules; if an old undocumented uapi is being extended
> to cover more than one driver we must first figure out what
> the de facto semantics are, and document things properly
> before anything else gets done.

That would be great. The documentation already has requirements for new
properties. Adding the requirement for extending driver-specific
properties to more drivers there would be great and make it "official".

>
> --
> Ville Syrj?l?
> Intel
>

2024-02-22 13:21:07

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v5 08/44] drm/connector: hdmi: Add Broadcast RGB property

On Thu, Feb 22, 2024 at 02:58:45PM +0200, Ville Syrj?l? wrote:
> On Thu, Feb 22, 2024 at 11:54:04AM +0100, Maxime Ripard wrote:
> > On Mon, Feb 19, 2024 at 03:01:44PM +0100, Sebastian Wick wrote:
> > > On Thu, Feb 15, 2024 at 12:00:01PM +0100, Maxime Ripard wrote:
> > > > On Mon, Feb 12, 2024 at 06:06:18PM +0100, Sebastian Wick wrote:
> > > > > On Mon, Feb 12, 2024 at 05:53:48PM +0100, Maxime Ripard wrote:
> > > > > > On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrj?l? wrote:
> > > > > > > On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote:
> > > > > > > > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote:
> > > > > > > > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote:
> > > > > > > > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote:
> > > > > > > > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote:
> > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > > > * DOC: HDMI connector properties
> > > > > > > > > > > > > > > > > *
> > > > > > > > > > > > > > > > > + * Broadcast RGB
> > > > > > > > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > > > > > > > > > > > > > > + * Infoframes will be generated according to that value.
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * The value of this property can be one of the following:
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * Automatic:
> > > > > > > > > > > > > > > > > + * RGB Range is selected automatically based on the mode
> > > > > > > > > > > > > > > > > + * according to the HDMI specifications.
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * Full:
> > > > > > > > > > > > > > > > > + * Full RGB Range is forced.
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * Limited 16:235:
> > > > > > > > > > > > > > > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > > > > > > > > > > > > > > + * this works for any number of bits-per-component.
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > > + * Drivers can set up this property by calling
> > > > > > > > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > This is a good time to document this in more detail. There might be two
> > > > > > > > > > > > > > > > different things being affected:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...)
> > > > > > > > > > > > > > > > 2. The color pipeline processing
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing
> > > > > > > > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or
> > > > > > > > > > > > > > > > limited-range, depending on what the monitor is supposed to accept.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling,
> > > > > > > > > > > > > > > > or that the signalling matches what the monitor is supposed to accept
> > > > > > > > > > > > > > > > according to the spec? Also, is this really HDMI specific?
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the
> > > > > > > > > > > > > > > > signalling, what happens?
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on
> > > > > > > > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect
> > > > > > > > > > > > > > > YCbCr?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if
> > > > > > > > > > > > > > we're using a YCbCr format, i915 will always use a limited range while
> > > > > > > > > > > > > > vc4 will follow the value of the property.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The property is literally called "Broadcast *RGB*".
> > > > > > > > > > > > > That should explain why it's only affecting RGB.
> > > > > > > > > > > >
> > > > > > > > > > > > Right. And the limited range option is called "Limited 16:235" despite
> > > > > > > > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens
> > > > > > > > > > > > to make names inconsistent too, that's fine and not an argument in
> > > > > > > > > > > > itself.
> > > > > > > > > > > >
> > > > > > > > > > > > > Full range YCbCr is a much rarer beast so we've never bothered
> > > > > > > > > > > > > to enable it.
> > > > > > > > > > > >
> > > > > > > > > > > > vc4 supports it.
> > > > > > > > > > >
> > > > > > > > > > > Someone implemented it incorrectly then.
> > > > > > > > > >
> > > > > > > > > > Incorrectly according to what documentation / specification? I'm sorry,
> > > > > > > > > > but I find it super ironic that i915 gets to do its own thing, not
> > > > > > > > > > document any of it, and when people try to clean things up they get told
> > > > > > > > > > that we got it all wrong.
> > > > > > > > >
> > > > > > > > > FWIW, this was an i915 property and if another driver uses the same
> > > > > > > > > property name it must have the same behavior. Yes, it isn't standardized
> > > > > > > > > and yes, it's not documented (hence this effort here) but it's still on
> > > > > > > > > vc4 to make the property compatible.
> > > > > > > >
> > > > > > > > How is it not compatible? It's a superset of what i915 provides, but
> > > > > > > > it's strictly compatible with it.
> > > > > > >
> > > > > > > No it is not.
> > > > > >
> > > > > > The property is compatible with i915 interpretation of it, whether you
> > > > > > like it or not. And that's what Sebastian was referring to.
> > > > > >
> > > > > > > Eg. what happens if you set the thing to full range for RGB (which you
> > > > > > > must on many broken monitors), and then the kernel automagically
> > > > > > > switches to YCbCr (for whatever reason) but the monitor doesn't
> > > > > > > support full range YCbCr? Answer: you get crap output.
> > > > > >
> > > > > > And that part is just moving goalposts.
> > > > >
> > > > > But it's really not.
> > > >
> > > > It really is. This whole discussion started by "well it would be nice if
> > > > we made that property handled by the core", and we're now at the "we
> > > > need to deal with broken YCbCr displays and i915 opinion about them"
> > > > stage. After creating documentation, unit tests, etc. It's the textbook
> > > > definition of moving goalposts. And while i915 won't be affected by all
> > > > that work.
> > >
> > > Sorry, but what you're saying is just not true.
> > >
> > > The Broadcast RGB property is an Intel specific property.
> >
> > No, it's not. vc4 has been using it for a year now.
> >
> > > It lacked documentation but the user space contract exists and it
> > > based on how i915 implemented it. By changing the semantics you're
> > > breaking user space. The documentation has to document the current
> > > contract between i915 and user space, not whatever you want the
> > > property to be like.
> > >
> > > I get that you're frustrated that you have to do work while i915 doesn't
> > > but none of that is relevant for what the property is and how user space
> > > expects it to work.
> >
> > That's not it, really. I don't mind doing the work. I do mind losing
> > functionalities on something that was working fine. And getting the
> > blame for something that is, at best, just as much of an documentation
> > issue on i915 devs.
>
> We've had a couple of these cases recently where people have taken
> some old property implemented by i915 and implemented it differently
> in some other driver. Dunno if the reason was that people didn't try
> to understand what i915 is doing and why, or they misundestood it,
> or they understood it but decided to ignore it anyway.

I can't tell for the other cases, but in this particular case it's
definitely in the misunderstanding category. And implying that we didn't
even try to understand it, or that we didn't consult anyone when the
patches were posted on the ML for months doesn't seem fair either.

> Unfortunately having undocumented corners in the uapi is simply
> a fact of life when dealing with a >15 year old legacy codebase.
> Also there were basically no rules regarding properties in the
> past, so everyone just added random properties whenever they
> felt like it.
>
> I think going forward we should probably lay down some extra
> ground rules; if an old undocumented uapi is being extended
> to cover more than one driver we must first figure out what
> the de facto semantics are, and document things properly
> before anything else gets done.

That sounds reasonable, but you (not you personally, but the i915 team
in general) also have to engage, you can't just impose that on everyone
else, and then just hope they will figure it out perfectly without your
help.

I think that whole story is a testament to that.

Maxime

Attachments:

(No filename) (9.63 kB)
signature.asc (235.00 B)
Download all attachments

2024-02-22 16:35:12

by Ville Syrjälä

[permalink] [raw]

Subject: Re: [PATCH v5 08/44] drm/connector: hdmi: Add Broadcast RGB property

On Thu, Feb 22, 2024 at 11:54:04AM +0100, Maxime Ripard wrote:
> On Mon, Feb 19, 2024 at 03:01:44PM +0100, Sebastian Wick wrote:
> > On Thu, Feb 15, 2024 at 12:00:01PM +0100, Maxime Ripard wrote:
> > > On Mon, Feb 12, 2024 at 06:06:18PM +0100, Sebastian Wick wrote:
> > > > On Mon, Feb 12, 2024 at 05:53:48PM +0100, Maxime Ripard wrote:
> > > > > On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrj?l? wrote:
> > > > > > On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote:
> > > > > > > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote:
> > > > > > > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote:
> > > > > > > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote:
> > > > > > > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrj?l? wrote:
> > > > > > > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote:
> > > > > > > > > > > > > > > > /**
> > > > > > > > > > > > > > > > * DOC: HDMI connector properties
> > > > > > > > > > > > > > > > *
> > > > > > > > > > > > > > > > + * Broadcast RGB
> > > > > > > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used.
> > > > > > > > > > > > > > > > + * Infoframes will be generated according to that value.
> > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > + * The value of this property can be one of the following:
> > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > + * Automatic:
> > > > > > > > > > > > > > > > + * RGB Range is selected automatically based on the mode
> > > > > > > > > > > > > > > > + * according to the HDMI specifications.
> > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > + * Full:
> > > > > > > > > > > > > > > > + * Full RGB Range is forced.
> > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > + * Limited 16:235:
> > > > > > > > > > > > > > > > + * Limited RGB Range is forced. Unlike the name suggests,
> > > > > > > > > > > > > > > > + * this works for any number of bits-per-component.
> > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > > > + * Drivers can set up this property by calling
> > > > > > > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property().
> > > > > > > > > > > > > > > > + *
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > This is a good time to document this in more detail. There might be two
> > > > > > > > > > > > > > > different things being affected:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...)
> > > > > > > > > > > > > > > 2. The color pipeline processing
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing
> > > > > > > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or
> > > > > > > > > > > > > > > limited-range, depending on what the monitor is supposed to accept.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling,
> > > > > > > > > > > > > > > or that the signalling matches what the monitor is supposed to accept
> > > > > > > > > > > > > > > according to the spec? Also, is this really HDMI specific?
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the
> > > > > > > > > > > > > > > signalling, what happens?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on
> > > > > > > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect
> > > > > > > > > > > > > > YCbCr?
> > > > > > > > > > > > >
> > > > > > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if
> > > > > > > > > > > > > we're using a YCbCr format, i915 will always use a limited range while
> > > > > > > > > > > > > vc4 will follow the value of the property.
> > > > > > > > > > > >
> > > > > > > > > > > > The property is literally called "Broadcast *RGB*".
> > > > > > > > > > > > That should explain why it's only affecting RGB.
> > > > > > > > > > >
> > > > > > > > > > > Right. And the limited range option is called "Limited 16:235" despite
> > > > > > > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens
> > > > > > > > > > > to make names inconsistent too, that's fine and not an argument in
> > > > > > > > > > > itself.
> > > > > > > > > > >
> > > > > > > > > > > > Full range YCbCr is a much rarer beast so we've never bothered
> > > > > > > > > > > > to enable it.
> > > > > > > > > > >
> > > > > > > > > > > vc4 supports it.
> > > > > > > > > >
> > > > > > > > > > Someone implemented it incorrectly then.
> > > > > > > > >
> > > > > > > > > Incorrectly according to what documentation / specification? I'm sorry,
> > > > > > > > > but I find it super ironic that i915 gets to do its own thing, not
> > > > > > > > > document any of it, and when people try to clean things up they get told
> > > > > > > > > that we got it all wrong.
> > > > > > > >
> > > > > > > > FWIW, this was an i915 property and if another driver uses the same
> > > > > > > > property name it must have the same behavior. Yes, it isn't standardized
> > > > > > > > and yes, it's not documented (hence this effort here) but it's still on
> > > > > > > > vc4 to make the property compatible.
> > > > > > >
> > > > > > > How is it not compatible? It's a superset of what i915 provides, but
> > > > > > > it's strictly compatible with it.
> > > > > >
> > > > > > No it is not.
> > > > >
> > > > > The property is compatible with i915 interpretation of it, whether you
> > > > > like it or not. And that's what Sebastian was referring to.
> > > > >
> > > > > > Eg. what happens if you set the thing to full range for RGB (which you
> > > > > > must on many broken monitors), and then the kernel automagically
> > > > > > switches to YCbCr (for whatever reason) but the monitor doesn't
> > > > > > support full range YCbCr? Answer: you get crap output.
> > > > >
> > > > > And that part is just moving goalposts.
> > > >
> > > > But it's really not.
> > >
> > > It really is. This whole discussion started by "well it would be nice if
> > > we made that property handled by the core", and we're now at the "we
> > > need to deal with broken YCbCr displays and i915 opinion about them"
> > > stage. After creating documentation, unit tests, etc. It's the textbook
> > > definition of moving goalposts. And while i915 won't be affected by all
> > > that work.
> >
> > Sorry, but what you're saying is just not true.
> >
> > The Broadcast RGB property is an Intel specific property.
>
> No, it's not. vc4 has been using it for a year now.
>
> > It lacked documentation but the user space contract exists and it
> > based on how i915 implemented it. By changing the semantics you're
> > breaking user space. The documentation has to document the current
> > contract between i915 and user space, not whatever you want the
> > property to be like.
> >
> > I get that you're frustrated that you have to do work while i915 doesn't
> > but none of that is relevant for what the property is and how user space
> > expects it to work.
>
> That's not it, really. I don't mind doing the work. I do mind losing
> functionalities on something that was working fine. And getting the
> blame for something that is, at best, just as much of an documentation
> issue on i915 devs.

We've had a couple of these cases recently where people have taken
some old property implemented by i915 and implemented it differently
in some other driver. Dunno if the reason was that people didn't try
to understand what i915 is doing and why, or they misundestood it,
or they understood it but decided to ignore it anyway.

Unfortunately having undocumented corners in the uapi is simply
a fact of life when dealing with a >15 year old legacy codebase.
Also there were basically no rules regarding properties in the
past, so everyone just added random properties whenever they
felt like it.

I think going forward we should probably lay down some extra
ground rules; if an old undocumented uapi is being extended
to cover more than one driver we must first figure out what
the de facto semantics are, and document things properly
before anything else gets done.

--
Ville Syrj?l?
Intel