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

[Jocelyn Falempe]

On 12/07/2023 12:44, Thomas Zimmermann wrote:
> Hi,
> 
> thanks for this patch.
> 
> Am 12.07.23 um 10:35 schrieb Jocelyn Falempe:
>> 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.
>>
>> 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 | 67 ++++++++++++++++++++++++++++++++++
>>   2 files changed, 71 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..8896b22eb5cf 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -1735,6 +1735,70 @@ 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, 1024, 768);
> 
> That's probably too small. The CRTC lists resolutions up to 1920x1200. 
> Returning 1024x768 could easily filter out those higher-res modes.
> 
> The BMC can probably just use whatever the CRTC offers. So rather call 
> drm_add_modes_noedid() with 4096x4096. At 32 bpp, this covers the max 
> memory of 64 MiB.  The CRTC will filter out unsupported modes.

Thanks for pointing this, I didn't realize it will prevent higher 
resolutions. With this change, the bmc resolution becomes 1920x1200 
(with "disabled vga" and no DP connected), which is much nicer.

With vga, it stays at 1024x768. So maybe adding a .detect_ctx() for vga 
too would be worth it. But that's for another series.

> 
> 
>> +}
>> +
>> +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 int ast_bmc_connector_init(struct drm_device *dev,
>> +                  struct drm_connector *connector)
>> +{
>> +    int ret;
>> +
>> +    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);
>> +
>> +    connector->interlace_allowed = 0;
>> +    connector->doublescan_allowed = 0;
>> +    connector->polled = 0;
> 
> It's zero-allocated memory. Please don't clear these fields manually. (I 
> know that ast doesn't get this right.)

Done

> 
>> +
>> +    return 0;
>> +}
>> +
>> +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_simple_encoder_init(dev, encoder, 
>> DRM_MODE_ENCODER_VIRTUAL);
> 
> Adding the simple_encoder helper was a mistake. Please open-code its 
> functionality in ast. (Also something that ast currently does not.)

ok, it's simple enough to call drm_encoder_init() directly.

> 
>> +    if (ret)
>> +        return ret;
>> +    encoder->possible_crtcs = drm_crtc_mask(crtc);
>> +
>> +    ret = ast_bmc_connector_init(dev, connector);
> 
> Maybe just inline this call. It's simple enough.
Done

> 
> Best regards
> Thomas

Thanks for the review, I will push a v4 shortly.

-- 

Jocelyn

> 
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = drm_connector_attach_encoder(connector, encoder);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return 0;
>> +}
>> +
>>   /*
>>    * Mode config
>>    */
>> @@ -1842,6 +1906,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
>