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@xxxxxxx>
Cc: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
Cc: Dave Airlie <airlied@xxxxxxxxxx>
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: <stable@xxxxxxxxxxxxxxx> # 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@xxxxxxxxxx>
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)
