Re: [PATCH 3/3] drm/mgag200: Protect concurrent access to I/O registers with lock

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

On 02/05/2022 16:25, Thomas Zimmermann wrote:
> Add a mutex lock to protect concurrent access to I/O registers
> against each other. This happens between invokataion of commit-

Typo in commit message, invokataion => invocation

> tail functions and get-mode operations. Both with use the CRTC
> index registers MGA1064_GEN_IO_DATA and MGA1064_GEN_IO_CTL.
> Concurrent access can lead to failed mode-setting operations.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

There is a trivial conflict with 
https://lists.freedesktop.org/archives/dri-devel/2022-April/352966.html
But I will need to send a v2 for it anyway.

It looks good to me,
Reviewed-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>

> ---
>   drivers/gpu/drm/mgag200/mgag200_drv.c  |  6 ++++++
>   drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 14 ++++++++++++++
>   3 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 217844d71ab5c..08839460606fe 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -14,6 +14,7 @@
>   #include <drm/drm_drv.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_ioctl.h>
> +#include <drm/drm_managed.h>
>   #include <drm/drm_module.h>
>   #include <drm/drm_pciids.h>
>   
> @@ -65,6 +66,11 @@ static int mgag200_regs_init(struct mga_device *mdev)
>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>   	u32 option, option2;
>   	u8 crtcext3;
> +	int ret;
> +
> +	ret = drmm_mutex_init(dev, &mdev->rmmio_lock);
> +	if (ret)
> +		return ret;
>   
>   	switch (mdev->type) {
>   	case G200_PCI:
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 4368112023f7c..18829eb8398a0 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -213,6 +213,7 @@ struct mga_device {
>   	struct drm_device		base;
>   	unsigned long			flags;
>   
> +	struct mutex			rmmio_lock;
>   	resource_size_t			rmmio_base;
>   	resource_size_t			rmmio_size;
>   	void __iomem			*rmmio;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 6e18d3bbd7207..abde7655477db 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -881,6 +881,14 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>   		.y2 = fb->height,
>   	};
>   
> +	/*
> +	 * Concurrent operations could possibly trigger a call to
> +	 * drm_connector_helper_funcs.get_modes by trying to read the
> +	 * display modes. Protect access to I/O registers by acquiring
> +	 * the I/O-register lock.
> +	 */
> +	mutex_lock(&mdev->rmmio_lock);
> +
>   	if (mdev->type == G200_WB || mdev->type == G200_EW3)
>   		mgag200_g200wb_hold_bmc(mdev);
>   
> @@ -904,6 +912,8 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>   	mgag200_enable_display(mdev);
>   
>   	mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->data[0]);
> +
> +	mutex_unlock(&mdev->rmmio_lock);
>   }
>   
>   static void
> @@ -963,8 +973,12 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
>   	if (!fb)
>   		return;
>   
> +	mutex_lock(&mdev->rmmio_lock);
> +
>   	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
>   		mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]);
> +
> +	mutex_unlock(&mdev->rmmio_lock);
>   }
>   
>   static struct drm_crtc_state *