Hi Am 28.09.20 um 10:53 schrieb Daniel Vetter: > On Mon, Sep 28, 2020 at 9:22 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: >> >> Hi >> >> Am 26.09.20 um 18:42 schrieb Daniel Vetter: >>> On Fri, Sep 25, 2020 at 4:55 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx> >>>>> >>>>> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> >>>>> >>>>> 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. > > Not really, right now that ship is simply leaking and sinking. If we > decide to patch this up from the kernel side, then indeed it has > sailed. And I'm not sure that's a good idea. We have code in at least cirrus, ast and mgag200 to support this. And userspace has been behaving like this since at least when I got involved (2017). > >>> 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. > > Yeah, but you don't do rendering on dumb buffers. Like ever. So this > smells like a userspace bug. It's also part of the software rendering. It is not a bug, but implemented deliberately in one of the userspace components that allocates framebuffers (but I cannot remember which one.) Best regards Thomas > > If it's for shared buffers then I think that sounds more reasonable. > -Daniel > >> >> 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 >> > > -- 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:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
