Message-ID: <f360d860-e02b-4751-b55d-6a078b261a7f@suse.de>
Date: Thu, 12 Sep 2024 13:08:06 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Jani Nikula <jani.nikula@...ux.intel.com>,
 Tejas Vipin <tejasvipin76@...il.com>, Laurent.pinchart@...asonboard.com,
 patrik.r.jakobsson@...il.com, maarten.lankhorst@...ux.intel.com,
 mripard@...nel.org, airlied@...il.com, daniel@...ll.ch
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/gma500: replace drm_detect_hdmi_monitor() with
 drm_display_info.is_hdmi

Hi

Am 12.09.24 um 11:30 schrieb Jani Nikula:
> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@...e.de> wrote:
>> Hi
>>
>> Am 12.09.24 um 10:48 schrieb Jani Nikula:
>>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@...e.de> wrote:
>>>> Hi
>>>>
>>>> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>>>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>>>>> monitor HDMI information is available after EDID is parsed. Additionally
>>>>> rewrite the code the code to have fewer indentation levels.
>>>> The problem is that the entire logic is outdated. The content
>>>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx
>>>> callback should be set to drm_connector_helper_detect_from_ddc() and
>>>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx
>>>> will detect the presence of a display and ->get_modes will update EDID
>>>> and other properties.
>>> I guess I didn't get the memo on this one.
>>>
>>> What's the problem with reading the EDID at detect? The subsequent
>>> drm_edid_connector_add_modes() called from .get_modes() does not need to
>>> read the EDID again.
>> With drm_connector_helper_detect_from_ddc() there is already a helper
>> for detection. It makes sense to use it. And if we continue to update
>> the properties in detect (instead of get_modes), what is the correct
>> connector_status on errors? Right now and with the patch applied, detect
>> returns status_disconnected on errors. But this isn't correct if there
>> actually is a display. By separating detect and get_modes cleanly, we
>> can detect the display reliably, but also handle errors better than we
>> currently do in gma500. Get_modes is already expected to update the EDID
>> property, [1] for detect it's not so clear AFAICT. I think that from a
>> design perspective, it makes sense to have a read-only function that
>> only detects the physical state of the connector and a read-write
>> function that updates the connector's properties. Best regards Thomas
>> [1]
>> https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865
> So what if you can probe DDC but can't actually read an EDID of any
> kind? IMO that's a detect failure.

Not being able to read the EDID is not a failure IMHO. It's better to 
report a detected display and only provide minimal support, than to 
outright reject it. The display is essential for most users being able 
to use the computer at all, so it's often better to display something at 
lower quality than display nothing at all.

>
> Or how about things like CEC attach? Seems natural to do it at
> .detect(). Doing it at .get_modes() just seems wrong. However, it needs
> the EDID for physical address.
>
> I just don't think one size fits all here.

The good thing about the EDID probe helpers is that it only reads a 
minimal amount of data, like a single byte or the EDID header, so 
something like that. Many drivers poll the DDC every 10 seconds via 
->detect. Which means that code running in ->detect possibly runs 
concurrently to other DRM operations, such as page flips. Hence, 
->detect can interfere with the driver's hot path. The call to 
->get_modes usually only runs after the connector status changes to 
connected, which rarely happens. It's also not time critical as no 
modeset has happened yet.

And as everyone copies code from everyone else, we should establish best 
practices that take such things into account. Even with drivers that 
don't use connector polling, such as gma500, we should encourage devs to 
do the right thing and move code out of ->detect.

Best regards
Thomas


>
> BR,
> Jani.
>
>
>>> I think it should be fine to do incremental refactors like the patch at
>>> hand (modulo some issues I mention below).
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>>> Do you have  a device for testing such a change?
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> Signed-off-by: Tejas Vipin <tejasvipin76@...il.com>
>>>>> ---
>>>>> Changes in v2:
>>>>>        - Use drm_edid instead of edid
>>>>>
>>>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
>>>>> ---
>>>>>     drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
>>>>>     1 file changed, 13 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>>> index 2d95e0471291..701f8bbd5f2b 100644
>>>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
>>>>>     {
>>>>>     	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>>>>>     	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
>>>>> -	struct edid *edid = NULL;
>>>>> +	const struct drm_edid *drm_edid;
>>>>> +	int ret;
>>>>>     	enum drm_connector_status status = connector_status_disconnected;
>>>>>     
>>>>> -	edid = drm_get_edid(connector, connector->ddc);
>>>>> +	drm_edid = drm_edid_read_ddc(connector, connector->ddc);
>>> Just drm_edid_read() is enough when you're using connector->ddc.
>>>
>>>>> +	ret = drm_edid_connector_update(connector, drm_edid);
>>>>>     
>>>>>     	hdmi_priv->has_hdmi_sink = false;
>>>>>     	hdmi_priv->has_hdmi_audio = false;
>>>>> -	if (edid) {
>>>>> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>>>>> -			status = connector_status_connected;
>>>>> -			hdmi_priv->has_hdmi_sink =
>>>>> -						drm_detect_hdmi_monitor(edid);
>>>>> -			hdmi_priv->has_hdmi_audio =
>>>>> -						drm_detect_monitor_audio(edid);
>>>>> -		}
>>>>> -		kfree(edid);
>>>>> +	if (ret)
>>> This error path leaks the EDID.
>>>
>>>>> +		return status;
>>>>> +
>>>>> +	if (drm_edid_is_digital(drm_edid)) {
>>>>> +		status = connector_status_connected;
>>>>> +		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
>>>>> +		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
>>>>>     	}
>>>>> +	drm_edid_free(drm_edid);
>>>>> +
>>>>>     	return status;
>>>>>     }
>>>>>     

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Powered by blists - more mailing lists