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

Jocelyn Falempe jfalempe at redhat.com
Mon Jul 10 08:48:44 UTC 2023


On 10/07/2023 08:57, Thomas Zimmermann wrote:
> Hi Jocelyn
> 
> Am 06.07.23 um 11:58 schrieb Jocelyn Falempe:
>> Aspeed always report the display port as "connected", because it
>> doesn't set a .detect 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.
> 
> The formating of these paraghraphs looks off.

Ok

> 
>>
>> v2: Add .detect callback to the dp/dp501 connector (Jani Nikula)
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> 
> Maybe add Fixes with the patch that added the DP default resolution. 
> It's not really correct, but at least tells people to consider your patch.
> 
> I assume that you'll send another patch with the BMC connector?

Yes, this patch alone will break more systems than what it tries to fix.

> 
>> ---
>>   drivers/gpu/drm/ast/ast_dp.c    |  9 ++++++++
>>   drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++-----------
>>   drivers/gpu/drm/ast/ast_drv.h   |  2 ++
>>   drivers/gpu/drm/ast/ast_mode.c  | 24 +++++++++++++++++++++
>>   4 files changed, 60 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
>> index 6dc1a09504e1..fbc154930fdf 100644
>> --- a/drivers/gpu/drm/ast/ast_dp.c
>> +++ b/drivers/gpu/drm/ast/ast_dp.c
>> @@ -7,6 +7,15 @@
>>   #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) &&
>> +        ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, 
>> ASTDP_LINK_SUCCESS) &&
>> +        ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, ASTDP_HPD))
> 
> The indention of those two lines is off.
> 
> You could also rewrite this as
> 
>   if (!a)
>     return false;
>   if (!b)
>     return false;
>   if (!c)
>     return false;
>   return true;

ok

