* [PATCH] allow 32bpp framebuffers for cirrus drm
@ 2014-10-07 19:49 Zach Reizner
2014-10-25 1:31 ` Stéphane Marchesin
0 siblings, 1 reply; 10+ messages in thread
From: Zach Reizner @ 2014-10-07 19:49 UTC (permalink / raw)
To: dri-devel; +Cc: Zach Reizner
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(-)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-07 19:49 [PATCH] allow 32bpp framebuffers for cirrus drm Zach Reizner
@ 2014-10-25 1:31 ` Stéphane Marchesin
2014-10-27 15:00 ` Adam Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Stéphane Marchesin @ 2014-10-25 1:31 UTC (permalink / raw)
To: Zach Reizner, Adam Jackson, Dave Airlie; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 3597 bytes --]
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
>
[-- Attachment #1.2: Type: text/html, Size: 4854 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-25 1:31 ` Stéphane Marchesin
@ 2014-10-27 15:00 ` Adam Jackson
2014-10-27 15:30 ` Gerd Hoffmann
0 siblings, 1 reply; 10+ messages in thread
From: Adam Jackson @ 2014-10-27 15:00 UTC (permalink / raw)
To: Stéphane Marchesin; +Cc: Zach Reizner, dri-devel@lists.freedesktop.org
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-27 15:00 ` Adam Jackson
@ 2014-10-27 15:30 ` Gerd Hoffmann
2014-10-27 15:36 ` Zachary Reizner
2014-10-27 16:21 ` Adam Jackson
0 siblings, 2 replies; 10+ messages in thread
From: Gerd Hoffmann @ 2014-10-27 15:30 UTC (permalink / raw)
To: Adam Jackson; +Cc: Zach Reizner, dri-devel@lists.freedesktop.org
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/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-27 15:30 ` Gerd Hoffmann
@ 2014-10-27 15:36 ` Zachary Reizner
2014-10-27 16:21 ` Adam Jackson
1 sibling, 0 replies; 10+ messages in thread
From: Zachary Reizner @ 2014-10-27 15:36 UTC (permalink / raw)
To: Gerd Hoffmann, Adam Jackson; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 1603 bytes --]
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/
>
>
[-- Attachment #1.2: Type: text/html, Size: 2070 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-27 15:30 ` Gerd Hoffmann
2014-10-27 15:36 ` Zachary Reizner
@ 2014-10-27 16:21 ` Adam Jackson
2014-10-27 22:09 ` Zachary Reizner
1 sibling, 1 reply; 10+ messages in thread
From: Adam Jackson @ 2014-10-27 16:21 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Zach Reizner, dri-devel@lists.freedesktop.org
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-27 16:21 ` Adam Jackson
@ 2014-10-27 22:09 ` Zachary Reizner
2014-10-28 9:09 ` Gerd Hoffmann
0 siblings, 1 reply; 10+ messages in thread
From: Zachary Reizner @ 2014-10-27 22:09 UTC (permalink / raw)
To: Adam Jackson, Gerd Hoffmann; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 1174 bytes --]
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
>
>
[-- Attachment #1.2: Type: text/html, Size: 1500 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-27 22:09 ` Zachary Reizner
@ 2014-10-28 9:09 ` Gerd Hoffmann
2014-10-28 17:04 ` Zachary Reizner
0 siblings, 1 reply; 10+ messages in thread
From: Gerd Hoffmann @ 2014-10-28 9:09 UTC (permalink / raw)
To: Zachary Reizner; +Cc: dri-devel@lists.freedesktop.org
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-28 9:09 ` Gerd Hoffmann
@ 2014-10-28 17:04 ` Zachary Reizner
2014-10-29 11:00 ` Gerd Hoffmann
0 siblings, 1 reply; 10+ messages in thread
From: Zachary Reizner @ 2014-10-28 17:04 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 387 bytes --]
>
> 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.
[-- Attachment #1.2: Type: text/html, Size: 548 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] allow 32bpp framebuffers for cirrus drm
2014-10-28 17:04 ` Zachary Reizner
@ 2014-10-29 11:00 ` Gerd Hoffmann
0 siblings, 0 replies; 10+ messages in thread
From: Gerd Hoffmann @ 2014-10-29 11:00 UTC (permalink / raw)
To: Zachary Reizner; +Cc: dri-devel@lists.freedesktop.org
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-10-29 11:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-07 19:49 [PATCH] allow 32bpp framebuffers for cirrus drm Zach Reizner
2014-10-25 1:31 ` Stéphane Marchesin
2014-10-27 15:00 ` Adam Jackson
2014-10-27 15:30 ` Gerd Hoffmann
2014-10-27 15:36 ` Zachary Reizner
2014-10-27 16:21 ` Adam Jackson
2014-10-27 22:09 ` Zachary Reizner
2014-10-28 9:09 ` Gerd Hoffmann
2014-10-28 17:04 ` Zachary Reizner
2014-10-29 11:00 ` Gerd Hoffmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.