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

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > ---
> >   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.


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,

--

Jocelyn

> > +
> >   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 c9659e72002f..848a9f1403e8 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.h
> > +++ b/drivers/gpu/drm/ast/ast_drv.h
> > @@ -514,6 +514,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);
> > @@ -522,6 +523,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 1a8293162fec..e7f36ec73817 100644
> > --- a/drivers/gpu/drm/ast/ast_mode.c
> > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > @@ -1582,8 +1582,21 @@ static int 
> > ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
> >       return 0;
> >   }
> > +static int ast_dp501_connector_helper_detect_ctx(struct drm_connector 
> > *connector,
> > +                         struct drm_modeset_acquire_ctx *ctx,
> > +                         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;
> > +}
> > +
> > +
> >   static const struct drm_connector_helper_funcs 
> > ast_dp501_connector_helper_funcs = {
> >       .get_modes = ast_dp501_connector_helper_get_modes,
> > +    .detect_ctx = ast_dp501_connector_helper_detect_ctx,
> >   };
> >   static const struct drm_connector_funcs ast_dp501_connector_funcs = {
> > @@ -1608,7 +1621,7 @@ static int ast_dp501_connector_init(struct 
> > drm_device *dev, struct drm_connector
> >       connector->interlace_allowed = 0;
> >       connector->doublescan_allowed = 0;
> > -    connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> > +    connector->polled = DRM_CONNECTOR_POLL_CONNECT | 
> > DRM_CONNECTOR_POLL_DISCONNECT;
> >       return 0;
> >   }
> > @@ -1680,8 +1693,20 @@ static int 
> > ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
> >       return 0;
> >   }
> > +static int ast_astdp_connector_helper_detect_ctx(struct drm_connector 
> > *connector,
> > +                         struct drm_modeset_acquire_ctx *ctx,
> > +                         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_helper_funcs 
> > ast_astdp_connector_helper_funcs = {
> >       .get_modes = ast_astdp_connector_helper_get_modes,
> > +    .detect_ctx = ast_astdp_connector_helper_detect_ctx,
> >   };
> >   static const struct drm_connector_funcs ast_astdp_connector_funcs = {
> > @@ -1706,7 +1731,7 @@ static int ast_astdp_connector_init(struct 
> > drm_device *dev, struct drm_connector
> >       connector->interlace_allowed = 0;
> >       connector->doublescan_allowed = 0;
> > -    connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> > +    connector->polled = DRM_CONNECTOR_POLL_CONNECT | 
> > DRM_CONNECTOR_POLL_DISCONNECT;
> >       return 0;
> >   }
> > @@ -1903,5 +1928,7 @@ int ast_mode_config_init(struct ast_device *ast)
> >       drm_mode_config_reset(dev);
> > +    drm_kms_helper_poll_init(dev);
> > +
> >       return 0;
> >   }