| Message ID | 1412711357-23299-1-git-send-email-zachr@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 7, 2014 at 12:49 PM, Zach Reizner <zachr@google.com> wrote: > This patch allows framebuffers for cirrus to be created with > 32bpp pixel formats provided that they do not violate certain > restrictions of the cirrus hardware. > > Signed-off-by: Zach Reizner <zachr@google.com> > Reviewed-by: Stéphane Marchesin <marcheu@chromium.org> Dave, Adam: are you ok with this patch? Stéphane > --- > drivers/gpu/drm/cirrus/cirrus_drv.h | 2 ++ > drivers/gpu/drm/cirrus/cirrus_fbdev.c | 4 +++- > drivers/gpu/drm/cirrus/cirrus_main.c | 22 ++++++++++++++++++++-- > 3 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.h > b/drivers/gpu/drm/cirrus/cirrus_drv.h > index 401c890..fac475c 100644 > --- a/drivers/gpu/drm/cirrus/cirrus_drv.h > +++ b/drivers/gpu/drm/cirrus/cirrus_drv.h > @@ -208,6 +208,8 @@ int cirrus_framebuffer_init(struct drm_device *dev, > struct drm_mode_fb_cmd2 *mode_cmd, > struct drm_gem_object *obj); > > +bool cirrus_check_framebuffer(int width, int height, int bpp, int pitch); > + > /* cirrus_display.c */ > int cirrus_modeset_init(struct cirrus_device *cdev); > void cirrus_modeset_fini(struct cirrus_device *cdev); > diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c > b/drivers/gpu/drm/cirrus/cirrus_fbdev.c > index 2a135f2..4a0756c 100644 > --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c > +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c > @@ -146,8 +146,10 @@ static int cirrusfb_create_object(struct cirrus_fbdev > *afbdev, > int ret = 0; > drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp); > > - if (bpp > 24) > + if (!cirrus_check_framebuffer(mode_cmd->width, mode_cmd->height, > bpp, > + mode_cmd->pitches[0])) > return -EINVAL; > + > size = mode_cmd->pitches[0] * mode_cmd->height; > ret = cirrus_gem_create(dev, size, true, &gobj); > if (ret) > diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c > b/drivers/gpu/drm/cirrus/cirrus_main.c > index 99c1983..029f9e9 100644 > --- a/drivers/gpu/drm/cirrus/cirrus_main.c > +++ b/drivers/gpu/drm/cirrus/cirrus_main.c > @@ -55,8 +55,9 @@ cirrus_user_framebuffer_create(struct drm_device *dev, > u32 bpp, depth; > > drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp); > - /* cirrus can't handle > 24bpp framebuffers at all */ > - if (bpp > 24) > + > + if (!cirrus_check_framebuffer(mode_cmd->width, mode_cmd->height, > + bpp, mode_cmd->pitches[0])) > return ERR_PTR(-EINVAL); > > obj = drm_gem_object_lookup(dev, filp, mode_cmd->handles[0]); > @@ -307,3 +308,20 @@ out_unlock: > return ret; > > } > + > +bool cirrus_check_framebuffer(int width, int height, int bpp, int pitch) > +{ > + const int max_pitch = 0x1FF << 3; /* (4096 - 1) & ~111b bytes */ > + const int max_size = 0x400000; /* 4MB */ > + > + if (bpp > 32) > + return false; > + > + if (pitch > max_pitch) > + return false; > + > + if (pitch * height > max_size) > + return false; > + > + return true; > +} > -- > 2.1.0.rc2.206.gedb03e5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Fri, 2014-10-24 at 18:31 -0700, Stéphane Marchesin wrote:
> Dave, Adam: are you ok with this patch?
Seems like it doesn't go far enough? You'd already need an "aware"
guest to have this work, since the chip actually being emulated didn't
have 32bpp. The pitch check would prevent 1024x768x32 and larger from
working, which makes it sort of a weak win on its own.
Could we read the BAR size from the device instead of hardcoding 4M?
That alone would make 1920x1200x16 work; and then, if that pitch limit
isn't encoded into the register space representation, the aware guest
would have resolutions worth using at least be possible.
- ajax
On Mo, 2014-10-27 at 11:00 -0400, Adam Jackson wrote: > On Fri, 2014-10-24 at 18:31 -0700, Stéphane Marchesin wrote: > > > Dave, Adam: are you ok with this patch? > > Seems like it doesn't go far enough? You'd already need an "aware" > guest to have this work, since the chip actually being emulated didn't > have 32bpp. The pitch check would prevent 1024x768x32 and larger from > working, which makes it sort of a weak win on its own. > > Could we read the BAR size from the device instead of hardcoding 4M? > That alone would make 1920x1200x16 work; and then, if that pitch limit > isn't encoded into the register space representation, the aware guest > would have resolutions worth using at least be possible. How about stop using cirrus and go for 'qemu -vga std' instead? Linux kernel 3.14+ comes with a modesetting driver for the qemu standard vga (CONFIG_DRM_BOCHS). Just switch over, and all your cirrus pain is gone. That is much better than trying use features the real cirrus hardware never had, then praying that all qemu versions in the wild are actually doing what you want qemu do. cheers, Gerd https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/
Real cirrus did have 32bpp according to both the reference manual and qemu's emulation of the hardware. The cirrus DRM driver even had the modesetting code for 32bpp but the driver prevented the creation of 32bpp framebuffers. Best, Zach On Mon, Oct 27, 2014, 08:30 Gerd Hoffmann <kraxel@redhat.com> wrote: > On Mo, 2014-10-27 at 11:00 -0400, Adam Jackson wrote: > > On Fri, 2014-10-24 at 18:31 -0700, Stéphane Marchesin wrote: > > > > > Dave, Adam: are you ok with this patch? > > > > Seems like it doesn't go far enough? You'd already need an "aware" > > guest to have this work, since the chip actually being emulated didn't > > have 32bpp. The pitch check would prevent 1024x768x32 and larger from > > working, which makes it sort of a weak win on its own. > > > > Could we read the BAR size from the device instead of hardcoding 4M? > > That alone would make 1920x1200x16 work; and then, if that pitch limit > > isn't encoded into the register space representation, the aware guest > > would have resolutions worth using at least be possible. > > How about stop using cirrus and go for 'qemu -vga std' instead? > > Linux kernel 3.14+ comes with a modesetting driver for the qemu standard > vga (CONFIG_DRM_BOCHS). Just switch over, and all your cirrus pain is > gone. > > That is much better than trying use features the real cirrus hardware > never had, then praying that all qemu versions in the wild are actually > doing what you want qemu do. > > cheers, > Gerd > > https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/ > >
On Mon, 2014-10-27 at 16:30 +0100, Gerd Hoffmann wrote: > How about stop using cirrus and go for 'qemu -vga std' instead? > > Linux kernel 3.14+ comes with a modesetting driver for the qemu standard > vga (CONFIG_DRM_BOCHS). Just switch over, and all your cirrus pain is > gone. > > That is much better than trying use features the real cirrus hardware > never had, then praying that all qemu versions in the wild are actually > doing what you want qemu do. I was going to ask for a pci revision id bump at the same time, so the guest could know. I heartily agree, people should stop using cirrus. And qemu should stop defaulting to it. Since neither has happened yet, enhancing the emulation holds the promise of making the future better... - ajax
ajax, What do you mean by a pci revision id bump? Do you want it in qemu or the kernel? Why is a revision bump needed when none of the behavior of the cirrus device has changed? I agree that reading the BAR size from the device would be an enhancement. I'm working on the patch to include that now. -Zach On Mon Oct 27 2014 at 9:21:42 AM Adam Jackson <ajax@redhat.com> wrote: > On Mon, 2014-10-27 at 16:30 +0100, Gerd Hoffmann wrote: > > > How about stop using cirrus and go for 'qemu -vga std' instead? > > > > Linux kernel 3.14+ comes with a modesetting driver for the qemu standard > > vga (CONFIG_DRM_BOCHS). Just switch over, and all your cirrus pain is > > gone. > > > > That is much better than trying use features the real cirrus hardware > > never had, then praying that all qemu versions in the wild are actually > > doing what you want qemu do. > > I was going to ask for a pci revision id bump at the same time, so the > guest could know. > > I heartily agree, people should stop using cirrus. And qemu should stop > defaulting to it. Since neither has happened yet, enhancing the > emulation holds the promise of making the future better... > > - ajax > >
On Mo, 2014-10-27 at 22:09 +0000, Zachary Reizner wrote: > ajax, > What do you mean by a pci revision id bump? Do you want it in qemu or > the kernel? Why is a revision bump needed when none of the behavior of > the cirrus device has changed? revision bump would be needed when adding a new interface (not present in real hardware) to allow for larger pitches, so you could use 32bpp with the default (1024x768) resolution. > I heartily agree, people should stop using cirrus. And qemu > should stop > defaulting to it. Since neither has happened yet, enhancing > the > emulation holds the promise of making the future better... I'll try flip the default. That is more sane than pimping up the cirrus emulation. cheers, Gerd
> > revision bump would be needed when adding a new interface (not present > in real hardware) to allow for larger pitches, so you could use 32bpp > with the default (1024x768) resolution. > I did not change the pitch mechanism. The hardware is limited to less than a 4KB pitch because of the registers, so I kept that limitation in the driver. That means no revision bump is necessary.
On Di, 2014-10-28 at 17:04 +0000, Zachary Reizner wrote: > revision bump would be needed when adding a new interface (not > present > in real hardware) to allow for larger pitches, so you could > use 32bpp > with the default (1024x768) resolution. > I did not change the pitch mechanism. Sure. That was Adams idea to make the patch more useful. > The hardware is limited to less than a 4KB pitch because of the > registers, so I kept that limitation in the driver. That means no > revision bump is necessary. Yes, but it also implies that 32bpp will work on very low resolutions only (excluding the default res of 1024x768). I don't think the patch is very useful because of that. Nevertheless no objections to include it from my side. cheers, Gerd
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.h b/drivers/gpu/drm/cirrus/cirrus_drv.h index 401c890..fac475c 100644 --- a/drivers/gpu/drm/cirrus/cirrus_drv.h +++ b/drivers/gpu/drm/cirrus/cirrus_drv.h @@ -208,6 +208,8 @@ int cirrus_framebuffer_init(struct drm_device *dev, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object *obj); +bool cirrus_check_framebuffer(int width, int height, int bpp, int pitch); + /* cirrus_display.c */ int cirrus_modeset_init(struct cirrus_device *cdev); void cirrus_modeset_fini(struct cirrus_device *cdev); diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c index 2a135f2..4a0756c 100644 --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c @@ -146,8 +146,10 @@ static int cirrusfb_create_object(struct cirrus_fbdev *afbdev, int ret = 0; drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp); - if (bpp > 24) + if (!cirrus_check_framebuffer(mode_cmd->width, mode_cmd->height, bpp, + mode_cmd->pitches[0])) return -EINVAL; + size = mode_cmd->pitches[0] * mode_cmd->height; ret = cirrus_gem_create(dev, size, true, &gobj); if (ret) diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c index 99c1983..029f9e9 100644 --- a/drivers/gpu/drm/cirrus/cirrus_main.c +++ b/drivers/gpu/drm/cirrus/cirrus_main.c @@ -55,8 +55,9 @@ cirrus_user_framebuffer_create(struct drm_device *dev, u32 bpp, depth; drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp); - /* cirrus can't handle > 24bpp framebuffers at all */ - if (bpp > 24) + + if (!cirrus_check_framebuffer(mode_cmd->width, mode_cmd->height, + bpp, mode_cmd->pitches[0])) return ERR_PTR(-EINVAL); obj = drm_gem_object_lookup(dev, filp, mode_cmd->handles[0]); @@ -307,3 +308,20 @@ out_unlock: return ret; } + +bool cirrus_check_framebuffer(int width, int height, int bpp, int pitch) +{ + const int max_pitch = 0x1FF << 3; /* (4096 - 1) & ~111b bytes */ + const int max_size = 0x400000; /* 4MB */ + + if (bpp > 32) + return false; + + if (pitch > max_pitch) + return false; + + if (pitch * height > max_size) + return false; + + return true; +}
This patch allows framebuffers for cirrus to be created with 32bpp pixel formats provided that they do not violate certain restrictions of the cirrus hardware. Signed-off-by: Zach Reizner <zachr@google.com> --- drivers/gpu/drm/cirrus/cirrus_drv.h | 2 ++ drivers/gpu/drm/cirrus/cirrus_fbdev.c | 4 +++- drivers/gpu/drm/cirrus/cirrus_main.c | 22 ++++++++++++++++++++-- 3 files changed, 25 insertions(+), 3 deletions(-)