Re: [PATCH v2 7/9] drm/mgag200: Rewrite cursor handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 11, 2019 at 03:03:42PM +0200, Thomas Zimmermann wrote:
> The cursor handling in mgag200 is complicated to understand. It touches a
> number of different BOs, but doesn't really use all of them.
> 
> Rewriting the cursor update reduces the amount of cursor state. There are
> two BOs for double-buffered HW updates. The source BO updates the one that
> is currently not displayed and then switches buffers. Explicit BO locking
> has been removed from the code. BOs are simply pinned and unpinned in video
> RAM.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Acked-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 165 ++++++++++-------------
>  drivers/gpu/drm/mgag200/mgag200_drv.h    |   3 -
>  drivers/gpu/drm/mgag200/mgag200_main.c   |   4 +-
>  3 files changed, 69 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> index de94a650077b..5a22ef825588 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> @@ -19,10 +19,9 @@ static void mga_hide_cursor(struct mga_device *mdev)
>  {
>  	WREG8(MGA_CURPOSXL, 0);
>  	WREG8(MGA_CURPOSXH, 0);
> -	if (mdev->cursor.pixels_1->pin_count)
> -		drm_gem_vram_unpin_locked(mdev->cursor.pixels_1);
> -	if (mdev->cursor.pixels_2->pin_count)
> -		drm_gem_vram_unpin_locked(mdev->cursor.pixels_2);
> +	if (mdev->cursor.pixels_current)
> +		drm_gem_vram_unpin(mdev->cursor.pixels_current);
> +	mdev->cursor.pixels_current = NULL;
>  }
>  
>  int mga_crtc_cursor_set(struct drm_crtc *crtc,
> @@ -36,7 +35,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
>  	struct drm_gem_vram_object *pixels_1 = mdev->cursor.pixels_1;
>  	struct drm_gem_vram_object *pixels_2 = mdev->cursor.pixels_2;
>  	struct drm_gem_vram_object *pixels_current = mdev->cursor.pixels_current;
> -	struct drm_gem_vram_object *pixels_prev = mdev->cursor.pixels_prev;
> +	struct drm_gem_vram_object *pixels_next;
>  	struct drm_gem_object *obj;
>  	struct drm_gem_vram_object *gbo = NULL;
>  	int ret = 0;
> @@ -49,6 +48,7 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
>  	bool found = false;
>  	int colour_count = 0;
>  	s64 gpu_addr;
> +	u64 dst_gpu;
>  	u8 reg_index;
>  	u8 this_row[48];
>  
> @@ -58,80 +58,66 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc,
>  		return -ENOTSUPP; /* Didn't allocate space for cursors */
>  	}
>  
> -	if ((width != 64 || height != 64) && handle) {
> -		WREG8(MGA_CURPOSXL, 0);
> -		WREG8(MGA_CURPOSXH, 0);
> -		return -EINVAL;
> +	if (WARN_ON(pixels_current &&
> +		    pixels_1 != pixels_current &&
> +		    pixels_2 != pixels_current)) {
> +		return -ENOTSUPP; /* inconsistent state */
>  	}
>  
> -	BUG_ON(pixels_1 != pixels_current && pixels_1 != pixels_prev);
> -	BUG_ON(pixels_2 != pixels_current && pixels_2 != pixels_prev);
> -	BUG_ON(pixels_current == pixels_prev);
> -
>  	if (!handle || !file_priv) {
>  		mga_hide_cursor(mdev);
>  		return 0;
>  	}
>  
> -	obj = drm_gem_object_lookup(file_priv, handle);
> -	if (!obj)
> -		return -ENOENT;
> -
> -	ret = drm_gem_vram_lock(pixels_1, true);
> -	if (ret) {
> +	if (width != 64 || height != 64) {
>  		WREG8(MGA_CURPOSXL, 0);
>  		WREG8(MGA_CURPOSXH, 0);
> -		goto out_unref;
> -	}
> -	ret = drm_gem_vram_lock(pixels_2, true);
> -	if (ret) {
> -		WREG8(MGA_CURPOSXL, 0);
> -		WREG8(MGA_CURPOSXH, 0);
> -		drm_gem_vram_unlock(pixels_1);
> -		goto out_unlock1;
> +		return -EINVAL;
>  	}
>  
> -	/* Move cursor buffers into VRAM if they aren't already */
> -	if (!pixels_1->pin_count) {
> -		ret = drm_gem_vram_pin_locked(pixels_1,
> -					      DRM_GEM_VRAM_PL_FLAG_VRAM);
> -		if (ret)
> -			goto out1;
> -		gpu_addr = drm_gem_vram_offset(pixels_1);
> -		if (gpu_addr < 0) {
> -			drm_gem_vram_unpin_locked(pixels_1);
> -			goto out1;
> -		}
> -		mdev->cursor.pixels_1_gpu_addr = gpu_addr;
> -	}
> -	if (!pixels_2->pin_count) {
> -		ret = drm_gem_vram_pin_locked(pixels_2,
> -					      DRM_GEM_VRAM_PL_FLAG_VRAM);
> -		if (ret) {
> -			drm_gem_vram_unpin_locked(pixels_1);
> -			goto out1;
> -		}
> -		gpu_addr = drm_gem_vram_offset(pixels_2);
> -		if (gpu_addr < 0) {
> -			drm_gem_vram_unpin_locked(pixels_1);
> -			drm_gem_vram_unpin_locked(pixels_2);
> -			goto out1;
> -		}
> -		mdev->cursor.pixels_2_gpu_addr = gpu_addr;
> -	}
> +	if (pixels_current == pixels_1)
> +		pixels_next = pixels_2;
> +	else
> +		pixels_next = pixels_1;
>  
> +	obj = drm_gem_object_lookup(file_priv, handle);
> +	if (!obj)
> +		return -ENOENT;
>  	gbo = drm_gem_vram_of_gem(obj);
> -	ret = drm_gem_vram_lock(gbo, true);
> +	ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_SYSTEM);

pl_flag = 0 should be fine here too.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux