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.

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

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 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/crtcr...

...

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

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.

...

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.

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/crtcr...

...

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

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/crtcr...

...

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

On Tue, Sep 29, 2020 at 11:39:21AM +0200, Thomas Zimmermann wrote:

Hi

Am 29.09.20 um 11:19 schrieb Daniel Vetter:

...

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?

They don't. These drivers adopt their HW stride to match whatever userspace framebuffers tell them. [1] And that's because userspace gives them framebuffer sizes like 832*640. And then they do a memcpy with the given width.

My sole point here was that userspace already relies on this behavior.

Yeah but the problem is, if we support this through copying, then we rob userspace of the change to do something better.

I guess the reason that userspace aligns to 64b is because of i915 gbm (iirc 64b is the requirement for being able to render, or maybe it's the requirement for scanout). So you get zero-copy buffer sharing.

But if the kernel now does copies on its own, without telling userspace, then we might end up with a fairly crappy path. It might not matter much for old devices like this one, but for cases where we really care about zero-copy it does. And unlike the kernel userspace can perhaps do the copying using a different gpu for these cases.

So that's kinda why I'm vary of rolling this out at large scale. Since once we've done it, we cannot ever undo it since that breaks existing userspace on existing hardware. Which means even if we fix userspace to have a better/different fallback that uses a correctly sized buffer and maybe even a gpu copy, that code wont run because the kernel papers over the problem with a dog-slow re-stride cpu copy. -Daniel

Best regards Thomas

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...

...

...

...

...

...

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/crtcr... > >> >> 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

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

On Thu, Jun 25, 2020 at 02:00:04PM +0200, Thomas Zimmermann wrote:

The blitter functions copy a framebuffer to I/O memory using one of the existing conversion functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Hm I guess reason for adding dst_pitch in the previous patch is so that there wouldn't be a special case here. Makes sense.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_format_helper.c | 87 +++++++++++++++++++++++++++++ include/drm/drm_format_helper.h | 8 +++ 2 files changed, 95 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c index 8d5a683afea7..0e885cd34107 100644 --- a/drivers/gpu/drm/drm_format_helper.c +++ b/drivers/gpu/drm/drm_format_helper.c @@ -344,3 +344,90 @@ void drm_fb_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, } EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray8);

+/**

• • drm_fb_blit_rect_dstclip - Copy parts of a framebuffer to display memory
• • @dst: The display memory to copy to
• • @dst_pitch: Number of bytes between two consecutive scanlines within dst
• • @dst_format: FOURCC code of the display's color format
• • @vmap: The framebuffer memory to copy from
• • @fb: The framebuffer to copy from
• • @clip: Clip rectangle area to copy
• • This function copies parts of a framebuffer to display memory. If the
• • formats of the display and the framebuffer mismatch, the blit function
• • will attempt to convert between them.
• • Use drm_fb_blit_dstclip() to copy the full framebuffer.
• • Returns:
• • 0 on success, or
• • -EINVAL if the color-format conversion failed, or
• • a negative error code otherwise.
• */

+int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,

• 	     uint32_t dst_format, void *vmap,
  

• 	     struct drm_framebuffer *fb,
  

• 	     struct drm_rect *clip)
  

+{

• uint32_t fb_format = fb->format->format;
• /* treat alpha channel like filler bits */
• if (fb_format == DRM_FORMAT_ARGB8888)

• fb_format = DRM_FORMAT_XRGB8888;
  

• if (dst_format == DRM_FORMAT_ARGB8888)

• dst_format = DRM_FORMAT_XRGB8888;
  

• if (dst_format == fb_format) {

• drm_fb_memcpy_dstclip(dst, dst_pitch, vmap, fb, clip);
  

• return 0;
  

• } else if (dst_format == DRM_FORMAT_RGB565) {

• if (fb_format == DRM_FORMAT_XRGB8888) {
  

• 	drm_fb_xrgb8888_to_rgb565_dstclip(dst, dst_pitch,
  

• 					  vmap, fb, clip,
  

• 					  false);
  

• 	return 0;
  

• }
  

• } else if (dst_format == DRM_FORMAT_RGB888) {

• if (fb_format == DRM_FORMAT_XRGB8888) {
  

• 	drm_fb_xrgb8888_to_rgb888_dstclip(dst, dst_pitch,
  

• 					  vmap, fb, clip);
  

• 	return 0;
  

• }
  

• }
• return -EINVAL;

+} +EXPORT_SYMBOL(drm_fb_blit_rect_dstclip);

+/**

• • drm_fb_blit_dstclip - Copy framebuffer to display memory
• • @dst: The display memory to copy to
• • @dst_pitch: Number of bytes between two consecutive scanlines within dst
• • @dst_format: FOURCC code of the display's color format
• • @vmap: The framebuffer memory to copy from
• • @fb: The framebuffer to copy from
• • This function copies a full framebuffer to display memory. If the formats
• • of the display and the framebuffer mismatch, the copy function will
• • attempt to convert between them.
• • See drm_fb_blit_rect_dstclip() for more inforamtion.
• • Returns:
• • 0 on success, or a negative error code otherwise.
• */

+int drm_fb_blit_dstclip(void __iomem *dst, unsigned int dst_pitch,

• 	uint32_t dst_format, void *vmap,
  

• 	struct drm_framebuffer *fb)
  

I do wonder whether we shouldn't have to some safety checks for this stuff, like maybe something like:

struct dst_info { void __iomem *dst; unsigned int size; unsigned int dst_pitch; uint32_t dst_format; };

And then the helpers would splat a WARNING if a driver ever gets this wrong. Thinking about this because syzkaller has found tons of little off-by-a-bit bugs in the vt/fbcon/fbdev code, and maybe we should try to be better :-)

But that's material for another patch, and maybe once we have a few more helpers in this library we can figure out a nice struct to package up these long&confusing argument lists a bit.

Cheers, Daniel

+{

• struct drm_rect fullscreen = {

• .x1 = 0,
  

• .x2 = fb->width,
  

• .y1 = 0,
  

• .y2 = fb->height,
  

• };
• return drm_fb_blit_rect_dstclip(dst, dst_pitch, dst_format, vmap, fb,

• 			&fullscreen);
  

+} +EXPORT_SYMBOL(drm_fb_blit_dstclip); diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h index 2b5036a5fbe7..4e0258a61311 100644 --- a/include/drm/drm_format_helper.h +++ b/include/drm/drm_format_helper.h @@ -28,4 +28,12 @@ void drm_fb_xrgb8888_to_rgb888_dstclip(void __iomem *dst, unsigned int dst_pitch void drm_fb_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, struct drm_rect *clip);

+int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,

• 	     uint32_t dst_format, void *vmap,
  

• 	     struct drm_framebuffer *fb,
  

• 	     struct drm_rect *rect);
  

+int drm_fb_blit_dstclip(void __iomem *dst, unsigned int dst_pitch,

• 	uint32_t dst_format, void *vmap,
  

• 	struct drm_framebuffer *fb);
  

#endif /* __LINUX_DRM_FORMAT_HELPER_H */

2.27.0

On Thu, Jun 25, 2020 at 02:00:05PM +0200, Thomas Zimmermann wrote:

The simplekms driver is a DRM driver for simplefb framebuffers as provided by the kernel's boot code. This driver enables basic graphical output on many different graphics devices that are provided by the platform (e.g., EFI, VESA, embedded framebuffers).

With the kernel's simplefb infrastructure, the kernel receives a pre-configured framebuffer from the system (i.e., firmware, boot loader). It creates a platform device to which simplekms attaches. The system's framebuffer consists of a memory range, size and format. Based on these values, simplekms creates a DRM devices. No actual modesetting is possible.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

