Store width and height of the bo. Needed in case userspace
sets up a framebuffer with fb->width != bo->width..
Signed-off-by: Gerd Hoffmann <[email protected]>
---
include/drm/drm_gem_vram_helper.h | 1 +
drivers/gpu/drm/drm_gem_vram_helper.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 1a0ea18e7a74..3692dba167df 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -39,6 +39,7 @@ struct drm_gem_vram_object {
struct drm_gem_object gem;
struct ttm_buffer_object bo;
struct ttm_bo_kmap_obj kmap;
+ unsigned int width, height;
/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
struct ttm_placement placement;
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 4de782ca26b2..c02bf7694117 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -377,6 +377,8 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
gbo = drm_gem_vram_create(dev, bdev, size, pg_align, interruptible);
if (IS_ERR(gbo))
return PTR_ERR(gbo);
+ gbo->width = args->width;
+ gbo->height = args->height;
ret = drm_gem_handle_create(file, &gbo->gem, &handle);
if (ret)
--
2.18.1
Hi Gerd.
On Wed, Jun 26, 2019 at 08:55:50AM +0200, Gerd Hoffmann wrote:
> Store width and height of the bo. Needed in case userspace
> sets up a framebuffer with fb->width != bo->width..
>
> Signed-off-by: Gerd Hoffmann <[email protected]>
> ---
> include/drm/drm_gem_vram_helper.h | 1 +
> drivers/gpu/drm/drm_gem_vram_helper.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index 1a0ea18e7a74..3692dba167df 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -39,6 +39,7 @@ struct drm_gem_vram_object {
> struct drm_gem_object gem;
> struct ttm_buffer_object bo;
> struct ttm_bo_kmap_obj kmap;
> + unsigned int width, height;
>
> /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> struct ttm_placement placement;
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 4de782ca26b2..c02bf7694117 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -377,6 +377,8 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> gbo = drm_gem_vram_create(dev, bdev, size, pg_align, interruptible);
> if (IS_ERR(gbo))
> return PTR_ERR(gbo);
> + gbo->width = args->width;
> + gbo->height = args->height;
>
> ret = drm_gem_handle_create(file, &gbo->gem, &handle);
> if (ret)
Be warned, I may have missed something in the bigger picture.
Your patch will set width and height only for dumb bo's
But we have several users of drm_gem_vram_create() that will
not set the width and height.
So only in some cases can we rely on them being set.
Should this be refactored so we always set width, height.
Or maybe say in a small comment that width,height are only
set for dumb bo's?
Sam
On Wed, Jun 26, 2019 at 04:40:13PM +0200, Sam Ravnborg wrote:
> Hi Gerd.
>
> On Wed, Jun 26, 2019 at 08:55:50AM +0200, Gerd Hoffmann wrote:
> > Store width and height of the bo. Needed in case userspace
> > sets up a framebuffer with fb->width != bo->width..
> >
> > Signed-off-by: Gerd Hoffmann <[email protected]>
> > ---
> > include/drm/drm_gem_vram_helper.h | 1 +
> > drivers/gpu/drm/drm_gem_vram_helper.c | 2 ++
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> > index 1a0ea18e7a74..3692dba167df 100644
> > --- a/include/drm/drm_gem_vram_helper.h
> > +++ b/include/drm/drm_gem_vram_helper.h
> > @@ -39,6 +39,7 @@ struct drm_gem_vram_object {
> > struct drm_gem_object gem;
> > struct ttm_buffer_object bo;
> > struct ttm_bo_kmap_obj kmap;
> > + unsigned int width, height;
> >
> > /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> > struct ttm_placement placement;
> > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> > index 4de782ca26b2..c02bf7694117 100644
> > --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> > @@ -377,6 +377,8 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> > gbo = drm_gem_vram_create(dev, bdev, size, pg_align, interruptible);
> > if (IS_ERR(gbo))
> > return PTR_ERR(gbo);
> > + gbo->width = args->width;
> > + gbo->height = args->height;
> >
> > ret = drm_gem_handle_create(file, &gbo->gem, &handle);
> > if (ret)
>
> Be warned, I may have missed something in the bigger picture.
>
> Your patch will set width and height only for dumb bo's
> But we have several users of drm_gem_vram_create() that will
> not set the width and height.
>
> So only in some cases can we rely on them being set.
> Should this be refactored so we always set width, height.
> Or maybe say in a small comment that width,height are only
> set for dumb bo's?
Also for dumb bo allocated buffers if userspace gets the dimensions wrong
between dumb_create and the addfb, something is wrong. Papering over that
by remembering the right dimensions doesn't look like a good solution.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi
Am 26.06.19 um 08:55 schrieb Gerd Hoffmann:
> Store width and height of the bo. Needed in case userspace
> sets up a framebuffer with fb->width != bo->width..
This seems like bug. I'd rather return an error to userspace if the BO
is incompatible.
For the Gnome issue, a fix would be to program the display HW's line
pitch to the correct value.
Best regards
Thomas
> Signed-off-by: Gerd Hoffmann <[email protected]>
> ---
> include/drm/drm_gem_vram_helper.h | 1 +
> drivers/gpu/drm/drm_gem_vram_helper.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index 1a0ea18e7a74..3692dba167df 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -39,6 +39,7 @@ struct drm_gem_vram_object {
> struct drm_gem_object gem;
> struct ttm_buffer_object bo;
> struct ttm_bo_kmap_obj kmap;
> + unsigned int width, height;
>
> /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> struct ttm_placement placement;
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 4de782ca26b2..c02bf7694117 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -377,6 +377,8 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> gbo = drm_gem_vram_create(dev, bdev, size, pg_align, interruptible);
> if (IS_ERR(gbo))
> return PTR_ERR(gbo);
> + gbo->width = args->width;
> + gbo->height = args->height;
>
> ret = drm_gem_handle_create(file, &gbo->gem, &handle);
> if (ret)
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Hi,
> For the Gnome issue, a fix would be to program the display HW's line
> pitch to the correct value.
Yes, and drm_framebuffer actually has the pitches,
so no need for this patch indeed.
cheers,
Gerd
On Wed, 26 Jun 2019 18:27:54 +0200
Daniel Vetter <[email protected]> wrote:
> On Wed, Jun 26, 2019 at 04:40:13PM +0200, Sam Ravnborg wrote:
> > Hi Gerd.
> >
> > On Wed, Jun 26, 2019 at 08:55:50AM +0200, Gerd Hoffmann wrote:
> > > Store width and height of the bo. Needed in case userspace
> > > sets up a framebuffer with fb->width != bo->width..
> > >
> > > Signed-off-by: Gerd Hoffmann <[email protected]>
> > > ---
> > > include/drm/drm_gem_vram_helper.h | 1 +
> > > drivers/gpu/drm/drm_gem_vram_helper.c | 2 ++
> > > 2 files changed, 3 insertions(+)
> > >
> > > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> > > index 1a0ea18e7a74..3692dba167df 100644
> > > --- a/include/drm/drm_gem_vram_helper.h
> > > +++ b/include/drm/drm_gem_vram_helper.h
> > > @@ -39,6 +39,7 @@ struct drm_gem_vram_object {
> > > struct drm_gem_object gem;
> > > struct ttm_buffer_object bo;
> > > struct ttm_bo_kmap_obj kmap;
> > > + unsigned int width, height;
> > >
> > > /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> > > struct ttm_placement placement;
> > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> > > index 4de782ca26b2..c02bf7694117 100644
> > > --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> > > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> > > @@ -377,6 +377,8 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> > > gbo = drm_gem_vram_create(dev, bdev, size, pg_align, interruptible);
> > > if (IS_ERR(gbo))
> > > return PTR_ERR(gbo);
> > > + gbo->width = args->width;
> > > + gbo->height = args->height;
> > >
> > > ret = drm_gem_handle_create(file, &gbo->gem, &handle);
> > > if (ret)
> >
> > Be warned, I may have missed something in the bigger picture.
> >
> > Your patch will set width and height only for dumb bo's
> > But we have several users of drm_gem_vram_create() that will
> > not set the width and height.
> >
> > So only in some cases can we rely on them being set.
> > Should this be refactored so we always set width, height.
> > Or maybe say in a small comment that width,height are only
> > set for dumb bo's?
>
> Also for dumb bo allocated buffers if userspace gets the dimensions wrong
> between dumb_create and the addfb, something is wrong. Papering over that
> by remembering the right dimensions doesn't look like a good solution.
Hi,
just a note irrelevant to this particular driver:
I have deliberately allocated a too high dumb buffer in userspace and
created multiple smaller DRM FBs out of it with different offsets
(i * 128 * stride). This has been a very useful hack to see that a
GPU-less driver actually honours fences correctly, because if it
doesn't, the whole image will be off by the offset delta, which is
epileptically easy to see.
So I'm not getting the height wrong, I am deliberately overallocating
and aliasing.
Thanks,
pq
On Mon, Jul 08, 2019 at 04:39:45PM +0300, Pekka Paalanen wrote:
> On Wed, 26 Jun 2019 18:27:54 +0200
> Daniel Vetter <[email protected]> wrote:
>
> > On Wed, Jun 26, 2019 at 04:40:13PM +0200, Sam Ravnborg wrote:
> > > Hi Gerd.
> > >
> > > On Wed, Jun 26, 2019 at 08:55:50AM +0200, Gerd Hoffmann wrote:
> > > > Store width and height of the bo. Needed in case userspace
> > > > sets up a framebuffer with fb->width != bo->width..
> > > >
> > > > Signed-off-by: Gerd Hoffmann <[email protected]>
> > > > ---
> > > > include/drm/drm_gem_vram_helper.h | 1 +
> > > > drivers/gpu/drm/drm_gem_vram_helper.c | 2 ++
> > > > 2 files changed, 3 insertions(+)
> > > >
> > > > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> > > > index 1a0ea18e7a74..3692dba167df 100644
> > > > --- a/include/drm/drm_gem_vram_helper.h
> > > > +++ b/include/drm/drm_gem_vram_helper.h
> > > > @@ -39,6 +39,7 @@ struct drm_gem_vram_object {
> > > > struct drm_gem_object gem;
> > > > struct ttm_buffer_object bo;
> > > > struct ttm_bo_kmap_obj kmap;
> > > > + unsigned int width, height;
> > > >
> > > > /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> > > > struct ttm_placement placement;
> > > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> > > > index 4de782ca26b2..c02bf7694117 100644
> > > > --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> > > > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> > > > @@ -377,6 +377,8 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file,
> > > > gbo = drm_gem_vram_create(dev, bdev, size, pg_align, interruptible);
> > > > if (IS_ERR(gbo))
> > > > return PTR_ERR(gbo);
> > > > + gbo->width = args->width;
> > > > + gbo->height = args->height;
> > > >
> > > > ret = drm_gem_handle_create(file, &gbo->gem, &handle);
> > > > if (ret)
> > >
> > > Be warned, I may have missed something in the bigger picture.
> > >
> > > Your patch will set width and height only for dumb bo's
> > > But we have several users of drm_gem_vram_create() that will
> > > not set the width and height.
> > >
> > > So only in some cases can we rely on them being set.
> > > Should this be refactored so we always set width, height.
> > > Or maybe say in a small comment that width,height are only
> > > set for dumb bo's?
> >
> > Also for dumb bo allocated buffers if userspace gets the dimensions wrong
> > between dumb_create and the addfb, something is wrong. Papering over that
> > by remembering the right dimensions doesn't look like a good solution.
>
> Hi,
>
> just a note irrelevant to this particular driver:
>
> I have deliberately allocated a too high dumb buffer in userspace and
> created multiple smaller DRM FBs out of it with different offsets
> (i * 128 * stride). This has been a very useful hack to see that a
> GPU-less driver actually honours fences correctly, because if it
> doesn't, the whole image will be off by the offset delta, which is
> epileptically easy to see.
>
> So I'm not getting the height wrong, I am deliberately overallocating
> and aliasing.
Yeah that's the other reason for why this patch is wrong: It would break
things like this trickery here :-) The separation between backing storage
and drm_fb is intentional, multiple fb in one overall fb is explicitly ok.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch