On Thu, Feb 01, 2024 at 03:33:24PM +0000, Dave Stevenson wrote:
> Hi Maxime
>
> On Thu, 1 Feb 2024 at 12:51, Maxime Ripard <mripard@xxxxxxxxxx> 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?
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.
Maxime
Attachment:
signature.asc
Description: PGP signature