MAINTAINERS | 6 + drivers/gpu/drm/tiny/Kconfig | 16 + drivers/gpu/drm/tiny/Makefile | 1 + drivers/gpu/drm/tiny/simplekms.c | 495 +++++++++++++++++++++++++++++++ 4 files changed, 518 insertions(+) create mode 100644 drivers/gpu/drm/tiny/simplekms.c

diff --git a/MAINTAINERS b/MAINTAINERS index f17d99164a77..ac517dc8d05d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5505,6 +5505,12 @@ S: Orphan / Obsolete F: drivers/gpu/drm/savage/ F: include/uapi/drm/savage_drm.h

+DRM DRIVER FOR SIMPLE FRAMEBUFFERS +M: Thomas Zimmermann tzimmermann@suse.de +S: Maintained +T: git git://anongit.freedesktop.org/drm/drm-misc +F: drivers/gpu/drm/tiny/simplekms.c

DRM DRIVER FOR SIS VIDEO CARDS S: Orphan / Obsolete F: drivers/gpu/drm/sis/ diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig index 2b6414f0fa75..50dbde8bdcb2 100644 --- a/drivers/gpu/drm/tiny/Kconfig +++ b/drivers/gpu/drm/tiny/Kconfig @@ -28,6 +28,22 @@ config DRM_GM12U320 This is a KMS driver for projectors which use the GM12U320 chipset for video transfer over USB2/3, such as the Acer C120 mini projector.

+config DRM_SIMPLEKMS

• tristate "Simple framebuffer driver"
• depends on DRM
• select DRM_GEM_SHMEM_HELPER
• select DRM_KMS_HELPER
• help

•  DRM driver for simple platform-provided framebuffers.
  

•  This driver assumes that the display hardware has been initialized
  

•  by the firmware or bootloader before the kernel boots. Scanout
  

•  buffer, size, and display format must be provided via device tree,
  

•  UEFI, VESA, etc.
  

•  On x86 and compatible, you should also select CONFIG_X86_SYSFB to
  

•  use UEFI and VESA framebuffers.
  

config TINYDRM_HX8357D tristate "DRM support for HX8357D display panels" depends on DRM && SPI diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile index 6ae4e9e5a35f..e83fbbfa7344 100644 --- a/drivers/gpu/drm/tiny/Makefile +++ b/drivers/gpu/drm/tiny/Makefile @@ -2,6 +2,7 @@

obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus.o obj-$(CONFIG_DRM_GM12U320) += gm12u320.o +obj-$(CONFIG_DRM_SIMPLEKMS) += simplekms.o obj-$(CONFIG_TINYDRM_HX8357D) += hx8357d.o obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c new file mode 100644 index 000000000000..dc7cf3983945 --- /dev/null +++ b/drivers/gpu/drm/tiny/simplekms.c @@ -0,0 +1,495 @@ +// SPDX-License-Identifier: GPL-2.0-only

+#include <linux/platform_data/simplefb.h> +#include <linux/platform_device.h>

+#include <drm/drm_atomic_state_helper.h> +#include <drm/drm_connector.h> +#include <drm/drm_damage_helper.h> +#include <drm/drm_device.h> +#include <drm/drm_drv.h> +#include <drm/drm_format_helper.h> +#include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_gem_shmem_helper.h> +#include <drm/drm_managed.h> +#include <drm/drm_modeset_helper_vtables.h> +#include <drm/drm_probe_helper.h> +#include <drm/drm_simple_kms_helper.h>

+#define DRIVER_NAME "simplekms" +#define DRIVER_DESC "DRM driver for simple-framebuffer platform devices" +#define DRIVER_DATE "20200625" +#define DRIVER_MAJOR 1 +#define DRIVER_MINOR 0

+/*

• • Assume a monitor resolution of 96 dpi to
• • get a somewhat reasonable screen size.
• */

+#define RES_MM(d) \

• (((d) * 254ul) / (96ul * 10ul))

+#define SIMPLEKMS_MODE(hd, vd) \

• DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))

+/*

• • Helpers for simplefb
• */

+static int +simplefb_get_validated_int(struct drm_device *dev, const char *name,

• 	   uint32_t value)
  

+{

• if (value > INT_MAX) {

• drm_err(dev, "simplefb: invalid framebuffer %s of %u\n",
  

• 	name, value);
  

• return -EINVAL;
  

• }
• return (int)value;

+}

+static int +simplefb_get_validated_int0(struct drm_device *dev, const char *name,

• 	    uint32_t value)
  

+{

• if (!value) {

• drm_err(dev, "simplefb: invalid framebuffer %s of %u\n",
  

• 	name, value);
  

• return -EINVAL;
  

• }
• return simplefb_get_validated_int(dev, name, value);

+}

+static const struct drm_format_info * +simplefb_get_validated_format(struct drm_device *dev, const char *format_name) +{

• static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
• const struct simplefb_format *fmt = formats;
• const struct simplefb_format *end = fmt + ARRAY_SIZE(formats);
• if (!format_name) {

• drm_err(dev, "simplefb: missing framebuffer format\n");
  

• return ERR_PTR(-EINVAL);
  

• }
• while (fmt < end) {

• if (!strcmp(format_name, fmt->name))
  

• 	return drm_format_info(fmt->fourcc);
  

• ++fmt;
  

• }
• drm_err(dev, "simplefb: unknown framebuffer format %s\n",

• format_name);
  

• return ERR_PTR(-EINVAL);

+}

+static int +simplefb_get_width_pd(struct drm_device *dev,

•       const struct simplefb_platform_data *pd)
  

+{

• return simplefb_get_validated_int0(dev, "width", pd->width);

+}

+static int +simplefb_get_height_pd(struct drm_device *dev,

•        const struct simplefb_platform_data *pd)
  

+{

• return simplefb_get_validated_int0(dev, "height", pd->height);

+}

+static int +simplefb_get_stride_pd(struct drm_device *dev,

•        const struct simplefb_platform_data *pd)
  

+{

• return simplefb_get_validated_int(dev, "stride", pd->stride);

+}

+static const struct drm_format_info * +simplefb_get_format_pd(struct drm_device *dev,

•        const struct simplefb_platform_data *pd)
  

+{

• return simplefb_get_validated_format(dev, pd->format);

+}

+/*

• • Simple Framebuffer device
• */

+struct simplekms_device {

• struct drm_device dev;
• struct platform_device *pdev;
• /* simplefb settings */
• struct drm_display_mode mode;
• const struct drm_format_info *format;
• unsigned int pitch;
• /* memory management */
• struct resource *mem;
• void __iomem *screen_base;
• /* modesetting */
• struct drm_connector connector;
• struct drm_simple_display_pipe pipe;

+};

+static struct simplekms_device *simplekms_device_of_dev(struct drm_device *dev) +{

• return container_of(dev, struct simplekms_device, dev);

+}

+/*

• • Simplefb settings
• */

+static struct drm_display_mode simplekms_mode(unsigned int width,

• 			      unsigned int height)
  

+{

• struct drm_display_mode mode = { SIMPLEKMS_MODE(width, height) };
• mode.clock = 60 /* Hz */ * mode.hdisplay * mode.vdisplay;
• drm_mode_set_name(&mode);
• return mode;

+}

