Re: [PATCH v2 07/11] drm/udl: Drop unneeded alignment

[Thomas Zimmermann]

Hi

Am 06.09.22 um 09:39 schrieb Takashi Iwai:
> The alignment of damaged area was needed for the original udlfb driver
> that tried to trim the superfluous copies between front and backend
> buffers and handle data in long int.  It's not the case for udl DRM
> driver, hence we can omit the whole unneeded alignment, as well as the
> dead code.
>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>

Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

with an entirely optional comment below.

> ---
>   drivers/gpu/drm/udl/udl_modeset.c  | 28 +--------------------
>   drivers/gpu/drm/udl/udl_transfer.c | 40 ------------------------------
>   2 files changed, 1 insertion(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index c34d564773a3..9896c16c74f5 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
>   	return __ffs(cpp);
>   }
>   
> -static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
> -				   int width, int height)
> -{
> -	int x1, x2;
> -
> -	if (WARN_ON_ONCE(x < 0) ||
> -	    WARN_ON_ONCE(y < 0) ||
> -	    WARN_ON_ONCE(width < 0) ||
> -	    WARN_ON_ONCE(height < 0))
> -		return -EINVAL;
> -
> -	x1 = ALIGN_DOWN(x, sizeof(unsigned long));
> -	x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
> -
> -	clip->x1 = x1;
> -	clip->y1 = y;
> -	clip->x2 = x2;
> -	clip->y2 = y + height;
> -
> -	return 0;
> -}
> -
>   static int udl_handle_damage(struct drm_framebuffer *fb,
>   			     const struct iosys_map *map,
>   			     int x, int y, int width, int height)
> @@ -282,11 +260,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
>   		return ret;
>   	log_bpp = ret;
>   
> -	ret = udl_aligned_damage_clip(&clip, x, y, width, height);
> -	if (ret)
> -		return ret;
> -	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> -		return -EINVAL;
> +	drm_rect_init(&clip, x, y, width, height);

The clip rectangle could be passed directly by the caller, which would 
simplify the update function. But that's really just nitpicking.

Best regards
Thomas

>   
>   	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>   	if (ret)
> diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
> index 176ef2a6a731..a431208dda85 100644
> --- a/drivers/gpu/drm/udl/udl_transfer.c
> +++ b/drivers/gpu/drm/udl/udl_transfer.c
> @@ -25,46 +25,6 @@
>   #define MIN_RAW_PIX_BYTES	2
>   #define MIN_RAW_CMD_BYTES	(RAW_HEADER_BYTES + MIN_RAW_PIX_BYTES)
>   
> -/*
> - * Trims identical data from front and back of line
> - * Sets new front buffer address and width
> - * And returns byte count of identical pixels
> - * Assumes CPU natural alignment (unsigned long)
> - * for back and front buffer ptrs and width
> - */
> -#if 0
> -static int udl_trim_hline(const u8 *bback, const u8 **bfront, int *width_bytes)
> -{
> -	int j, k;
> -	const unsigned long *back = (const unsigned long *) bback;
> -	const unsigned long *front = (const unsigned long *) *bfront;
> -	const int width = *width_bytes / sizeof(unsigned long);
> -	int identical = width;
> -	int start = width;
> -	int end = width;
> -
> -	for (j = 0; j < width; j++) {
> -		if (back[j] != front[j]) {
> -			start = j;
> -			break;
> -		}
> -	}
> -
> -	for (k = width - 1; k > j; k--) {
> -		if (back[k] != front[k]) {
> -			end = k+1;
> -			break;
> -		}
> -	}
> -
> -	identical = start + (width - end);
> -	*bfront = (u8 *) &front[start];
> -	*width_bytes = (end - start) * sizeof(unsigned long);
> -
> -	return identical * sizeof(unsigned long);
> -}
> -#endif
> -
>   static inline u16 pixel32_to_be16(const uint32_t pixel)
>   {
>   	return (((pixel >> 3) & 0x001f) |

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature