[PATCH v5 2/2] drm/ast: report connection status on Display Port.

[Jocelyn Falempe]

On 13/07/2023 10:17, Jocelyn Falempe wrote:
> On 13/07/2023 09:00, Jammy Huang wrote:
>> Hi Jocelyn,
>>
>> Thanks for your work.
>>
>> On 2023/7/13 下午 02:40, Jocelyn Falempe wrote:
>>> Aspeed always report the display port as "connected", because it
>>> doesn't set a .detect_ctx callback.
>>> Fix this by providing the proper detect callback for astdp and dp501.
>>>
>>> This also fixes the following regression:
>>> Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no
>>> EDID on DP") The default resolution is now 640x480 when no monitor is
>>> connected. But Aspeed graphics is mostly used in servers, where no 
>>> monitor
>>> is attached. This also affects the remote BMC resolution to 640x480, 
>>> which
>>> is inconvenient, and breaks the anaconda installer.
>>>
>>> v2: Add .detect callback to the dp/dp501 connector (Jani Nikula)
>>> v3: Use .detect_ctx callback, and refactors (Thomas Zimmermann)
>>>      Add a BMC virtual connector
>>> v4: Better indent detect_ctx() functions (Thomas Zimmermann)
>>> v5: Enable polling of the dp and dp501 connector status
>>>      (Thomas Zimmermann)
>>>
>>> Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID 
>>> on DP")
>>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>>> Reviewed-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> ---
>>>   drivers/gpu/drm/ast/ast_dp.c    | 11 ++++++++++
>>>   drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++-----------
>>>   drivers/gpu/drm/ast/ast_drv.h   |  2 ++
>>>   drivers/gpu/drm/ast/ast_mode.c  | 31 +++++++++++++++++++++++++--
>>>   4 files changed, 67 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
>>> index 6dc1a09504e1..bf78f3d4aa3f 100644
>>> --- a/drivers/gpu/drm/ast/ast_dp.c
>>> +++ b/drivers/gpu/drm/ast/ast_dp.c
>>> @@ -7,6 +7,17 @@
>>>   #include <drm/drm_print.h>
>>>   #include "ast_drv.h"
>>> +bool ast_astdp_is_connected(struct ast_device *ast)
>>> +{
>>> +    if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, 
>>> ASTDP_MCU_FW_EXECUTING))
>>> +        return false;
>>> +    if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, 
>>> ASTDP_LINK_SUCCESS))
>>> +        return false;
>>> +    if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, 
>>> ASTDP_HPD))
>>> +        return false;
>>> +    return true;
>>> +}
>>
>> * ASPDP_LINK_SUCCESS is true, when DP link training is finished. The 
>> DP link quality is good
>>
>> enough to deliver video data.
>>
>> * ASTDP_HPD is true, when there is DP sink pull high HPD.
>>
>> Thus, ASTDP_HPD is the prerequisite of ASTDP_LINK_SUCCESS. I would 
>> suggest to remove
>>
>> the check for ASTDP_LINK_SUCCESS here. ASTDP_HPD is good enough for 
>> connected status.
>>
>> If you want to check all these three status, please change the order, 
>> FW_EXECUTING -> HPD ->
>>
>> LINK_SUCCESS.
> 
> Thanks for the detailed explanations.
> I looked at other drivers to see if HPD is good enough for "connected" 
> status, but I didn't find a clear answer.
> There is also a drm_link_status, but that looks to be mostly unused.
> https://elixir.bootlin.com/linux/latest/source/include/drm/drm_connector.h#L331
> 
> So I think I will follow your suggestion, and remove the check for 
> ASTDP_LINK_SUCCESS.

I've run a test on my server, and even when nothing is plugged, the 
ASTDP_HPD is still true. So I will just change the order of the checks, 
and keep the ASTDP_LINK_SUCCESS.

Best regards,

-- 

Jocelyn
> 
> 
> For the BMC connector patch, you know if there is a register setting I 
> can check to see if a BMC is present or not ?
> 
> Best regards,
>