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

[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?

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

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 (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,

-- 

Jocelyn

> 
> Best regards
> Thomas
> 
>> @@ -504,19 +504,19 @@ static const struct drm_encoder_helper_funcs 
>> ast_dp501_encoder_helper_funcs = {
>>   static int ast_dp501_connector_helper_get_modes(struct drm_connector 
>> *connector)
>>   {
>>       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_dp512_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
>>
>> base-commit: 798047e63ac970f105c49c22e6d44df901c486b2
>