| Message ID | 20230712083733.223275-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] drm/ast: Add BMC virtual connector | expand |
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@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. > +} > + > +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.) > + > + 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.) > + 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. Best regards Thomas > + 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
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@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.
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); +} + +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; + + 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); + if (ret) + return ret; + encoder->possible_crtcs = drm_crtc_mask(crtc); + + ret = ast_bmc_connector_init(dev, connector); + 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);
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@redhat.com> --- drivers/gpu/drm/ast/ast_drv.h | 4 ++ drivers/gpu/drm/ast/ast_mode.c | 67 ++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2