> 
>> +        return true;
>> +    return false;
>> +}
>> +
>>   int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
>>   {
>>       struct ast_device *ast = to_ast_device(dev);
>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c 
>> b/drivers/gpu/drm/ast/ast_dp501.c
>> index a5d285850ffb..f10d53b0c94f 100644
>> --- a/drivers/gpu/drm/ast/ast_dp501.c
>> +++ b/drivers/gpu/drm/ast/ast_dp501.c
>> @@ -272,11 +272,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
>>       return true;
>>   }
>> -bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
>> +bool ast_dp501_is_connected(struct ast_device *ast)
>>   {
>> -    struct ast_device *ast = to_ast_device(dev);
>> -    u32 i, boot_address, offset, data;
>> -    u32 *pEDIDidx;
>> +    u32 boot_address, offset, data;
>>       if (ast->config_mode == ast_use_p2a) {
>>           boot_address = get_fw_base(ast);
>> @@ -292,14 +290,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, 
>> u8 *ediddata)
>>           data = ast_mindwm(ast, boot_address + offset);
>>           if (!(data & AST_DP501_PNP_CONNECTED))
>>               return false;
>> -
>> -        /* Read EDID */
>> -        offset = AST_DP501_EDID_DATA;
>> -        for (i = 0; i < 128; i += 4) {
>> -            data = ast_mindwm(ast, boot_address + offset + i);
>> -            pEDIDidx = (u32 *)(ediddata + i);
>> -            *pEDIDidx = data;
>> -        }
>>       } else {
>>           if (!ast->dp501_fw_buf)
>>               return false;
>> @@ -319,7 +309,30 @@ bool ast_dp501_read_edid(struct drm_device *dev, 
>> u8 *ediddata)
>>           data = readl(ast->dp501_fw_buf + offset);
>>           if (!(data & AST_DP501_PNP_CONNECTED))
>>               return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
>> +{
>> +    struct ast_device *ast = to_ast_device(dev);
>> +    u32 i, boot_address, offset, data;
>> +    u32 *pEDIDidx;
>> +
>> +    if (!ast_dp501_is_connected(ast))
>> +        return false;
>> +
>> +    if (ast->config_mode == ast_use_p2a) {
>> +        boot_address = get_fw_base(ast);
>> +        /* Read EDID */
>> +        offset = AST_DP501_EDID_DATA;
>> +        for (i = 0; i < 128; i += 4) {
>> +            data = ast_mindwm(ast, boot_address + offset + i);
>> +            pEDIDidx = (u32 *)(ediddata + i);
>> +            *pEDIDidx = data;
>> +        }
>> +    } else {
>>           /* Read EDID */
>>           offset = AST_DP501_EDID_DATA;
>>           for (i = 0; i < 128; i += 4) {
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>> b/drivers/gpu/drm/ast/ast_drv.h
>> index 3f6e0c984523..99a24d779b9c 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -510,6 +510,7 @@ void ast_patch_ahb_2500(struct ast_device *ast);
>>   /* ast dp501 */
>>   void ast_set_dp501_video_output(struct drm_device *dev, u8 mode);
>>   bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
>> +bool ast_dp501_is_connected(struct ast_device *ast);
>>   bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
>>   u8 ast_get_dp501_max_clk(struct drm_device *dev);
>>   void ast_init_3rdtx(struct drm_device *dev);
>> @@ -518,6 +519,7 @@ void ast_init_3rdtx(struct drm_device *dev);
>>   struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
>>   /* aspeed DP */
>> +bool ast_astdp_is_connected(struct ast_device *ast);
>>   int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata);
>>   void ast_dp_launch(struct drm_device *dev);
>>   void ast_dp_power_on_off(struct drm_device *dev, bool no);
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>> b/drivers/gpu/drm/ast/ast_mode.c
>> index f711d592da52..ccf48d57d239 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -1586,12 +1586,24 @@ static const struct drm_connector_helper_funcs 
>> ast_dp501_connector_helper_funcs
>>       .get_modes = ast_dp501_connector_helper_get_modes,
>>   };
>> +static enum drm_connector_status ast_dp501_connector_helper_detect(
>> +    struct drm_connector *connector,
>> +    bool force)
>> +{
>> +    struct ast_device *ast = to_ast_device(connector->dev);
>> +
>> +    if (ast_dp501_is_connected(ast))
>> +        return connector_status_connected;
>> +    return connector_status_disconnected;
>> +}
> 
> It seems more appropriate to implement this as connector helper func 
> detect_ctx. It does the same in practice, but the detect funtionality is 
> rather part of the helpers.

ok, it looks easy to switch to detect_ctx().

> 
>> +
>>   static const struct drm_connector_funcs ast_dp501_connector_funcs = {
>>       .reset = drm_atomic_helper_connector_reset,
>>       .fill_modes = drm_helper_probe_single_connector_modes,
>>       .destroy = drm_connector_cleanup,
>>       .atomic_duplicate_state = 
>> drm_atomic_helper_connector_duplicate_state,
>>       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +    .detect = ast_dp501_connector_helper_detect,
> 
> That might sound pedantic, but I'd prefer to have the assignments sorted 
> as in the struct's definiation. :) So .detect would be set just below 
> .reset.

ok, I was wondering what the order was, as it was not alphabetically.

> 
>>   };
>>   static int ast_dp501_connector_init(struct drm_device *dev, struct 
>> drm_connector *connector)
>> @@ -1684,12 +1696,24 @@ static const struct drm_connector_helper_funcs 
>> ast_astdp_connector_helper_funcs
>>       .get_modes = ast_astdp_connector_helper_get_modes,
>>   };
>> +static enum drm_connector_status ast_astdp_connector_helper_detect(
>> +    struct drm_connector *connector,
>> +    bool force)
>> +{
>> +    struct ast_device *ast = to_ast_device(connector->dev);
>> +
>> +    if (ast_astdp_is_connected(ast))
>> +        return connector_status_connected;
>> +    return connector_status_disconnected;
>> +}
>> +
>>   static const struct drm_connector_funcs ast_astdp_connector_funcs = {
>>       .reset = drm_atomic_helper_connector_reset,
>>       .fill_modes = drm_helper_probe_single_connector_modes,
>>       .destroy = drm_connector_cleanup,
>>       .atomic_duplicate_state = 
>> drm_atomic_helper_connector_duplicate_state,
>>       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +    .detect = ast_astdp_connector_helper_detect,
> 
> Same here.
> 
> Best regards
> Thomas
> 
>>   };
>>   static int ast_astdp_connector_init(struct drm_device *dev, struct 
>> drm_connector *connector)
>>
>> base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2
> 

Thanks for your review.
I will try to add a virtual BMC connector, and send a v3.

-- 

Jocelyn



More information about the dri-devel mailing list