+static int simplekms_device_init_fb(struct simplekms_device *sdev) +{

• int width, height, stride;
• const struct drm_format_info *format;
• struct drm_format_name_buf buf;
• struct drm_device *dev = &sdev->dev;
• struct platform_device *pdev = sdev->pdev;
• const struct simplefb_platform_data *pd = dev_get_platdata(&pdev->dev);
• if (pd) {

• width = simplefb_get_width_pd(dev, pd);
  

• if (width < 0)
  

• 	return width;
  

• height = simplefb_get_height_pd(dev, pd);
  

• if (height < 0)
  

• 	return height;
  

• stride = simplefb_get_stride_pd(dev, pd);
  

• if (stride < 0)
  

• 	return stride;
  

• format = simplefb_get_format_pd(dev, pd);
  

• if (IS_ERR(format))
  

• 	return PTR_ERR(format);
  

• } else {

• drm_err(dev, "no simplefb configuration found\n");
  

• return -ENODEV;
  

• }
• sdev->mode = simplekms_mode(width, height);
• sdev->format = format;
• sdev->pitch = stride;
• drm_dbg_kms(dev, "display mode={" DRM_MODE_FMT "}\n",

•     DRM_MODE_ARG(&sdev->mode));
  

• drm_dbg_kms(dev,

•     "framebuffer format=\"%s\", size=%dx%d, stride=%d byte\n",
  

•     drm_get_format_name(format->format, &buf), width,
  

•     height, stride);
  

• return 0;

+}

+/*

• • Memory management
• */

+static int simplekms_device_init_mm(struct simplekms_device *sdev) +{

• struct platform_device *pdev = sdev->pdev;
• struct resource *mem;
• void __iomem *screen_base;
• mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
• if (!mem)

• return -EINVAL;
  

• screen_base = devm_ioremap_wc(&pdev->dev, mem->start,

• 		      resource_size(mem));
  

• if (!screen_base)

• return -ENOMEM;
  

• sdev->mem = mem;
• sdev->screen_base = screen_base;
• return 0;

+}

+/*

• • Modesetting
• */

+/*

• • Support all formats of simplefb and maybe more; in order
• • of preference. The display's update function will do any
• • conversion necessary.
• • TODO: Add blit helpers for remaining formats and uncomment

• •   constants.
    

• */

+static const uint32_t simplekms_formats[] = {

• DRM_FORMAT_XRGB8888,
• DRM_FORMAT_ARGB8888,
• DRM_FORMAT_RGB565,
• //DRM_FORMAT_XRGB1555,
• //DRM_FORMAT_ARGB1555,
• DRM_FORMAT_RGB888,
• //DRM_FORMAT_XRGB2101010,
• //DRM_FORMAT_ARGB2101010,

+};

+static const uint64_t simplekms_format_modifiers[] = {

• DRM_FORMAT_MOD_LINEAR,
• DRM_FORMAT_MOD_INVALID

+};

+static int simplekms_connector_get_modes(struct drm_connector *connector) +{

• struct simplekms_device *sdev = simplekms_device_of_dev(connector->dev);
• struct drm_display_mode *mode;
• mode = drm_mode_duplicate(connector->dev, &sdev->mode);
• if (!mode)

•            return 0;
  

• if (mode->name[0] == '\0')

• drm_mode_set_name(mode);
  

• mode->type |= DRM_MODE_TYPE_PREFERRED;
• drm_mode_probed_add(connector, mode);
• if (mode->width_mm)

•            connector->display_info.width_mm = mode->width_mm;
  

• if (mode->height_mm)

•            connector->display_info.height_mm = mode->height_mm;
  

•    return 1;
  

+}

+static const struct drm_connector_helper_funcs simplekms_connector_helper_funcs = {

• .get_modes = simplekms_connector_get_modes,

+};

+static const struct drm_connector_funcs simplekms_connector_funcs = {

• .reset = drm_atomic_helper_connector_reset,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = drm_connector_cleanup,
• .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,

+};

+static int +simplekms_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,

• 		    const struct drm_display_mode *mode)
  

+{

• struct simplekms_device *sdev = simplekms_device_of_dev(pipe->connector->dev);
• if (mode->hdisplay != sdev->mode.hdisplay &&

•    mode->vdisplay != sdev->mode.vdisplay)
  

• return MODE_ONE_SIZE;
  

I'd simplify this to an || and delete the two below. Userspace wont care anyway.

• else if (mode->hdisplay != sdev->mode.hdisplay)

• return MODE_ONE_WIDTH;
  

• else if (mode->vdisplay != sdev->mode.vdisplay)

• return MODE_ONE_HEIGHT;
  

• return MODE_OK;

+}

+static void +simplekms_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,

• 		struct drm_crtc_state *crtc_state,
  

• 		struct drm_plane_state *plane_state)
  

+{

• struct simplekms_device *sdev = simplekms_device_of_dev(pipe->connector->dev);
• struct drm_plane_state *state = pipe->plane.state;
• struct drm_framebuffer *fb = state->fb;
• void *vmap;
• vmap = drm_gem_shmem_vmap(fb->obj[0]);
• if (!vmap)

• return;
  

• drm_fb_blit_dstclip(sdev->screen_base, sdev->pitch,

• 	    sdev->format->format, vmap, fb);
  

• drm_gem_shmem_vunmap(fb->obj[0], vmap);

+}

+static void +simplekms_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,

• 		struct drm_plane_state *old_plane_state)
  

+{

• struct simplekms_device *sdev = simplekms_device_of_dev(pipe->connector->dev);
• struct drm_plane_state *state = pipe->plane.state;
• struct drm_framebuffer *fb = state->fb;
• struct drm_rect clip;
• void *vmap;
• if (!drm_atomic_helper_damage_merged(old_plane_state, state, &clip))

• return;
  

• vmap = drm_gem_shmem_vmap(fb->obj[0]);
• if (!vmap)

• return;
  

• drm_fb_blit_rect_dstclip(sdev->screen_base, sdev->pitch,

• 		 sdev->format->format, vmap, fb, &clip);
  

• drm_gem_shmem_vunmap(fb->obj[0], vmap);

+}

+static const struct drm_simple_display_pipe_funcs +simplekms_simple_display_pipe_funcs = {

• .mode_valid = simplekms_simple_display_pipe_mode_valid,
• .enable = simplekms_simple_display_pipe_enable,

A disable hook that clears it all to black would be nice.

• .update = simplekms_simple_display_pipe_update,
• .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,

+};

+static const struct drm_mode_config_funcs simplekms_mode_config_funcs = {

• .fb_create = drm_gem_fb_create_with_dirty,
• .atomic_check = drm_atomic_helper_check,
• .atomic_commit = drm_atomic_helper_commit,

+};

+static int simplekms_device_init_modeset(struct simplekms_device *sdev) +{

• struct drm_device *dev = &sdev->dev;
• struct drm_display_mode *mode = &sdev->mode;
• struct drm_connector *connector = &sdev->connector;
• struct drm_simple_display_pipe *pipe = &sdev->pipe;
• int ret;
• ret = drmm_mode_config_init(dev);
• if (ret)

• return ret;
  

• dev->mode_config.min_width = mode->hdisplay;
• dev->mode_config.max_width = mode->hdisplay;
• dev->mode_config.min_height = mode->vdisplay;
• dev->mode_config.max_height = mode->vdisplay;
• dev->mode_config.prefer_shadow = true;
• dev->mode_config.preferred_depth = sdev->format->cpp[0] * 8;
• dev->mode_config.funcs = &simplekms_mode_config_funcs;
• drm_connector_helper_add(connector, &simplekms_connector_helper_funcs);
• ret = drm_connector_init(dev, connector, &simplekms_connector_funcs,

• 		 DRM_MODE_CONNECTOR_Unknown);
  

Quite bad amounts of boilerplate for the connector, but I guess everyone else with a simple output that's totally fixed just uses drm_panel. Which doesn't really fit here (but most likely would reduce code a bunch I suspect).

• if (ret)

• return ret;
  

• ret = drm_simple_display_pipe_init(dev, pipe,

• 			   &simplekms_simple_display_pipe_funcs,
  

• 			   simplekms_formats,
  

I don't think this works, you need to allocate a custom format list with 3 entries: - actual format of the underlying fb, so we avoid conversion if possible - the xrgb/argb8888 fallbacks as usual

Announcing all of them userspace might pick something that you can't do.

• 			   ARRAY_SIZE(simplekms_formats),
  

• 			   simplekms_format_modifiers,
  

• 			   connector);
  

• if (ret)

• return ret;
  

• drm_mode_config_reset(dev);

This breaks fastboot. I think ideally we'd have the state represent everything is on, and allocate an fb + buffer with the current contents of the framebuffer. Since we can allocate an fb that matches this shouldn't be a problem, just a raw memcpy_fromio should do the job.

Having a nice new simplekms drm driver and then losing fastboot feels like slightly off tradeoff.

Maybe in a follow-up patch, but before fbcon setup? Since ideally fbcon also takes over the already existing framebuffer we allocated, so that as long as nothing clears the fb (i.e. fbcon is quiet) we'd preserve the original framebuffer throughout the boot-up sequence.

• return 0;

+}

