[PATCH] drm/ast: Fix default resolution on BMC when DP is not connected

[Thomas Zimmermann]

>
>
> Link training is only to be done at atomic_enable or _check time. I 
> specifically asked about this back then.

Apologies, I meant only at _enable time BUT NOT _check.

>
> But having said that, ast doesn't really do Link Training; it just 
> reads a bit from the VBIOS, which does it automatically. So it likely 
> doesn't matter for ast. So yeah, maybe add it back if that works. Plus 
> a comment that HPD alone is unreliable. I think this shoudl go into 
> stable as well.
>
> Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during 
> atomic_enable")
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: Jocelyn Falempe <jfalempe at redhat.com>
> Cc: Dave Airlie <airlied at redhat.com>
> Cc: dri-devel at lists.freedesktop.org
> Cc: <stable at vger.kernel.org> # v6.12+
>
>>
>>>
>>>> This makes the default
>>>> BMC resolution to be 640x480 for remote access.
>>>> So consider that if there is no edid, no monitor is connected, and
>>>> add the BMC 1024x768 default resolution.
>>>> I've debugged this regression on ast_dp, but as dp501 is similar, I
>>>> fixed both in this patch.
>>>>
>>>> This regression was likely introduced by commit 2281475168d2
>>>> ("drm/ast: astdp: Perform link training during atomic_enable")
>>>> But I fixed it in the BMC get_modes handling.
>>>>
>>>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>>>> Fixes: bbad0090b9f4 ("drm/ast: astdp: Transparently handle BMC 
>>>> support")
>>>> ---
>>>>   drivers/gpu/drm/ast/ast_dp.c    | 14 +++++++-------
>>>>   drivers/gpu/drm/ast/ast_dp501.c | 14 +++++++-------
>>>>   2 files changed, 14 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ast/ast_dp.c 
>>>> b/drivers/gpu/drm/ast/ast_dp.c
>>>> index 0e282b7b167c..6c8ea95a2230 100644
>>>> --- a/drivers/gpu/drm/ast/ast_dp.c
>>>> +++ b/drivers/gpu/drm/ast/ast_dp.c
>>>> @@ -361,19 +361,19 @@ static const struct drm_encoder_helper_funcs 
>>>> ast_astdp_encoder_helper_funcs = {
>>>>   static int ast_astdp_connector_helper_get_modes(struct 
>>>> drm_connector *connector)
>>>
>>> I don't think this is the right place to fix the problem. The field 
>>> physical_status should always contain the correct physical status. 
>>> So the fix should go into ast_dp_connector_helper_detect_ctx(). 
>>> There's [1] something like
>>
>> If a DP monitor is connected, but the EDID is not readable, 
>> defaulting to 1024x768 is still a good choice.
>
> If the EDID is not readable, we should assume that no monitor is 
> connected. For this case, ast already sets 1024x768 to optimize for 
> the BMC.
>
>>
>> The default to 640x480 is only to comply with the DP specification, 
>> but in practice some DP monitors doesn't support 640x480.
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e7c254d75d16b75abf1958095fd34e2ecdc0d645 
>>
>
> If the defaults don't work for a certain system, users can force other 
> resolutions via the kernel's command line. This is definitely not 
> something that DRM drivers should address.
>
> Best regards
> Thomas
>
>>
>>>
>>>    if (ast_dp_status_is_connected(ast))
>>>      status = connected
>>>
>>> and that's where it should read the EDID without updating the 
>>> connector's EDID property. Example code:
>>>
>>>    if (ast_dp_status_is_connected(ast)) {
>>>      edid = drm_edid_read_custom(/* like in get_modes */)
>>>      if (drm_edid_valid(edid))
>>>        status = connected
>>>      drm_edid_free(edid)
>>>    }
>>>
>>> The EDID test could also go into _is_connected() directly. A comment 
>>> about false positives from VGACRDF might make sense as well.
>>>
>>> [1] 
>>> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/ast/ 
>>> ast_dp.c#L397
>>>
>>>>   {
>>>>       struct ast_connector *ast_connector = 
>>>> to_ast_connector(connector);
>>>> +    struct ast_device *ast = to_ast_device(connector->dev);
>>>> +    const struct drm_edid *drm_edid = NULL;
>>>>       int count;
>>>> -    if (ast_connector->physical_status == 
>>>> connector_status_connected) {
>>>> -        struct ast_device *ast = to_ast_device(connector->dev);
>>>> -        const struct drm_edid *drm_edid;
>>>> -
>>>> +    if (ast_connector->physical_status == connector_status_connected)
>>>>           drm_edid = drm_edid_read_custom(connector, 
>>>> ast_astdp_read_edid_block, ast);
>>>> -        drm_edid_connector_update(connector, drm_edid);
>>>> +
>>>> +    drm_edid_connector_update(connector, drm_edid);
>>>> +
>>>> +    if (drm_edid) {
>>>>           count = drm_edid_connector_add_modes(connector);
>>>>           drm_edid_free(drm_edid);
>>>>       } else {
>>>> -        drm_edid_connector_update(connector, NULL);
>>>> -
>>>>           /*
>>>>            * There's no EDID data without a connected monitor. Set 
>>>> BMC-
>>>>            * compatible modes in this case. The XGA default resolution
>>>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ 
>>>> ast_dp501.c
>>>> index 9e19d8c17730..c92db65e3f20 100644
>>>> --- a/drivers/gpu/drm/ast/ast_dp501.c
>>>> +++ b/drivers/gpu/drm/ast/ast_dp501.c
>>>
>>> I'd rather leave this out. The detection works differently for DP501.
>>
>> ast_dp501_is_connected() hasn't changed, so yes I can drop this part.
>>
>> Best regards,
>>
>

-- 
--
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)