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

[Jocelyn Falempe]

On 26/07/2023 17:46, Thomas Zimmermann wrote:
> 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 at redhat.com>
>>> ---
>>>   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?

Yes, sure.
> 
>>> +
>>> +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

Thanks a lot, I will push it on Friday, if there are no other comments.
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann at suse.de>
> 
> Thanks for the fix.
> 
> Best regards
> Thomas
> 
>>
>> Thanks,
>>
>