Re: [PATCH 2/4] drm/mgag200: Start/stop BMC scanout in BMC encoder helpers

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

On 03/07/2024 15:40, Thomas Zimmermann wrote:
> Start and stop the BMC scanout from the BMC encoder's atomic_enable
> and atomic_disable helpers. The BMC stops scanning out at the beginning
> of a modeset operation and restarts the scanout at the end of the
> modeset.
>
> Only G200EW3 and G200WB require this procedure. Drop the original
> vidrst callbacks for these model's support, as they are now obsolete.

Thanks, that makes the BMC connector more useful.

I'm just wondering, in case you use both a VGA output and the BMC, 
before this patch, vidrst would be enabled, and after, as the BMC 
connector is "unconnected" it should be disabled ?

Will that have a visible impact for users ?


> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
>   drivers/gpu/drm/mgag200/mgag200_bmc.c     | 24 +++++++++++++++++++++--
>   drivers/gpu/drm/mgag200/mgag200_drv.h     |  4 +---
>   drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  2 --
>   drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  2 --
>   4 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> index cb5400333862..74feb6d8db45 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> @@ -14,7 +14,7 @@ static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connect
>   	return container_of(connector, struct mgag200_bmc_connector, base);
>   }
>   
> -void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
> +static void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
>   {
>   	u8 tmp;
>   	int iter_max;
> @@ -73,7 +73,16 @@ void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
>   	}
>   }
>   
> -void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
> +static void mgag200_bmc_encoder_helper_atomic_disable(struct drm_encoder *encoder,
> +						      struct drm_atomic_state *state)
> +{
> +	struct mga_device *mdev = to_mga_device(encoder->dev);
> +
> +	if (mdev->info->has_vidrst)
> +		mgag200_bmc_disable_vidrst(mdev);
> +}
> +
> +static void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
>   {
>   	u8 tmp;
>   
> @@ -103,6 +112,15 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
>   	WREG_DAC(MGA1064_GEN_IO_DATA, tmp);
>   }
>   
> +static void mgag200_bmc_encoder_helper_atomic_enable(struct drm_encoder *encoder,
> +						     struct drm_atomic_state *state)
> +{
> +	struct mga_device *mdev = to_mga_device(encoder->dev);
> +
> +	if (mdev->info->has_vidrst)
> +		mgag200_bmc_enable_vidrst(mdev);
> +}
> +
>   static int mgag200_bmc_encoder_helper_atomic_check(struct drm_encoder *encoder,
>   						   struct drm_crtc_state *crtc_state,
>   						   struct drm_connector_state *conn_state)
> @@ -119,6 +137,8 @@ static int mgag200_bmc_encoder_helper_atomic_check(struct drm_encoder *encoder,
>   }
>   
>   static const struct drm_encoder_helper_funcs mgag200_bmc_encoder_helper_funcs = {
> +	.atomic_disable = mgag200_bmc_encoder_helper_atomic_disable,
> +	.atomic_enable = mgag200_bmc_encoder_helper_atomic_enable,
>   	.atomic_check = mgag200_bmc_encoder_helper_atomic_check,
>   };
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 4b75613de882..b1365795234b 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -442,9 +442,7 @@ int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_avail
>   /* mgag200_vga.c */
>   int mgag200_vga_output_init(struct mga_device *mdev);
>   
> -				/* mgag200_bmc.c */
> -void mgag200_bmc_disable_vidrst(struct mga_device *mdev);
> -void mgag200_bmc_enable_vidrst(struct mga_device *mdev);
> +/* mgag200_bmc.c */
>   int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector);
>   
>   #endif				/* __MGAG200_DRV_H__ */
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> index 839401e8b465..265f3e95830a 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> @@ -146,8 +146,6 @@ static const struct mgag200_device_info mgag200_g200ew3_device_info =
>   	MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, true, 0, 1, false);
>   
>   static const struct mgag200_device_funcs mgag200_g200ew3_device_funcs = {
> -	.disable_vidrst = mgag200_bmc_disable_vidrst,
> -	.enable_vidrst = mgag200_bmc_enable_vidrst,
>   	.pixpllc_atomic_check = mgag200_g200ew3_pixpllc_atomic_check,
>   	.pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update, // same as G200WB
>   };
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> index 835df0f4fc13..e25477347c3e 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> @@ -280,8 +280,6 @@ static const struct mgag200_device_info mgag200_g200wb_device_info =
>   	MGAG200_DEVICE_INFO_INIT(1280, 1024, 31877, true, 0, 1, false);
>   
>   static const struct mgag200_device_funcs mgag200_g200wb_device_funcs = {
> -	.disable_vidrst = mgag200_bmc_disable_vidrst,
> -	.enable_vidrst = mgag200_bmc_enable_vidrst,
>   	.pixpllc_atomic_check = mgag200_g200wb_pixpllc_atomic_check,
>   	.pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update,
>   };