[Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Stefano Stabellini]

Hi all,
this is the forth update of the "DisplayState interface change" series.

This update fixes the few remaining issue raised last time.

The series is made of 7 patches:

1) accessors again
some other substitutions in hw/sm501.c;

2) remove bgr
the new DisplayState interface does not contain any host specific
display detail, it is an abstraction of the backend display, hence we
don't need to memorize the bgr flag in DisplayState.
The frontend must be able to handle a bgr display by itself, in fact sdl
is perfectly capable of that;

3) DisplayState interface change
this is the big patch that actually changes the interface;

4) vnc improvements
this patch introduces DisplaySurfaces in vnc, simplifying the code.

5) graphical_console_init change
this is the patch that changes the graphical_console_init function
to return an allocated DisplayState instead of a QEMUConsole, as Anthony
suggested.
This patch does *not* include any required changes to any devices, these
changes come with the two following patches.

6) machine changes
this patch changes the QEMUMachine init functions of all the machine types
not to take a DisplayState as an argument because is not needed any
more;

7) graphic device changes
this patch updates the graphic device code to use the new
graphical_console_init function.

Patch number 5 requires also 6 and 7 to compile.

More comments on single patches.

Cheers,

Stefano

Re: [Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Anthony Liguori]

Stefano Stabellini wrote:
> Hi all,
> this is the forth update of the "DisplayState interface change" series.
>   

I'm pretty happy with the state of this patch series.  I'm going to do 
thorough testing of each patch but if all goes well, I'd like to apply 
them.  I think there's room to improve things but I think it's good 
enough now to take into the tree.

Please take a look through these patches and speak up if you think 
someone needs to be changed before applying them.  I'll wait at least 
until the middle of next week before applying them to give everyone time 
to look closely.

Regards,

Anthony Liguori

> This update fixes the few remaining issue raised last time.
>
> The series is made of 7 patches:
>
> 1) accessors again
> some other substitutions in hw/sm501.c;
>
> 2) remove bgr
> the new DisplayState interface does not contain any host specific
> display detail, it is an abstraction of the backend display, hence we
> don't need to memorize the bgr flag in DisplayState.
> The frontend must be able to handle a bgr display by itself, in fact sdl
> is perfectly capable of that;
>
> 3) DisplayState interface change
> this is the big patch that actually changes the interface;
>
> 4) vnc improvements
> this patch introduces DisplaySurfaces in vnc, simplifying the code.
>
> 5) graphical_console_init change
> this is the patch that changes the graphical_console_init function
> to return an allocated DisplayState instead of a QEMUConsole, as Anthony
> suggested.
> This patch does *not* include any required changes to any devices, these
> changes come with the two following patches.
>
> 6) machine changes
> this patch changes the QEMUMachine init functions of all the machine types
> not to take a DisplayState as an argument because is not needed any
> more;
>
> 7) graphic device changes
> this patch updates the graphic device code to use the new
> graphical_console_init function.
>
> Patch number 5 requires also 6 and 7 to compile.
>
> More comments on single patches.
>
> Cheers,
>
> Stefano
>
>
>
>
>
>   

Re: [Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Andreas Färber]

Am 18.12.2008 um 20:12 schrieb Stefano Stabellini:

> this is the forth update of the "DisplayState interface change"  
> series.
>
> This update fixes the few remaining issue raised last time.
>
> The series is made of 7 patches:
>
> 1) accessors again
> some other substitutions in hw/sm501.c;
>
> 2) remove bgr
> the new DisplayState interface does not contain any host specific
> display detail, it is an abstraction of the backend display, hence we
> don't need to memorize the bgr flag in DisplayState.
> The frontend must be able to handle a bgr display by itself, in fact  
> sdl
> is perfectly capable of that;

Sorry if I've asked that before, but doesn't this (and 3) require  
changes to non-SDL backends as well, especially Cocoa?
The MacBook display appeared to be bgr so that TCX had to be adapted  
and bgr forcedly set to 1 in cocoa.m (r2974).

Thanks,
Andreas

> 3) DisplayState interface change
> this is the big patch that actually changes the interface;
>
> 4) vnc improvements
> this patch introduces DisplaySurfaces in vnc, simplifying the code.
>
> 5) graphical_console_init change
> this is the patch that changes the graphical_console_init function
> to return an allocated DisplayState instead of a QEMUConsole, as  
> Anthony
> suggested.
> This patch does *not* include any required changes to any devices,  
> these
> changes come with the two following patches.
>
> 6) machine changes
> this patch changes the QEMUMachine init functions of all the machine  
> types
> not to take a DisplayState as an argument because is not needed any
> more;
>
> 7) graphic device changes
> this patch updates the graphic device code to use the new
> graphical_console_init function.
>
> Patch number 5 requires also 6 and 7 to compile.
>
> More comments on single patches.
>
> Cheers,
>
> Stefano
>
>
>
>
>

