[PATCH] drm/ast: Fix default resolution on BMC when DP is not connected
Thomas Zimmermann
tzimmermann at suse.de
Fri Jan 24 12:16:21 UTC 2025
Hi
Am 24.01.25 um 11:04 schrieb Jocelyn Falempe:
> On 24/01/2025 10:22, Thomas Zimmermann wrote:
>> Hi Jocelyn
>>
>>
>> Am 24.01.25 um 09:45 schrieb Jocelyn Falempe:
>>> The physical_status of ast_dp is not reliable, as it reports as
>>> connected even when no monitor is connected.
>>
>> This status comes from VGACRDF, which is undocumented unfortunately.
>> So IDK the exact semantics. Do you know if the other case can also
>> happen where a connected monitor is not reported?
>
> Previously, astdp_is_connected() also checked for ASTDP_LINK_SUCCESS
> https://elixir.bootlin.com/linux/v6.10.14/source/drivers/gpu/drm/ast/ast_dp.c#L16
>
>
> which used to work more reliably, se maybe I can add it back?
That's a bit of a complicated issue. Protocol-wise Link Training runs
only after HPD, EDID and DPCD. A nice overview is at
https://www.ti.com/content/dam/videos/external-videos/en-us/8/3816841626001/6275649988001.mp4/subassets/understanding-dp-link-training-presentation-quiz.pdf
Link training is only to be done at atomic_enable or _check time. I
specifically asked about this back then.
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)
More information about the dri-devel
mailing list