Re: [PATCH v6 1/2] drm/ast: Add BMC virtual connector

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 26.07.23 um 10:41 schrieb Jocelyn Falempe:
> On 13/07/2023 15:41, Jocelyn Falempe wrote:
> > Most aspeed devices have a BMC, which allows to remotely see the screen.
> > Also in the common use case, those servers don't have a display 
> > connected.
> > So add a Virtual connector, to reflect that even if no display is
> > connected, the framebuffer can still be seen remotely.
> > This prepares the work to implement a detect_ctx() for the Display port
> > connector.
> >
> > v4: call drm_add_modes_noedid() with 4096x4096 (Thomas Zimmermann)
> >      remove useless struct field init to 0 (Thomas Zimmermann)
> >      don't use drm_simple_encoder_init() (Thomas Zimmermann)
> >      inline ast_bmc_connector_init() (Thomas Zimmermann)
> >
> > Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID 
> > on DP")
> > Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/ast/ast_drv.h  |  4 +++
> >   drivers/gpu/drm/ast/ast_mode.c | 58 ++++++++++++++++++++++++++++++++++
> >   2 files changed, 62 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_drv.h 
> > b/drivers/gpu/drm/ast/ast_drv.h
> > index 3f6e0c984523..c9659e72002f 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.h
> > +++ b/drivers/gpu/drm/ast/ast_drv.h
> > @@ -214,6 +214,10 @@ struct ast_device {
> >               struct drm_encoder encoder;
> >               struct drm_connector connector;
> >           } astdp;
> > +        struct {
> > +            struct drm_encoder encoder;
> > +            struct drm_connector connector;
> > +        } bmc;
> >       } output;
> >       bool support_wide_screen;
> > diff --git a/drivers/gpu/drm/ast/ast_mode.c 
> > b/drivers/gpu/drm/ast/ast_mode.c
> > index f711d592da52..1a8293162fec 100644
> > --- a/drivers/gpu/drm/ast/ast_mode.c
> > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > @@ -1735,6 +1735,61 @@ static int ast_astdp_output_init(struct 
> > ast_device *ast)
> >       return 0;
> >   }
> > +/*
> > + * BMC virtual Connector
> > + */
> > +
> > +static int ast_bmc_connector_helper_get_modes(struct drm_connector 
> > *connector)
> > +{
> > +    return drm_add_modes_noedid(connector, 4096, 4096);
> > +}
> > +
> > +static const struct drm_connector_helper_funcs 
> > ast_bmc_connector_helper_funcs = {
> > +    .get_modes = ast_bmc_connector_helper_get_modes,
> > +};
> > +
> > +static const struct drm_connector_funcs ast_bmc_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,
> > +};
> > +
> > +static const struct drm_encoder_funcs ast_bmc_encoder_funcs = {
> > +    .destroy = drm_encoder_cleanup,
> > +};

Can you please move the encoder_funcs struct before the connector stuff?

> > +
> > +static int ast_bmc_output_init(struct ast_device *ast)
> > +{
> > +    struct drm_device *dev = &ast->base;
> > +    struct drm_crtc *crtc = &ast->crtc;
> > +    struct drm_encoder *encoder = &ast->output.bmc.encoder;
> > +    struct drm_connector *connector = &ast->output.bmc.connector;
> > +    int ret;
> > +
> > +
> > +    ret = drm_encoder_init(dev, encoder,
> > +                &ast_bmc_encoder_funcs,
> > +                DRM_MODE_ENCODER_VIRTUAL, "ast_bmc");
> > +    if (ret)
> > +        return ret;
> > +    encoder->possible_crtcs = drm_crtc_mask(crtc);
> > +
> > +    ret = drm_connector_init(dev, connector, &ast_bmc_connector_funcs,
> > +                 DRM_MODE_CONNECTOR_VIRTUAL);
> > +    if (ret)
> > +        return ret;
> > +
> > +    drm_connector_helper_add(connector, 
> > &ast_bmc_connector_helper_funcs);
> > +
> > +    ret = drm_connector_attach_encoder(connector, encoder);
> > +    if (ret)
> > +        return ret;
> > +
> > +    return 0;
> > +}
> > +
> >   /*
> >    * Mode config
> >    */
> > @@ -1842,6 +1897,9 @@ int ast_mode_config_init(struct ast_device *ast)
> >           if (ret)
> >               return ret;
> >       }
> > +    ret = ast_bmc_output_init(ast);
> > +    if (ret)
> > +        return ret;
> >       drm_mode_config_reset(dev);
> >
> > base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2
>
>
> I'm missing a review on this patch. The VGA CRD0[7] register check 
> doesn't work on my server, so I'm adding the BMC virtual connector for 
> all aspeed hardware, but as discussed, it shouldn't have a negative impact.

Sorry. I simply missed this patch. With my little nitpick fixed, you can add

Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

Thanks for the fix.

Best regards
Thomas

> Thanks,

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature