[display] Refactor to avoid implicit framebuffer allocation.

[display] Refactor to avoid implicit framebuffer allocation.

Preparation to allow other images to be presented to the display.
Separate definitions of display and image formats.
display_get_info no longer provides a framebuffer.
display_get_framebuffer allocates a framebuffer for convenience.

Committed: c1255f7f471cab1024440e107faa63c5cf49262b

[cdotstout, 2016-03-09 00:41:47]

cstout@chromium.org changed reviewers:
+ gkalsi@google.com, travisg@gmail.com

[travisg1, 2016-03-09 02:08:21]

Looks pretty good. 

FYI over in the other project I just tweaked some code in this area to add flags
to both gfx surfaces and the display stuff to state if it's a hardware
framebuffer and/or if it needs cache flushing on gfx_flush. Will try to sync it
up once we get both of these checked in.

[cdotstout, 2016-03-09 17:04:58]

On 2016/03/09 02:08:21, travisg1 wrote:
> Looks pretty good. 
> 
> FYI over in the other project I just tweaked some code in this area to add
flags
> to both gfx surfaces and the display stuff to state if it's a hardware
> framebuffer and/or if it needs cache flushing on gfx_flush. Will try to sync
it
> up once we get both of these checked in.

Ok yeah we should try to keep them in sync.

And... I guess I should not break all the other targets that implement the
display apis right? Is there an easy way to compile them all?

[travisg1, 2016-03-09 17:21:47]

./scripts/buildall will attempt to build everything. Also there's a travis-ci
build on github that will try to build most of the projects.

[cdotstout, 2016-03-09 19:17:17]

scripts/buildall all pass except untested:
pc-x86-test qemu-microblaze-test qemu-mips-test zybo-microblaze-test

[gkalsi, 2016-03-09 21:13:38]

This patch is fine by me but I'd wait for travisg@ for the final word.

Added a few nits re: null checks -- some of them are my fault, sorry!

Also this patch seems to build against the QEMU targets on my machine -- are
there some differences between our environments?

https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
File dev/virtio/gpu/virtio-gpu.c (left):

https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
dev/virtio/gpu/virtio-gpu.c:560: status_t display_get_info(struct display_info
*info)
nit: DEBUG_ASSERT(info);

https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
File dev/virtio/gpu/virtio-gpu.c (right):

https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
dev/virtio/gpu/virtio-gpu.c:560: {
nit: DEBUG_ASSERT(fb);

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h
File include/dev/display.h (right):

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
include/dev/display.h:65: status_t display_get_info(struct display_info *info);
Might be worth also suffixing this with __NONNULL((1))
like we've done in include/kernel/vm.h for example

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
include/dev/display.h:75: status_t display_present(struct display_image *image,
uint starty, uint endy);
ditto from above

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
include/dev/display.h:84: status_t display_get_framebuffer(struct
display_framebuffer *fb);
ditto from above

https://codereview.chromium.org/1777783003/diff/40001/include/lib/gfx.h
File include/lib/gfx.h (right):

https://codereview.chromium.org/1777783003/diff/40001/include/lib/gfx.h#newco...
include/lib/gfx.h:108: gfx_surface *gfx_create_surface_from_display(struct
display_framebuffer *);
__NONNULL((1));

https://codereview.chromium.org/1777783003/diff/40001/lib/gfx/gfx.c
File lib/gfx/gfx.c (right):

https://codereview.chromium.org/1777783003/diff/40001/lib/gfx/gfx.c#newcode660
lib/gfx/gfx.c:660: gfx_surface *gfx_create_surface_from_display(struct
display_framebuffer *fb)
DEBUG_ASSERT(fb);

https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display.c
File platform/armemu/display.c (left):

https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display...
platform/armemu/display.c:57: {
DEBUG_ASSERT(info)

https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display.c
File platform/armemu/display.c (right):

https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display...
platform/armemu/display.c:57: {
DEBUG_ASSERT(fb)

https://codereview.chromium.org/1777783003/diff/40001/target/dartuinoP0/memor...
File target/dartuinoP0/memory_lcd.c (right):

https://codereview.chromium.org/1777783003/diff/40001/target/dartuinoP0/memor...
target/dartuinoP0/memory_lcd.c:125: {
DEBUG_ASSERT(fb)

https://codereview.chromium.org/1777783003/diff/40001/target/qemu-m4/m4display.c
File target/qemu-m4/m4display.c (right):

https://codereview.chromium.org/1777783003/diff/40001/target/qemu-m4/m4displa...
target/qemu-m4/m4display.c:204: status_t display_get_framebuffer(struct
display_framebuffer *fb)
DEBUG_ASSERT(fb);

[cdotstout, 2016-03-09 21:56:13]

On 2016/03/09 21:13:38, gkalsi wrote:
> This patch is fine by me but I'd wait for travisg@ for the final word.
> 
> Added a few nits re: null checks -- some of them are my fault, sorry!
> 
> Also this patch seems to build against the QEMU targets on my machine -- are
> there some differences between our environments?
> 
>
https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
> File dev/virtio/gpu/virtio-gpu.c (left):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
> dev/virtio/gpu/virtio-gpu.c:560: status_t display_get_info(struct display_info
> *info)
> nit: DEBUG_ASSERT(info);
> 
>
https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
> File dev/virtio/gpu/virtio-gpu.c (right):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
> dev/virtio/gpu/virtio-gpu.c:560: {
> nit: DEBUG_ASSERT(fb);
> 
> https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h
> File include/dev/display.h (right):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
> include/dev/display.h:65: status_t display_get_info(struct display_info
*info);
> Might be worth also suffixing this with __NONNULL((1))
> like we've done in include/kernel/vm.h for example
> 
>
https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
> include/dev/display.h:75: status_t display_present(struct display_image
*image,
> uint starty, uint endy);
> ditto from above
> 
>
https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
> include/dev/display.h:84: status_t display_get_framebuffer(struct
> display_framebuffer *fb);
> ditto from above
> 
> https://codereview.chromium.org/1777783003/diff/40001/include/lib/gfx.h
> File include/lib/gfx.h (right):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/include/lib/gfx.h#newco...
> include/lib/gfx.h:108: gfx_surface *gfx_create_surface_from_display(struct
> display_framebuffer *);
> __NONNULL((1));
> 
> https://codereview.chromium.org/1777783003/diff/40001/lib/gfx/gfx.c
> File lib/gfx/gfx.c (right):
> 
> https://codereview.chromium.org/1777783003/diff/40001/lib/gfx/gfx.c#newcode660
> lib/gfx/gfx.c:660: gfx_surface *gfx_create_surface_from_display(struct
> display_framebuffer *fb)
> DEBUG_ASSERT(fb);
> 
>
https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display.c
> File platform/armemu/display.c (left):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display...
> platform/armemu/display.c:57: {
> DEBUG_ASSERT(info)
> 
>
https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display.c
> File platform/armemu/display.c (right):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display...
> platform/armemu/display.c:57: {
> DEBUG_ASSERT(fb)
> 
>
https://codereview.chromium.org/1777783003/diff/40001/target/dartuinoP0/memor...
> File target/dartuinoP0/memory_lcd.c (right):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/target/dartuinoP0/memor...
> target/dartuinoP0/memory_lcd.c:125: {
> DEBUG_ASSERT(fb)
> 
>
https://codereview.chromium.org/1777783003/diff/40001/target/qemu-m4/m4display.c
> File target/qemu-m4/m4display.c (right):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/target/qemu-m4/m4displa...
> target/qemu-m4/m4display.c:204: status_t display_get_framebuffer(struct
> display_framebuffer *fb)
> DEBUG_ASSERT(fb);

For the targets I listed they're basically untested because I don't have the
required toolchains.

[travisg1, 2016-03-09 22:40:11]

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h
File include/dev/display.h (right):

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
include/dev/display.h:72: int rowbytes;
is rowbytes in bytes or pixels? It gets passed to the create surface stuff which
is turned into stride, which is IIRC in pixels, not bytes.

[cdotstout, 2016-03-09 22:48:00]

https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
File dev/virtio/gpu/virtio-gpu.c (left):

https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
dev/virtio/gpu/virtio-gpu.c:560: status_t display_get_info(struct display_info
*info)
On 2016/03/09 21:13:37, gkalsi wrote:
> nit: DEBUG_ASSERT(info);

Done.

https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
File dev/virtio/gpu/virtio-gpu.c (right):

https://codereview.chromium.org/1777783003/diff/40001/dev/virtio/gpu/virtio-g...
dev/virtio/gpu/virtio-gpu.c:560: {
On 2016/03/09 21:13:37, gkalsi wrote:
> nit: DEBUG_ASSERT(fb);

Done.

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h
File include/dev/display.h (right):

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
include/dev/display.h:65: status_t display_get_info(struct display_info *info);
On 2016/03/09 21:13:37, gkalsi wrote:
> Might be worth also suffixing this with __NONNULL((1))
> like we've done in include/kernel/vm.h for example

Done.

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
include/dev/display.h:75: status_t display_present(struct display_image *image,
uint starty, uint endy);
On 2016/03/09 21:13:37, gkalsi wrote:
> ditto from above

Done.

https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
include/dev/display.h:84: status_t display_get_framebuffer(struct
display_framebuffer *fb);
On 2016/03/09 21:13:37, gkalsi wrote:
> ditto from above

Done.

https://codereview.chromium.org/1777783003/diff/40001/include/lib/gfx.h
File include/lib/gfx.h (right):

https://codereview.chromium.org/1777783003/diff/40001/include/lib/gfx.h#newco...
include/lib/gfx.h:108: gfx_surface *gfx_create_surface_from_display(struct
display_framebuffer *);
On 2016/03/09 21:13:37, gkalsi wrote:
> __NONNULL((1));

Done.

https://codereview.chromium.org/1777783003/diff/40001/lib/gfx/gfx.c
File lib/gfx/gfx.c (right):

https://codereview.chromium.org/1777783003/diff/40001/lib/gfx/gfx.c#newcode660
lib/gfx/gfx.c:660: gfx_surface *gfx_create_surface_from_display(struct
display_framebuffer *fb)
On 2016/03/09 21:13:37, gkalsi wrote:
> DEBUG_ASSERT(fb);

Done.

https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display.c
File platform/armemu/display.c (left):

https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display...
platform/armemu/display.c:57: {
On 2016/03/09 21:13:37, gkalsi wrote:
> DEBUG_ASSERT(info)

Done.

https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display.c
File platform/armemu/display.c (right):

https://codereview.chromium.org/1777783003/diff/40001/platform/armemu/display...
platform/armemu/display.c:57: {
On 2016/03/09 21:13:37, gkalsi wrote:
> DEBUG_ASSERT(fb)

Done.

https://codereview.chromium.org/1777783003/diff/40001/target/dartuinoP0/memor...
File target/dartuinoP0/memory_lcd.c (right):

https://codereview.chromium.org/1777783003/diff/40001/target/dartuinoP0/memor...
target/dartuinoP0/memory_lcd.c:125: {
On 2016/03/09 21:13:37, gkalsi wrote:
> DEBUG_ASSERT(fb)

Done.

https://codereview.chromium.org/1777783003/diff/40001/target/qemu-m4/m4display.c
File target/qemu-m4/m4display.c (right):

https://codereview.chromium.org/1777783003/diff/40001/target/qemu-m4/m4displa...
target/qemu-m4/m4display.c:204: status_t display_get_framebuffer(struct
display_framebuffer *fb)
On 2016/03/09 21:13:37, gkalsi wrote:
> DEBUG_ASSERT(fb);

Done.

[cdotstout, 2016-03-09 22:57:29]

On 2016/03/09 22:40:11, travisg1 wrote:
> https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h
> File include/dev/display.h (right):
> 
>
https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
> include/dev/display.h:72: int rowbytes;
> is rowbytes in bytes or pixels? It gets passed to the create surface stuff
which
> is turned into stride, which is IIRC in pixels, not bytes.

Ha ha, I changed stride to rowbytes to remove the ambiguity but didn't realize
stride was in pixels :)

I prefer to have stride in units of bytes instead of pixels, b/c pixels almost
always has to be converted to bytes.

If you prefer pixel units though, np we can go with that, let me know.

[travisg1, 2016-03-09 23:43:11]

On 2016/03/09 22:57:29, cdotstout wrote:
> On 2016/03/09 22:40:11, travisg1 wrote:
> > https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h
> > File include/dev/display.h (right):
> > 
> >
>
https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
> > include/dev/display.h:72: int rowbytes;
> > is rowbytes in bytes or pixels? It gets passed to the create surface stuff
> which
> > is turned into stride, which is IIRC in pixels, not bytes.
> 
> Ha ha, I changed stride to rowbytes to remove the ambiguity but didn't realize
> stride was in pixels :)
> 
> I prefer to have stride in units of bytes instead of pixels, b/c pixels almost
> always has to be converted to bytes.
> 
> If you prefer pixel units though, np we can go with that, let me know.

I think stride in bytes is fine, but then you have to fix all of the lib/gfx
code that deals with it. If you do, change the stride name there too to reflect
bytes.

[cdotstout, 2016-03-10 00:19:47]

On 2016/03/09 23:43:11, travisg1 wrote:
> On 2016/03/09 22:57:29, cdotstout wrote:
> > On 2016/03/09 22:40:11, travisg1 wrote:
> > >
https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h
> > > File include/dev/display.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
> > > include/dev/display.h:72: int rowbytes;
> > > is rowbytes in bytes or pixels? It gets passed to the create surface stuff
> > which
> > > is turned into stride, which is IIRC in pixels, not bytes.
> > 
> > Ha ha, I changed stride to rowbytes to remove the ambiguity but didn't
realize
> > stride was in pixels :)
> > 
> > I prefer to have stride in units of bytes instead of pixels, b/c pixels
almost
> > always has to be converted to bytes.
> > 
> > If you prefer pixel units though, np we can go with that, let me know.
> 
> I think stride in bytes is fine, but then you have to fix all of the lib/gfx
> code that deals with it. If you do, change the stride name there too to
reflect
> bytes.

why not both

[travisg1, 2016-03-10 00:22:00]

On 2016/03/10 00:19:47, cdotstout wrote:
> On 2016/03/09 23:43:11, travisg1 wrote:
> > On 2016/03/09 22:57:29, cdotstout wrote:
> > > On 2016/03/09 22:40:11, travisg1 wrote:
> > > >
> https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h
> > > > File include/dev/display.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1777783003/diff/40001/include/dev/display.h#n...
> > > > include/dev/display.h:72: int rowbytes;
> > > > is rowbytes in bytes or pixels? It gets passed to the create surface
stuff
> > > which
> > > > is turned into stride, which is IIRC in pixels, not bytes.
> > > 
> > > Ha ha, I changed stride to rowbytes to remove the ambiguity but didn't
> realize
> > > stride was in pixels :)
> > > 
> > > I prefer to have stride in units of bytes instead of pixels, b/c pixels
> almost
> > > always has to be converted to bytes.
> > > 
> > > If you prefer pixel units though, np we can go with that, let me know.
> > 
> > I think stride in bytes is fine, but then you have to fix all of the lib/gfx
> > code that deals with it. If you do, change the stride name there too to
> reflect
> > bytes.
> 
> why not both

why indeed... Genius! make it so!

[cdotstout, 2016-03-10 17:48:42]

attempting self lgtm

[cdotstout, 2016-03-10 17:49:30]

Description was changed from

==========
[display] Refactor to avoid implicit framebuffer allocation.

Preparation to allow other images to be presented to the display.
Separate definitions of display and image formats.
display_get_info no longer provides a framebuffer.
display_get_framebuffer allocates a framebuffer for convenience.
==========

to

==========
[display] Refactor to avoid implicit framebuffer allocation.

Preparation to allow other images to be presented to the display.
Separate definitions of display and image formats.
display_get_info no longer provides a framebuffer.
display_get_framebuffer allocates a framebuffer for convenience.

Committed: c1255f7f471cab1024440e107faa63c5cf49262b
==========

[cdotstout, 2016-03-10 17:49:32]

Committed patchset #5 (id:80001) manually as
c1255f7f471cab1024440e107faa63c5cf49262b (presubmit successful).

[travisg1, 2016-03-10 18:47:35]

lgtm