Re: [Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4700 bytes --]

Andreas Färber wrote:

>> 2) remove bgr
>> the new DisplayState interface does not contain any host specific
>> display detail, it is an abstraction of the backend display, hence we
>> don't need to memorize the bgr flag in DisplayState.
>> The frontend must be able to handle a bgr display by itself, in fact sdl
>> is perfectly capable of that;
> 
> Sorry if I've asked that before, but doesn't this (and 3) require
> changes to non-SDL backends as well, especially Cocoa?
> The MacBook display appeared to be bgr so that TCX had to be adapted and
> bgr forcedly set to 1 in cocoa.m (r2974).
> 


I forgot about cocoa, and yes, some changes are required.
I am afraid I don't know much about it and I don't have any MacOS so I
may not be the right person do it.

In any case I wrote a simple patch that probably won't even compile but
will give you an idea about what has to be done.
I am both attaching the patch to this email and commenting it inline so
that I can better explain the reason behind each change.


>-static void cocoa_resize(DisplayState *ds, int w, int h)
>+static void cocoa_resize(DisplayState *ds)
> {
>     COCOA_DEBUG("qemu_cocoa: cocoa_resize\n");
> 
>-    [cocoaView resizeContentToWidth:w height:h displayState:ds];
>+    [cocoaView resizeContentToWidth:ds_get_width(ds) height:ds_get_height(ds) displayState:ds];
> }
> 
> static void cocoa_refresh(DisplayState *ds)
>
>


The new resize callback interface signature is changed: now the only
argument is DisplayState, because is the backend that is supposed to set the
the properties in the DisplayState structure.


>-    screenBuffer = malloc( w * 4 * h );
> 
>-    ds->data = screenBuffer;
>-    ds->linesize =  (w * 4);
>-    ds->depth = 32;
>-    ds->width = w;
>-    ds->height = h;
>-#ifdef __LITTLE_ENDIAN__
>-    ds->bgr = 1;
>-#else
>-    ds->bgr = 0;
>-#endif
>-
>-    dataProviderRef = CGDataProviderCreateWithData(NULL, screenBuffer, w * 4 * h, NULL);
>+    dataProviderRef = CGDataProviderCreateWithData(NULL, ds->data, ds_get_linesize(ds) * ds_get_height(ds), NULL);
> 


Again we don't have to set any property in ds anymore, the backend does
that for us.
data is already allocated and width, height, linesize already set.


> int gArgc;
> char **gArgv;
>@@ -316,18 +317,13 @@
>     // draw screen bitmap directly to Core Graphics context
>     if (dataProviderRef) {
>         CGImageRef imageRef = CGImageCreate(
>-            screen.width, //width
>-            screen.height, //height
>-            screen.bitsPerComponent, //bitsPerComponent
>-            screen.bitsPerPixel, //bitsPerPixel
>-            (screen.width * 4), //bytesPerRow
>-#if __LITTLE_ENDIAN__
>-            CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB), //colorspace for OS X >= 10.4
>-            kCGImageAlphaNoneSkipLast,
>-#else
>+            ds_get_width(ds), //width
>+            ds_get_height(ds), //height
>+            8, //bitsPerComponent
>+            ds_get_bits_per_pixel(ds), //bitsPerPixel
>+            ds_get_linesize(ds), //bytesPerRow
>             CGColorSpaceCreateDeviceRGB(), //colorspace for OS X < 10.4 (actually ppc)
>             kCGImageAlphaNoneSkipFirst, //bitmapInfo
>-#endif
>             dataProviderRef, //provider
>             NULL, //decode
>             0, //interpolate
>@@ -397,20 +393,8 @@
>         CGDataProviderRelease(dataProviderRef);
>     if (screenBuffer)
>         free(screenBuffer);


This is the critical part where we ask cocoa to do any needed conversions
between the source image format given by the backend and the real
display format.
In order to do this we call CGImageCreate with the image source
properties from DisplayState.
>From a little googling it seems to me that CGImageCreate and
CGContextDrawImage are able to handle the conversions.


>@@ -983,12 +967,13 @@
>     COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
> 
>     // register vga outpu callbacks
>-    ds->dpy_update = cocoa_update;
>-    ds->dpy_resize = cocoa_resize;
>-    ds->dpy_refresh = cocoa_refresh;
>-
>-    // give window a initial Size
>-    cocoa_resize(ds, 640, 400);
>+    dcl = qemu_mallocz(sizeof(DisplayChangeListener));
>+    if (!dcl)
>+        exit(1);
>+    dcl->dpy_update = cocoa_update;
>+    dcl->dpy_resize = cocoa_resize;
>+    dcl->dpy_refresh = cocoa_refresh;
>+    register_displaychangelistener(ds, dcl);
> 
>     // register cleanup function
>     atexit(cocoa_cleanup);
>


These changes are needed to support the new
register_displaychangelistener interface, they shouldn't need any
explanation.

It could also be useful to look at the changes I made to vnc.c and sdl.c
in the third patch.

Please ask if you need anything else.

Cheers,

Stefano Stabellini

[-- Attachment #2: cocoa.patch --]
[-- Type: text/x-diff, Size: 2945 bytes --]


diff -r 51cf78066aca cocoa.m
--- a/cocoa.m	Fri Dec 19 10:38:50 2008 +0000
+++ b/cocoa.m	Fri Dec 19 11:22:53 2008 +0000
@@ -58,6 +58,7 @@
 NSWindow *normalWindow;
 id cocoaView;
 static void *screenBuffer;
+static DisplayChangeListener *dcl;
 
 int gArgc;
 char **gArgv;
@@ -316,18 +317,13 @@
     // draw screen bitmap directly to Core Graphics context
     if (dataProviderRef) {
         CGImageRef imageRef = CGImageCreate(
-            screen.width, //width
-            screen.height, //height
-            screen.bitsPerComponent, //bitsPerComponent
-            screen.bitsPerPixel, //bitsPerPixel
-            (screen.width * 4), //bytesPerRow
-#if __LITTLE_ENDIAN__
-            CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB), //colorspace for OS X >= 10.4
-            kCGImageAlphaNoneSkipLast,
-#else
+            ds_get_width(ds), //width
+            ds_get_height(ds), //height
+            8, //bitsPerComponent
+            ds_get_bits_per_pixel(ds), //bitsPerPixel
+            ds_get_linesize(ds), //bytesPerRow
             CGColorSpaceCreateDeviceRGB(), //colorspace for OS X < 10.4 (actually ppc)
             kCGImageAlphaNoneSkipFirst, //bitmapInfo
-#endif
             dataProviderRef, //provider
             NULL, //decode
             0, //interpolate
@@ -397,20 +393,8 @@
         CGDataProviderRelease(dataProviderRef);
     if (screenBuffer)
         free(screenBuffer);
-    screenBuffer = malloc( w * 4 * h );
 
-    ds->data = screenBuffer;
-    ds->linesize =  (w * 4);
-    ds->depth = 32;
-    ds->width = w;
-    ds->height = h;
-#ifdef __LITTLE_ENDIAN__
-    ds->bgr = 1;
-#else
-    ds->bgr = 0;
-#endif
-
-    dataProviderRef = CGDataProviderCreateWithData(NULL, screenBuffer, w * 4 * h, NULL);
+    dataProviderRef = CGDataProviderCreateWithData(NULL, ds->data, ds_get_linesize(ds) * ds_get_height(ds), NULL);
 
     // update windows
     if (isFullscreen) {
@@ -939,11 +923,11 @@
     [cocoaView displayRect:rect];
 }
 
-static void cocoa_resize(DisplayState *ds, int w, int h)
+static void cocoa_resize(DisplayState *ds)
 {
     COCOA_DEBUG("qemu_cocoa: cocoa_resize\n");
 
-    [cocoaView resizeContentToWidth:w height:h displayState:ds];
+    [cocoaView resizeContentToWidth:ds_get_width(ds) height:ds_get_height(ds) displayState:ds];
 }
 
 static void cocoa_refresh(DisplayState *ds)
@@ -983,12 +967,13 @@
     COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
 
     // register vga outpu callbacks
-    ds->dpy_update = cocoa_update;
-    ds->dpy_resize = cocoa_resize;
-    ds->dpy_refresh = cocoa_refresh;
-
-    // give window a initial Size
-    cocoa_resize(ds, 640, 400);
+    dcl = qemu_mallocz(sizeof(DisplayChangeListener));
+    if (!dcl)
+        exit(1);
+    dcl->dpy_update = cocoa_update;
+    dcl->dpy_resize = cocoa_resize;
+    dcl->dpy_refresh = cocoa_refresh;
+    register_displaychangelistener(ds, dcl);
 
     // register cleanup function
     atexit(cocoa_cleanup);

Re: [Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Stefano Stabellini]

Stefano Stabellini wrote:

> I forgot about cocoa, and yes, some changes are required.
> I am afraid I don't know much about it and I don't have any MacOS so I
> may not be the right person do it.
> 
> In any case I wrote a simple patch that probably won't even compile but
> will give you an idea about what has to be done.
> I am both attaching the patch to this email and commenting it inline so
> that I can better explain the reason behind each change.

Can someone please take a look at the cocoa patch I sent and fix it?
It shouldn't take long at all.

Or you could just apply the series and wait for someone to fix the cocoa
interface later :)

I think this patch series has already been delayed enough...

Re: [Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Anthony Liguori]

Stefano Stabellini wrote:
> Stefano Stabellini wrote:
>
>   
>> I forgot about cocoa, and yes, some changes are required.
>> I am afraid I don't know much about it and I don't have any MacOS so I
>> may not be the right person do it.
>>
>> In any case I wrote a simple patch that probably won't even compile but
>> will give you an idea about what has to be done.
>> I am both attaching the patch to this email and commenting it inline so
>> that I can better explain the reason behind each change.
>>     
>
> Can someone please take a look at the cocoa patch I sent and fix it?
> It shouldn't take long at all.
>
> Or you could just apply the series and wait for someone to fix the cocoa
> interface later :)
>   

If noone volunteers to do the cocoa patch, I'll do this.

> I think this patch series has already been delayed enough...
>   

Yes, I'm pretty behind right now but I'm trying to catch up.  Your 
series is high on my TODO.

Regards,

Anthony Liguori

>
>   

Re: [Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Andreas Färber]

Hi,

Am 15.01.2009 um 18:03 schrieb Stefano Stabellini:

> Stefano Stabellini wrote:
>
>> I forgot about cocoa, and yes, some changes are required.
>> I am afraid I don't know much about it and I don't have any MacOS  
>> so I
>> may not be the right person do it.
>>
>> In any case I wrote a simple patch that probably won't even compile  
>> but
>> will give you an idea about what has to be done.
>> I am both attaching the patch to this email and commenting it  
>> inline so
>> that I can better explain the reason behind each change.
>
> Can someone please take a look at the cocoa patch I sent and fix it?
> It shouldn't take long at all.
>
> Or you could just apply the series and wait for someone to fix the  
> cocoa
> interface later :)
>
> I think this patch series has already been delayed enough...

