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 else. The driver is supposed to apply any rounding necessary for the size. Or is this a buffer allocated somewhere else and then shared? -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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
