[PATCH 1/9] drm/format-helper: Pass destination pitch to drm_fb_memcpy_dstclip()
Thomas Zimmermann
tzimmermann at suse.de
Mon Sep 28 07:22:34 UTC 2020
Hi
Am 26.09.20 um 18:42 schrieb Daniel Vetter:
> On Fri, Sep 25, 2020 at 4:55 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>>
>> 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 at suse.de>
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter at 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.
>
> At least with dumb buffer allocation ioctls userspace should not do
> that. If it wants 800x600, it needs to allocate 800x600, not something
That ship has sailed.
> else. The driver is supposed to apply any rounding necessary for the
> size. Or is this a buffer allocated somewhere else and then shared?
I don't quite remember where exactly this was implemented. It was not a
shared buffer, though. IIRC the buffer allocation code in one of the
libs rounded the size towards multiples of 64. I remember thinking that
it was probably done for tiled rendering.
Best regards
Thomas
> -Daniel
>
>> 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
>>
>
>
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 516 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200928/e4208cc1/attachment.sig>
More information about the dri-devel
mailing list