| Message ID | 20230713134316.332502-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/2] drm/ast: Add BMC virtual connector | expand |
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@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, > +}; > + > +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. Thanks,
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@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? >> + >> +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@suse.de> Thanks for the fix. Best regards Thomas > > Thanks, >
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@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@suse.de> > > Thanks for the fix. > > Best regards > Thomas > >> >> Thanks, >> >
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, +}; + +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);
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@redhat.com> --- drivers/gpu/drm/ast/ast_drv.h | 4 +++ drivers/gpu/drm/ast/ast_mode.c | 58 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2