Re: [PATCH 1/9] drm/format-helper: Pass destination pitch to drm_fb_memcpy_dstclip()

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5216 bytes --]

Hi

Am 29.06.20 um 10:40 schrieb Daniel Vetter:
> On Thu, Jun 25, 2020 at 02:00:03PM +0200, Thomas Zimmermann wrote:
>> The memcpy's destination buffer might have a different pitch than the
>> source. Support different pitches as function argument.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But I do have questions ... why did we allocate a source drm_framebuffer
> with mismatching pitch? That sounds backwards, especially for simplekms.

There's userspace that allocates framebuffers in tiles of 64x64 pixels.
I think I've seen this with Gnome. So if you have a 800x600 display
mode, the allocated framebuffer has a scanline pitch of 832 pixels and
the final 32 pixels are ignored.

In regular drivers, we can handle this with the VGA offset register [1]
or some equivalent. That's obviously not an option with simplekms, so
the different pitch is required.

Best regards
Thomas

[1]
https://web.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/crtcreg.htm#13

> 
> Would be good to add the reasons why we need this to the commit message,
> I'm sure I'll discover it later on eventually.
> -Daniel
> 
>> ---
>>  drivers/gpu/drm/drm_format_helper.c    | 9 +++++----
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 2 +-
>>  drivers/gpu/drm/tiny/cirrus.c          | 2 +-
>>  include/drm/drm_format_helper.h        | 2 +-
>>  4 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index c043ca364c86..8d5a683afea7 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -52,6 +52,7 @@ EXPORT_SYMBOL(drm_fb_memcpy);
>>  /**
>>   * drm_fb_memcpy_dstclip - Copy clip buffer
>>   * @dst: Destination buffer (iomem)
>> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
>>   * @vaddr: Source buffer
>>   * @fb: DRM framebuffer
>>   * @clip: Clip rectangle area to copy
>> @@ -59,12 +60,12 @@ EXPORT_SYMBOL(drm_fb_memcpy);
>>   * This function applies clipping on dst, i.e. the destination is a
>>   * full (iomem) framebuffer but only the clip rect content is copied over.
>>   */
>> -void drm_fb_memcpy_dstclip(void __iomem *dst, void *vaddr,
>> -			   struct drm_framebuffer *fb,
>> +void drm_fb_memcpy_dstclip(void __iomem *dst, unsigned int dst_pitch,
>> +			   void *vaddr, struct drm_framebuffer *fb,
>>  			   struct drm_rect *clip)
>>  {
>>  	unsigned int cpp = fb->format->cpp[0];
>> -	unsigned int offset = clip_offset(clip, fb->pitches[0], cpp);
>> +	unsigned int offset = clip_offset(clip, dst_pitch, cpp);
>>  	size_t len = (clip->x2 - clip->x1) * cpp;
>>  	unsigned int y, lines = clip->y2 - clip->y1;
>>  
>> @@ -73,7 +74,7 @@ void drm_fb_memcpy_dstclip(void __iomem *dst, void *vaddr,
>>  	for (y = 0; y < lines; y++) {
>>  		memcpy_toio(dst, vaddr, len);
>>  		vaddr += fb->pitches[0];
>> -		dst += fb->pitches[0];
>> +		dst += dst_pitch;
>>  	}
>>  }
>>  EXPORT_SYMBOL(drm_fb_memcpy_dstclip);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index f16bd278ab7e..7d4f3a62d885 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -1586,7 +1586,7 @@ mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb,
>>  	if (drm_WARN_ON(dev, !vmap))
>>  		return; /* BUG: SHMEM BO should always be vmapped */
>>  
>> -	drm_fb_memcpy_dstclip(mdev->vram, vmap, fb, clip);
>> +	drm_fb_memcpy_dstclip(mdev->vram, fb->pitches[0], vmap, fb, clip);
>>  
>>  	drm_gem_shmem_vunmap(fb->obj[0], vmap);
>>  
>> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
>> index 744a8e337e41..2dd9e5e31e3d 100644
>> --- a/drivers/gpu/drm/tiny/cirrus.c
>> +++ b/drivers/gpu/drm/tiny/cirrus.c
>> @@ -327,7 +327,7 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
>>  		goto out_dev_exit;
>>  
>>  	if (cirrus->cpp == fb->format->cpp[0])
>> -		drm_fb_memcpy_dstclip(cirrus->vram,
>> +		drm_fb_memcpy_dstclip(cirrus->vram, fb->pitches[0],
>>  				      vmap, fb, rect);
>>  
>>  	else if (fb->format->cpp[0] == 4 && cirrus->cpp == 2)
>> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
>> index 5f9e37032468..2b5036a5fbe7 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -11,7 +11,7 @@ struct drm_rect;
>>  
>>  void drm_fb_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb,
>>  		   struct drm_rect *clip);
>> -void drm_fb_memcpy_dstclip(void __iomem *dst, void *vaddr,
>> +void drm_fb_memcpy_dstclip(void __iomem *dst, unsigned int dst_pitch, void *vaddr,
>>  			   struct drm_framebuffer *fb,
>>  			   struct drm_rect *clip);
>>  void drm_fb_swab(void *dst, void *src, struct drm_framebuffer *fb,
>> -- 
>> 2.27.0
>>
> 

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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