Sorry, I am currently in the process of moving. Do you have a Git repo  
I could pull from, to speed things up?

Andreas

Re: [Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Stefano Stabellini]

Anthony Liguori wrote:

> Stefano Stabellini wrote:
>> Stefano Stabellini wrote:
>>
>>  
>>> I forgot about cocoa, and yes, some changes are required.
>>> I am afraid I don't know much about it and I don't have any MacOS so I
>>> may not be the right person do it.
>>>
>>> In any case I wrote a simple patch that probably won't even compile but
>>> will give you an idea about what has to be done.
>>> I am both attaching the patch to this email and commenting it inline so
>>> that I can better explain the reason behind each change.
>>>     
>>
>> Can someone please take a look at the cocoa patch I sent and fix it?
>> It shouldn't take long at all.
>>
>> Or you could just apply the series and wait for someone to fix the cocoa
>> interface later :)
>>   
> 
> If noone volunteers to do the cocoa patch, I'll do this.
> 
>> I think this patch series has already been delayed enough...
>>   
> 
> Yes, I'm pretty behind right now but I'm trying to catch up.  Your
> series is high on my TODO.
> 

Thanks, appreciated.

Re: [Qemu-devel] [PATCH 0 of 7] DisplayState interface change

[Stefano Stabellini]

Andreas Färber wrote:

> Hi,
> 
> Am 15.01.2009 um 18:03 schrieb Stefano Stabellini:
> 
>> Stefano Stabellini wrote:
>>
>>> I forgot about cocoa, and yes, some changes are required.
>>> I am afraid I don't know much about it and I don't have any MacOS so I
>>> may not be the right person do it.
>>>
>>> In any case I wrote a simple patch that probably won't even compile but
>>> will give you an idea about what has to be done.
>>> I am both attaching the patch to this email and commenting it inline so
>>> that I can better explain the reason behind each change.
>>
>> Can someone please take a look at the cocoa patch I sent and fix it?
>> It shouldn't take long at all.
>>
>> Or you could just apply the series and wait for someone to fix the cocoa
>> interface later :)
>>
>> I think this patch series has already been delayed enough...
> 
> Sorry, I am currently in the process of moving. Do you have a Git repo I
> could pull from, to speed things up?
> 

I am afraid not: I am using a mercurial patchqueue and currently is not
even public.
However if you have issues applying my patches I can send another update
of the series, rediffed against the current qemu.