On Wed, 20 Nov 2024, Ian Forbes <ian.forbes@xxxxxxxxxxxx> wrote: > On Wed, Nov 20, 2024 at 4:22 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > wrote: >> >> Please avoid all struct edid based interfaces, in this case >> drm_connector_update_edid_property(). They will be removed in the >> future, and adding more is counter-productive. Everything should be >> struct drm_edid based going forward. >> >> Of course, actually grafting the EDID needs struct edid. And that's kind >> of annoying too. Do we really want to spread the EDID details all over >> the place? This one combines drm_edid.h structs and magic numbers in a >> jumble. I'm kind of hoping we'd get rid of driver usage of struct edid, >> though that's a long road. But we've made a lot of progress towards it, >> there aren't that many places left that directly look at the guts of >> EDID, and most of it is centralized in drm_edid.c. >> > > drm_edid isn't exported so we can't use it. I know you've read the EDID > spec so complaining about the magic numbers is silly. > I didn't choose to make the whole spec based on bizarrely packed 10 and 12 > bit numbers, they are unavoidable. I'm complaining about adding those details in drivers. That is, if we really have to have EDID generation. It's hard to get all the EDID details right. Drivers would invariably get them wrong, in different ways. The main reason struct drm_edid is opaque is to prevent drivers from attempting to parse it themselves. (Yes, drm_edid.c gets stuff wrong too, but at least it's the same for everyone and can be fixed in one place.) >> >> > Maybe it resolves problems with >> > compositors, but it is a step backwards for the overall ecosystem. If >> > the connector changes, your driver should increment the epoch counter. >> > [1] That will send a hotplug event to userspace. The EDID alone does not >> > say anything about connector status. >> >> Yeah, unplugging and replugging the same display with the same EDID >> isn't a problem for other drivers, and they don't have to do this kind >> of stuff to trick userspace. Maybe vmwgfx should handle (or simulate) >> hotplugs better? >> >> And preferrably let the EDID functions handle epoch counter updates >> instead of doing it yourself, if at all possible. >> >> BR, >> Jani. >> > > You're both missing the fact that virtual devices never disconnect active > displays. We don't have to do a disconnect/connect cycle like a physical > monitor and we wouldn't want to because it would be poor user experience. > The issue is not sending the hotplug event, it's that userspace will ignore > hotplug events on connectors that were previously connected because they > assume a disconnect/connect cycle must occur for changes to occur. The > whole point of hotplug_mode_update was as a hint to userspace that the > display can be "re-plugged" without a disconnect first and to always rescan > the connector for changes on hotplug. > > Currently compositors are taking an arbitrary set of connector properties > that they've organically collected over the years and doing a diff to > trigger a refresh in the modes/display layout. EDID is the only property > that they universally agree should trigger a display layout change. The > fact that Autofit works on any compsitors using vmwgfx, qxl, or virtio is > completely by accident. > > EDID is also a standardized connector property so it's not really fair to > say that all devices should export this property and then when we fix our > deficiency to block it. Now that it's standardized it is part of the uapi > and if userspace does weird things with it it's not really our concern. The > fact that it's standardized is also likely the reason it is being used in > such a fashion. If you want standardized, you should probably consider using drm_edid_read_custom() to fetch the generated EDID the same way all the other EDIDs get fetched, with support for firmware/debugfs EDID, and validity checks. And you get to use drm_edid based interfaces. And you get connector->display_info filled with the parsed data, so it aligns with everyone else. Arguably it's also backwards to first generate modes, update them using drm_mode_probed_add() and drm_add_modes_noedid(), then generate an EDID based on them and update the property with that. Userspace should not even look at the modes from the EDID directly, only the modes provided via KMS API, and nobody would even know if they differ. So if we go the EDID generation route, IMO this should go all the way. Generate EDID based on modes, and then use drm_edid_connector_add_modes() to add the modes. BR, Jani. -- Jani Nikula, Intel