+/*

• • Init / Cleanup
• */

+static void simplekms_device_cleanup(struct simplekms_device* sdev) +{

• struct drm_device *dev = &sdev->dev;
• drm_dev_unregister(dev);

I'd inline this, I guess there was once more before you switched everything over to devm_

+}

+static struct simplekms_device * +simplekms_device_create(struct drm_driver *drv, struct platform_device *pdev) +{

• struct simplekms_device *sdev;
• int ret;
• sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simplekms_device,

• 		  dev);
  

• if (IS_ERR(sdev))

• return ERR_CAST(sdev);
  

• sdev->pdev = pdev;
• ret = simplekms_device_init_fb(sdev);
• if (ret)

• return ERR_PTR(ret);
  

• ret = simplekms_device_init_mm(sdev);
• if (ret)

• return ERR_PTR(ret);
  

• ret = simplekms_device_init_modeset(sdev);
• if (ret)

• return ERR_PTR(ret);
  

• return sdev;

+}

+/*

• • DRM driver
• */

+DEFINE_DRM_GEM_FOPS(simplekms_fops);

+static struct drm_driver simplekms_driver = {

• DRM_GEM_SHMEM_DRIVER_OPS,
• .name = DRIVER_NAME,
• .desc = DRIVER_DESC,
• .date = DRIVER_DATE,
• .major = DRIVER_MAJOR,
• .minor = DRIVER_MINOR,
• .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
• .fops = &simplekms_fops,

+};

+/*

• • Platform driver
• */

+static int simplekms_probe(struct platform_device *pdev) +{

• struct simplekms_device *sdev;
• struct drm_device *dev;
• int ret;
• sdev = simplekms_device_create(&simplekms_driver, pdev);
• if (IS_ERR(sdev))

• return PTR_ERR(sdev);
  

• dev = &sdev->dev;
• ret = drm_dev_register(dev, 0);
• if (ret)

• return ret;
  

• return 0;

+}

+static int simplekms_remove(struct platform_device *pdev) +{

• struct simplekms_device *sdev = platform_get_drvdata(pdev);
• simplekms_device_cleanup(sdev);

If you add the ->disable hook then a comment here that we don't want to shut down to allow fastboot would be nice.

• return 0;

+}

+static struct platform_driver simplekms_platform_driver = {

• .driver = {

• .name = "simple-framebuffer", /* connect to sysfb */
  

• },
• .probe = simplekms_probe,
• .remove = simplekms_remove,

+};

+module_platform_driver(simplekms_platform_driver);

+MODULE_DESCRIPTION(DRIVER_DESC);

+MODULE_LICENSE("GPL v2");

2.27.0

Cheers, Daniel

On Thu, Jun 25, 2020 at 02:00:06PM +0200, Thomas Zimmermann wrote:

This displays a console on the simplefb framebuffer. The default framebuffer format is being used.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/tiny/simplekms.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c index dc7cf3983945..ac2ebfcedd22 100644 --- a/drivers/gpu/drm/tiny/simplekms.c +++ b/drivers/gpu/drm/tiny/simplekms.c @@ -8,6 +8,7 @@ #include <drm/drm_damage_helper.h> #include <drm/drm_device.h> #include <drm/drm_drv.h> +#include <drm/drm_fb_helper.h> #include <drm/drm_format_helper.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_shmem_helper.h> @@ -469,6 +470,8 @@ static int simplekms_probe(struct platform_device *pdev) if (ret) return ret;

• drm_fbdev_generic_setup(dev, 0);

I think for the fastboot stuff what we could do is improve the generic fbdev code with essentially what intel_fbdev_init_bios does. Well actually just shovel that code into the helpers maybe, the other code to pick initial configs for fastboot has been moved already too.

Otherwise lgtm ofc.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• return 0;

}

-- 2.27.0

On Thu, Jun 25, 2020 at 6:00 AM Thomas Zimmermann tzimmermann@suse.de wrote:

A firmware framebuffer might also be specified via device-tree files. If no device platform data is given, try the DT device node.

You are missing a DT match table for driver matching and module loading (if a module is supported).

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/tiny/simplekms.c | 84 ++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)

diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c index ac2ebfcedd22..87636307aa4f 100644 --- a/drivers/gpu/drm/tiny/simplekms.c +++ b/drivers/gpu/drm/tiny/simplekms.c @@ -113,6 +113,76 @@ simplefb_get_format_pd(struct drm_device *dev, return simplefb_get_validated_format(dev, pd->format); }

+static int +simplefb_read_u32_of(struct drm_device *dev, struct device_node *of_node,

•                const char* name, u32 *value)
  

+{

•   int ret = of_property_read_u32(of_node, name, value);
  

•   if (ret)
  

•           drm_err(dev, "simplefb: can't parse framebuffer %s: error %d\n",
  

•                   name, ret);
  

•   return ret;
  

+}

+static int +simplefb_read_string_of(struct drm_device *dev, struct device_node *of_node,

•                   const char* name, const char **value)
  

+{

•   int ret = of_property_read_string(of_node, name, value);
  

•   if (ret)
  

•           drm_err(dev, "simplefb: can't parse framebuffer %s: error %d\n",
  

•                   name, ret);
  

•   return ret;
  

+}

+static int +simplefb_get_width_of(struct drm_device *dev, struct device_node *of_node) +{

•   int ret;
  

•   u32 width;
  

•   ret = simplefb_read_u32_of(dev, of_node, "width", &width);
  

•   if (ret)
  

•           return ret;
  

•   return simplefb_get_validated_int0(dev, "width", width);
  

+}

+static int +simplefb_get_height_of(struct drm_device *dev, struct device_node *of_node) +{

•   int ret;
  

•   u32 height;
  

•   ret = simplefb_read_u32_of(dev, of_node, "height", &height);
  

•   if (ret)
  

•           return ret;
  

•   return simplefb_get_validated_int0(dev, "height", height);
  

+}

+static int +simplefb_get_stride_of(struct drm_device *dev, struct device_node *of_node) +{

•   int ret;
  

•   u32 stride;
  

•   ret = simplefb_read_u32_of(dev, of_node, "stride", &stride);
  

•   if (ret)
  

•           return ret;
  

•   return simplefb_get_validated_int(dev, "stride", stride);
  

+}

+static const struct drm_format_info * +simplefb_get_format_of(struct drm_device *dev, struct device_node *of_node) +{

•   int ret;
  

•   const char *format;
  

•   ret = simplefb_read_string_of(dev, of_node, "format", &format);
  

•   if (ret)
  

•           return ERR_PTR(ret);
  

•   return simplefb_get_validated_format(dev, format);
  

+}

