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

From: Daniel Vetter <daniel@ffwll.ch>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>,
	Dave Airlie <airlied@linux.ie>,
	Emil Velikov <emil.l.velikov@gmail.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Mark Brown <broonie@kernel.org>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH 1/9] drm/format-helper: Pass destination pitch to drm_fb_memcpy_dstclip()
Date: Tue, 29 Sep 2020 11:19:06 +0200	[thread overview]
Message-ID: <20200929091906.GJ438822@phenom.ffwll.local> (raw)
In-Reply-To: <a4cd3180-b21d-7464-ad63-5b616acf2c7a@suse.de>

On Mon, Sep 28, 2020 at 11:13:06AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 28.09.20 um 10:53 schrieb Daniel Vetter:
> > On Mon, Sep 28, 2020 at 9:22 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Hi
> >>
> >> Am 26.09.20 um 18:42 schrieb Daniel Vetter:
> >>> On Fri, Sep 25, 2020 at 4:55 PM Thomas Zimmermann <tzimmermann@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@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.
> >>>
> >>> 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).

Hm where do these drivers copy stuff around and rematch the stride?

> >>> 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.)

I think it would be good to document this.

We already fake xrgb8888 everywhere because userspace is not flexible
enough, I guess having to fake 64b stride support everywhere isn't that
much worse.

But it's definitely not great either :-/
-Daniel

> 
> 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel