Hi Maxime On Tue, 20 Feb 2024 at 16:10, Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > The EDID firmware loading mechanism introduced a few built-in EDIDs that > could be forced on any connector, bypassing the EDIDs it exposes. > > While convenient, this limited set of EDIDs doesn't take into account > the connector type, and we can end up with an EDID that is completely > invalid for a given connector. > > For example, the edid/800x600.bin file matches the following 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 1b 14 78 ea 5e c0 a4 59 4a 98 25 > 20 50 54 01 00 00 45 40 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 a0 0f 20 00 31 58 1c 20 28 80 > 14 00 15 d0 10 00 00 1e 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 24 26 05 00 0a 20 20 20 20 20 20 00 00 00 fc > 00 4c 69 6e 75 78 20 53 56 47 41 0a 20 20 00 c2 > > ---------------- > > 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: 27 cm x 20 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 0x09: 800x600 60.316541 Hz 4:3 37.879 kHz 40.000000 MHz > Standard Timings: > DMT 0x09: 800x600 60.316541 Hz 4:3 37.879 kHz 40.000000 MHz > Detailed Timing Descriptors: > DTD 1: 800x600 60.316541 Hz 4:3 37.879 kHz 40.000000 MHz (277 mm x 208 mm) > Hfront 40 Hsync 128 Hback 88 Hpol P > Vfront 1 Vsync 4 Vback 23 Vpol P > Display Product Serial Number: 'Linux #0' > Display Range Limits: > Monitor ranges (GTF): 59-61 Hz V, 36-38 kHz H, max dotclock 50 MHz > Display Product Name: 'Linux SVGA' > Checksum: 0xc2 > > So, an analog monitor EDID. However, if the connector was an HDMI > monitor for example, it breaks the HDMI specification that requires, > among other things, a digital display, the VIC 1 mode and an HDMI Forum > Vendor Specific Data Block in an CTA-861 extension. > > We thus end up with a completely invalid EDID, which thus might confuse > HDMI-related code that could parse it. > > After some discussions on IRC, we identified mainly two ways to fix > this: > > - We can either create more EDIDs for each connector type to provide > a built-in EDID that matches the resolution passed in the name, and > still be a sensible EDID for that connector type; > > - Or we can just prevent the EDID to be exposed to userspace if it's > built-in. > > Or possibly both. > > However, the conclusion was that maybe we just don't need the built-in > EDIDs at all and we should just get rid of them. So here we are. > > Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx> > --- > drivers/gpu/drm/drm_edid_load.c | 160 +++----------------------------- Needs to be removed from the docs too: "The code (see drivers/gpu/drm/drm_edid_load.c) contains built-in data sets for commonly used screen resolutions (800x600, 1024x768, 1280x1024, 1600x1200, 1680x1050, 1920x1080) as binary blobs,..." https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/edid.rst I'm sad to see these go, but c'est la vie. Descriptions of using these built in EDIDs abound in various tutorials, so those are all now invalid :/ Dave > 1 file changed, 13 insertions(+), 147 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c > index 60fcb80bce61..d1c7e8298702 100644 > --- a/drivers/gpu/drm/drm_edid_load.c > +++ b/drivers/gpu/drm/drm_edid_load.c > @@ -20,162 +20,28 @@ > > static char edid_firmware[PATH_MAX]; > module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644); > -MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob " > - "from built-in data or /lib/firmware instead. "); > - > -#define GENERIC_EDIDS 6 > -static const char * const generic_edid_name[GENERIC_EDIDS] = { > - "edid/800x600.bin", > - "edid/1024x768.bin", > - "edid/1280x1024.bin", > - "edid/1600x1200.bin", > - "edid/1680x1050.bin", > - "edid/1920x1080.bin", > -}; > - > -static const u8 generic_edid[GENERIC_EDIDS][128] = { > - { > - 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, > - 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > - 0x05, 0x16, 0x01, 0x03, 0x6d, 0x1b, 0x14, 0x78, > - 0xea, 0x5e, 0xc0, 0xa4, 0x59, 0x4a, 0x98, 0x25, > - 0x20, 0x50, 0x54, 0x01, 0x00, 0x00, 0x45, 0x40, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0xa0, 0x0f, > - 0x20, 0x00, 0x31, 0x58, 0x1c, 0x20, 0x28, 0x80, > - 0x14, 0x00, 0x15, 0xd0, 0x10, 0x00, 0x00, 0x1e, > - 0x00, 0x00, 0x00, 0xff, 0x00, 0x4c, 0x69, 0x6e, > - 0x75, 0x78, 0x20, 0x23, 0x30, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x3b, > - 0x3d, 0x24, 0x26, 0x05, 0x00, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfc, > - 0x00, 0x4c, 0x69, 0x6e, 0x75, 0x78, 0x20, 0x53, > - 0x56, 0x47, 0x41, 0x0a, 0x20, 0x20, 0x00, 0xc2, > - }, > - { > - 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, > - 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > - 0x05, 0x16, 0x01, 0x03, 0x6d, 0x23, 0x1a, 0x78, > - 0xea, 0x5e, 0xc0, 0xa4, 0x59, 0x4a, 0x98, 0x25, > - 0x20, 0x50, 0x54, 0x00, 0x08, 0x00, 0x61, 0x40, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x64, 0x19, > - 0x00, 0x40, 0x41, 0x00, 0x26, 0x30, 0x08, 0x90, > - 0x36, 0x00, 0x63, 0x0a, 0x11, 0x00, 0x00, 0x18, > - 0x00, 0x00, 0x00, 0xff, 0x00, 0x4c, 0x69, 0x6e, > - 0x75, 0x78, 0x20, 0x23, 0x30, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x3b, > - 0x3d, 0x2f, 0x31, 0x07, 0x00, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfc, > - 0x00, 0x4c, 0x69, 0x6e, 0x75, 0x78, 0x20, 0x58, > - 0x47, 0x41, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x55, > - }, > - { > - 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, > - 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > - 0x05, 0x16, 0x01, 0x03, 0x6d, 0x2c, 0x23, 0x78, > - 0xea, 0x5e, 0xc0, 0xa4, 0x59, 0x4a, 0x98, 0x25, > - 0x20, 0x50, 0x54, 0x00, 0x00, 0x00, 0x81, 0x80, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x30, 0x2a, > - 0x00, 0x98, 0x51, 0x00, 0x2a, 0x40, 0x30, 0x70, > - 0x13, 0x00, 0xbc, 0x63, 0x11, 0x00, 0x00, 0x1e, > - 0x00, 0x00, 0x00, 0xff, 0x00, 0x4c, 0x69, 0x6e, > - 0x75, 0x78, 0x20, 0x23, 0x30, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x3b, > - 0x3d, 0x3e, 0x40, 0x0b, 0x00, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfc, > - 0x00, 0x4c, 0x69, 0x6e, 0x75, 0x78, 0x20, 0x53, > - 0x58, 0x47, 0x41, 0x0a, 0x20, 0x20, 0x00, 0xa0, > - }, > - { > - 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, > - 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > - 0x05, 0x16, 0x01, 0x03, 0x6d, 0x37, 0x29, 0x78, > - 0xea, 0x5e, 0xc0, 0xa4, 0x59, 0x4a, 0x98, 0x25, > - 0x20, 0x50, 0x54, 0x00, 0x00, 0x00, 0xa9, 0x40, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x48, 0x3f, > - 0x40, 0x30, 0x62, 0xb0, 0x32, 0x40, 0x40, 0xc0, > - 0x13, 0x00, 0x2b, 0xa0, 0x21, 0x00, 0x00, 0x1e, > - 0x00, 0x00, 0x00, 0xff, 0x00, 0x4c, 0x69, 0x6e, > - 0x75, 0x78, 0x20, 0x23, 0x30, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x3b, > - 0x3d, 0x4a, 0x4c, 0x11, 0x00, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfc, > - 0x00, 0x4c, 0x69, 0x6e, 0x75, 0x78, 0x20, 0x55, > - 0x58, 0x47, 0x41, 0x0a, 0x20, 0x20, 0x00, 0x9d, > - }, > - { > - 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, > - 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > - 0x05, 0x16, 0x01, 0x03, 0x6d, 0x2b, 0x1b, 0x78, > - 0xea, 0x5e, 0xc0, 0xa4, 0x59, 0x4a, 0x98, 0x25, > - 0x20, 0x50, 0x54, 0x00, 0x00, 0x00, 0xb3, 0x00, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x21, 0x39, > - 0x90, 0x30, 0x62, 0x1a, 0x27, 0x40, 0x68, 0xb0, > - 0x36, 0x00, 0xb5, 0x11, 0x11, 0x00, 0x00, 0x1e, > - 0x00, 0x00, 0x00, 0xff, 0x00, 0x4c, 0x69, 0x6e, > - 0x75, 0x78, 0x20, 0x23, 0x30, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x3b, > - 0x3d, 0x40, 0x42, 0x0f, 0x00, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfc, > - 0x00, 0x4c, 0x69, 0x6e, 0x75, 0x78, 0x20, 0x57, > - 0x53, 0x58, 0x47, 0x41, 0x0a, 0x20, 0x00, 0x26, > - }, > - { > - 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, > - 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > - 0x05, 0x16, 0x01, 0x03, 0x6d, 0x32, 0x1c, 0x78, > - 0xea, 0x5e, 0xc0, 0xa4, 0x59, 0x4a, 0x98, 0x25, > - 0x20, 0x50, 0x54, 0x00, 0x00, 0x00, 0xd1, 0xc0, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, > - 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x3a, > - 0x80, 0x18, 0x71, 0x38, 0x2d, 0x40, 0x58, 0x2c, > - 0x45, 0x00, 0xf4, 0x19, 0x11, 0x00, 0x00, 0x1e, > - 0x00, 0x00, 0x00, 0xff, 0x00, 0x4c, 0x69, 0x6e, > - 0x75, 0x78, 0x20, 0x23, 0x30, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x3b, > - 0x3d, 0x42, 0x44, 0x0f, 0x00, 0x0a, 0x20, 0x20, > - 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfc, > - 0x00, 0x4c, 0x69, 0x6e, 0x75, 0x78, 0x20, 0x46, > - 0x48, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x05, > - }, > -}; > +MODULE_PARM_DESC(edid_firmware, > + "Do not probe monitor, use specified EDID blob from /lib/firmware instead."); > > static const struct drm_edid *edid_load(struct drm_connector *connector, const char *name) > { > const struct firmware *fw = NULL; > - const u8 *fwdata; > const struct drm_edid *drm_edid; > - int fwsize, builtin; > + int err; > > - builtin = match_string(generic_edid_name, GENERIC_EDIDS, name); > - if (builtin >= 0) { > - fwdata = generic_edid[builtin]; > - fwsize = sizeof(generic_edid[builtin]); > - } else { > - int err; > - > - err = request_firmware(&fw, name, connector->dev->dev); > - if (err) { > - drm_err(connector->dev, > - "[CONNECTOR:%d:%s] Requesting EDID firmware \"%s\" failed (err=%d)\n", > - connector->base.id, connector->name, > - name, err); > - return ERR_PTR(err); > - } > - > - fwdata = fw->data; > - fwsize = fw->size; > + err = request_firmware(&fw, name, connector->dev->dev); > + if (err) { > + drm_err(connector->dev, > + "[CONNECTOR:%d:%s] Requesting EDID firmware \"%s\" failed (err=%d)\n", > + connector->base.id, connector->name, > + name, err); > + return ERR_PTR(err); > } > > - drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Loaded %s firmware EDID \"%s\"\n", > - connector->base.id, connector->name, > - builtin >= 0 ? "built-in" : "external", name); > + drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Loaded external firmware EDID \"%s\"\n", > + connector->base.id, connector->name, name); > > - drm_edid = drm_edid_alloc(fwdata, fwsize); > + drm_edid = drm_edid_alloc(fw->data, fw->size); > if (!drm_edid_valid(drm_edid)) { > drm_err(connector->dev, "Invalid firmware EDID \"%s\"\n", name); > drm_edid_free(drm_edid); > -- > 2.43.2 >