/*

• Simple Framebuffer device

*/ @@ -163,6 +233,7 @@ static int simplekms_device_init_fb(struct simplekms_device *sdev) struct drm_device *dev = &sdev->dev; struct platform_device *pdev = sdev->pdev; const struct simplefb_platform_data *pd = dev_get_platdata(&pdev->dev);

•   struct device_node *of_node = pdev->dev.of_node;
  
    if (pd) {
            width = simplefb_get_width_pd(dev, pd);
  

@@ -177,6 +248,19 @@ static int simplekms_device_init_fb(struct simplekms_device *sdev) format = simplefb_get_format_pd(dev, pd); if (IS_ERR(format)) return PTR_ERR(format);

•   } else if (of_node) {
  

•           width = simplefb_get_width_of(dev, of_node);
  

•           if (width < 0)
  

•                   return width;
  

•           height = simplefb_get_height_of(dev, of_node);
  

•           if (height < 0)
  

•                   return height;
  

•           stride = simplefb_get_stride_of(dev, of_node);
  

•           if (stride < 0)
  

•                   return stride;
  

•           format = simplefb_get_format_of(dev, of_node);
  

•           if (IS_ERR(format))
  

•                   return PTR_ERR(format);
    } else {
            drm_err(dev, "no simplefb configuration found\n");
            return -ENODEV;
  

-- 2.27.0

Hi Thomas,

On Thu, Jun 25, 2020 at 2:00 PM Thomas Zimmermann tzimmermann@suse.de wrote:

Make sure required hardware clocks are enabled while the firmware framebuffer is in use.

The basic code has been taken from the simplefb driver and adapted to DRM. Clocks are released automatically via devres helpers.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Thanks for your patch!

--- a/drivers/gpu/drm/tiny/simplekms.c +++ b/drivers/gpu/drm/tiny/simplekms.c

@@ -210,6 +218,103 @@ static struct simplekms_device *simplekms_device_of_dev(struct drm_device *dev) return container_of(dev, struct simplekms_device, dev); }

+/*

• • Hardware
• */

+#if defined CONFIG_OF && defined CONFIG_COMMON_CLK +/*

• • Clock handling code.
• • Here we handle the clocks property of our "simple-framebuffer" dt node.
• • This is necessary so that we can make sure that any clocks needed by
• • the display engine that the bootloader set up for us (and for which it
• • provided a simplefb dt node), stay up, for the life of the simplefb
• • driver.
• • When the driver unloads, we cleanly disable, and then release the clocks.
• • We only complain about errors here, no action is taken as the most likely
• • error can only happen due to a mismatch between the bootloader which set
• • up simplefb, and the clock definitions in the device tree. Chances are
• • that there are no adverse effects, and if there are, a clean teardown of
• • the fb probe will not help us much either. So just complain and carry on,
• • and hope that the user actually gets a working fb at the end of things.
• */

+static void simplekms_device_release_clocks(void *res) +{

•   struct simplekms_device *sdev = simplekms_device_of_dev(res);
  

•   unsigned int i;
  

•   for (i = 0; i < sdev->clk_count; ++i) {
  

•           if (sdev->clks[i]) {
  

•                   clk_disable_unprepare(sdev->clks[i]);
  

•                   clk_put(sdev->clks[i]);
  

•           }
  

•   }
  

+}

