Re: [PATCH v2] drm/gma500: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi

[Jani Nikula]

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