On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > Hi > > Am 12.09.24 um 13:25 schrieb Jani Nikula: >> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: >>> Hi >>> >>> Am 12.09.24 um 11:38 schrieb Jani Nikula: >>>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: >>>>> Am 12.09.24 um 10:56 schrieb Jani Nikula: >>>>>> Moreover, in this case .detect() only detects digital displays as >>>>>> reported by EDID. If you postpone that to .get_modes(), the probe helper >>>>>> will still report connected, and invent non-EDID fallback modes. The >>>>>> behaviour changes. >>>>> The change in behavior is intentional, because the current test seems >>>>> arbitrary. Does the driver not work with analog outputs? >>>> Not on a DVI/HDMI port. Same with i915. >>>> >>>> That's possibly the only way to distinguish a DVI-A display connected to >>>> DVI-D source. >>> That's a detect failure, but IMHO our probe helpers should really handle >>> this case. >> How? Allow returning detect failures from .get_modes()? > > Something like that, I guess. > > For the specific problem it would be enough to read the first 20 bytes > of EDID data on DVI connectors and test the digital-input flag bit > against the exact connector requirements. drm_probe_ddc() could do this. Just a quick reply on this particular point: I'm very strongly against doing partial EDID reads. It's all geared towards EDID block sized handling. And you can't do checksum checking on the 20 bytes. It would be a maze of special casing, something the EDID code could have less, not more. BR, Jani. > Non-DVI connectors would continue to read a single bytes to detect the DDC. > > For more sophisticated problems, it would be good to introduce an > intermediate callback that updates the connector state. So the probe > logic would look like: > > 1) call ->detect to read physical connector status > 2) return if physical status did not change > 3) increment epoch counter > 4) call ->update to update connector state and properties (EDID, etc) > get new connector status > 5) call ->get_modes if connected > > The initial ->detect would be minimal. The ->update, if implemented, > could do more processing and error checking. It's result would be the > connector's new status. > > On a side note, I've recently spend quite a few patches on getting the > BMC output for ast and mgag200 usable. Something like the above logic > would have helped, I think. Because with the current probe logic, I had > to implement steps 1 to 4 in ->detect itself. The result has to maintain > physical status and epoch counter by itself. [1] > > Best regards > Thomas > > [1] > https://gitlab.freedesktop.org/drm/kernel/-/commit/2a2391f857cdc5cf16f8df030944cef8d3d2bc30 > >> >> BR, >> Jani. >> >> -- Jani Nikula, Intel