+static int simplekms_device_init_clocks(struct simplekms_device *sdev) +{

•   struct drm_device *dev = &sdev->dev;
  

•   struct platform_device *pdev = sdev->pdev;
  

•   struct device_node *of_node = pdev->dev.of_node;
  

•   struct clk *clock;
  

•   unsigned int i;
  

•   int ret;
  

•   if (dev_get_platdata(&pdev->dev) || !of_node)
  

•           return 0;
  

•   sdev->clk_count = of_clk_get_parent_count(of_node);
  

•   if (!sdev->clk_count)
  

•           return 0;
  

•   sdev->clks = drmm_kzalloc(dev, sdev->clk_count * sizeof(sdev->clks[0]),
  

•                             GFP_KERNEL);
  

•   if (!sdev->clks)
  

•           return -ENOMEM;
  

•   for (i = 0; i < sdev->clk_count; ++i) {
  

•           clock = of_clk_get(of_node, i);
  

clk_bulk_get_all()?

Gr{oetje,eeting}s,

Geert

Hi

Am 29.06.20 um 11:27 schrieb Daniel Vetter:

On Thu, Jun 25, 2020 at 02:00:10PM +0200, Thomas Zimmermann wrote:

...

Platform devices might operate on firmware framebuffers, such as VESA or EFI. Before a native driver for the graphics hardware can take over the device, it has to remove any platform driver that operates on the firmware framebuffer. Platform helpers provide the infrastructure for platform drivers to acquire firmware framebuffers, and for native drivers to remove them lateron.

It works similar to the related fbdev mechanism. During initialization, the platform driver acquires the firmware framebuffer's I/O memory and provides a callback to be removed. The native driver later uses this inforamtion to remove any platform driver for it's framebuffer I/O memory.

The platform helper's removal code is integrated into the existing code for removing conflicting fraembuffers, so native drivers use it automatically.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

I have some ideas for how to do this a notch cleaner in the next patch. Maybe best to discuss the actual implmenentation stuff there.

Aside from that usual nits:

• kerneldoc for these please, pulled into drm-kms.rst.
• naming isn't super ocd with drm_platform.c but that prefix not used, but I also don't have better ideas.

I was never really happy with the naming either. Maybe call it aperture helpers with drm_aperture_ and CONFIG_DRM_APERTURE_HELPER?

• I think the functions from drm_fb_helper.h for removing other framebuffers should be moved here, and function name prefix adjusted acoordingly

I'm wondering about the locking and deadlock potential here, is lockdep all happy with this?

I haven't seen any complains, but I'll double check.

Best regards Thomas

Cheers, Daniel

...

---

drivers/gpu/drm/Kconfig | 6 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_platform.c | 118 +++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 18 ++++- include/drm/drm_platform.h | 42 ++++++++++++ 5 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_platform.c create mode 100644 include/drm/drm_platform.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index c4fd57d8b717..e9d6892f9d38 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -229,6 +229,12 @@ config DRM_SCHED tristate depends on DRM

+config DRM_PLATFORM_HELPER

• bool
• depends on DRM
• help

•  Helpers for DRM platform devices
  

source "drivers/gpu/drm/i2c/Kconfig"

source "drivers/gpu/drm/arm/Kconfig" diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 2c0e5a7e5953..8ceb21d0770a 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -32,6 +32,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o drm-$(CONFIG_PCI) += drm_pci.o drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o +drm-$(CONFIG_DRM_PLATFORM_HELPER) += drm_platform.o

drm_vram_helper-y := drm_gem_vram_helper.o obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c new file mode 100644 index 000000000000..09a2f2a31aa5 --- /dev/null +++ b/drivers/gpu/drm/drm_platform.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+#include <linux/mutex.h> +#include <linux/slab.h>

+#include <drm/drm_drv.h> +#include <drm/drm_managed.h> +#include <drm/drm_platform.h>

+static LIST_HEAD(drm_apertures);

+static DEFINE_MUTEX(drm_apertures_lock);

+static bool overlap(resource_size_t base1, resource_size_t end1,

•     resource_size_t base2, resource_size_t end2)
  

+{

• return (base1 < end2) && (end1 > base2);

+}

+static struct drm_aperture * +drm_aperture_acquire(struct drm_device *dev,

•      resource_size_t base, resource_size_t size,
  

•      const struct drm_aperture_funcs *funcs)
  

+{

• size_t end = base + size;
• struct list_head *pos;
• struct drm_aperture *ap;
• mutex_lock(&drm_apertures_lock);
• list_for_each(pos, &drm_apertures) {

• ap = container_of(pos, struct drm_aperture, lh);
  

• if (overlap(base, end, ap->base, ap->base + ap->size))
  

• 	return ERR_PTR(-EBUSY);
  

• }
• ap = drmm_kzalloc(dev, sizeof(*ap), GFP_KERNEL);
• if (!ap)

• return ERR_PTR(-ENOMEM);
  

• ap->dev = dev;
• ap->base = base;
• ap->size = size;
• ap->funcs = funcs;
• INIT_LIST_HEAD(&ap->lh);
• list_add(&ap->lh, &drm_apertures);
• mutex_unlock(&drm_apertures_lock);
• return ap;

+}

+static void drm_aperture_release(struct drm_aperture *ap) +{

• bool kicked_out = ap->kicked_out;
• if (!kicked_out)

• mutex_lock(&drm_apertures_lock);
  

• list_del(&ap->lh);
• if (ap->funcs->release)

• ap->funcs->release(ap);
  

• if (!kicked_out)

• mutex_unlock(&drm_apertures_lock);
  

+}

+static void drm_aperture_acquire_release(struct drm_device *dev, void *ptr) +{

• struct drm_aperture *ap = ptr;
• drm_aperture_release(ap);

+}

+struct drm_aperture * +drmm_aperture_acquire(struct drm_device *dev,

•       resource_size_t base, resource_size_t size,
  

•       const struct drm_aperture_funcs *funcs)
  

+{

• struct drm_aperture *ap;
• int ret;
• ap = drm_aperture_acquire(dev, base, size, funcs);
• if (IS_ERR(ap))

• return ap;
  

• ret = drmm_add_action_or_reset(dev, drm_aperture_acquire_release, ap);
• if (ret)

• return ERR_PTR(ret);
  

• return ap;

+} +EXPORT_SYMBOL(drmm_aperture_acquire);

+void drm_kickout_apertures_at(resource_size_t base, resource_size_t size) +{

• resource_size_t end = base + size;
• struct list_head *pos, *n;
• mutex_lock(&drm_apertures_lock);
• list_for_each_safe(pos, n, &drm_apertures) {

• struct drm_aperture *ap =
  

• 	container_of(pos, struct drm_aperture, lh);
  

• if (!overlap(base, end, ap->base, ap->base + ap->size))
  

• 	continue;
  

• ap->kicked_out = true;
  

• if (ap->funcs->kickout)
  

• 	ap->funcs->kickout(ap);
  

• else
  

• 	drm_dev_put(ap->dev);
  

• }
• mutex_unlock(&drm_apertures_lock);

+} +EXPORT_SYMBOL(drm_kickout_apertures_at); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 306aa3a60be9..a919b78b1961 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -35,7 +35,9 @@ struct drm_fb_helper; #include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_device.h> +#include <drm/drm_platform.h> #include <linux/kgdb.h> +#include <linux/pci.h> #include <linux/vgaarb.h>

enum mode_set_atomic { @@ -465,6 +467,11 @@ static inline int drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a, const char *name, bool primary) {

• int i;
• for (i = 0; i < a->count; ++i)

• drm_kickout_apertures_at(a->ranges[i].base, a->ranges[i].size);
  

#if IS_REACHABLE(CONFIG_FB) return remove_conflicting_framebuffers(a, name, primary); #else @@ -487,7 +494,16 @@ static inline int drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev, const char *name) {

• int ret = 0;

• resource_size_t base, size;

• int bar, ret = 0;

• for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {

• if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
  

• 	continue;
  

• base = pci_resource_start(pdev, bar);
  

• size = pci_resource_len(pdev, bar);
  

• drm_kickout_apertures_at(base, size);
  

• }

  /*

  • WARNING: Apparently we must kick fbdev drivers before vgacon,

diff --git a/include/drm/drm_platform.h b/include/drm/drm_platform.h new file mode 100644 index 000000000000..475e88ee1fbd --- /dev/null +++ b/include/drm/drm_platform.h @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+#ifndef _DRM_PLATFORM_H_ +#define _DRM_PLATFORM_H_

+#include <linux/list.h> +#include <linux/types.h>

+struct drm_aperture; +struct drm_device;

+struct drm_aperture_funcs {

• void (*kickout)(struct drm_aperture *ap);
• void (*release)(struct drm_aperture *ap);

+};

+struct drm_aperture {

• struct drm_device *dev;
• resource_size_t base;
• resource_size_t size;
• const struct drm_aperture_funcs *funcs;
• struct list_head lh;
• bool kicked_out;

+};

+struct drm_aperture * +drmm_aperture_acquire(struct drm_device *dev,

•       resource_size_t base, resource_size_t size,
  

•       const struct drm_aperture_funcs *funcs);
  

+#if defined (CONFIG_DRM_PLATFORM_HELPER) +void drm_kickout_apertures_at(resource_size_t base, resource_size_t size); +#else +static inline void +drm_kickout_apertures_at(resource_size_t base, resource_size_t size) +{ +} +#endif

+#endif

2.27.0

Hi

Am 28.09.20 um 10:50 schrieb Daniel Vetter:

On Mon, Sep 28, 2020 at 10:40 AM Thomas Zimmermann tzimmermann@suse.de wrote:

...

Hi

Am 29.06.20 um 11:27 schrieb Daniel Vetter:

...

On Thu, Jun 25, 2020 at 02:00:10PM +0200, Thomas Zimmermann wrote:

...

Platform devices might operate on firmware framebuffers, such as VESA or EFI. Before a native driver for the graphics hardware can take over the device, it has to remove any platform driver that operates on the firmware framebuffer. Platform helpers provide the infrastructure for platform drivers to acquire firmware framebuffers, and for native drivers to remove them lateron.

It works similar to the related fbdev mechanism. During initialization, the platform driver acquires the firmware framebuffer's I/O memory and provides a callback to be removed. The native driver later uses this inforamtion to remove any platform driver for it's framebuffer I/O memory.

The platform helper's removal code is integrated into the existing code for removing conflicting fraembuffers, so native drivers use it automatically.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

I have some ideas for how to do this a notch cleaner in the next patch. Maybe best to discuss the actual implmenentation stuff there.

Aside from that usual nits:

• kerneldoc for these please, pulled into drm-kms.rst.
• naming isn't super ocd with drm_platform.c but that prefix not used, but I also don't have better ideas.

I was never really happy with the naming either. Maybe call it aperture helpers with drm_aperture_ and CONFIG_DRM_APERTURE_HELPER?

+1

...

...

• I think the functions from drm_fb_helper.h for removing other framebuffers should be moved here, and function name prefix adjusted acoordingly

I'm wondering about the locking and deadlock potential here, is lockdep all happy with this?

I haven't seen any complains, but I'll double check.

Might be worth it to cc Greg on this, since the device driver model has a lot of corner cases to make sure it doesn't get stuck here with hiararchical drivers/device getting removed while something else is probing them at the same time.

Oh, OK. I'll keep that in mind when sending out v2 of these patches.

Best regards Thomas

-Daniel

...

Best regards Thomas

...

Cheers, Daniel

...

---

drivers/gpu/drm/Kconfig | 6 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_platform.c | 118 +++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 18 ++++- include/drm/drm_platform.h | 42 ++++++++++++ 5 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_platform.c create mode 100644 include/drm/drm_platform.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index c4fd57d8b717..e9d6892f9d38 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -229,6 +229,12 @@ config DRM_SCHED tristate depends on DRM

+config DRM_PLATFORM_HELPER

• bool
• depends on DRM
• help

•  Helpers for DRM platform devices
  

source "drivers/gpu/drm/i2c/Kconfig"

source "drivers/gpu/drm/arm/Kconfig" diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 2c0e5a7e5953..8ceb21d0770a 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -32,6 +32,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o drm-$(CONFIG_PCI) += drm_pci.o drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o +drm-$(CONFIG_DRM_PLATFORM_HELPER) += drm_platform.o

drm_vram_helper-y := drm_gem_vram_helper.o obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c new file mode 100644 index 000000000000..09a2f2a31aa5 --- /dev/null +++ b/drivers/gpu/drm/drm_platform.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+#include <linux/mutex.h> +#include <linux/slab.h>

+#include <drm/drm_drv.h> +#include <drm/drm_managed.h> +#include <drm/drm_platform.h>

+static LIST_HEAD(drm_apertures);

+static DEFINE_MUTEX(drm_apertures_lock);

+static bool overlap(resource_size_t base1, resource_size_t end1,

•            resource_size_t base2, resource_size_t end2)
  

+{

• return (base1 < end2) && (end1 > base2);

+}

+static struct drm_aperture * +drm_aperture_acquire(struct drm_device *dev,

•             resource_size_t base, resource_size_t size,
  

•             const struct drm_aperture_funcs *funcs)
  

+{

• size_t end = base + size;
• struct list_head *pos;
• struct drm_aperture *ap;
• mutex_lock(&drm_apertures_lock);
• list_for_each(pos, &drm_apertures) {

•        ap = container_of(pos, struct drm_aperture, lh);
  

•        if (overlap(base, end, ap->base, ap->base + ap->size))
  

•                return ERR_PTR(-EBUSY);
  

• }
• ap = drmm_kzalloc(dev, sizeof(*ap), GFP_KERNEL);
• if (!ap)

•        return ERR_PTR(-ENOMEM);
  

• ap->dev = dev;
• ap->base = base;
• ap->size = size;
• ap->funcs = funcs;
• INIT_LIST_HEAD(&ap->lh);
• list_add(&ap->lh, &drm_apertures);
• mutex_unlock(&drm_apertures_lock);
• return ap;

+}

+static void drm_aperture_release(struct drm_aperture *ap) +{

• bool kicked_out = ap->kicked_out;
• if (!kicked_out)

•        mutex_lock(&drm_apertures_lock);
  

• list_del(&ap->lh);
• if (ap->funcs->release)

•        ap->funcs->release(ap);
  

• if (!kicked_out)

•        mutex_unlock(&drm_apertures_lock);
  

+}

+static void drm_aperture_acquire_release(struct drm_device *dev, void *ptr) +{

• struct drm_aperture *ap = ptr;
• drm_aperture_release(ap);

+}

+struct drm_aperture * +drmm_aperture_acquire(struct drm_device *dev,

•              resource_size_t base, resource_size_t size,
  

•              const struct drm_aperture_funcs *funcs)
  

+{

• struct drm_aperture *ap;
• int ret;
• ap = drm_aperture_acquire(dev, base, size, funcs);
• if (IS_ERR(ap))

•        return ap;
  

• ret = drmm_add_action_or_reset(dev, drm_aperture_acquire_release, ap);
• if (ret)

•        return ERR_PTR(ret);
  

• return ap;

+} +EXPORT_SYMBOL(drmm_aperture_acquire);

+void drm_kickout_apertures_at(resource_size_t base, resource_size_t size) +{

• resource_size_t end = base + size;
• struct list_head *pos, *n;
• mutex_lock(&drm_apertures_lock);
• list_for_each_safe(pos, n, &drm_apertures) {

•        struct drm_aperture *ap =
  

•                container_of(pos, struct drm_aperture, lh);
  

•        if (!overlap(base, end, ap->base, ap->base + ap->size))
  

•                continue;
  

•        ap->kicked_out = true;
  

•        if (ap->funcs->kickout)
  

•                ap->funcs->kickout(ap);
  

•        else
  

•                drm_dev_put(ap->dev);
  

• }
• mutex_unlock(&drm_apertures_lock);

+} +EXPORT_SYMBOL(drm_kickout_apertures_at); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 306aa3a60be9..a919b78b1961 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -35,7 +35,9 @@ struct drm_fb_helper; #include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_device.h> +#include <drm/drm_platform.h> #include <linux/kgdb.h> +#include <linux/pci.h> #include <linux/vgaarb.h>

enum mode_set_atomic { @@ -465,6 +467,11 @@ static inline int drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a, const char *name, bool primary) {

• int i;
• for (i = 0; i < a->count; ++i)

•        drm_kickout_apertures_at(a->ranges[i].base, a->ranges[i].size);
  

#if IS_REACHABLE(CONFIG_FB) return remove_conflicting_framebuffers(a, name, primary); #else @@ -487,7 +494,16 @@ static inline int drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev, const char *name) {

• int ret = 0;

• resource_size_t base, size;

• int bar, ret = 0;

• for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {

•        if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
  

•                continue;
  

•        base = pci_resource_start(pdev, bar);
  

•        size = pci_resource_len(pdev, bar);
  

•        drm_kickout_apertures_at(base, size);
  

• }

  /*

  • WARNING: Apparently we must kick fbdev drivers before vgacon,

diff --git a/include/drm/drm_platform.h b/include/drm/drm_platform.h new file mode 100644 index 000000000000..475e88ee1fbd --- /dev/null +++ b/include/drm/drm_platform.h @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+#ifndef _DRM_PLATFORM_H_ +#define _DRM_PLATFORM_H_

+#include <linux/list.h> +#include <linux/types.h>

+struct drm_aperture; +struct drm_device;

+struct drm_aperture_funcs {

• void (*kickout)(struct drm_aperture *ap);
• void (*release)(struct drm_aperture *ap);

+};

+struct drm_aperture {

• struct drm_device *dev;
• resource_size_t base;
• resource_size_t size;
• const struct drm_aperture_funcs *funcs;
• struct list_head lh;
• bool kicked_out;

+};

+struct drm_aperture * +drmm_aperture_acquire(struct drm_device *dev,

•              resource_size_t base, resource_size_t size,
  

•              const struct drm_aperture_funcs *funcs);
  

+#if defined (CONFIG_DRM_PLATFORM_HELPER) +void drm_kickout_apertures_at(resource_size_t base, resource_size_t size); +#else +static inline void +drm_kickout_apertures_at(resource_size_t base, resource_size_t size) +{ +} +#endif

+#endif

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

On Tue, Sep 29, 2020 at 10:59:10AM +0200, Thomas Zimmermann wrote:

Hi

Am 29.06.20 um 11:27 schrieb Daniel Vetter:

...

On Thu, Jun 25, 2020 at 02:00:10PM +0200, Thomas Zimmermann wrote:

...

Platform devices might operate on firmware framebuffers, such as VESA or EFI. Before a native driver for the graphics hardware can take over the device, it has to remove any platform driver that operates on the firmware framebuffer. Platform helpers provide the infrastructure for platform drivers to acquire firmware framebuffers, and for native drivers to remove them lateron.

It works similar to the related fbdev mechanism. During initialization, the platform driver acquires the firmware framebuffer's I/O memory and provides a callback to be removed. The native driver later uses this inforamtion to remove any platform driver for it's framebuffer I/O memory.

The platform helper's removal code is integrated into the existing code for removing conflicting fraembuffers, so native drivers use it automatically.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

I have some ideas for how to do this a notch cleaner in the next patch. Maybe best to discuss the actual implmenentation stuff there.

Aside from that usual nits:

• kerneldoc for these please, pulled into drm-kms.rst.

Any specific reason for drm-kms?

The aperture helpers are used to manage ownership of memory and most drivers begin with drm_fb_helper_remove_conflicting_framebuffers(). It seems more approprite to put this into drm-internals as part of the driver initialization.

Simple because the only reason you might want to use this is for display. In no other case do we load a firmware driver to make hw semi-useable. Putting it where people might look for it and all that. -Daniel

Best regards Thomas

...

• naming isn't super ocd with drm_platform.c but that prefix not used, but I also don't have better ideas.
• I think the functions from drm_fb_helper.h for removing other framebuffers should be moved here, and function name prefix adjusted acoordingly

I'm wondering about the locking and deadlock potential here, is lockdep all happy with this?

Cheers, Daniel

...

---

drivers/gpu/drm/Kconfig | 6 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_platform.c | 118 +++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 18 ++++- include/drm/drm_platform.h | 42 ++++++++++++ 5 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_platform.c create mode 100644 include/drm/drm_platform.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index c4fd57d8b717..e9d6892f9d38 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -229,6 +229,12 @@ config DRM_SCHED tristate depends on DRM

+config DRM_PLATFORM_HELPER

• bool
• depends on DRM
• help

•  Helpers for DRM platform devices
  

source "drivers/gpu/drm/i2c/Kconfig"

source "drivers/gpu/drm/arm/Kconfig" diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 2c0e5a7e5953..8ceb21d0770a 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -32,6 +32,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o drm-$(CONFIG_PCI) += drm_pci.o drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o +drm-$(CONFIG_DRM_PLATFORM_HELPER) += drm_platform.o

drm_vram_helper-y := drm_gem_vram_helper.o obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c new file mode 100644 index 000000000000..09a2f2a31aa5 --- /dev/null +++ b/drivers/gpu/drm/drm_platform.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+#include <linux/mutex.h> +#include <linux/slab.h>

+#include <drm/drm_drv.h> +#include <drm/drm_managed.h> +#include <drm/drm_platform.h>

+static LIST_HEAD(drm_apertures);

+static DEFINE_MUTEX(drm_apertures_lock);

+static bool overlap(resource_size_t base1, resource_size_t end1,

•     resource_size_t base2, resource_size_t end2)
  

+{

• return (base1 < end2) && (end1 > base2);

+}

+static struct drm_aperture * +drm_aperture_acquire(struct drm_device *dev,

•      resource_size_t base, resource_size_t size,
  

•      const struct drm_aperture_funcs *funcs)
  

+{

• size_t end = base + size;
• struct list_head *pos;
• struct drm_aperture *ap;
• mutex_lock(&drm_apertures_lock);
• list_for_each(pos, &drm_apertures) {

• ap = container_of(pos, struct drm_aperture, lh);
  

• if (overlap(base, end, ap->base, ap->base + ap->size))
  

• 	return ERR_PTR(-EBUSY);
  

• }
• ap = drmm_kzalloc(dev, sizeof(*ap), GFP_KERNEL);
• if (!ap)

• return ERR_PTR(-ENOMEM);
  

• ap->dev = dev;
• ap->base = base;
• ap->size = size;
• ap->funcs = funcs;
• INIT_LIST_HEAD(&ap->lh);
• list_add(&ap->lh, &drm_apertures);
• mutex_unlock(&drm_apertures_lock);
• return ap;

+}

+static void drm_aperture_release(struct drm_aperture *ap) +{

• bool kicked_out = ap->kicked_out;
• if (!kicked_out)

• mutex_lock(&drm_apertures_lock);
  

• list_del(&ap->lh);
• if (ap->funcs->release)

• ap->funcs->release(ap);
  

• if (!kicked_out)

• mutex_unlock(&drm_apertures_lock);
  

+}

+static void drm_aperture_acquire_release(struct drm_device *dev, void *ptr) +{

• struct drm_aperture *ap = ptr;
• drm_aperture_release(ap);

+}

+struct drm_aperture * +drmm_aperture_acquire(struct drm_device *dev,

•       resource_size_t base, resource_size_t size,
  

•       const struct drm_aperture_funcs *funcs)
  

+{

• struct drm_aperture *ap;
• int ret;
• ap = drm_aperture_acquire(dev, base, size, funcs);
• if (IS_ERR(ap))

• return ap;
  

• ret = drmm_add_action_or_reset(dev, drm_aperture_acquire_release, ap);
• if (ret)

• return ERR_PTR(ret);
  

• return ap;

+} +EXPORT_SYMBOL(drmm_aperture_acquire);

+void drm_kickout_apertures_at(resource_size_t base, resource_size_t size) +{

• resource_size_t end = base + size;
• struct list_head *pos, *n;
• mutex_lock(&drm_apertures_lock);
• list_for_each_safe(pos, n, &drm_apertures) {

• struct drm_aperture *ap =
  

• 	container_of(pos, struct drm_aperture, lh);
  

• if (!overlap(base, end, ap->base, ap->base + ap->size))
  

• 	continue;
  

• ap->kicked_out = true;
  

• if (ap->funcs->kickout)
  

• 	ap->funcs->kickout(ap);
  

• else
  

• 	drm_dev_put(ap->dev);
  

• }
• mutex_unlock(&drm_apertures_lock);

+} +EXPORT_SYMBOL(drm_kickout_apertures_at); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 306aa3a60be9..a919b78b1961 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -35,7 +35,9 @@ struct drm_fb_helper; #include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_device.h> +#include <drm/drm_platform.h> #include <linux/kgdb.h> +#include <linux/pci.h> #include <linux/vgaarb.h>

enum mode_set_atomic { @@ -465,6 +467,11 @@ static inline int drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a, const char *name, bool primary) {

• int i;
• for (i = 0; i < a->count; ++i)

• drm_kickout_apertures_at(a->ranges[i].base, a->ranges[i].size);
  

#if IS_REACHABLE(CONFIG_FB) return remove_conflicting_framebuffers(a, name, primary); #else @@ -487,7 +494,16 @@ static inline int drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev, const char *name) {

• int ret = 0;

• resource_size_t base, size;

• int bar, ret = 0;

• for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {

• if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
  

• 	continue;
  

• base = pci_resource_start(pdev, bar);
  

• size = pci_resource_len(pdev, bar);
  

• drm_kickout_apertures_at(base, size);
  

• }

  /*

  • WARNING: Apparently we must kick fbdev drivers before vgacon,

diff --git a/include/drm/drm_platform.h b/include/drm/drm_platform.h new file mode 100644 index 000000000000..475e88ee1fbd --- /dev/null +++ b/include/drm/drm_platform.h @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+#ifndef _DRM_PLATFORM_H_ +#define _DRM_PLATFORM_H_

+#include <linux/list.h> +#include <linux/types.h>

+struct drm_aperture; +struct drm_device;

+struct drm_aperture_funcs {

• void (*kickout)(struct drm_aperture *ap);
• void (*release)(struct drm_aperture *ap);

+};

+struct drm_aperture {

• struct drm_device *dev;
• resource_size_t base;
• resource_size_t size;
• const struct drm_aperture_funcs *funcs;
• struct list_head lh;
• bool kicked_out;

+};

+struct drm_aperture * +drmm_aperture_acquire(struct drm_device *dev,

•       resource_size_t base, resource_size_t size,
  

•       const struct drm_aperture_funcs *funcs);
  

+#if defined (CONFIG_DRM_PLATFORM_HELPER) +void drm_kickout_apertures_at(resource_size_t base, resource_size_t size); +#else +static inline void +drm_kickout_apertures_at(resource_size_t base, resource_size_t size) +{ +} +#endif

+#endif

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