[PATCH] drm/ast: Fix default resolution on BMC when DP is not connected
Jocelyn Falempe
jfalempe at redhat.com
Fri Jan 24 12:24:17 UTC 2025
On 24/01/2025 13:16, Thomas Zimmermann wrote:
> 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
Thanks, I don't know much about the internal of DP protocol, but I
though reading a register is faster than the whole EDID.
>
> 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.
ok, thanks a lot, I will send a v2 with this.
Best regards,
--
Jocelyn
>
> 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,
>>
>
More information about the dri-devel
mailing list