Re: [Spice-devel] issues linking with openssl 1.1.0

[i iordanov]

Hi Marc-André,

Thanks for your reply. I think something was not quite right with my build
environment. I am unable to reproduce this anymore!

Apologies for the false alarm, and thanks for confirming compatibility with
future openssl versions.

Sincerely,
iordan

On Thu, Jan 17, 2019 at 12:46 AM Marc-André Lureau <
marcandre.lur...@gmail.com> wrote:

> Hey Iordan
>
> On Thu, Jan 17, 2019 at 3:45 AM i iordanov  wrote:
> >
> > Hello,
> >
> > Happy new year!
> >
> > I tried to build spice-gtk 0.34 and 0.35 with OpenSSL 1.1.0, and I got
> some linking errors:
> >
> >
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
> spice_channel_class_intern_init: error: undefined reference to
> 'SSL_library_init'
> >
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
> spice_channel_class_intern_init: error: undefined reference to
> 'SSL_load_error_strings'
> >
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
> spice_channel_coroutine: error: undefined reference to 'SSLv23_method'
> >
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
> spice_channel_coroutine: error: undefined reference to 'sk_value'
> >
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
> spice_channel_coroutine: error: undefined reference to 'sk_num'
> >
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
> spice_channel_coroutine: error: undefined reference to 'sk_pop_free'
> >
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(ssl_verify.o):ssl_verify.c:function
> openssl_verify: error: undefined reference to 'sk_num'
> >
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(ssl_verify.o):ssl_verify.c:function
> openssl_verify: error: undefined reference to 'sk_value'
> > clang++: error: linker command failed with exit code 1 (use -v to see
> invocation)
> > make: *** [obj/local/x86/libspice.so] Error 1
> >
> > I had to revert to 1.0.2, but wanted to ask - is there a plan to support
> 1.1.0/1.1.1?
> >
>
> Happy new year! :)
>
> It compiles fine against openssl-libs-1.1.1-3.fc29.x86_64
>
> I checked 'SSL_library_init', it is is not exported by the library,
> but the header redefines it:
>
> openssl/ssl.h
> 1098:#  define OpenSSL_add_ssl_algorithms()   SSL_library_init()
> 1099:#  define SSLeay_add_ssl_algorithms()SSL_library_init()
> 1938:#  define SSL_library_init() OPENSSL_init_ssl(0, NULL)
>
> Is this under your own build environment or it can be reproduced on a
> known distro?
>
> thanks
>
>
>
> --
> Marc-André Lureau
>


-- 
The conscious mind has only one thread of execution.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 23/23] drm/qxl: add overflow checks to qxl_mode_dumb_create()

[Daniel Vetter]

On Fri, Jan 18, 2019 at 5:32 PM Ville Syrjälä
 wrote:
>
> On Fri, Jan 18, 2019 at 04:49:44PM +0100, Daniel Vetter wrote:
> > On Fri, Jan 18, 2019 at 01:20:20PM +0100, Gerd Hoffmann wrote:
> > > Signed-off-by: Gerd Hoffmann 
> >
> > We already do all reasonable overflow checks in drm_mode_create_dumb(). If
> > you don't trust them I think would be better time spent typing an igt to
> > test this than adding redundant check in all drivers.
> >
> > You're also missing one check for bpp underflows :-)
>
> BTW I just noticed that we don't seem to validating
> create_dumb->flags at all. Someone should probably add some
> checks for that, or mark it as deprecated in case we already
> lost the battle with userspace stack garbage.

Given that every kms client/compositor under the sun uses this (or
well, all the generic ones at least) I think we can safely assume to
have lost that battle :-/
-Daniel

>
> > -Daniel
> >
> > > ---
> > >  drivers/gpu/drm/qxl/qxl_dumb.c | 10 ++
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c 
> > > b/drivers/gpu/drm/qxl/qxl_dumb.c
> > > index 272d19b677..bed6d06ee4 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_dumb.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_dumb.c
> > > @@ -37,11 +37,13 @@ int qxl_mode_dumb_create(struct drm_file *file_priv,
> > > uint32_t handle;
> > > int r;
> > > struct qxl_surface surf;
> > > -   uint32_t pitch, format;
> > > +   uint32_t pitch, size, format;
> > >
> > > -   pitch = args->width * ((args->bpp + 1) / 8);
> > > -   args->size = pitch * args->height;
> > > -   args->size = ALIGN(args->size, PAGE_SIZE);
> > > +   if (check_mul_overflow(args->width, ((args->bpp + 1) / 8), ))
> > > +   return -EINVAL;
> > > +   if (check_mul_overflow(pitch, args->height, ))
> > > +   return -EINVAL;
> > > +   args->size = ALIGN(size, PAGE_SIZE);
> > >
> > > switch (args->bpp) {
> > > case 16:
> > > --
> > > 2.9.3
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] gstaudio: check before set mute/volume properties

[Christophe Fergeau]

On Fri, Jan 18, 2019 at 04:40:08PM +0100, Victor Toso wrote:
> Hi,
> 
> On Fri, Jan 18, 2019 at 04:35:09PM +0100, Christophe Fergeau wrote:
> > "Check before setting ..."
> 
> Indeed, fixed.
> 
> > On Fri, Jan 18, 2019 at 03:49:21PM +0100, Victor Toso wrote:
> > > From: Victor Toso 
> > > 
> > > Otherwise we can get warnings such as:
> > > 
> > > GLib-GObject-WARNING **: 16:10:00.857:
> > > g_object_set_is_valid_property: object class 'GstAutoAudioSink' has 
> > > no property named 'volume'
> > > 
> > > Signed-off-by: Victor Toso 
> > > ---
> > >  src/spice-gstaudio.c | 32 +++-
> > >  1 file changed, 23 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > index 98c1f4f..51ff028 100644
> > > --- a/src/spice-gstaudio.c
> > > +++ b/src/spice-gstaudio.c
> > > @@ -368,10 +368,13 @@ static void playback_volume_changed(GObject 
> > > *object, GParamSpec *pspec, gpointer
> > >  if (!e)
> > >  e = g_object_ref(p->playback.sink);
> > >  
> > > -if (GST_IS_STREAM_VOLUME(e))
> > > +if (GST_IS_STREAM_VOLUME(e)) {
> > >  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
> > > GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > > -else
> > > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> > > "volume") != NULL) {
> > >  g_object_set(e, "volume", vol, NULL);
> > 
> > Dunno how expensive g_object_class_find_property is, and if we should
> > cache this? Looks good otherwise.
> 
> No clue about how expensive it is, shouldn't be bad but
> considering that this is only triggered after volume/mute
> messages, which are not too happen often in normal use case, it
> should be fine.

Ok,

Acked-by: Christophe Fergeau 

Christophe

> 
> > Christophe
> > 
> > > +} else {
> > > +g_warning("playback: ignoring volume change on %s", 
> > > gst_element_get_name(e));
> > > +}
> > >  
> > >  g_object_unref(e);
> > >  }
> > > @@ -394,8 +397,13 @@ static void playback_mute_changed(GObject *object, 
> > > GParamSpec *pspec, gpointer d
> > >  if (!e)
> > >  e = g_object_ref(p->playback.sink);
> > >  
> > > -if (GST_IS_STREAM_VOLUME(e))
> > > +if (GST_IS_STREAM_VOLUME(e)) {
> > >  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> > > "mute") != NULL) {
> > > +g_object_set(e, "mute", mute, NULL);
> > > +} else {
> > > +g_warning("playback: ignoring mute change on %s", 
> > > gst_element_get_name(e));
> > > +}
> > >  
> > >  g_object_unref(e);
> > >  }
> > > @@ -427,10 +435,13 @@ static void record_volume_changed(GObject *object, 
> > > GParamSpec *pspec, gpointer d
> > >  if (!e)
> > >  e = g_object_ref(p->record.src);
> > >  
> > > -if (GST_IS_STREAM_VOLUME(e))
> > > +if (GST_IS_STREAM_VOLUME(e)) {
> > >  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
> > > GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > > -else
> > > -g_warning("gst lacks volume capabilities on src");
> > > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> > > "volume") != NULL) {
> > > +g_object_set(e, "volume", vol, NULL);
> > > +} else {
> > > +g_warning("record: ignoring volume change on %s", 
> > > gst_element_get_name(e));
> > > +}
> > >  
> > >  g_object_unref(e);
> > >  }
> > > @@ -453,10 +464,13 @@ static void record_mute_changed(GObject *object, 
> > > GParamSpec *pspec, gpointer dat
> > >  if (!e)
> > >  e = g_object_ref(p->record.src);
> > >  
> > > -if (GST_IS_STREAM_VOLUME (e))
> > > +if (GST_IS_STREAM_VOLUME (e)) {
> > >  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > > -else
> > > -g_warning("gst lacks mute capabilities on src: %d", mute);
> > > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> > > "mute") != NULL) {
> > > +g_object_set(e, "mute", mute, NULL);
> > > +} else {
> > > +g_warning("record: ignoring mute change on %s", 
> > > gst_element_get_name(e));
> > > +}
> > >  
> > >  g_object_unref(e);
> > >  }
> > > -- 
> > > 2.20.1
> > > 
> > > ___
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 




signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v2] gstaudio: check before setting mute/volume properties

[Victor Toso]

Hi,

On Fri, Jan 18, 2019 at 11:53:28AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso 
> > 
> > Otherwise we can get warnings such as:
> > 
> > GLib-GObject-WARNING **: 16:10:00.857:
> > g_object_set_is_valid_property: object class 'GstAutoAudioSink' has no
> > property named 'volume'
> > 
> > Signed-off-by: Victor Toso 
> 
> The patch remove the warning... but who control the volume?

We do! This is a message that host sent to us.

> Is it just constant at the maximum?

Did not understand the question!

> Maybe we should detect and add a "volume" element to the
> pipeline?

That's doable but different. Better is to request how is actually
in control to set the right volume/mute for that application. If
we use volume element, we are doing it inside the pipeline.

> Does GstAutoAudioSink forward the volume settings to the chosen
> element (if the property is available) ?

AFAIK, yes, the sink/src elements. This was happening with
wasapi{sink, src} elements, same should happen with alsa based on
Snir's patch.

Btw, volume/mute for wasapi is ongoing

https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/715

So, we should check if volume/mute property exists as they are
not really mandatory.

Cheers,

> > ---
> >  src/spice-gstaudio.c | 32 +++-
> >  1 file changed, 23 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > index 98c1f4f..51ff028 100644
> > --- a/src/spice-gstaudio.c
> > +++ b/src/spice-gstaudio.c
> > @@ -368,10 +368,13 @@ static void playback_volume_changed(GObject *object,
> > GParamSpec *pspec, gpointer
> >  if (!e)
> >  e = g_object_ref(p->playback.sink);
> >  
> > -if (GST_IS_STREAM_VOLUME(e))
> > +if (GST_IS_STREAM_VOLUME(e)) {
> >  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
> >  GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > -else
> > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > "volume") != NULL) {
> >  g_object_set(e, "volume", vol, NULL);
> > +} else {
> > +g_warning("playback: ignoring volume change on %s",
> > gst_element_get_name(e));
> > +}
> >  
> >  g_object_unref(e);
> >  }
> > @@ -394,8 +397,13 @@ static void playback_mute_changed(GObject *object,
> > GParamSpec *pspec, gpointer d
> >  if (!e)
> >  e = g_object_ref(p->playback.sink);
> >  
> > -if (GST_IS_STREAM_VOLUME(e))
> > +if (GST_IS_STREAM_VOLUME(e)) {
> >  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute")
> > != NULL) {
> > +g_object_set(e, "mute", mute, NULL);
> > +} else {
> > +g_warning("playback: ignoring mute change on %s",
> > gst_element_get_name(e));
> > +}
> >  
> >  g_object_unref(e);
> >  }
> > @@ -427,10 +435,13 @@ static void record_volume_changed(GObject *object,
> > GParamSpec *pspec, gpointer d
> >  if (!e)
> >  e = g_object_ref(p->record.src);
> >  
> > -if (GST_IS_STREAM_VOLUME(e))
> > +if (GST_IS_STREAM_VOLUME(e)) {
> >  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
> >  GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > -else
> > -g_warning("gst lacks volume capabilities on src");
> > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > "volume") != NULL) {
> > +g_object_set(e, "volume", vol, NULL);
> > +} else {
> > +g_warning("record: ignoring volume change on %s",
> > gst_element_get_name(e));
> > +}
> >  
> >  g_object_unref(e);
> >  }
> > @@ -453,10 +464,13 @@ static void record_mute_changed(GObject *object,
> > GParamSpec *pspec, gpointer dat
> >  if (!e)
> >  e = g_object_ref(p->record.src);
> >  
> > -if (GST_IS_STREAM_VOLUME (e))
> > +if (GST_IS_STREAM_VOLUME (e)) {
> >  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > -else
> > -g_warning("gst lacks mute capabilities on src: %d", mute);
> > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute")
> > != NULL) {
> > +g_object_set(e, "mute", mute, NULL);
> > +} else {
> > +g_warning("record: ignoring mute change on %s",
> > gst_element_get_name(e));
> > +}
> >  
> >  g_object_unref(e);
> >  }


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v2] gstaudio: check before setting mute/volume properties

[Frediano Ziglio]

> 
> From: Victor Toso 
> 
> Otherwise we can get warnings such as:
> 
> GLib-GObject-WARNING **: 16:10:00.857:
> g_object_set_is_valid_property: object class 'GstAutoAudioSink' has no
> property named 'volume'
> 
> Signed-off-by: Victor Toso 

The patch remove the warning... but who control the volume?
Is it just constant at the maximum?
Maybe we should detect and add a "volume" element to the
pipeline?

Does GstAutoAudioSink forward the volume settings to the chosen
element (if the property is available) ?

> ---
>  src/spice-gstaudio.c | 32 +++-
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> index 98c1f4f..51ff028 100644
> --- a/src/spice-gstaudio.c
> +++ b/src/spice-gstaudio.c
> @@ -368,10 +368,13 @@ static void playback_volume_changed(GObject *object,
> GParamSpec *pspec, gpointer
>  if (!e)
>  e = g_object_ref(p->playback.sink);
>  
> -if (GST_IS_STREAM_VOLUME(e))
> +if (GST_IS_STREAM_VOLUME(e)) {
>  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
>  GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> -else
> +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> "volume") != NULL) {
>  g_object_set(e, "volume", vol, NULL);
> +} else {
> +g_warning("playback: ignoring volume change on %s",
> gst_element_get_name(e));
> +}
>  
>  g_object_unref(e);
>  }
> @@ -394,8 +397,13 @@ static void playback_mute_changed(GObject *object,
> GParamSpec *pspec, gpointer d
>  if (!e)
>  e = g_object_ref(p->playback.sink);
>  
> -if (GST_IS_STREAM_VOLUME(e))
> +if (GST_IS_STREAM_VOLUME(e)) {
>  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute")
> != NULL) {
> +g_object_set(e, "mute", mute, NULL);
> +} else {
> +g_warning("playback: ignoring mute change on %s",
> gst_element_get_name(e));
> +}
>  
>  g_object_unref(e);
>  }
> @@ -427,10 +435,13 @@ static void record_volume_changed(GObject *object,
> GParamSpec *pspec, gpointer d
>  if (!e)
>  e = g_object_ref(p->record.src);
>  
> -if (GST_IS_STREAM_VOLUME(e))
> +if (GST_IS_STREAM_VOLUME(e)) {
>  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
>  GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> -else
> -g_warning("gst lacks volume capabilities on src");
> +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> "volume") != NULL) {
> +g_object_set(e, "volume", vol, NULL);
> +} else {
> +g_warning("record: ignoring volume change on %s",
> gst_element_get_name(e));
> +}
>  
>  g_object_unref(e);
>  }
> @@ -453,10 +464,13 @@ static void record_mute_changed(GObject *object,
> GParamSpec *pspec, gpointer dat
>  if (!e)
>  e = g_object_ref(p->record.src);
>  
> -if (GST_IS_STREAM_VOLUME (e))
> +if (GST_IS_STREAM_VOLUME (e)) {
>  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> -else
> -g_warning("gst lacks mute capabilities on src: %d", mute);
> +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute")
> != NULL) {
> +g_object_set(e, "mute", mute, NULL);
> +} else {
> +g_warning("record: ignoring mute change on %s",
> gst_element_get_name(e));
> +}
>  
>  g_object_unref(e);
>  }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 23/23] drm/qxl: add overflow checks to qxl_mode_dumb_create()

[Ville Syrjälä]

On Fri, Jan 18, 2019 at 04:49:44PM +0100, Daniel Vetter wrote:
> On Fri, Jan 18, 2019 at 01:20:20PM +0100, Gerd Hoffmann wrote:
> > Signed-off-by: Gerd Hoffmann 
> 
> We already do all reasonable overflow checks in drm_mode_create_dumb(). If
> you don't trust them I think would be better time spent typing an igt to
> test this than adding redundant check in all drivers.
> 
> You're also missing one check for bpp underflows :-)

BTW I just noticed that we don't seem to validating 
create_dumb->flags at all. Someone should probably add some
checks for that, or mark it as deprecated in case we already
lost the battle with userspace stack garbage.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/qxl/qxl_dumb.c | 10 ++
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c
> > index 272d19b677..bed6d06ee4 100644
> > --- a/drivers/gpu/drm/qxl/qxl_dumb.c
> > +++ b/drivers/gpu/drm/qxl/qxl_dumb.c
> > @@ -37,11 +37,13 @@ int qxl_mode_dumb_create(struct drm_file *file_priv,
> > uint32_t handle;
> > int r;
> > struct qxl_surface surf;
> > -   uint32_t pitch, format;
> > +   uint32_t pitch, size, format;
> >  
> > -   pitch = args->width * ((args->bpp + 1) / 8);
> > -   args->size = pitch * args->height;
> > -   args->size = ALIGN(args->size, PAGE_SIZE);
> > +   if (check_mul_overflow(args->width, ((args->bpp + 1) / 8), ))
> > +   return -EINVAL;
> > +   if (check_mul_overflow(pitch, args->height, ))
> > +   return -EINVAL;
> > +   args->size = ALIGN(size, PAGE_SIZE);
> >  
> > switch (args->bpp) {
> > case 16:
> > -- 
> > 2.9.3
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 23/23] drm/qxl: add overflow checks to qxl_mode_dumb_create()

[Daniel Vetter]

On Fri, Jan 18, 2019 at 01:20:20PM +0100, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 

We already do all reasonable overflow checks in drm_mode_create_dumb(). If
you don't trust them I think would be better time spent typing an igt to
test this than adding redundant check in all drivers.

You're also missing one check for bpp underflows :-)
-Daniel

> ---
>  drivers/gpu/drm/qxl/qxl_dumb.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c
> index 272d19b677..bed6d06ee4 100644
> --- a/drivers/gpu/drm/qxl/qxl_dumb.c
> +++ b/drivers/gpu/drm/qxl/qxl_dumb.c
> @@ -37,11 +37,13 @@ int qxl_mode_dumb_create(struct drm_file *file_priv,
>   uint32_t handle;
>   int r;
>   struct qxl_surface surf;
> - uint32_t pitch, format;
> + uint32_t pitch, size, format;
>  
> - pitch = args->width * ((args->bpp + 1) / 8);
> - args->size = pitch * args->height;
> - args->size = ALIGN(args->size, PAGE_SIZE);
> + if (check_mul_overflow(args->width, ((args->bpp + 1) / 8), ))
> + return -EINVAL;
> + if (check_mul_overflow(pitch, args->height, ))
> + return -EINVAL;
> + args->size = ALIGN(size, PAGE_SIZE);
>  
>   switch (args->bpp) {
>   case 16:
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h

[Sam Ravnborg]

On Thu, Jan 17, 2019 at 05:45:41PM +0100, Daniel Vetter wrote:
> On Wed, Jan 16, 2019 at 07:10:18PM +0100, Sam Ravnborg wrote:
> > Hi Daniel.
> > 
> > > v5: Actually try to sort them, and while at it, sort all the ones I
> > > touch.
> > 
> > Applied this variant on top of drm-misc and did a build test.
> > Looked good for ia64, x86 and alpha.
> > 
> > Took a closer look at the changes to atmel_hlcd - and they looked OK.
> > 
> > But I noticed that atmel_hlcdc uses only drm_kms_helper_poll_init() and
> > drm_kms_helper_poll_fini().
> > But there are no hits on DRM_CONNECTOR_POLL - so I think we maybe
> > have a driver here where we have plugged the drm_poll infrastructure,
> > but it is not in use.
> > 
> > >  include/drm/drm_crtc_helper.h | 16 ---
> > 
> > The list of include files in this file could be dropped and replaced by:
> > struct drm_connector;
> > struct drm_device;
> > struct drm_display_mode;
> > struct drm_encoder;
> > struct drm_framebuffer;
> > struct drm_mode_set;
> > struct drm_modeset_acquire_ctx;
> > 
> > I tried to do so on top of your patch.
> > But there were too many build errros and I somehow lost the motivation.
> 
> Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy
> kms drivers. Just removing it from all the atomic drivers caused lots of
> fallout, I expect even more if you entirely remove the includes it has.
> Maybe a todo, care to pls create that patch since it's your idea?

The main reason I bailed out initially was that this would create
small changes to several otherwise seldomly touched files.
And then we would later come and remove drmP.h - so lots of
small but incremental changes to the same otherwise seldomly
edited files.
And the job was only partially done.

I will try to experiment with an approach where I clean up the
include/drm/*.h files a little (like suggested above, +delete drmP.h
and maybe a bit more).

Then to try on a driver by driver basis to make it build with a
cleaned set of include files.
I hope that the cleaned up driver can still build without the
cleaned header files so the changes can be submitted piecemal.

Will do so with an eye on the lesser maintained drivers to try it
out to avoid creating too much chrunch for others.

And if it works out I expect the active drivers to follow the
example.

todo.rst item will wait until I run out of energy.

Sam

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h

[Daniel Vetter]

On Wed, Jan 16, 2019 at 07:10:18PM +0100, Sam Ravnborg wrote:
> Hi Daniel.
> 
> > v5: Actually try to sort them, and while at it, sort all the ones I
> > touch.
> 
> Applied this variant on top of drm-misc and did a build test.
> Looked good for ia64, x86 and alpha.
> 
> Took a closer look at the changes to atmel_hlcd - and they looked OK.
> 
> But I noticed that atmel_hlcdc uses only drm_kms_helper_poll_init() and
> drm_kms_helper_poll_fini().
> But there are no hits on DRM_CONNECTOR_POLL - so I think we maybe
> have a driver here where we have plugged the drm_poll infrastructure,
> but it is not in use.
> 
> >  include/drm/drm_crtc_helper.h | 16 ---
> 
> The list of include files in this file could be dropped and replaced by:
> struct drm_connector;
> struct drm_device;
> struct drm_display_mode;
> struct drm_encoder;
> struct drm_framebuffer;
> struct drm_mode_set;
> struct drm_modeset_acquire_ctx;
> 
> I tried to do so on top of your patch.
> But there were too many build errros and I somehow lost the motivation.

Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy
kms drivers. Just removing it from all the atomic drivers caused lots of
fallout, I expect even more if you entirely remove the includes it has.
Maybe a todo, care to pls create that patch since it's your idea?
-Daniel

> 
> 
> >  include/drm/drm_probe_helper.h| 27 +++
> This on the other hand is fine - as expected as this is a new file.
> 
> But the above is just some random comments so:
> 
> Acked-by: Sam Ravnborg 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk v2] gstaudio: check before setting mute/volume properties

[Victor Toso]

From: Victor Toso 

Otherwise we can get warnings such as:

GLib-GObject-WARNING **: 16:10:00.857:
g_object_set_is_valid_property: object class 'GstAutoAudioSink' has no 
property named 'volume'

Signed-off-by: Victor Toso 
---
 src/spice-gstaudio.c | 32 +++-
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
index 98c1f4f..51ff028 100644
--- a/src/spice-gstaudio.c
+++ b/src/spice-gstaudio.c
@@ -368,10 +368,13 @@ static void playback_volume_changed(GObject *object, 
GParamSpec *pspec, gpointer
 if (!e)
 e = g_object_ref(p->playback.sink);
 
-if (GST_IS_STREAM_VOLUME(e))
+if (GST_IS_STREAM_VOLUME(e)) {
 gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
-else
+} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "volume") 
!= NULL) {
 g_object_set(e, "volume", vol, NULL);
+} else {
+g_warning("playback: ignoring volume change on %s", 
gst_element_get_name(e));
+}
 
 g_object_unref(e);
 }
@@ -394,8 +397,13 @@ static void playback_mute_changed(GObject *object, 
GParamSpec *pspec, gpointer d
 if (!e)
 e = g_object_ref(p->playback.sink);
 
-if (GST_IS_STREAM_VOLUME(e))
+if (GST_IS_STREAM_VOLUME(e)) {
 gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
+} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute") != 
NULL) {
+g_object_set(e, "mute", mute, NULL);
+} else {
+g_warning("playback: ignoring mute change on %s", 
gst_element_get_name(e));
+}
 
 g_object_unref(e);
 }
@@ -427,10 +435,13 @@ static void record_volume_changed(GObject *object, 
GParamSpec *pspec, gpointer d
 if (!e)
 e = g_object_ref(p->record.src);
 
-if (GST_IS_STREAM_VOLUME(e))
+if (GST_IS_STREAM_VOLUME(e)) {
 gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
-else
-g_warning("gst lacks volume capabilities on src");
+} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "volume") 
!= NULL) {
+g_object_set(e, "volume", vol, NULL);
+} else {
+g_warning("record: ignoring volume change on %s", 
gst_element_get_name(e));
+}
 
 g_object_unref(e);
 }
@@ -453,10 +464,13 @@ static void record_mute_changed(GObject *object, 
GParamSpec *pspec, gpointer dat
 if (!e)
 e = g_object_ref(p->record.src);
 
-if (GST_IS_STREAM_VOLUME (e))
+if (GST_IS_STREAM_VOLUME (e)) {
 gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
-else
-g_warning("gst lacks mute capabilities on src: %d", mute);
+} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute") != 
NULL) {
+g_object_set(e, "mute", mute, NULL);
+} else {
+g_warning("record: ignoring mute change on %s", 
gst_element_get_name(e));
+}
 
 g_object_unref(e);
 }
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] gstaudio: check before set mute/volume properties

[Victor Toso]

Hi,

On Fri, Jan 18, 2019 at 04:35:09PM +0100, Christophe Fergeau wrote:
> "Check before setting ..."

Indeed, fixed.

> On Fri, Jan 18, 2019 at 03:49:21PM +0100, Victor Toso wrote:
> > From: Victor Toso 
> > 
> > Otherwise we can get warnings such as:
> > 
> > GLib-GObject-WARNING **: 16:10:00.857:
> > g_object_set_is_valid_property: object class 'GstAutoAudioSink' has no 
> > property named 'volume'
> > 
> > Signed-off-by: Victor Toso 
> > ---
> >  src/spice-gstaudio.c | 32 +++-
> >  1 file changed, 23 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > index 98c1f4f..51ff028 100644
> > --- a/src/spice-gstaudio.c
> > +++ b/src/spice-gstaudio.c
> > @@ -368,10 +368,13 @@ static void playback_volume_changed(GObject *object, 
> > GParamSpec *pspec, gpointer
> >  if (!e)
> >  e = g_object_ref(p->playback.sink);
> >  
> > -if (GST_IS_STREAM_VOLUME(e))
> > +if (GST_IS_STREAM_VOLUME(e)) {
> >  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
> > GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > -else
> > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> > "volume") != NULL) {
> >  g_object_set(e, "volume", vol, NULL);
> 
> Dunno how expensive g_object_class_find_property is, and if we should
> cache this? Looks good otherwise.

No clue about how expensive it is, shouldn't be bad but
considering that this is only triggered after volume/mute
messages, which are not too happen often in normal use case, it
should be fine.

> Christophe
> 
> > +} else {
> > +g_warning("playback: ignoring volume change on %s", 
> > gst_element_get_name(e));
> > +}
> >  
> >  g_object_unref(e);
> >  }
> > @@ -394,8 +397,13 @@ static void playback_mute_changed(GObject *object, 
> > GParamSpec *pspec, gpointer d
> >  if (!e)
> >  e = g_object_ref(p->playback.sink);
> >  
> > -if (GST_IS_STREAM_VOLUME(e))
> > +if (GST_IS_STREAM_VOLUME(e)) {
> >  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> > "mute") != NULL) {
> > +g_object_set(e, "mute", mute, NULL);
> > +} else {
> > +g_warning("playback: ignoring mute change on %s", 
> > gst_element_get_name(e));
> > +}
> >  
> >  g_object_unref(e);
> >  }
> > @@ -427,10 +435,13 @@ static void record_volume_changed(GObject *object, 
> > GParamSpec *pspec, gpointer d
> >  if (!e)
> >  e = g_object_ref(p->record.src);
> >  
> > -if (GST_IS_STREAM_VOLUME(e))
> > +if (GST_IS_STREAM_VOLUME(e)) {
> >  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
> > GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > -else
> > -g_warning("gst lacks volume capabilities on src");
> > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> > "volume") != NULL) {
> > +g_object_set(e, "volume", vol, NULL);
> > +} else {
> > +g_warning("record: ignoring volume change on %s", 
> > gst_element_get_name(e));
> > +}
> >  
> >  g_object_unref(e);
> >  }
> > @@ -453,10 +464,13 @@ static void record_mute_changed(GObject *object, 
> > GParamSpec *pspec, gpointer dat
> >  if (!e)
> >  e = g_object_ref(p->record.src);
> >  
> > -if (GST_IS_STREAM_VOLUME (e))
> > +if (GST_IS_STREAM_VOLUME (e)) {
> >  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > -else
> > -g_warning("gst lacks mute capabilities on src: %d", mute);
> > +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> > "mute") != NULL) {
> > +g_object_set(e, "mute", mute, NULL);
> > +} else {
> > +g_warning("record: ignoring mute change on %s", 
> > gst_element_get_name(e));
> > +}
> >  
> >  g_object_unref(e);
> >  }
> > -- 
> > 2.20.1
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel




signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] gstaudio: check before set mute/volume properties

[Christophe Fergeau]

"Check before setting ..."

On Fri, Jan 18, 2019 at 03:49:21PM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> Otherwise we can get warnings such as:
> 
> GLib-GObject-WARNING **: 16:10:00.857:
> g_object_set_is_valid_property: object class 'GstAutoAudioSink' has no 
> property named 'volume'
> 
> Signed-off-by: Victor Toso 
> ---
>  src/spice-gstaudio.c | 32 +++-
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> index 98c1f4f..51ff028 100644
> --- a/src/spice-gstaudio.c
> +++ b/src/spice-gstaudio.c
> @@ -368,10 +368,13 @@ static void playback_volume_changed(GObject *object, 
> GParamSpec *pspec, gpointer
>  if (!e)
>  e = g_object_ref(p->playback.sink);
>  
> -if (GST_IS_STREAM_VOLUME(e))
> +if (GST_IS_STREAM_VOLUME(e)) {
>  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
> GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> -else
> +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> "volume") != NULL) {
>  g_object_set(e, "volume", vol, NULL);

Dunno how expensive g_object_class_find_property is, and if we should
cache this? Looks good otherwise.

Christophe

> +} else {
> +g_warning("playback: ignoring volume change on %s", 
> gst_element_get_name(e));
> +}
>  
>  g_object_unref(e);
>  }
> @@ -394,8 +397,13 @@ static void playback_mute_changed(GObject *object, 
> GParamSpec *pspec, gpointer d
>  if (!e)
>  e = g_object_ref(p->playback.sink);
>  
> -if (GST_IS_STREAM_VOLUME(e))
> +if (GST_IS_STREAM_VOLUME(e)) {
>  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute") 
> != NULL) {
> +g_object_set(e, "mute", mute, NULL);
> +} else {
> +g_warning("playback: ignoring mute change on %s", 
> gst_element_get_name(e));
> +}
>  
>  g_object_unref(e);
>  }
> @@ -427,10 +435,13 @@ static void record_volume_changed(GObject *object, 
> GParamSpec *pspec, gpointer d
>  if (!e)
>  e = g_object_ref(p->record.src);
>  
> -if (GST_IS_STREAM_VOLUME(e))
> +if (GST_IS_STREAM_VOLUME(e)) {
>  gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
> GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> -else
> -g_warning("gst lacks volume capabilities on src");
> +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), 
> "volume") != NULL) {
> +g_object_set(e, "volume", vol, NULL);
> +} else {
> +g_warning("record: ignoring volume change on %s", 
> gst_element_get_name(e));
> +}
>  
>  g_object_unref(e);
>  }
> @@ -453,10 +464,13 @@ static void record_mute_changed(GObject *object, 
> GParamSpec *pspec, gpointer dat
>  if (!e)
>  e = g_object_ref(p->record.src);
>  
> -if (GST_IS_STREAM_VOLUME (e))
> +if (GST_IS_STREAM_VOLUME (e)) {
>  gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> -else
> -g_warning("gst lacks mute capabilities on src: %d", mute);
> +} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute") 
> != NULL) {
> +g_object_set(e, "mute", mute, NULL);
> +} else {
> +g_warning("record: ignoring mute change on %s", 
> gst_element_get_name(e));
> +}
>  
>  g_object_unref(e);
>  }
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] gstaudio: check before set mute/volume properties

[Victor Toso]

From: Victor Toso 

Otherwise we can get warnings such as:

GLib-GObject-WARNING **: 16:10:00.857:
g_object_set_is_valid_property: object class 'GstAutoAudioSink' has no 
property named 'volume'

Signed-off-by: Victor Toso 
---
 src/spice-gstaudio.c | 32 +++-
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
index 98c1f4f..51ff028 100644
--- a/src/spice-gstaudio.c
+++ b/src/spice-gstaudio.c
@@ -368,10 +368,13 @@ static void playback_volume_changed(GObject *object, 
GParamSpec *pspec, gpointer
 if (!e)
 e = g_object_ref(p->playback.sink);
 
-if (GST_IS_STREAM_VOLUME(e))
+if (GST_IS_STREAM_VOLUME(e)) {
 gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
-else
+} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "volume") 
!= NULL) {
 g_object_set(e, "volume", vol, NULL);
+} else {
+g_warning("playback: ignoring volume change on %s", 
gst_element_get_name(e));
+}
 
 g_object_unref(e);
 }
@@ -394,8 +397,13 @@ static void playback_mute_changed(GObject *object, 
GParamSpec *pspec, gpointer d
 if (!e)
 e = g_object_ref(p->playback.sink);
 
-if (GST_IS_STREAM_VOLUME(e))
+if (GST_IS_STREAM_VOLUME(e)) {
 gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
+} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute") != 
NULL) {
+g_object_set(e, "mute", mute, NULL);
+} else {
+g_warning("playback: ignoring mute change on %s", 
gst_element_get_name(e));
+}
 
 g_object_unref(e);
 }
@@ -427,10 +435,13 @@ static void record_volume_changed(GObject *object, 
GParamSpec *pspec, gpointer d
 if (!e)
 e = g_object_ref(p->record.src);
 
-if (GST_IS_STREAM_VOLUME(e))
+if (GST_IS_STREAM_VOLUME(e)) {
 gst_stream_volume_set_volume(GST_STREAM_VOLUME(e), 
GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
-else
-g_warning("gst lacks volume capabilities on src");
+} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "volume") 
!= NULL) {
+g_object_set(e, "volume", vol, NULL);
+} else {
+g_warning("record: ignoring volume change on %s", 
gst_element_get_name(e));
+}
 
 g_object_unref(e);
 }
@@ -453,10 +464,13 @@ static void record_mute_changed(GObject *object, 
GParamSpec *pspec, gpointer dat
 if (!e)
 e = g_object_ref(p->record.src);
 
-if (GST_IS_STREAM_VOLUME (e))
+if (GST_IS_STREAM_VOLUME (e)) {
 gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
-else
-g_warning("gst lacks mute capabilities on src: %d", mute);
+} else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), "mute") != 
NULL) {
+g_object_set(e, "mute", mute, NULL);
+} else {
+g_warning("record: ignoring mute change on %s", 
gst_element_get_name(e));
+}
 
 g_object_unref(e);
 }
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [qxl-wddm-dod] Fix for black screen on driver uninstall on ovmf platform

[Frediano Ziglio]

By the way, the name of the function was not a regression, I acked and merged 
the patch. 

Maybe I'll think about a better name 

Frediano 

> On Thu, Jan 17, 2019 at 6:20 PM Frediano Ziglio < fzig...@redhat.com > wrote:

> > >
> 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1661147
> 
> > > If display miniport driver does not pass valid display info to
> 
> > > the basic display driver on disable/uninstall, the basic
> 
> > > display driver raises run-time error and stays with yellow
> 
> > > bang. This behavior is specific to UEFI machines.
> 
> > > Reference:
> 
> > > https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/dispmprt/nc-dispmprt-dxgkddi_stop_device_and_release_post_display_ownership
> 
> > >
> 
> > > Signed-off-by: Yuri Benditovich < yuri.benditov...@daynix.com >
> 
> > > ---
> 
> > > qxldod/QxlDod.cpp | 47 ++-
> 
> > > qxldod/QxlDod.h | 6 +-
> 
> > > 2 files changed, 51 insertions(+), 2 deletions(-)
> 
> > >
> 
> > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> 
> > > index b72e12c..dea78e2 100755
> 
> > > --- a/qxldod/QxlDod.cpp
> 
> > > +++ b/qxldod/QxlDod.cpp
> 
> > > @@ -189,6 +189,12 @@ NTSTATUS QxlDod::StartDevice(_In_ DXGK_START_INFO*
> 
> > > pDxgkStartInfo,
> 
> > > return Status;
> 
> > > }
> 
> > >
> 
> > > + DXGK_DISPLAY_INFORMATION *pInfo = _CurrentModes[0].DispInfo;
> 
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: Initial display info:\n",
> 
> > > __FUNCTION__));
> 
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("ACPI/Target Id: %d:%d\n",
> 
> > > pInfo->AcpiId, pInfo->TargetId));
> 
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("W/H/Pitch/Color:%d:%d:%d:%d\n",
> 
> > > pInfo->Width, pInfo->Height, pInfo->Pitch, pInfo->ColorFormat));
> 
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("Physical address:%I64x\n",
> 
> > > pInfo->PhysicAddress.QuadPart));
> 
> > > +
> 
> > > Status = RegisterHWInfo(m_pHWDevice->GetId());
> 
> > > if (!NT_SUCCESS(Status))
> 
> > > {
> 
> > > @@ -668,6 +674,12 @@ NTSTATUS
> 
> > > QxlDod::StopDeviceAndReleasePostDisplayOwnership(_In_ D3DDDI_VIDEO_PRE
> 
> > > m_pHWDevice->BlackOutScreen(_CurrentModes[SourceId]);
> 
> > >
> 
> > > *pDisplayInfo = m_CurrentModes[SourceId].DispInfo;
> 
> > > + m_pHWDevice->GetFinalDisplayInfo(pDisplayInfo);
> 
> > > +
> 
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: Final display info:\n",
> 
> > > __FUNCTION__));
> 
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("ACPI/Target Id: %d:%d\n",
> 
> > > pDisplayInfo->AcpiId, pDisplayInfo->TargetId));
> 
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("W/H/Pitch/Color:%d:%d:%d:%d\n",
> 
> > > pDisplayInfo->Width, pDisplayInfo->Height, pDisplayInfo->Pitch,
> 
> > > pDisplayInfo->ColorFormat));
> 
> > > + DbgPrint(TRACE_LEVEL_INFORMATION, ("Physical address:%I64x\n",
> 
> > > pDisplayInfo->PhysicAddress.QuadPart));
> 
> > >
> 
> > > return StopDevice();
> 
> > > }
> 
> > > @@ -3055,6 +3067,19 @@ NTSTATUS VgaDevice::Escape(_In_ CONST
> > > DXGKARG_ESCAPE*
> 
> > > pEscap)
> 
> > > return STATUS_NOT_IMPLEMENTED;
> 
> > > }
> 
> > >
> 
> > > +static BOOLEAN IsUefiMode()
> 
> > > +{
> 
> > > + PAGED_CODE();
> 
> > > + UNICODE_STRING usName;
> 
> > > + GUID guid = {};
> 
> > > + ULONG res, len = sizeof(res);
> 
> > > + RtlInitUnicodeString(, L"dummy");
> 
> > > + NTSTATUS status = ExGetFirmwareEnvironmentVariable(, ,
> > > ,
> 
> > > , NULL);
> 
> > > + DbgPrint(TRACE_LEVEL_VERBOSE, ("%s, status %X\n", __FUNCTION__,
> 
> > > status));
> 
> > > + // on UEFI system the status is STATUS_VARIABLE_NOT_FOUND
> 
> > > + return status != STATUS_NOT_IMPLEMENTED;
> 
> > > +}
> 
> > > +
> 
> > > QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
> 
> > > {
> 
> > > PAGED_CODE();
> 
> > > @@ -3068,6 +3093,8 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
> 
> > > m_Pending = 0;
> 
> > > m_PresentThread = NULL;
> 
> > > m_bActive = FALSE;
> 
> > > + m_bUefiMode = IsUefiMode();
> 
> > > + DbgPrint(TRACE_LEVEL_VERBOSE, ("%s, %s mode\n", __FUNCTION__,
> 
> > > m_bUefiMode ? "UEFI" : "BIOS"));
> 
> > > }
> 
> > >
> 
> > > QxlDevice::~QxlDevice(void)
> 
> > > @@ -3331,6 +3358,17 @@ NTSTATUS
> > > QxlDevice::SetPowerState(DEVICE_POWER_STATE
> 
> > > DevicePowerState, DXGK_DISP
> 
> > > return STATUS_SUCCESS;
> 
> > > }
> 
> > >
> 
> > > +void QxlDevice::GetFinalDisplayInfo(DXGK_DISPLAY_INFORMATION*
> > > pDisplayInfo)
> 
> > > +{
> 
> > > + PAGED_CODE();
> 
> > > + *pDisplayInfo = m_InitialDisplayInfo;
> 
> > > + pDisplayInfo->TargetId = pDisplayInfo->AcpiId = 0;
> 
> > > + if (!pDisplayInfo->PhysicAddress.QuadPart)
> 
> > > + {
> 
> > > + pDisplayInfo->PhysicAddress = m_RamPA;
> 
> > > + }
> 
> > > +}
> 
> > > +
> 
> > > NTSTATUS QxlDevice::HWInit(PCM_RESOURCE_LIST pResList,
> 
> > > DXGK_DISPLAY_INFORMATION* pDispInfo)
> 
> > > {
> 
> > > PAGED_CODE();
> 
> > > @@ -3530,10 +3568,11 @@ NTSTATUS
> > > QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION*
> 
> > > pDispInfo)
> 
> > > DbgPrint(TRACE_LEVEL_ERROR, ("InitMonitorConfig failed with status
> 
> > 

Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer

[Christophe Fergeau]

On Fri, Jan 18, 2019 at 10:23:04AM +0200, Yuri Benditovich wrote:
> On Wed, Jan 16, 2019 at 6:11 PM Christophe Fergeau 
> wrote:
> 
> > On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:
> > > On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau 
> > > wrote:
> > > > In _SpiceUsbDeviceManagerPrivate, you replaced
> > > > #ifndef G_OS_WIN32
> > > > libusb_device *libdev;
> > > > #endif
> > > >
> > > > with
> > > >
> > > > #ifndef G_OS_WIN32
> > > > SpiceUsbBackendDevice *bdev;
> > > > #endif
> > > >
> > > > The #ifdef is there because of this comment in
> > > > spice_usb_device_manager_device_to_bdev:
> > > >
> > > >   /*
> > > >* On win32 we need to do this the hard and slow way, by asking
> > libusb to
> > > >* re-enumerate all devices and then finding a matching device.
> > > >* We cannot cache the libusb_device like we do under Linux since the
> > > >* driver swap we do under windows invalidates the cached libdev.
> > > >*/
> > > >
> > > > After your patch, spice_usb_device_manager_device_to_bdev is no longer
> > > > at the right level of indirection imo, it looks up the 'bdev' when
> > > > needed on Windows in usb-device-manager.c, but my understanding of that
> > > > comment is that any libusb call within SpiceUsbBackendDevice should not
> > > > use a cached libusb_device?
> > > >
> > > >
> > > >
> > > As fas as I understand, there is no change in the level of indirection.
> > > Previously was:
> > > On Linux: usb-device-manager receives libusb device on hotplug indication
> > > and uses it until the device disappears.
> > > On Windows: each time the usb-device-manager needs the libusb device it
> > > takes fresh list of libusb devices and finds one according to known
> > device
> > > properties.
> > >
> > > Now:
> > > On Linux: usb-device-manager receives backend device wrapping libusb
> > device
> > > on hotplug indication and uses it until the device disappears
> > > On Windows: each time the usb-device-manager needs the backend device it
> > > takes fresh list of new backend devices and finds one according to known
> > > device properties.
> > > So, the backend device is always fresh one and it always have fresh
> > libusb
> > > device.
> > >
> > > This can be simplified with removal of all these lookups in the
> > > usb-device-manager (and I already illustrated how) but after this commit
> > is
> > > done.
> > > If from your point of view this is the requirement to existing commit,
> > > please let me know (then we will need to reevaluate our plans and our
> > > ability to deliver the patches in reasonable time).
> >
> > I'll quote again the comment:
> >
> >/*
> > * On win32 we need to do this the hard and slow way, by asking libusb
> > to
> > * re-enumerate all devices and then finding a matching device.
> > * We cannot cache the libusb_device like we do under Linux since the
> > * driver swap we do under windows invalidates the cached libdev.
> > */
> >
> > It says we cannot cache a libusb_device because it can change behind our
> > back, so we need to reenumerate all devices and lookup the right
> > libusb_device before every call to libusb.
> > Before your changes, we did not hold any long-lasting references to a
> > libusb_device on Windows, so we were guaranteed to do that lookup before
> > every call. After your changes, SpiceUsbBackendDevice is holding a
> > long-lasting reference to a libusb_device, but does not care about the
> > win32 issue. Only usb-device-manager is making sure the libusb_device
> > it's going to use is valid. This does not make sense to me to leak this
> > implementation detail to usb-device-manager. SpiceUsbBackend should hide
> > it, otherwise its API is going to be error-prone.
> >
> > When you say this can be simplified by removing this lookup, are you
> > confirming that this comment I quoted above is obsolete?
> >
> 
> This comment, IMO, is wrong and permanent enumeration is not mandatory.
> None of existing objects can disappear if it is is referenced.

The libusb object does not disappear, but apparently there were some
situations where the device 'moved', and the libusb object pointed to a
no-longer existing device, thus the need to enumerate again.
I believe this is related to
https://gitlab.freedesktop.org/spice/spice-gtk/commit/76e94509cf29a78aa39740c81dcdd2eee355c7b9
https://bugzilla.redhat.com/show_bug.cgi?id=842816


> Changes in existing patch are intentionally minimal to allow
> comparison between previous and existing code. After the current patch
> is merged I will be able to solve the problem of persistency as I
> should be, I believe (together with unification of the structure
> holding device properties). But mixing all these things together
> discards the current patch and I can't predict the time frame of next
> round.

I'm not suggesting to squash everything in the current patch, but we are
not limited to one single patch. If you intend to remove that
persistency code right 

Re: [Spice-devel] [PATCH spice-gtk 1/2] spice-widget-egl: Remove double preprocessor check

[Christophe Fergeau]

On Fri, Jan 18, 2019 at 12:37:37PM +, Frediano Ziglio wrote:
> Do not disable and enable code with same condition.

Acked-by: Christophe Fergeau 

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/spice-widget-egl.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
> index 011d8c76..5ad84bfc 100644
> --- a/src/spice-widget-egl.c
> +++ b/src/spice-widget-egl.c
> @@ -307,8 +307,6 @@ gl_make_current(SpiceDisplay *display, GError **err)
>  return FALSE;
>  }
>  }
> -#endif
> -#ifdef GDK_WINDOWING_X11
>  else
>  #endif
>  {
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH linux vdagent v4 9/9] Make cursor work with mjpeg streaming

[Lukáš Hrázký]

I'd also squash this up to 7/9. It just reworks what was not a complete
solution.

Cheers,
Lukas


On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> In the case where we have an mjpeg plugin running in the streaming
> agent, we have two spice displays representing the same guest display.
> In that scenario, the session agent may maintain the following guest
> output mapping:
>  spice channel 0 (QXL) => X output 0
>  spice channel 1 (streaming-agent) => X output 0
> 
> While this is not necessarily a supported scenario, it would be nice if
> the cursor input worked properly in this case. The root problem is that
> when the session agent sends down the guest xorg resolutions to the
> system daemon, it simply loops through the list of xorg displays, and
> for each X display it looks up the first spice display ID associated
> with it and sends that down to the daemon. In the scenario mentioned
> above, since there is only a single X display configured (albeit
> represented by two different spice displays), we would send down a
> single display resolution to the system daemon:
>  - { width=1280, height=1024, x=0, y=0, display_id=0 }
> 
> Notice that there is no entry for display_id=1. When the agent receives
> a cursor input message for display channel 1, that message will get
> passed to the systemn daemon, which will attempt to look up display_id 1
> in order to convert the event coordinates to global coordinates. Finding
> no entry for display_id=1, the mouse events do not work.
> 
> In this patch, when we want to send the guest resolutions down to the
> system daemon, we still loop through the list of X outputs, but for each
> output we also loop through the guest output mapping table and send a
> resolution structure down to the daemon for each registered output
> mapping.  This means that in the previously mentioned scenario, we would
> send down the following information:
>  - { width=1280, height=1024, x=0, y=0, display_id=0 }
>  - { width=1280, height=1024, x=0, y=0, display_id=1 }
> 
>  This means that when the client sends a mouse event for display_id=1,
>  the system daemon knows the coordinates of the guest display associated
>  with that ID and can process the mouse event properly.
> ---
>  src/vdagent/x11-randr.c | 106 
>  1 file changed, 53 insertions(+), 53 deletions(-)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 283dbc8..fe48c46 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -688,35 +688,6 @@ static int config_size(int num_of_monitors)
> num_of_monitors * sizeof(VDAgentMonConfig);
>  }
>  
> -static int get_display_id_for_output_index(struct vdagent_x11 *x11, int 
> output_index)
> -{
> -// invalid output index
> -if (output_index >= x11->randr.res->noutput) {
> -syslog(LOG_WARNING, "Invalid output index %d (>%d)", output_index, 
> x11->randr.res->noutput);
> -return -1;
> -}
> -
> -if (g_hash_table_size(x11->guest_output_map) == 0) {
> -syslog(LOG_DEBUG, "No guest output map, using output index as 
> display id");
> -return output_index;
> -}
> -
> -int display_id = -1;
> -RROutput output_id = x11->randr.res->outputs[output_index];
> -GHashTableIter iter;
> -gpointer key, value;
> -g_hash_table_iter_init(, x11->guest_output_map);
> -while (g_hash_table_iter_next(, , )) {
> -gint64 *other_id = value;
> -if (*other_id == output_id) {
> -return GPOINTER_TO_INT(key);
> -}
> -}
> -
> -syslog(LOG_WARNING, "Unable to find a display id for output index %d)", 
> output_index);
> -return display_id;
> -}
> -
>  // gets monitor information about the specified output index and returns 
> true if there was no error
>  static bool get_monitor_info_for_output_index(struct vdagent_x11 *x11, int 
> output_index, int *x, int *y, int *width, int *height)
>  {
> @@ -1093,7 +1064,7 @@ exit:
>  
>  void vdagent_x11_send_daemon_guest_xorg_res(struct vdagent_x11 *x11, int 
> update)
>  {
> -struct vdagentd_guest_xorg_resolution *res = NULL;
> +GArray *res_array = g_array_new(FALSE, FALSE, sizeof(struct 
> vdagentd_guest_xorg_resolution));
>  int i, width = 0, height = 0, screen_count = 0;
>  
>  if (x11->has_xrandr) {
> @@ -1101,17 +1072,39 @@ void vdagent_x11_send_daemon_guest_xorg_res(struct 
> vdagent_x11 *x11, int update)
>  update_randr_res(x11, 0);
>  
>  screen_count = x11->randr.res->noutput;
> -res = g_new(struct vdagentd_guest_xorg_resolution, screen_count);
>  
>  for (i = 0; i < screen_count; i++) {
> -struct vdagentd_guest_xorg_resolution *curr = [i];
> -if (!get_monitor_info_for_output_index(x11, i, >x, 
> >y,
> -   >width, 
> >height))
> +struct vdagentd_guest_xorg_resolution curr;
> +if 

Re: [Spice-devel] [PATCH linux vdagent v4 8/9] Send display IDs to daemon when device info changes

[Lukáš Hrázký]

I'd squash this to the previous patch.


On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> When the agent gets a new device info message (from the daemon), we need
> to re-calculate the guest output map and send that information back down
> to the daemon so that it can handle mouse input events correctly.
> ---
>  src/vdagent/x11-randr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 5749ae7..283dbc8 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -865,6 +865,9 @@ void vdagent_x11_handle_graphics_device_info(struct 
> vdagent_x11 *x11, uint8_t *d
>  device_display_info = (VDAgentDeviceDisplayInfo*) ((char*) 
> device_display_info +
>  sizeof(VDAgentDeviceDisplayInfo) + 
> device_display_info->device_address_len);
>  }
> +
> +// make sure daemon is up-to-date with (possibly updated) device IDs
> +vdagent_x11_send_daemon_guest_xorg_res(x11, 1);
>  }
>  
>  static int get_output_index_for_display_id(struct vdagent_x11 *x11, int 
> display_id)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH linux vdagent v4 7/9] Send display_id down to the vdagentd daemon

[Lukáš Hrázký]

On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> Add a display_id field to the structure that we use to send down the
> list of guest display resolutions to the vdagentd daemon. This allows us
> to map the spice display id to the proper X display for determining
> mouse locations, etc.
> ---
>  src/vdagent/x11-randr.c | 35 ---
>  src/vdagentd-proto.h|  1 +
>  src/vdagentd/uinput.c   | 23 ++-
>  3 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index f3972c8..5749ae7 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -688,6 +688,34 @@ static int config_size(int num_of_monitors)
> num_of_monitors * sizeof(VDAgentMonConfig);
>  }
>  
> +static int get_display_id_for_output_index(struct vdagent_x11 *x11, int 
> output_index)
> +{
> +// invalid output index
> +if (output_index >= x11->randr.res->noutput) {
> +syslog(LOG_WARNING, "Invalid output index %d (>%d)", output_index, 
> x11->randr.res->noutput);
> +return -1;
> +}
> +
> +if (g_hash_table_size(x11->guest_output_map) == 0) {
> +syslog(LOG_DEBUG, "No guest output map, using output index as 
> display id");
> +return output_index;
> +}
> +
> +int display_id = -1;
> +RROutput output_id = x11->randr.res->outputs[output_index];
> +GHashTableIter iter;
> +gpointer key, value;
> +g_hash_table_iter_init(, x11->guest_output_map);
> +while (g_hash_table_iter_next(, , )) {
> +gint64 *other_id = value;
> +if (*other_id == output_id) {
> +return GPOINTER_TO_INT(key);
> +}
> +}
> +
> +syslog(LOG_WARNING, "Unable to find a display id for output index %d)", 
> output_index);
> +return display_id;
> +}
>  
>  // gets monitor information about the specified output index and returns 
> true if there was no error
>  static bool get_monitor_info_for_output_index(struct vdagent_x11 *x11, int 
> output_index, int *x, int *y, int *width, int *height)
> @@ -1075,11 +1103,12 @@ void vdagent_x11_send_daemon_guest_xorg_res(struct 
> vdagent_x11 *x11, int update)
>  for (i = 0; i < screen_count; i++) {
>  struct vdagentd_guest_xorg_resolution *curr = [i];
>  if (!get_monitor_info_for_output_index(x11, i, >x, 
> >y,
> -   >width, 
> >height)
> +   >width, 
> >height))

Oh, so this definitely belongs to the previous patch.

Besides that,
Reviewed-by: Lukáš Hrázký 

>  {
>  g_free(res);
>  goto no_info;
>  }
> +curr->display_id = get_display_id_for_output_index(x11, i);
>  }
>  width  = x11->width[0];
>  height = x11->height[0];
> @@ -1131,8 +1160,8 @@ no_info:
>  if (x11->debug) {
>  syslog(LOG_DEBUG, "Sending guest screen resolutions to vdagentd:");
>  for (i = 0; i < screen_count; i++) {
> -syslog(LOG_DEBUG, "   screen %d %dx%d%+d%+d", i,
> -   res[i].width, res[i].height, res[i].x, res[i].y);
> +syslog(LOG_DEBUG, "   screen %d %dx%d%+d%+d, id=%d", i,
> +   res[i].width, res[i].height, res[i].x, res[i].y, 
> res[i].display_id);
>  }
>  }
>  
> diff --git a/src/vdagentd-proto.h b/src/vdagentd-proto.h
> index 243a9c6..7eff71d 100644
> --- a/src/vdagentd-proto.h
> +++ b/src/vdagentd-proto.h
> @@ -53,6 +53,7 @@ struct vdagentd_guest_xorg_resolution {
>  int height;
>  int x;
>  int y;
> +int display_id;
>  };
>  
>  #endif
> diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> index 4f854bf..ff37e1e 100644
> --- a/src/vdagentd/uinput.c
> +++ b/src/vdagentd/uinput.c
> @@ -175,6 +175,18 @@ static void uinput_send_event(struct vdagentd_uinput 
> **uinputp,
>  }
>  }
>  
> +static struct vdagentd_guest_xorg_resolution* lookup_screen_info(struct 
> vdagentd_uinput *uinput, int display_id)
> +{
> +int i;
> +for (i = 0; i < uinput->screen_count; i++) {
> +if (uinput->screen_info[i].display_id == display_id) {
> +return >screen_info[i];
> +}
> +}
> +syslog(LOG_WARNING, "Unable to find output index for display id %d", 
> display_id);
> +return NULL;
> +}
> +
>  void vdagentd_uinput_do_mouse(struct vdagentd_uinput **uinputp,
>  VDAgentMouseState *mouse)
>  {
> @@ -196,16 +208,17 @@ void vdagentd_uinput_do_mouse(struct vdagentd_uinput 
> **uinputp,
>  int i, down;
>  
>  if (*uinputp) {
> -if (mouse->display_id >= uinput->screen_count) {
> -syslog(LOG_WARNING, "mouse event for unknown monitor (%d >= %d)",
> -   mouse->display_id, uinput->screen_count);
> +struct vdagentd_guest_xorg_resolution *screen_info = 
> lookup_screen_info(uinput, 

Re: [Spice-devel] [PATCH linux vdagent v4 6/9] Use new function in vdagent_x11_send_daemon_guest_xorg_res()

[Lukáš Hrázký]

Reviewed-by: Lukáš Hrázký 


On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> Rather than getting the current guest resolution in a
> VDAgentMonitorsConfig struct and then translating it to a new struct
> type for sending down to the daemon, simply use the new function that
> was factored out in a previous commit and populate the message struct
> directly.
> ---
>  src/vdagent/x11-randr.c | 20 
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 05a001e..f3972c8 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -1066,25 +1066,21 @@ void vdagent_x11_send_daemon_guest_xorg_res(struct 
> vdagent_x11 *x11, int update)
>  int i, width = 0, height = 0, screen_count = 0;
>  
>  if (x11->has_xrandr) {
> -VDAgentMonitorsConfig *curr;
> -
>  if (update)
>  update_randr_res(x11, 0);
>  
> -curr = get_current_mon_config(x11);
> -if (!curr)
> -goto no_info;
> -
> -screen_count = curr->num_of_monitors;
> +screen_count = x11->randr.res->noutput;
>  res = g_new(struct vdagentd_guest_xorg_resolution, screen_count);
>  
>  for (i = 0; i < screen_count; i++) {
> -res[i].width  = curr->monitors[i].width;
> -res[i].height = curr->monitors[i].height;
> -res[i].x = curr->monitors[i].x;
> -res[i].y = curr->monitors[i].y;
> +struct vdagentd_guest_xorg_resolution *curr = [i];
> +if (!get_monitor_info_for_output_index(x11, i, >x, 
> >y,
> +   >width, 
> >height)
> +{
> +g_free(res);
> +goto no_info;
> +}
>  }
> -g_free(curr);
>  width  = x11->width[0];
>  height = x11->height[0];
>  } else if (x11->has_xinerama) {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH linux vdagent v4 5/9] Factor a function out of get_current_mon_config()

[Lukáš Hrázký]

On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> When sending the guest xorg resolution to the vdagentd daemon, we get
> the current resolution with this function, which returns a
> VDAgentMonitorsConfig structure that must then be converted to the
> structure that we send to the daemon. Rather than re-using this function
> that returns the wrong type, factor out most of the functionality into a
> separate function. This function can be used by a new function to return
> a type appropriate for sending the xorg resolution to the daemon.
> 
> This is necessary because in the next few commits I'll be changing the
> vdagentd protocol to send an additional display_id parameter to the
> daemon.
> ---
>  src/vdagent/x11-randr.c | 73 +
>  1 file changed, 52 insertions(+), 21 deletions(-)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 5eff225..05a001e 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -688,38 +688,69 @@ static int config_size(int num_of_monitors)
> num_of_monitors * sizeof(VDAgentMonConfig);
>  }
>  
> +
> +// gets monitor information about the specified output index and returns 
> true if there was no error
> +static bool get_monitor_info_for_output_index(struct vdagent_x11 *x11, int 
> output_index, int *x, int *y, int *width, int *height)

Lines above too long.

> +{
> +g_return_val_if_fail (output_index < x11->randr.res->noutput, false);
> +g_return_val_if_fail (x != NULL, false);
> +g_return_val_if_fail (y != NULL, false);
> +g_return_val_if_fail (width != NULL, false);
> +g_return_val_if_fail (height != NULL, false);
> +
> +int j;
> +XRRCrtcInfo *crtc = NULL;
> +XRRModeInfo *mode;
> +
> +if (x11->randr.outputs[output_index]->ncrtc == 0)
> +goto zeroed; /* Monitor disabled */
> +
> +for (j = 0; crtc == NULL && j < x11->randr.outputs[output_index]->ncrtc; 
> j++) {
> +crtc = crtc_from_id(x11, x11->randr.outputs[output_index]->crtcs[j]);
> +}
> +if (!crtc) {
> +// error. stale xrandr info?
> +return false;
> +}
> +
> +mode = mode_from_id(x11, crtc->mode);
> +if (!mode)
> +goto zeroed; /* monitor disabled */
> +
> +*x = crtc->x;
> +*y = crtc->y;
> +*width = mode->width;
> +*height = mode->height;
> +return true;
> +
> +zeroed:
> +*x = 0;
> +*y = 0;
> +*width = 0;
> +*height = 0;
> +return true;
> +}
> +
>  static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
>  {
>  int i, num_of_monitors = 0;
> -XRRModeInfo *mode;
>  XRRScreenResources *res = x11->randr.res;
>  VDAgentMonitorsConfig *mon_config;
>  
>  mon_config = g_malloc0(config_size(res->noutput));
>  
>  for (i = 0 ; i < res->noutput; i++) {
> -int j;
> -XRRCrtcInfo *crtc = NULL;
> -
> -if (x11->randr.outputs[i]->ncrtc == 0)
> -continue; /* Monitor disabled, already zero-ed by calloc */
> -if (x11->randr.outputs[i]->crtc == 0)
> -continue; /* Monitor disabled */
> -
> -for (j = 0; crtc == NULL && j < x11->randr.outputs[i]->ncrtc; j++) {
> -crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[j]);
> -}
> -if (!crtc)
> +int x, y, width, height;
> +if (!get_monitor_info_for_output_index(x11, i, , , , 
> )) {
> +syslog(LOG_WARNING, "Unable to get monitor info for output id 
> %d", i);
>  goto error;
> +}
>  
> -mode = mode_from_id(x11, crtc->mode);
> -if (!mode)
> -continue; /* Monitor disabled, already zero-ed by calloc */
> -
> -mon_config->monitors[i].x  = crtc->x;
> -mon_config->monitors[i].y  = crtc->y;
> -mon_config->monitors[i].width  = mode->width;
> -mon_config->monitors[i].height = mode->height;
> +VDAgentMonConfig *mon = _config->monitors[i];
> +mon->x = x;
> +mon->y = y;
> +mon->width = width;
> +mon->height = height;
>  num_of_monitors = i + 1;
>  }
>  mon_config->num_of_monitors = num_of_monitors;

Reviewed-by: Lukáš Hrázký 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH linux vdagent v4 4/9] Use guest output map to determine xrandr output

[Lukáš Hrázký]

On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> instead of using the spice display id directly as the xrandr output,
> look it up using our new guest output map
> ---
>  src/vdagent/x11-randr.c | 78 -
>  1 file changed, 69 insertions(+), 9 deletions(-)
> 
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 10a1325..5eff225 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -399,6 +399,28 @@ static int xrandr_add_and_set(struct vdagent_x11 *x11, 
> int output_index, int x,
>  return 1;
>  }
>  
> +// Looks up the xrandr output id associated with the given spice display id
> +static RROutput get_xrandr_output_for_display_id(struct vdagent_x11 *x11, 
> int display_id)
> +{
> +guint map_size = g_hash_table_size(x11->guest_output_map);
> +if (map_size == 0) {
> +// we never got a device info message from the server, so just use 
> old
> +// assumptions that the spice display id is equal to the index into 
> the
> +// array of xrandr outputs
> +if (display_id < x11->randr.res->noutput) {
> +return x11->randr.res->outputs[display_id];
> +}
> +} else {
> +gpointer value;
> +if (g_hash_table_lookup_extended(x11->guest_output_map, 
> GINT_TO_POINTER(display_id), NULL, )) {

Line too long.

> +return *(gint64*)value;
> +}
> +}
> +
> +// unable to find a valid output id
> +return -1;
> +}
> +
>  static void xrandr_disable_nth_output(struct vdagent_x11 *x11, int 
> output_index)
>  {
>  Status s;
> @@ -786,6 +808,16 @@ void vdagent_x11_handle_graphics_device_info(struct 
> vdagent_x11 *x11, uint8_t *d
>  }
>  }
>  
> +static int get_output_index_for_display_id(struct vdagent_x11 *x11, int 
> display_id)
> +{
> +RROutput oid = get_xrandr_output_for_display_id(x11, display_id);
> +for (int i = 0; i < x11->randr.res->noutput; i++) {
> +if (oid == x11->randr.res->outputs[i]) {
> +return i;
> +}
> +}
> +return -1;
> +}
>  
>  /*
>   * Set monitor configuration according to client request.
> @@ -862,13 +894,32 @@ void vdagent_x11_set_monitor_config(struct vdagent_x11 
> *x11,
>  g_unlink(config);
>  g_free(config);
>  
> -for (i = mon_config->num_of_monitors; i < x11->randr.res->noutput; i++)
> -xrandr_disable_nth_output(x11, i);
> +// disable all outputs that don't have associated entries in the 
> MonitorConfig
> +for (i = 0; i < x11->randr.res->noutput; i++) {
> +bool disable = true;
> +// check if this xrandr output is represented by an item in 
> mon_config
> +for (int j = 0; j < mon_config->num_of_monitors; j++) {
> +// j represents the display id of an enabled monitor. Check 
> whether
> +// an enabled xrandr output is represented by this id.
> +RROutput oid = get_xrandr_output_for_display_id(x11, j);
> +if (oid == x11->randr.res->outputs[i]) {
> +disable = false;
> +}
> +}
> +if (disable) {
> +xrandr_disable_nth_output(x11, i);
> +}
> +}
>  
> -/* First, disable disabled CRTCs... */
> +/* disable CRTCs that are present but explicitly disabled in the
> + * MonitorConfig */
>  for (i = 0; i < mon_config->num_of_monitors; ++i) {
>  if (!monitor_enabled(_config->monitors[i])) {
> -xrandr_disable_nth_output(x11, i);
> +int output_index = get_output_index_for_display_id(x11, i);
> +if (output_index != -1)
> +xrandr_disable_nth_output(x11, output_index);
> +else
> +syslog(LOG_WARNING, "Unable to find a guest output index for 
> spice display %i", i);

Brackets missing.

>  }
>  }
>  
> @@ -891,7 +942,11 @@ void vdagent_x11_set_monitor_config(struct vdagent_x11 
> *x11,
>  syslog(LOG_DEBUG, "Disabling monitor %d: %dx%d+%d+%d > 
> (%d,%d)",
> i, width, height, x, y, primary_w, primary_h);
>  
> -xrandr_disable_nth_output(x11, i);
> +int output_index = get_output_index_for_display_id(x11, i);
> +if (output_index != -1)
> +xrandr_disable_nth_output(x11, output_index);
> +else
> +syslog(LOG_WARNING, "Unable to find a guest output index for 
> spice display %i", i);

Brackets missing.

>  }
>  }
>  
> @@ -943,11 +998,16 @@ void vdagent_x11_set_monitor_config(struct vdagent_x11 
> *x11,
> i, width, height, x, y);
>  }
>  
> -if (!xrandr_add_and_set(x11, i, x, y, width, height) &&
> +int output_index = get_output_index_for_display_id(x11, i);
> +if (output_index != -1) {
> +if (!xrandr_add_and_set(x11, output_index, x, y, width, height) 
> &&
>  enabled_monitors(mon_config) == 1) {

Re: [Spice-devel] [PATCH spice-gtk 2/2] spice-widget-egl: Simplify gl_make_current reducing indentation

[Marc-André Lureau]

On Fri, Jan 18, 2019 at 4:37 PM Frediano Ziglio  wrote:
>
> Signed-off-by: Frediano Ziglio 

ack

> ---
>  src/spice-widget-egl.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
> index 5ad84bfc..43fccd77 100644
> --- a/src/spice-widget-egl.c
> +++ b/src/spice-widget-egl.c
> @@ -306,14 +306,13 @@ gl_make_current(SpiceDisplay *display, GError **err)
>  "failed to activate context");
>  return FALSE;
>  }
> +return TRUE;
>  }
> -else
>  #endif
> -{
> -GtkWidget *area = gtk_stack_get_child_by_name(d->stack, "gl-area");
>
> -gtk_gl_area_make_current(GTK_GL_AREA(area));
> -}
> +GtkWidget *area = gtk_stack_get_child_by_name(d->stack, "gl-area");
> +
> +gtk_gl_area_make_current(GTK_GL_AREA(area));
>
>  return TRUE;
>  }
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk 1/2] spice-widget-egl: Remove double preprocessor check

[Marc-André Lureau]

On Fri, Jan 18, 2019 at 4:37 PM Frediano Ziglio  wrote:
>
> Do not disable and enable code with same condition.
>
> Signed-off-by: Frediano Ziglio 

ack

> ---
>  src/spice-widget-egl.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
> index 011d8c76..5ad84bfc 100644
> --- a/src/spice-widget-egl.c
> +++ b/src/spice-widget-egl.c
> @@ -307,8 +307,6 @@ gl_make_current(SpiceDisplay *display, GError **err)
>  return FALSE;
>  }
>  }
> -#endif
> -#ifdef GDK_WINDOWING_X11
>  else
>  #endif
>  {
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH linux vdagent v4 2/9] Look up and store xrandr output in display map

[Lukáš Hrázký]

On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> Instead of storing each device address and device display ID in the hash
> table, simply use the lookup_xrandr_output_for_device_info() function to
> look up the ID of the xrandr output and store that in the hash table.
> ---
>  src/vdagent/x11-priv.h  |  2 +-
>  src/vdagent/x11-randr.c | 53 +++--
>  src/vdagent/x11.c   | 20 ++--
>  3 files changed, 32 insertions(+), 43 deletions(-)
> 
> diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
> index 0e954cf..e487aa2 100644
> --- a/src/vdagent/x11-priv.h
> +++ b/src/vdagent/x11-priv.h
> @@ -139,7 +139,7 @@ struct vdagent_x11 {
>  int xrandr_minor;
>  int has_xinerama;
>  int dont_send_guest_xorg_res;
> -GHashTable *graphics_display_infos;
> +GHashTable *guest_output_map;
>  };
>  
>  extern int (*vdagent_x11_prev_error_handler)(Display *, XErrorEvent *);
> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
> index 405fca9..e65d8d0 100644
> --- a/src/vdagent/x11-randr.c
> +++ b/src/vdagent/x11-randr.c
> @@ -31,6 +31,7 @@
>  
>  #include 
>  
> +#include "device-info.h"
>  #include "vdagentd-proto.h"
>  #include "x11.h"
>  #include "x11-priv.h"
> @@ -727,11 +728,6 @@ static void dump_monitors_config(struct vdagent_x11 *x11,
>  }
>  }
>  
> -typedef struct GraphicsDisplayInfo {
> -char device_address[256];
> -uint32_t device_display_id;
> -} GraphicsDisplayInfo;
> -
>  // handle the device info message from the server. This will allow us to
>  // maintain a mapping from spice display id to xrandr output
>  void vdagent_x11_handle_graphics_device_info(struct vdagent_x11 *x11, 
> uint8_t *data, size_t size)
> @@ -752,39 +748,38 @@ void vdagent_x11_handle_graphics_device_info(struct 
> vdagent_x11 *x11, uint8_t *d
>  break;
>  }
>  
> -GraphicsDisplayInfo *value = g_malloc(sizeof(GraphicsDisplayInfo));
> -value->device_address[0] = '\0';
> -
> -size_t device_address_len = device_display_info->device_address_len;
> -if (device_address_len > sizeof(value->device_address)) {
> -syslog(LOG_ERR, "Received a device address longer than %lu, "
> -   "will be truncated!", device_address_len);
> -device_address_len = sizeof(value->device_address);
> -}
> -
> -strncpy(value->device_address,
> -(char*) device_display_info->device_address,
> -device_address_len);
> -
> -if (device_address_len > 0) {
> -value->device_address[device_address_len - 1] = '\0';  // make 
> sure the string is terminated
> +// make sure the string is terminated:
> +if (device_display_info->device_address_len > 0) {
> +
> device_display_info->device_address[device_display_info->device_address_len - 
> 1] = '\0';
>  } else {
>  syslog(LOG_WARNING, "Zero length device_address received for 
> channel_id: %u, monitor_id: %u",
> device_display_info->channel_id, 
> device_display_info->monitor_id);
>  }
>  
> -value->device_display_id = device_display_info->device_display_id;
> +RROutput x_output;
> +if (lookup_xrandr_output_for_device_info(device_display_info, 
> x11->display, x11->randr.res, _output)) {
> +gint64 *value = g_new(gint64, 1);
> +*value = x_output;
>  
> -syslog(LOG_INFO, "   channel_id: %u monitor_id: %u device_address: 
> %s, "
> -   "device_display_id: %u",
> -   device_display_info->channel_id,
> -   device_display_info->monitor_id,
> -   value->device_address,
> -   value->device_display_id);
> +syslog(LOG_INFO, "channel_id: %u monitor_id: %u device_address: 
> %s, "
> +   "device_display_id: %u xrandr output ID: %lu",
> +   device_display_info->channel_id,
> +   device_display_info->monitor_id,
> +   device_display_info->device_address,
> +   device_display_info->device_display_id,
> +   x_output);

Since you've dropped the indent, I'd prefix the logline with some
context, like "Adding draphics device info ..." or something.

> -g_hash_table_insert(x11->graphics_display_infos,
> +g_hash_table_insert(x11->guest_output_map,
>  GUINT_TO_POINTER(device_display_info->channel_id + 
> device_display_info->monitor_id),
>  value);
> +} else {
> +syslog(LOG_INFO, "channel_id: %u monitor_id: %u device_address: 
> %s, "
> +   "device_display_id: %u xrandr output ID NOT FOUND",
> +   device_display_info->channel_id,
> +   device_display_info->monitor_id,
> +   device_display_info->device_address,
> +   

[Spice-devel] [PATCH spice-gtk 2/2] spice-widget-egl: Simplify gl_make_current reducing indentation

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 src/spice-widget-egl.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
index 5ad84bfc..43fccd77 100644
--- a/src/spice-widget-egl.c
+++ b/src/spice-widget-egl.c
@@ -306,14 +306,13 @@ gl_make_current(SpiceDisplay *display, GError **err)
 "failed to activate context");
 return FALSE;
 }
+return TRUE;
 }
-else
 #endif
-{
-GtkWidget *area = gtk_stack_get_child_by_name(d->stack, "gl-area");
 
-gtk_gl_area_make_current(GTK_GL_AREA(area));
-}
+GtkWidget *area = gtk_stack_get_child_by_name(d->stack, "gl-area");
+
+gtk_gl_area_make_current(GTK_GL_AREA(area));
 
 return TRUE;
 }
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-gtk 1/2] spice-widget-egl: Remove double preprocessor check

[Frediano Ziglio]

Do not disable and enable code with same condition.

Signed-off-by: Frediano Ziglio 
---
 src/spice-widget-egl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
index 011d8c76..5ad84bfc 100644
--- a/src/spice-widget-egl.c
+++ b/src/spice-widget-egl.c
@@ -307,8 +307,6 @@ gl_make_current(SpiceDisplay *display, GError **err)
 return FALSE;
 }
 }
-#endif
-#ifdef GDK_WINDOWING_X11
 else
 #endif
 {
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH linux vdagent v4 1/9] Add lookup_xrand_output_for_device_info()

[Lukáš Hrázký]

On Thu, 2019-01-17 at 16:14 -0600, Jonathon Jongsma wrote:
> Add a function to look up an xrandr output for a given device display
> id. This uses sysfs and the drm subsystem to lookup information about a
> graphics device output. It then compares the drm output name to xrandr
> output names to try to match that device output to an xrandr output.
> This is necesary for guests that have multiple graphics devices.
> ---
>  Makefile.am   |  14 ++
>  configure.ac  |   1 +
>  src/vdagent/device-info.c | 499 ++
>  src/vdagent/device-info.h |  27 +++
>  tests/test-device-info.c  | 262 
>  5 files changed, 803 insertions(+)
>  create mode 100644 src/vdagent/device-info.c
>  create mode 100644 src/vdagent/device-info.h
>  create mode 100644 tests/test-device-info.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 97b8bf0..c2e9668 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -14,6 +14,7 @@ common_sources =\
>   $(NULL)
>  
>  src_spice_vdagent_CFLAGS =   \
> + $(DRM_CFLAGS)   \
>   $(X_CFLAGS) \
>   $(SPICE_CFLAGS) \
>   $(GLIB2_CFLAGS) \
> @@ -24,6 +25,7 @@ src_spice_vdagent_CFLAGS =  \
>   $(NULL)
>  
>  src_spice_vdagent_LDADD =\
> + $(DRM_LIBS) \
>   $(X_LIBS)   \
>   $(SPICE_LIBS)   \
>   $(GLIB2_LIBS)   \
> @@ -37,6 +39,8 @@ src_spice_vdagent_SOURCES = \
>   src/vdagent/audio.h \
>   src/vdagent/clipboard.c \
>   src/vdagent/clipboard.h \
> + src/vdagent/device-info.c   \
> + src/vdagent/device-info.h   \
>   src/vdagent/file-xfers.c\
>   src/vdagent/file-xfers.h\
>   src/vdagent/x11-priv.h  \
> @@ -159,3 +163,13 @@ EXTRA_DIST = \
>  DISTCHECK_CONFIGURE_FLAGS =  \
>   --with-init-script=redhat   \
>   $(NULL)
> +
> +tests_test_device_info_LDADD = $(src_spice_vdagent_LDADD)
> +tests_test_device_info_CFLAGS = $(src_spice_vdagent_CFLAGS)
> +
> +check_PROGRAMS = \
> + tests/test-device-info  \
> + $(NULL)
> +
> +TESTS = $(check_PROGRAMS)
> +
> diff --git a/configure.ac b/configure.ac
> index 7cb44db..55b031e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -105,6 +105,7 @@ PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
>  PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.13])
>  PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
>  PKG_CHECK_MODULES([DBUS], [dbus-1])
> +PKG_CHECK_MODULES([DRM], [libdrm])
>  
>  if test "$with_session_info" = "auto" || test "$with_session_info" = 
> "systemd"; then
>  PKG_CHECK_MODULES([LIBSYSTEMD_LOGIN],
> diff --git a/src/vdagent/device-info.c b/src/vdagent/device-info.c
> new file mode 100644
> index 000..09ba579
> --- /dev/null
> +++ b/src/vdagent/device-info.c
> @@ -0,0 +1,499 @@
> +/*  device-info.c utility function for looking up the xrandr output id for a
> + *  given device address and display id
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * Red Hat Authors:
> + * Jonathon Jongsma 
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "device-info.h"
> +
> +#define PCI_VENDOR_ID_REDHAT 0x1b36
> +#define PCI_VENDOR_ID_REDHAT_QUMRANET 0x1af4 // virtio-gpu
> +#define PCI_VENDOR_ID_INTEL 0x8086
> +#define PCI_VENDOR_ID_NVIDIA 0x10de
> +
> +#define PCI_DEVICE_ID_QXL 0x0100
> +#define PCI_DEVICE_ID_VIRTIO_GPU 0x1050
> +
> +typedef struct PciDevice {
> +int domain;
> +uint8_t bus;
> +uint8_t slot;
> +uint8_t function;
> +} PciDevice;
> +
> +typedef struct PciAddress {
> +int domain;
> +GList *devices; /* PciDevice */
> +} PciAddress;
> +
> +static PciAddress* pci_address_new()
> +{
> +return g_new0(PciAddress, 

[Spice-devel] [PATCH v3 01/23] drm/qxl: drop ttm_mem_reg arg from qxl_hw_surface_alloc()

[Gerd Hoffmann]

Not used, is always NULL.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h|  3 +--
 drivers/gpu/drm/qxl/qxl_cmd.c| 14 ++
 drivers/gpu/drm/qxl/qxl_object.c |  2 +-
 3 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 13a0254b59..38c5a8b1df 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -497,8 +497,7 @@ int qxl_surface_id_alloc(struct qxl_device *qdev,
 void qxl_surface_id_dealloc(struct qxl_device *qdev,
uint32_t surface_id);
 int qxl_hw_surface_alloc(struct qxl_device *qdev,
-struct qxl_bo *surf,
-struct ttm_mem_reg *mem);
+struct qxl_bo *surf);
 int qxl_hw_surface_dealloc(struct qxl_device *qdev,
   struct qxl_bo *surf);
 
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 2e100f6442..5ba831c78c 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -460,8 +460,7 @@ void qxl_surface_id_dealloc(struct qxl_device *qdev,
 }
 
 int qxl_hw_surface_alloc(struct qxl_device *qdev,
-struct qxl_bo *surf,
-struct ttm_mem_reg *new_mem)
+struct qxl_bo *surf)
 {
struct qxl_surface_cmd *cmd;
struct qxl_release *release;
@@ -487,16 +486,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
cmd->u.surface_create.width = surf->surf.width;
cmd->u.surface_create.height = surf->surf.height;
cmd->u.surface_create.stride = surf->surf.stride;
-   if (new_mem) {
-   int slot_id = surf->type == QXL_GEM_DOMAIN_VRAM ? 
qdev->main_mem_slot : qdev->surfaces_mem_slot;
-   struct qxl_memslot *slot = &(qdev->mem_slots[slot_id]);
-
-   /* TODO - need to hold one of the locks to read tbo.offset */
-   cmd->u.surface_create.data = slot->high_bits;
-
-   cmd->u.surface_create.data |= (new_mem->start << PAGE_SHIFT) + 
surf->tbo.bdev->man[new_mem->mem_type].gpu_offset;
-   } else
-   cmd->u.surface_create.data = qxl_bo_physical_address(qdev, 
surf, 0);
+   cmd->u.surface_create.data = qxl_bo_physical_address(qdev, surf, 0);
cmd->surface_id = surf->surface_id;
qxl_release_unmap(qdev, release, >release_info);
 
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 91f3bbc73e..34eff8b21e 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -332,7 +332,7 @@ int qxl_bo_check_id(struct qxl_device *qdev, struct qxl_bo 
*bo)
if (ret)
return ret;
 
-   ret = qxl_hw_surface_alloc(qdev, bo, NULL);
+   ret = qxl_hw_surface_alloc(qdev, bo);
if (ret)
return ret;
}
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 14/23] drm/qxl: cover all crtcs in shadow bo.

[Gerd Hoffmann]

The qxl device supports only a single active framebuffer ("primary
surface" in spice terminology).  In multihead configurations are handled
by defining rectangles within the primary surface for each head/crtc.

Userspace which uses the qxl ioctl interface (xorg qxl driver) is aware
of this limitation and will setup framebuffers and crtcs accordingly.

Userspace which uses dumb framebuffers (xorg modesetting driver,
wayland) is not aware of this limitation and tries to use two
framebuffers (one for each crtc) instead.

The qxl kms driver already has the dumb bo separated from the primary
surface, by using a (shared) shadow bo as primary surface.  This is
needed to support pageflips without having to re-create the primary
surface.  The qxl driver will blit from the dumb bo to the shadow bo
instead.

So we can extend the shadow logic:  Maintain a global shadow bo (aka
primary surface), make it big enough that dumb bo's for all crtcs fit in
side-by-side.  Adjust the pageflip blits to place the heads next to each
other in the shadow.

With this patch in place multihead qxl works with wayland.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h |   5 +-
 drivers/gpu/drm/qxl/qxl_display.c | 119 +-
 drivers/gpu/drm/qxl/qxl_draw.c|   9 ++-
 3 files changed, 104 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 150b1a4f66..43c6df9cf9 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -230,6 +230,8 @@ struct qxl_device {
struct qxl_ram_header *ram_header;
 
struct qxl_bo *primary_bo;
+   struct qxl_bo *dumb_shadow_bo;
+   struct qxl_head *dumb_heads;
 
struct qxl_memslot main_slot;
struct qxl_memslot surfaces_slot;
@@ -437,7 +439,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
   struct qxl_bo *bo,
   unsigned int flags, unsigned int color,
   struct drm_clip_rect *clips,
-  unsigned int num_clips, int inc);
+  unsigned int num_clips, int inc,
+  uint32_t dumb_shadow_offset);
 
 void qxl_draw_fill(struct qxl_draw_fill *qxl_draw_fill_rec);
 
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index ff13bc6a4a..d9de43e5fd 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -323,6 +323,8 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc 
*crtc,
head.y = crtc->y;
if (qdev->monitors_config->count < i + 1)
qdev->monitors_config->count = i + 1;
+   if (qdev->primary_bo == qdev->dumb_shadow_bo)
+   head.x += qdev->dumb_heads[i].x;
} else if (i > 0) {
head.width = 0;
head.height = 0;
@@ -426,7 +428,7 @@ static int qxl_framebuffer_surface_dirty(struct 
drm_framebuffer *fb,
}
 
qxl_draw_dirty_fb(qdev, fb, qobj, flags, color,
- clips, num_clips, inc);
+ clips, num_clips, inc, 0);
 
drm_modeset_unlock_all(fb->dev);
 
@@ -535,6 +537,7 @@ static void qxl_primary_atomic_update(struct drm_plane 
*plane,
.x2 = plane->state->fb->width,
.y2 = plane->state->fb->height
};
+   uint32_t dumb_shadow_offset = 0;
 
if (old_state->fb) {
bo_old = gem_to_qxl_bo(old_state->fb->obj[0]);
@@ -551,7 +554,12 @@ static void qxl_primary_atomic_update(struct drm_plane 
*plane,
qxl_primary_apply_cursor(plane);
}
 
-   qxl_draw_dirty_fb(qdev, plane->state->fb, bo, 0, 0, , 1, 1);
+   if (bo->is_dumb)
+   dumb_shadow_offset =
+   qdev->dumb_heads[plane->state->crtc->index].x;
+
+   qxl_draw_dirty_fb(qdev, plane->state->fb, bo, 0, 0, , 1, 1,
+ dumb_shadow_offset);
 }
 
 static void qxl_primary_atomic_disable(struct drm_plane *plane,
@@ -707,12 +715,68 @@ static void qxl_cursor_atomic_disable(struct drm_plane 
*plane,
qxl_release_fence_buffer_objects(release);
 }
 
+static void qxl_update_dumb_head(struct qxl_device *qdev,
+int index, struct qxl_bo *bo)
+{
+   uint32_t width, height;
+
+   if (index >= qdev->monitors_config->max_allowed)
+   return;
+
+   if (bo && bo->is_dumb) {
+   width = bo->surf.width;
+   height = bo->surf.height;
+   } else {
+   width = 0;
+   height = 0;
+   }
+
+   if (qdev->dumb_heads[index].width == width &&
+   qdev->dumb_heads[index].height == height)
+   return;
+
+   DRM_DEBUG("#%d: %dx%d -> %dx%d\n", index,
+ qdev->dumb_heads[index].width,
+ qdev->dumb_heads[index].height,
+ width, height);
+  

[Spice-devel] [PATCH v3 02/23] drm/qxl: drop unused qxl_fb_virtual_address

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 38c5a8b1df..7eabf4a9ed 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -308,13 +308,6 @@ void qxl_ring_free(struct qxl_ring *ring);
 void qxl_ring_init_hdr(struct qxl_ring *ring);
 int qxl_check_idle(struct qxl_ring *ring);
 
-static inline void *
-qxl_fb_virtual_address(struct qxl_device *qdev, unsigned long physical)
-{
-   DRM_DEBUG_DRIVER("not implemented (%lu)\n", physical);
-   return 0;
-}
-
 static inline uint64_t
 qxl_bo_physical_address(struct qxl_device *qdev, struct qxl_bo *bo,
unsigned long offset)
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 19/23] drm/qxl: implement qxl_gem_prime_(un)pin

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_prime.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
index 708378844c..22e1faf047 100644
--- a/drivers/gpu/drm/qxl/qxl_prime.c
+++ b/drivers/gpu/drm/qxl/qxl_prime.c
@@ -22,6 +22,7 @@
  * Authors: Andreas Pokorny
  */
 
+#include "qxl_drv.h"
 #include "qxl_object.h"
 
 /* Empty Implementations as there should not be any other driver for a virtual
@@ -29,13 +30,16 @@
 
 int qxl_gem_prime_pin(struct drm_gem_object *obj)
 {
-   WARN_ONCE(1, "not implemented");
-   return -ENOSYS;
+   struct qxl_bo *bo = gem_to_qxl_bo(obj);
+
+   return qxl_bo_pin(bo);
 }
 
 void qxl_gem_prime_unpin(struct drm_gem_object *obj)
 {
-   WARN_ONCE(1, "not implemented");
+   struct qxl_bo *bo = gem_to_qxl_bo(obj);
+
+   qxl_bo_unpin(bo);
 }
 
 struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj)
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 22/23] drm/qxl: use kernel mode db

[Gerd Hoffmann]

Add all standard modes from the kernel's video mode data base.
Keep a few non-standard modes in the qxl mode list.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 27 +++
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 926fcb49b2..df768b0c83 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -262,34 +262,20 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector)
 static struct mode_size {
int w;
int h;
-} common_modes[] = {
-   { 640,  480},
+} extra_modes[] = {
{ 720,  480},
-   { 800,  600},
-   { 848,  480},
-   {1024,  768},
{1152,  768},
-   {1280,  720},
-   {1280,  800},
{1280,  854},
-   {1280,  960},
-   {1280, 1024},
-   {1440,  900},
-   {1400, 1050},
-   {1680, 1050},
-   {1600, 1200},
-   {1920, 1080},
-   {1920, 1200}
 };
 
-static int qxl_add_common_modes(struct drm_connector *connector)
+static int qxl_add_extra_modes(struct drm_connector *connector)
 {
int i, ret = 0;
 
-   for (i = 0; i < ARRAY_SIZE(common_modes); i++)
+   for (i = 0; i < ARRAY_SIZE(extra_modes); i++)
ret += qxl_add_mode(connector,
-   common_modes[i].w,
-   common_modes[i].h,
+   extra_modes[i].w,
+   extra_modes[i].h,
false);
return ret;
 }
@@ -1010,7 +996,8 @@ static int qxl_conn_get_modes(struct drm_connector 
*connector)
pheight = head->height;
}
 
-   ret += qxl_add_common_modes(connector);
+   ret += drm_add_modes_noedid(connector, 8192, 8192);
+   ret += qxl_add_extra_modes(connector);
ret += qxl_add_monitors_config_modes(connector);
drm_set_preferred_mode(connector, pwidth, pheight);
return ret;
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 21/23] drm/qxl: add qxl_add_mode helper function

[Gerd Hoffmann]

Add a helper function to add custom video modes to a connector.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 84 +++
 1 file changed, 49 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index fed2ea018d..926fcb49b2 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -212,15 +212,36 @@ static int qxl_check_framebuffer(struct qxl_device *qdev,
return qxl_check_mode(qdev, bo->surf.width, bo->surf.height);
 }
 
-static int qxl_add_monitors_config_modes(struct drm_connector *connector,
- unsigned *pwidth,
- unsigned *pheight)
+static int qxl_add_mode(struct drm_connector *connector,
+   unsigned int width,
+   unsigned int height,
+   bool preferred)
+{
+   struct drm_device *dev = connector->dev;
+   struct qxl_device *qdev = dev->dev_private;
+   struct drm_display_mode *mode = NULL;
+   int rc;
+
+   rc = qxl_check_mode(qdev, width, height);
+   if (rc != 0)
+   return 0;
+
+   mode = drm_cvt_mode(dev, width, height, 60, false, false, false);
+   if (preferred)
+   mode->type |= DRM_MODE_TYPE_PREFERRED;
+   mode->hdisplay = width;
+   mode->vdisplay = height;
+   drm_mode_set_name(mode);
+   drm_mode_probed_add(connector, mode);
+   return 1;
+}
+
+static int qxl_add_monitors_config_modes(struct drm_connector *connector)
 {
struct drm_device *dev = connector->dev;
struct qxl_device *qdev = dev->dev_private;
struct qxl_output *output = drm_connector_to_qxl_output(connector);
int h = output->index;
-   struct drm_display_mode *mode = NULL;
struct qxl_head *head;
 
if (!qdev->monitors_config)
@@ -235,19 +256,7 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
head = >client_monitors_config->heads[h];
DRM_DEBUG_KMS("head %d is %dx%d\n", h, head->width, head->height);
 
-   mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
-   false);
-   mode->type |= DRM_MODE_TYPE_PREFERRED;
-   mode->hdisplay = head->width;
-   mode->vdisplay = head->height;
-   drm_mode_set_name(mode);
-   *pwidth = head->width;
-   *pheight = head->height;
-   drm_mode_probed_add(connector, mode);
-   /* remember the last custom size for mode validation */
-   qdev->monitors_config_width = mode->hdisplay;
-   qdev->monitors_config_height = mode->vdisplay;
-   return 1;
+   return qxl_add_mode(connector, head->width, head->height, true);
 }
 
 static struct mode_size {
@@ -273,22 +282,16 @@ static struct mode_size {
{1920, 1200}
 };
 
-static int qxl_add_common_modes(struct drm_connector *connector,
-unsigned int pwidth,
-unsigned int pheight)
+static int qxl_add_common_modes(struct drm_connector *connector)
 {
-   struct drm_device *dev = connector->dev;
-   struct drm_display_mode *mode = NULL;
-   int i;
+   int i, ret = 0;
 
-   for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
-   mode = drm_cvt_mode(dev, common_modes[i].w, common_modes[i].h,
-   60, false, false, false);
-   if (common_modes[i].w == pwidth && common_modes[i].h == pheight)
-   mode->type |= DRM_MODE_TYPE_PREFERRED;
-   drm_mode_probed_add(connector, mode);
-   }
-   return i - 1;
+   for (i = 0; i < ARRAY_SIZE(common_modes); i++)
+   ret += qxl_add_mode(connector,
+   common_modes[i].w,
+   common_modes[i].h,
+   false);
+   return ret;
 }
 
 static void qxl_send_monitors_config(struct qxl_device *qdev)
@@ -991,14 +994,25 @@ static int qdev_crtc_init(struct drm_device *dev, int 
crtc_id)
 
 static int qxl_conn_get_modes(struct drm_connector *connector)
 {
+   struct drm_device *dev = connector->dev;
+   struct qxl_device *qdev = dev->dev_private;
+   struct qxl_output *output = drm_connector_to_qxl_output(connector);
unsigned int pwidth = 1024;
unsigned int pheight = 768;
int ret = 0;
 
-   ret = qxl_add_monitors_config_modes(connector, , );
-   if (ret < 0)
-   return ret;
-   ret += qxl_add_common_modes(connector, pwidth, pheight);
+   if (qdev->client_monitors_config) {
+   struct qxl_head *head;
+   head = >client_monitors_config->heads[output->index];
+   if (head->width)
+   pwidth = head->width;
+   if (head->height)
+   pheight = head->height;

[Spice-devel] [PATCH v3 05/23] drm/qxl: drop unused fields from struct qxl_device

[Gerd Hoffmann]

slot_id_bits and slot_gen_bits can be read directly from qxlrom instead.
va_slot_mask is never used anywhere.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h |  3 ---
 drivers/gpu/drm/qxl/qxl_kms.c | 10 ++
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index d015d4fff1..3ebe66abf2 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -232,9 +232,6 @@ struct qxl_device {
 
struct qxl_memslot main_slot;
struct qxl_memslot surfaces_slot;
-   uint8_t slot_id_bits;
-   uint8_t slot_gen_bits;
-   uint64_tva_slot_mask;
 
spinlock_t  release_lock;
struct idr  release_idr;
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index a9288100ae..3c1753667d 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -78,9 +78,9 @@ static void setup_slot(struct qxl_device *qdev,
 
slot->generation = qdev->rom->slot_generation;
high_bits = (qdev->rom->slots_start + slot->index)
-   << qdev->slot_gen_bits;
+   << qdev->rom->slot_gen_bits;
high_bits |= slot->generation;
-   high_bits <<= (64 - (qdev->slot_gen_bits + qdev->slot_id_bits));
+   high_bits <<= (64 - (qdev->rom->slot_gen_bits + 
qdev->rom->slot_id_bits));
slot->high_bits = high_bits;
 
DRM_INFO("slot %d (%s): base 0x%08lx, size 0x%08lx\n",
@@ -235,12 +235,6 @@ int qxl_device_init(struct qxl_device *qdev,
r = -ENOMEM;
goto cursor_ring_free;
}
-   /* TODO - slot initialization should happen on reset. where is our
-* reset handler? */
-   qdev->slot_gen_bits = qdev->rom->slot_gen_bits;
-   qdev->slot_id_bits = qdev->rom->slot_id_bits;
-   qdev->va_slot_mask =
-   (~(uint64_t)0) >> (qdev->slot_id_bits + qdev->slot_gen_bits);
 
idr_init(>release_idr);
spin_lock_init(>release_idr_lock);
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 07/23] drm/qxl: allow both PRIV and VRAM placement for QXL_GEM_DOMAIN_SURFACE

[Gerd Hoffmann]

qxl surfaces (used for framebuffers and gem objects) can live in both
VRAM and PRIV ttm domains.  Update placement setup to include both.
Put PRIV first in the list so it is preferred, so VRAM will have more
room for objects which must be allocated there.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_object.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 34eff8b21e..024c8dd317 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -60,8 +60,10 @@ void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 
domain, bool pinned)
qbo->placement.busy_placement = qbo->placements;
if (domain == QXL_GEM_DOMAIN_VRAM)
qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_VRAM | pflag;
-   if (domain == QXL_GEM_DOMAIN_SURFACE)
+   if (domain == QXL_GEM_DOMAIN_SURFACE) {
qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_PRIV | pflag;
+   qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | 
TTM_PL_FLAG_VRAM | pflag;
+   }
if (domain == QXL_GEM_DOMAIN_CPU)
qbo->placements[c++].flags = TTM_PL_MASK_CACHING | 
TTM_PL_FLAG_SYSTEM | pflag;
if (!c)
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 10/23] drm/qxl: move qxl_primary_apply_cursor to correct place

[Gerd Hoffmann]

The cursor must be set again after creating the primary surface.
Also drop the error message.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 86bfc19bea..1b700ef503 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -533,7 +533,6 @@ static void qxl_primary_atomic_update(struct drm_plane 
*plane,
.x2 = plane->state->fb->width,
.y2 = plane->state->fb->height
};
-   int ret;
bool same_shadow = false;
 
if (old_state->fb) {
@@ -554,16 +553,13 @@ static void qxl_primary_atomic_update(struct drm_plane 
*plane,
if (!same_shadow)
qxl_io_destroy_primary(qdev);
bo_old->is_primary = false;
-
-   ret = qxl_primary_apply_cursor(plane);
-   if (ret)
-   DRM_ERROR(
-   "could not set cursor after creating primary");
}
 
if (!bo->is_primary) {
-   if (!same_shadow)
+   if (!same_shadow) {
qxl_io_create_primary(qdev, 0, bo);
+   qxl_primary_apply_cursor(plane);
+   }
bo->is_primary = true;
}
 
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 08/23] drm/qxl: use QXL_GEM_DOMAIN_SURFACE for shadow bo.

[Gerd Hoffmann]

The shadow bo is used as qxl surface, so allocate it as
QXL_GEM_DOMAIN_SURFACE.  Should usually be allocated in
PRIV ttm domain then, so this reduces VRAM memory pressure.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 1f8fddcc34..86bfc19bea 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -758,7 +758,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
user_bo->shadow = old_bo->shadow;
} else {
qxl_bo_create(qdev, user_bo->gem_base.size,
- true, true, QXL_GEM_DOMAIN_VRAM, NULL,
+ true, true, QXL_GEM_DOMAIN_SURFACE, NULL,
  _bo->shadow);
}
}
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 17/23] drm/qxl: use generic fbdev emulation

[Gerd Hoffmann]

Switch qxl over to the new generic fbdev emulation.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 7 ---
 drivers/gpu/drm/qxl/qxl_drv.c | 2 ++
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index ef832f98ab..9c751f01e3 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1221,18 +1221,11 @@ int qxl_modeset_init(struct qxl_device *qdev)
qxl_display_read_client_monitors_config(qdev);
 
drm_mode_config_reset(>ddev);
-
-   /* primary surface must be created by this point, to allow
-* issuing command queue commands and having them read by
-* spice server. */
-   qxl_fbdev_init(qdev);
return 0;
 }
 
 void qxl_modeset_fini(struct qxl_device *qdev)
 {
-   qxl_fbdev_fini(qdev);
-
qxl_destroy_monitors_object(qdev);
drm_mode_config_cleanup(>ddev);
 }
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 13c8a662f9..3fce7d16df 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -93,6 +93,8 @@ qxl_pci_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
if (ret)
goto modeset_cleanup;
 
+   drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
+   drm_fbdev_generic_setup(>ddev, 32);
return 0;
 
 modeset_cleanup:
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 16/23] drm/qxl: implement prime kmap/kunmap

[Gerd Hoffmann]

Generic fbdev emulation needs this.  Also: We must keep track of the
number of mappings now, so we don't unmap early in case two users want a
kmap of the same bo.  Add a sanity check to destroy callback to make
sure kmap/kunmap is balanced.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h|  1 +
 drivers/gpu/drm/qxl/qxl_object.c |  6 ++
 drivers/gpu/drm/qxl/qxl_prime.c  | 17 +
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 43c6df9cf9..8c3af1cdbe 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -84,6 +84,7 @@ struct qxl_bo {
struct ttm_bo_kmap_obj  kmap;
unsigned int pin_count;
void*kptr;
+   unsigned intmap_count;
int type;
 
/* Constant after initialization */
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 024c8dd317..4928fa6029 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -36,6 +36,7 @@ static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo)
qdev = (struct qxl_device *)bo->gem_base.dev->dev_private;
 
qxl_surface_evict(qdev, bo, false);
+   WARN_ON_ONCE(bo->map_count > 0);
mutex_lock(>gem.mutex);
list_del_init(>list);
mutex_unlock(>gem.mutex);
@@ -131,6 +132,7 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
if (bo->kptr) {
if (ptr)
*ptr = bo->kptr;
+   bo->map_count++;
return 0;
}
r = ttm_bo_kmap(>tbo, 0, bo->tbo.num_pages, >kmap);
@@ -139,6 +141,7 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
bo->kptr = ttm_kmap_obj_virtual(>kmap, _iomem);
if (ptr)
*ptr = bo->kptr;
+   bo->map_count = 1;
return 0;
 }
 
@@ -180,6 +183,9 @@ void qxl_bo_kunmap(struct qxl_bo *bo)
 {
if (bo->kptr == NULL)
return;
+   bo->map_count--;
+   if (bo->map_count > 0)
+   return;
bo->kptr = NULL;
ttm_bo_kunmap(>kmap);
 }
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
index a55dece118..708378844c 100644
--- a/drivers/gpu/drm/qxl/qxl_prime.c
+++ b/drivers/gpu/drm/qxl/qxl_prime.c
@@ -22,7 +22,7 @@
  * Authors: Andreas Pokorny
  */
 
-#include "qxl_drv.h"
+#include "qxl_object.h"
 
 /* Empty Implementations as there should not be any other driver for a virtual
  * device that might share buffers with qxl */
@@ -54,13 +54,22 @@ struct drm_gem_object *qxl_gem_prime_import_sg_table(
 
 void *qxl_gem_prime_vmap(struct drm_gem_object *obj)
 {
-   WARN_ONCE(1, "not implemented");
-   return ERR_PTR(-ENOSYS);
+   struct qxl_bo *bo = gem_to_qxl_bo(obj);
+   void *ptr;
+   int ret;
+
+   ret = qxl_bo_kmap(bo, );
+   if (ret < 0)
+   return ERR_PTR(ret);
+
+   return ptr;
 }
 
 void qxl_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
 {
-   WARN_ONCE(1, "not implemented");
+   struct qxl_bo *bo = gem_to_qxl_bo(obj);
+
+   qxl_bo_kunmap(bo);
 }
 
 int qxl_gem_prime_mmap(struct drm_gem_object *obj,
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 13/23] drm/qxl: use shadow bo directly

[Gerd Hoffmann]

Pass the shadow bo to qxl_io_create_primary() instead of expecting
qxl_io_create_primary to check bo->shadow.  Set is_primary flag on the
shadow bo.  Move the is_primary tracking into qxl_io_create_primary()
and qxl_io_destroy_primary() functions.

That simplifies primary surface tracking and the workflow in
qxl_primary_atomic_update().

Signed-off-by: Gerd Hoffmann 

qxl_io_create/destroy_primary: primary_bo tracking [fixup]
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 10 +-
 drivers/gpu/drm/qxl/qxl_display.c | 33 +++--
 2 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 8e64127259..0a2e51af12 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -374,6 +374,8 @@ void qxl_io_flush_surfaces(struct qxl_device *qdev)
 void qxl_io_destroy_primary(struct qxl_device *qdev)
 {
wait_for_io_cmd(qdev, 0, QXL_IO_DESTROY_PRIMARY_ASYNC);
+   qdev->primary_bo->is_primary = false;
+   drm_gem_object_put_unlocked(>primary_bo->gem_base);
qdev->primary_bo = NULL;
 }
 
@@ -390,11 +392,7 @@ void qxl_io_create_primary(struct qxl_device *qdev, struct 
qxl_bo *bo)
create->width = bo->surf.width;
create->height = bo->surf.height;
create->stride = bo->surf.stride;
-   if (bo->shadow) {
-   create->mem = qxl_bo_physical_address(qdev, bo->shadow, 0);
-   } else {
-   create->mem = qxl_bo_physical_address(qdev, bo, 0);
-   }
+   create->mem = qxl_bo_physical_address(qdev, bo, 0);
 
DRM_DEBUG_DRIVER("mem = %llx, from %p\n", create->mem, bo->kptr);
 
@@ -403,6 +401,8 @@ void qxl_io_create_primary(struct qxl_device *qdev, struct 
qxl_bo *bo)
 
wait_for_io_cmd(qdev, 0, QXL_IO_CREATE_PRIMARY_ASYNC);
qdev->primary_bo = bo;
+   qdev->primary_bo->is_primary = true;
+   drm_gem_object_get(>primary_bo->gem_base);
 }
 
 void qxl_io_memslot_add(struct qxl_device *qdev, uint8_t id)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index d3215eac9b..ff13bc6a4a 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -401,13 +401,15 @@ static int qxl_framebuffer_surface_dirty(struct 
drm_framebuffer *fb,
struct qxl_device *qdev = fb->dev->dev_private;
struct drm_clip_rect norect;
struct qxl_bo *qobj;
+   bool is_primary;
int inc = 1;
 
drm_modeset_lock_all(fb->dev);
 
qobj = gem_to_qxl_bo(fb->obj[0]);
/* if we aren't primary surface ignore this */
-   if (!qobj->is_primary) {
+   is_primary = qobj->shadow ? qobj->shadow->is_primary : qobj->is_primary;
+   if (!is_primary) {
drm_modeset_unlock_all(fb->dev);
return 0;
}
@@ -526,14 +528,13 @@ static void qxl_primary_atomic_update(struct drm_plane 
*plane,
 {
struct qxl_device *qdev = plane->dev->dev_private;
struct qxl_bo *bo = gem_to_qxl_bo(plane->state->fb->obj[0]);
-   struct qxl_bo *bo_old;
+   struct qxl_bo *bo_old, *primary;
struct drm_clip_rect norect = {
.x1 = 0,
.y1 = 0,
.x2 = plane->state->fb->width,
.y2 = plane->state->fb->height
};
-   bool same_shadow = false;
 
if (old_state->fb) {
bo_old = gem_to_qxl_bo(old_state->fb->obj[0]);
@@ -541,26 +542,13 @@ static void qxl_primary_atomic_update(struct drm_plane 
*plane,
bo_old = NULL;
}
 
-   if (bo == bo_old)
-   return;
+   primary = bo->shadow ? bo->shadow : bo;
 
-   if (bo_old && bo_old->shadow && bo->shadow &&
-   bo_old->shadow == bo->shadow) {
-   same_shadow = true;
-   }
-
-   if (bo_old && bo_old->is_primary) {
-   if (!same_shadow)
+   if (!primary->is_primary) {
+   if (qdev->primary_bo)
qxl_io_destroy_primary(qdev);
-   bo_old->is_primary = false;
-   }
-
-   if (!bo->is_primary) {
-   if (!same_shadow) {
-   qxl_io_create_primary(qdev, bo);
-   qxl_primary_apply_cursor(plane);
-   }
-   bo->is_primary = true;
+   qxl_io_create_primary(qdev, primary);
+   qxl_primary_apply_cursor(plane);
}
 
qxl_draw_dirty_fb(qdev, plane->state->fb, bo, 0, 0, , 1, 1);
@@ -756,6 +744,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
qxl_bo_create(qdev, user_bo->gem_base.size,
  true, true, QXL_GEM_DOMAIN_SURFACE, NULL,
  _bo->shadow);
+   user_bo->shadow->surf = user_bo->surf;
}
}
 
@@ -784,7 +773,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
user_bo = 

[Spice-devel] [PATCH v3 12/23] drm/qxl: track primary bo

[Gerd Hoffmann]

Track which bo is used as primary surface.  With that in place we don't
need the primary_created flag any more, we can just check the primary bo
pointer instead.

Also verify we don't already have a primary surface in
qxl_io_create_primary().

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
 drivers/gpu/drm/qxl/qxl_cmd.c | 7 +--
 drivers/gpu/drm/qxl/qxl_display.c | 2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index cb767aaef6..150b1a4f66 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -229,7 +229,7 @@ struct qxl_device {
 
struct qxl_ram_header *ram_header;
 
-   unsigned int primary_created:1;
+   struct qxl_bo *primary_bo;
 
struct qxl_memslot main_slot;
struct qxl_memslot surfaces_slot;
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index bc13539249..8e64127259 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -374,13 +374,16 @@ void qxl_io_flush_surfaces(struct qxl_device *qdev)
 void qxl_io_destroy_primary(struct qxl_device *qdev)
 {
wait_for_io_cmd(qdev, 0, QXL_IO_DESTROY_PRIMARY_ASYNC);
-   qdev->primary_created = false;
+   qdev->primary_bo = NULL;
 }
 
 void qxl_io_create_primary(struct qxl_device *qdev, struct qxl_bo *bo)
 {
struct qxl_surface_create *create;
 
+   if (WARN_ON(qdev->primary_bo))
+   return;
+
DRM_DEBUG_DRIVER("qdev %p, ram_header %p\n", qdev, qdev->ram_header);
create = >ram_header->create_surface;
create->format = bo->surf.format;
@@ -399,7 +402,7 @@ void qxl_io_create_primary(struct qxl_device *qdev, struct 
qxl_bo *bo)
create->type = QXL_SURF_TYPE_PRIMARY;
 
wait_for_io_cmd(qdev, 0, QXL_IO_CREATE_PRIMARY_ASYNC);
-   qdev->primary_created = true;
+   qdev->primary_bo = bo;
 }
 
 void qxl_io_memslot_add(struct qxl_device *qdev, uint8_t id)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 21165ab514..d3215eac9b 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -302,7 +302,7 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc 
*crtc,
struct qxl_head head;
int oldcount, i = qcrtc->index;
 
-   if (!qdev->primary_created) {
+   if (!qdev->primary_bo) {
DRM_DEBUG_KMS("no primary surface, skip (%s)\n", reason);
return;
}
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 20/23] drm/qxl: add mode/framebuffer check functions

[Gerd Hoffmann]

Add a helper functions to check video modes.  Also add a helper to check
framebuffer buffer objects, using the former for consistency.  That way
we should not fail in qxl_primary_atomic_check() because video modes
which are too big will not be added to the mode list in the first place.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 44 +++
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 9c751f01e3..fed2ea018d 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -190,6 +190,28 @@ void qxl_display_read_client_monitors_config(struct 
qxl_device *qdev)
}
 }
 
+static int qxl_check_mode(struct qxl_device *qdev,
+ unsigned int width,
+ unsigned int height)
+{
+   unsigned int stride;
+   unsigned int size;
+
+   if (check_mul_overflow(width, 4u, ))
+   return -EINVAL;
+   if (check_mul_overflow(stride, height, ))
+   return -EINVAL;
+   if (size > qdev->vram_size)
+   return -ENOMEM;
+   return 0;
+}
+
+static int qxl_check_framebuffer(struct qxl_device *qdev,
+struct qxl_bo *bo)
+{
+   return qxl_check_mode(qdev, bo->surf.width, bo->surf.height);
+}
+
 static int qxl_add_monitors_config_modes(struct drm_connector *connector,
  unsigned *pwidth,
  unsigned *pheight)
@@ -469,12 +491,7 @@ static int qxl_primary_atomic_check(struct drm_plane 
*plane,
 
bo = gem_to_qxl_bo(state->fb->obj[0]);
 
-   if (bo->surf.stride * bo->surf.height > qdev->vram_size) {
-   DRM_ERROR("Mode doesn't fit in vram size (vgamem)");
-   return -EINVAL;
-   }
-
-   return 0;
+   return qxl_check_framebuffer(qdev, bo);
 }
 
 static int qxl_primary_apply_cursor(struct drm_plane *plane)
@@ -990,20 +1007,11 @@ static enum drm_mode_status qxl_conn_mode_valid(struct 
drm_connector *connector,
 {
struct drm_device *ddev = connector->dev;
struct qxl_device *qdev = ddev->dev_private;
-   int i;
 
-   /* TODO: is this called for user defined modes? (xrandr --add-mode)
-* TODO: check that the mode fits in the framebuffer */
+   if (qxl_check_mode(qdev, mode->hdisplay, mode->vdisplay) != 0)
+   return MODE_BAD;
 
-   if (qdev->monitors_config_width == mode->hdisplay &&
-   qdev->monitors_config_height == mode->vdisplay)
-   return MODE_OK;
-
-   for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
-   if (common_modes[i].w == mode->hdisplay && common_modes[i].h == 
mode->vdisplay)
-   return MODE_OK;
-   }
-   return MODE_BAD;
+   return MODE_OK;
 }
 
 static struct drm_encoder *qxl_best_encoder(struct drm_connector *connector)
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 23/23] drm/qxl: add overflow checks to qxl_mode_dumb_create()

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_dumb.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c
index 272d19b677..bed6d06ee4 100644
--- a/drivers/gpu/drm/qxl/qxl_dumb.c
+++ b/drivers/gpu/drm/qxl/qxl_dumb.c
@@ -37,11 +37,13 @@ int qxl_mode_dumb_create(struct drm_file *file_priv,
uint32_t handle;
int r;
struct qxl_surface surf;
-   uint32_t pitch, format;
+   uint32_t pitch, size, format;
 
-   pitch = args->width * ((args->bpp + 1) / 8);
-   args->size = pitch * args->height;
-   args->size = ALIGN(args->size, PAGE_SIZE);
+   if (check_mul_overflow(args->width, ((args->bpp + 1) / 8), ))
+   return -EINVAL;
+   if (check_mul_overflow(pitch, args->height, ))
+   return -EINVAL;
+   args->size = ALIGN(size, PAGE_SIZE);
 
switch (args->bpp) {
case 16:
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 15/23] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

qdev->monitors_config->max_allowed is effectively set by the
qxl.num_heads module parameter, stored in the qxl_num_crtc variable.
Lets get rid of the indirection and use the variable qxl_num_crtc
directly.  The kernel doesn't need to dereference pointers each time it
needs the value, and when reading the code you don't have to trace where
and why qdev->monitors_config->max_allowed is set.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 25 +++--
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index d9de43e5fd..ef832f98ab 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -80,10 +80,10 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
DRM_DEBUG_KMS("no client monitors configured\n");
return status;
}
-   if (num_monitors > qdev->monitors_config->max_allowed) {
+   if (num_monitors > qxl_num_crtc) {
DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
%d\n",
- qdev->monitors_config->max_allowed, num_monitors);
-   num_monitors = qdev->monitors_config->max_allowed;
+ qxl_num_crtc, num_monitors);
+   num_monitors = qxl_num_crtc;
} else {
num_monitors = qdev->rom->client_monitors_config.count;
}
@@ -96,8 +96,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct 
qxl_device *qdev)
return status;
}
/* we copy max from the client but it isn't used */
-   qdev->client_monitors_config->max_allowed =
-   qdev->monitors_config->max_allowed;
+   qdev->client_monitors_config->max_allowed = qxl_num_crtc;
for (i = 0 ; i < qdev->client_monitors_config->count ; ++i) {
struct qxl_urect *c_rect =
>rom->client_monitors_config.heads[i];
@@ -204,7 +203,7 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
 
if (!qdev->monitors_config)
return 0;
-   if (h >= qdev->monitors_config->max_allowed)
+   if (h >= qxl_num_crtc)
return 0;
if (!qdev->client_monitors_config)
return 0;
@@ -307,8 +306,7 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc 
*crtc,
return;
}
 
-   if (!qdev->monitors_config ||
-   qdev->monitors_config->max_allowed <= i)
+   if (!qdev->monitors_config || qxl_num_crtc <= i)
return;
 
head.id = i;
@@ -350,9 +348,10 @@ static void qxl_crtc_update_monitors_config(struct 
drm_crtc *crtc,
if (oldcount != qdev->monitors_config->count)
DRM_DEBUG_KMS("active heads %d -> %d (%d total)\n",
  oldcount, qdev->monitors_config->count,
- qdev->monitors_config->max_allowed);
+ qxl_num_crtc);
 
qdev->monitors_config->heads[i] = head;
+   qdev->monitors_config->max_allowed = qxl_num_crtc;
qxl_send_monitors_config(qdev);
 }
 
@@ -1146,9 +1145,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 {
int ret;
struct drm_gem_object *gobj;
-   int max_allowed = qxl_num_crtc;
int monitors_config_size = sizeof(struct qxl_monitors_config) +
-   max_allowed * sizeof(struct qxl_head);
+   qxl_num_crtc * sizeof(struct qxl_head);
 
ret = qxl_gem_object_create(qdev, monitors_config_size, 0,
QXL_GEM_DOMAIN_VRAM,
@@ -1170,9 +1168,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
 
memset(qdev->monitors_config, 0, monitors_config_size);
-   qdev->monitors_config->max_allowed = max_allowed;
-
-   qdev->dumb_heads = kcalloc(max_allowed, sizeof(qdev->dumb_heads[0]), 
GFP_KERNEL);
+   qdev->dumb_heads = kcalloc(qxl_num_crtc, sizeof(qdev->dumb_heads[0]),
+  GFP_KERNEL);
return 0;
 }
 
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 18/23] drm/qxl: remove dead qxl fbdev emulation code

[Gerd Hoffmann]

Lovely diffstat, thanks to the new generic fbdev emulation.

 drm/qxl/Makefile   |2
 drm/qxl/qxl_draw.c |  232 
 drm/qxl/qxl_drv.h  |   21 ---
 drm/qxl/qxl_fb.c   |  300 -

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h  |  21 ---
 drivers/gpu/drm/qxl/qxl_draw.c | 232 ---
 drivers/gpu/drm/qxl/qxl_fb.c   | 300 -
 drivers/gpu/drm/qxl/Makefile   |   2 +-
 4 files changed, 1 insertion(+), 554 deletions(-)
 delete mode 100644 drivers/gpu/drm/qxl/qxl_fb.c

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 8c3af1cdbe..4a0331b3ff 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -220,8 +220,6 @@ struct qxl_device {
struct qxl_mman mman;
struct qxl_gem  gem;
 
-   struct drm_fb_helperfb_helper;
-
void *ram_physical;
 
struct qxl_ring *release_ring;
@@ -322,12 +320,6 @@ qxl_bo_physical_address(struct qxl_device *qdev, struct 
qxl_bo *bo,
return slot->high_bits | (bo->tbo.offset - slot->gpu_offset + offset);
 }
 
-/* qxl_fb.c */
-#define QXLFB_CONN_LIMIT 1
-
-int qxl_fbdev_init(struct qxl_device *qdev);
-void qxl_fbdev_fini(struct qxl_device *qdev);
-
 /* qxl_display.c */
 void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
@@ -432,9 +424,6 @@ int qxl_alloc_bo_reserved(struct qxl_device *qdev,
  struct qxl_bo **_bo);
 /* qxl drawing commands */
 
-void qxl_draw_opaque_fb(const struct qxl_fb_image *qxl_fb_image,
-   int stride /* filled in if 0 */);
-
 void qxl_draw_dirty_fb(struct qxl_device *qdev,
   struct drm_framebuffer *fb,
   struct qxl_bo *bo,
@@ -443,13 +432,6 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
   unsigned int num_clips, int inc,
   uint32_t dumb_shadow_offset);
 
-void qxl_draw_fill(struct qxl_draw_fill *qxl_draw_fill_rec);
-
-void qxl_draw_copyarea(struct qxl_device *qdev,
-  u32 width, u32 height,
-  u32 sx, u32 sy,
-  u32 dx, u32 dy);
-
 void qxl_release_free(struct qxl_device *qdev,
  struct qxl_release *release);
 
@@ -481,9 +463,6 @@ int qxl_gem_prime_mmap(struct drm_gem_object *obj,
 int qxl_irq_init(struct qxl_device *qdev);
 irqreturn_t qxl_irq_handler(int irq, void *arg);
 
-/* qxl_fb.c */
-bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
-
 int qxl_debugfs_add_files(struct qxl_device *qdev,
  struct drm_info_list *files,
  unsigned int nfiles);
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
index 5313ad21c1..97c3f1a95a 100644
--- a/drivers/gpu/drm/qxl/qxl_draw.c
+++ b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -109,152 +109,6 @@ make_drawable(struct qxl_device *qdev, int surface, 
uint8_t type,
return 0;
 }
 
-static int alloc_palette_object(struct qxl_device *qdev,
-   struct qxl_release *release,
-   struct qxl_bo **palette_bo)
-{
-   return qxl_alloc_bo_reserved(qdev, release,
-sizeof(struct qxl_palette) + 
sizeof(uint32_t) * 2,
-palette_bo);
-}
-
-static int qxl_palette_create_1bit(struct qxl_bo *palette_bo,
-  struct qxl_release *release,
-  const struct qxl_fb_image *qxl_fb_image)
-{
-   const struct fb_image *fb_image = _fb_image->fb_image;
-   uint32_t visual = qxl_fb_image->visual;
-   const uint32_t *pseudo_palette = qxl_fb_image->pseudo_palette;
-   struct qxl_palette *pal;
-   int ret;
-   uint32_t fgcolor, bgcolor;
-   static uint64_t unique; /* we make no attempt to actually set this
-* correctly globaly, since that would require
-* tracking all of our palettes. */
-   ret = qxl_bo_kmap(palette_bo, (void **));
-   if (ret)
-   return ret;
-   pal->num_ents = 2;
-   pal->unique = unique++;
-   if (visual == FB_VISUAL_TRUECOLOR || visual == FB_VISUAL_DIRECTCOLOR) {
-   /* NB: this is the only used branch currently. */
-   fgcolor = pseudo_palette[fb_image->fg_color];
-   bgcolor = pseudo_palette[fb_image->bg_color];
-   } else {
-   fgcolor = fb_image->fg_color;
-   bgcolor = fb_image->bg_color;
-   }
-   pal->ents[0] = bgcolor;
-   pal->ents[1] = fgcolor;
-   qxl_bo_kunmap(palette_bo);
-   return 0;
-}
-
-void qxl_draw_opaque_fb(const struct qxl_fb_image *qxl_fb_image,
-   

[Spice-devel] [PATCH v3 09/23] drm/qxl: use QXL_GEM_DOMAIN_SURFACE for dumb gem objects

[Gerd Hoffmann]

dumb buffers are used as qxl surfaces, so allocate them as
QXL_GEM_DOMAIN_SURFACE.  Should usually be allocated in
PRIV ttm domain then, so this reduces VRAM memory pressure.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_dumb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c
index e3765739c3..272d19b677 100644
--- a/drivers/gpu/drm/qxl/qxl_dumb.c
+++ b/drivers/gpu/drm/qxl/qxl_dumb.c
@@ -59,7 +59,7 @@ int qxl_mode_dumb_create(struct drm_file *file_priv,
surf.stride = pitch;
surf.format = format;
r = qxl_gem_object_create_with_handle(qdev, file_priv,
- QXL_GEM_DOMAIN_VRAM,
+ QXL_GEM_DOMAIN_SURFACE,
  args->size, , ,
  );
if (r)
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 03/23] drm/qxl: simplify slot management

[Gerd Hoffmann]

Drop pointless indirection, remove the mem_slots array and index
variables, drop dynamic allocation.  Store memslots in qxl_device
instead.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 15 +
 drivers/gpu/drm/qxl/qxl_kms.c | 72 +--
 2 files changed, 36 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 7eabf4a9ed..f9dddfe7d9 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -130,9 +130,11 @@ struct qxl_mman {
 };
 
 struct qxl_memslot {
+   int index;
+   const char  *name;
uint8_t generation;
uint64_tstart_phys_addr;
-   uint64_tend_phys_addr;
+   uint64_tsize;
uint64_thigh_bits;
 };
 
@@ -228,11 +230,8 @@ struct qxl_device {
 
unsigned int primary_created:1;
 
-   struct qxl_memslot  *mem_slots;
-   uint8_t n_mem_slots;
-
-   uint8_t main_mem_slot;
-   uint8_t surfaces_mem_slot;
+   struct qxl_memslot main_slot;
+   struct qxl_memslot surfaces_slot;
uint8_t slot_id_bits;
uint8_t slot_gen_bits;
uint64_tva_slot_mask;
@@ -312,8 +311,8 @@ static inline uint64_t
 qxl_bo_physical_address(struct qxl_device *qdev, struct qxl_bo *bo,
unsigned long offset)
 {
-   int slot_id = bo->type == QXL_GEM_DOMAIN_VRAM ? qdev->main_mem_slot : 
qdev->surfaces_mem_slot;
-   struct qxl_memslot *slot = &(qdev->mem_slots[slot_id]);
+   struct qxl_memslot *slot = bo->type == QXL_GEM_DOMAIN_VRAM
+   ? >main_slot : >surfaces_slot;
 
/* TODO - need to hold one of the locks to read tbo.offset */
return slot->high_bits | (bo->tbo.offset + offset);
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 15238a413f..a9288100ae 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -53,40 +53,46 @@ static bool qxl_check_device(struct qxl_device *qdev)
return true;
 }
 
-static void setup_hw_slot(struct qxl_device *qdev, int slot_index,
- struct qxl_memslot *slot)
+static void setup_hw_slot(struct qxl_device *qdev, struct qxl_memslot *slot)
 {
qdev->ram_header->mem_slot.mem_start = slot->start_phys_addr;
-   qdev->ram_header->mem_slot.mem_end = slot->end_phys_addr;
-   qxl_io_memslot_add(qdev, slot_index);
+   qdev->ram_header->mem_slot.mem_end = slot->start_phys_addr + slot->size;
+   qxl_io_memslot_add(qdev, qdev->rom->slots_start + slot->index);
 }
 
-static uint8_t setup_slot(struct qxl_device *qdev, uint8_t slot_index_offset,
-   unsigned long start_phys_addr, unsigned long end_phys_addr)
+static void setup_slot(struct qxl_device *qdev,
+  struct qxl_memslot *slot,
+  unsigned int slot_index,
+  const char *slot_name,
+  unsigned long start_phys_addr,
+  unsigned long size)
 {
uint64_t high_bits;
-   struct qxl_memslot *slot;
-   uint8_t slot_index;
 
-   slot_index = qdev->rom->slots_start + slot_index_offset;
-   slot = >mem_slots[slot_index];
+   slot->index = slot_index;
+   slot->name = slot_name;
slot->start_phys_addr = start_phys_addr;
-   slot->end_phys_addr = end_phys_addr;
+   slot->size = size;
 
-   setup_hw_slot(qdev, slot_index, slot);
+   setup_hw_slot(qdev, slot);
 
slot->generation = qdev->rom->slot_generation;
-   high_bits = slot_index << qdev->slot_gen_bits;
+   high_bits = (qdev->rom->slots_start + slot->index)
+   << qdev->slot_gen_bits;
high_bits |= slot->generation;
high_bits <<= (64 - (qdev->slot_gen_bits + qdev->slot_id_bits));
slot->high_bits = high_bits;
-   return slot_index;
+
+   DRM_INFO("slot %d (%s): base 0x%08lx, size 0x%08lx\n",
+slot->index, slot->name,
+(unsigned long)slot->start_phys_addr,
+(unsigned long)slot->size);
 }
 
 void qxl_reinit_memslots(struct qxl_device *qdev)
 {
-   setup_hw_slot(qdev, qdev->main_mem_slot, 
>mem_slots[qdev->main_mem_slot]);
-   setup_hw_slot(qdev, qdev->surfaces_mem_slot, 
>mem_slots[qdev->surfaces_mem_slot]);
+   setup_hw_slot(qdev, >main_slot);
+   setup_hw_slot(qdev, >surfaces_slot);
 }
 
 static void qxl_gc_work(struct work_struct *work)
@@ -231,22 +237,11 @@ int qxl_device_init(struct qxl_device *qdev,
}
/* TODO - slot initialization should happen on reset. where is our
 * reset handler? */
-   qdev->n_mem_slots = qdev->rom->slots_end;
qdev->slot_gen_bits = qdev->rom->slot_gen_bits;
qdev->slot_id_bits = qdev->rom->slot_id_bits;
qdev->va_slot_mask =
(~(uint64_t)0) >> (qdev->slot_id_bits 

[Spice-devel] [PATCH v3 06/23] drm/qxl: use separate offset spaces for the two slots / ttm memory types.

[Gerd Hoffmann]

Without that ttm offsets are not unique, they can refer to objects
in both VRAM and PRIV memory (aka main and surfaces slot).

One of those "why things didn't blow up without this" moments.
Probably offset conflicts are rare enough by pure luck.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h |  5 -
 drivers/gpu/drm/qxl/qxl_kms.c |  5 +++--
 drivers/gpu/drm/qxl/qxl_ttm.c | 10 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 3ebe66abf2..27e0a3fc08 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -136,6 +136,7 @@ struct qxl_memslot {
uint64_tstart_phys_addr;
uint64_tsize;
uint64_thigh_bits;
+   uint64_tgpu_offset;
 };
 
 enum {
@@ -312,8 +313,10 @@ qxl_bo_physical_address(struct qxl_device *qdev, struct 
qxl_bo *bo,
(bo->tbo.mem.mem_type == TTM_PL_VRAM)
? >main_slot : >surfaces_slot;
 
+   WARN_ON_ONCE((bo->tbo.offset & slot->gpu_offset) != slot->gpu_offset);
+
/* TODO - need to hold one of the locks to read tbo.offset */
-   return slot->high_bits | (bo->tbo.offset + offset);
+   return slot->high_bits | (bo->tbo.offset - slot->gpu_offset + offset);
 }
 
 /* qxl_fb.c */
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 3c1753667d..82c764623f 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -83,10 +83,11 @@ static void setup_slot(struct qxl_device *qdev,
high_bits <<= (64 - (qdev->rom->slot_gen_bits + 
qdev->rom->slot_id_bits));
slot->high_bits = high_bits;
 
-   DRM_INFO("slot %d (%s): base 0x%08lx, size 0x%08lx\n",
+   DRM_INFO("slot %d (%s): base 0x%08lx, size 0x%08lx, gpu_offset 0x%lx\n",
 slot->index, slot->name,
 (unsigned long)slot->start_phys_addr,
-(unsigned long)slot->size);
+(unsigned long)slot->size,
+(unsigned long)slot->gpu_offset);
 }
 
 void qxl_reinit_memslots(struct qxl_device *qdev)
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 886f61e94f..36ea993aac 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -100,6 +100,11 @@ static int qxl_invalidate_caches(struct ttm_bo_device 
*bdev, uint32_t flags)
 static int qxl_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 struct ttm_mem_type_manager *man)
 {
+   struct qxl_device *qdev = qxl_get_qdev(bdev);
+   unsigned int gpu_offset_shift =
+   64 - (qdev->rom->slot_gen_bits + qdev->rom->slot_id_bits + 8);
+   struct qxl_memslot *slot;
+
switch (type) {
case TTM_PL_SYSTEM:
/* System memory */
@@ -110,8 +115,11 @@ static int qxl_init_mem_type(struct ttm_bo_device *bdev, 
uint32_t type,
case TTM_PL_VRAM:
case TTM_PL_PRIV:
/* "On-card" video ram */
+   slot = (type == TTM_PL_VRAM) ?
+   >main_slot : >surfaces_slot;
+   slot->gpu_offset = (uint64_t)type << gpu_offset_shift;
man->func = _bo_manager_func;
-   man->gpu_offset = 0;
+   man->gpu_offset = slot->gpu_offset;
man->flags = TTM_MEMTYPE_FLAG_FIXED |
 TTM_MEMTYPE_FLAG_MAPPABLE;
man->available_caching = TTM_PL_MASK_CACHING;
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 11/23] drm/qxl: drop unused offset parameter from qxl_io_create_primary()

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 1 -
 drivers/gpu/drm/qxl/qxl_cmd.c | 7 +++
 drivers/gpu/drm/qxl/qxl_display.c | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 27e0a3fc08..cb767aaef6 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -385,7 +385,6 @@ void qxl_update_screen(struct qxl_device *qxl);
 /* qxl io operations (qxl_cmd.c) */
 
 void qxl_io_create_primary(struct qxl_device *qdev,
-  unsigned int offset,
   struct qxl_bo *bo);
 void qxl_io_destroy_primary(struct qxl_device *qdev);
 void qxl_io_memslot_add(struct qxl_device *qdev, uint8_t id);
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 5ba831c78c..bc13539249 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -377,8 +377,7 @@ void qxl_io_destroy_primary(struct qxl_device *qdev)
qdev->primary_created = false;
 }
 
-void qxl_io_create_primary(struct qxl_device *qdev,
-  unsigned int offset, struct qxl_bo *bo)
+void qxl_io_create_primary(struct qxl_device *qdev, struct qxl_bo *bo)
 {
struct qxl_surface_create *create;
 
@@ -389,9 +388,9 @@ void qxl_io_create_primary(struct qxl_device *qdev,
create->height = bo->surf.height;
create->stride = bo->surf.stride;
if (bo->shadow) {
-   create->mem = qxl_bo_physical_address(qdev, bo->shadow, offset);
+   create->mem = qxl_bo_physical_address(qdev, bo->shadow, 0);
} else {
-   create->mem = qxl_bo_physical_address(qdev, bo, offset);
+   create->mem = qxl_bo_physical_address(qdev, bo, 0);
}
 
DRM_DEBUG_DRIVER("mem = %llx, from %p\n", create->mem, bo->kptr);
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 1b700ef503..21165ab514 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -557,7 +557,7 @@ static void qxl_primary_atomic_update(struct drm_plane 
*plane,
 
if (!bo->is_primary) {
if (!same_shadow) {
-   qxl_io_create_primary(qdev, 0, bo);
+   qxl_io_create_primary(qdev, bo);
qxl_primary_apply_cursor(plane);
}
bo->is_primary = true;
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 04/23] drm/qxl: change the way slot is detected

[Gerd Hoffmann]

From: Frediano Ziglio 

Instead of relaying on surface type use the actual placement.
This allow to have different placement for a single type of
surface.

Signed-off-by: Frediano Ziglio 

[ kraxel: rebased, adapted to upstream changes ]

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index f9dddfe7d9..d015d4fff1 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -311,7 +311,8 @@ static inline uint64_t
 qxl_bo_physical_address(struct qxl_device *qdev, struct qxl_bo *bo,
unsigned long offset)
 {
-   struct qxl_memslot *slot = bo->type == QXL_GEM_DOMAIN_VRAM
+   struct qxl_memslot *slot =
+   (bo->tbo.mem.mem_type == TTM_PL_VRAM)
? >main_slot : >surfaces_slot;
 
/* TODO - need to hold one of the locks to read tbo.offset */
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk 04/12] Drop autotools

[Marc-André Lureau]

Hi

On Fri, Jan 18, 2019 at 3:47 PM Frediano Ziglio  wrote:
>
> >
> > From: Marc-André Lureau 
> >
> > Maintaining 1 build system is hard. Maintaining 2 is even harder.
> >
> > It seems the meson build system is now in good shape to replace
> > autotools. Like many desktop projects, let's move entirely to meson
> > and drop autotools support.
> >
> > Known changes:
> > - no git version: a following patch will add it back in a limited form
> > - generating changelog & thanks files in the dist tarball. This is not
> >   strictly required, and can be added back later.
> > - generated files are not included in the dist tarball. In some ways,
> >   this can be considered a good thing. Since code generation is done
> >   with python, and meson requires python anyway, this is not an issue.
> >
> > Signed-off-by: Marc-André Lureau 
>
> Patience.
>
> It's IMO too early, we are still fixing bugs on Meson build, distros
> we support still hasn't the version we need packaged and we are
> already removing it?
>
> Christophe already mentioned that we should have not bumped Meson
> requirements that much.
>
> Keep it in some branch but for the moment I would say no.

Given that some pretty conservative core desktop projects (including
mesa, glib, gtk+) have already made the move upstream, I think it's
the right time for spice-gtk too.

The question is which distro we want to support with upstream and
upcoming versions. I would say we should target current Fedora
release.

Anything else can be backported to previous releases, and we can
maintain older version branches upstream.


> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk 04/12] Drop autotools

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Maintaining 1 build system is hard. Maintaining 2 is even harder.
> 
> It seems the meson build system is now in good shape to replace
> autotools. Like many desktop projects, let's move entirely to meson
> and drop autotools support.
> 
> Known changes:
> - no git version: a following patch will add it back in a limited form
> - generating changelog & thanks files in the dist tarball. This is not
>   strictly required, and can be added back later.
> - generated files are not included in the dist tarball. In some ways,
>   this can be considered a good thing. Since code generation is done
>   with python, and meson requires python anyway, this is not an issue.
> 
> Signed-off-by: Marc-André Lureau 

Patience.

It's IMO too early, we are still fixing bugs on Meson build, distros
we support still hasn't the version we need packaged and we are
already removing it?

Christophe already mentioned that we should have not bumped Meson
requirements that much.

Keep it in some branch but for the moment I would say no.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk 02/12] meson: fix ninja dist, and building from tarball

[Marc-André Lureau]

Hi

On Fri, Jan 18, 2019 at 3:44 PM Frediano Ziglio  wrote:
>
> >
> > From: Marc-André Lureau 
> >
> > meson doesn't handle git-version-gen correctly yet (see
> > meson#688). Let's set the version manually for now.
> >
>
> Why? Current code works, why removing it?

It doesn't work well enough, ninja dist is broken at least.

>
> > And a tag version vX.X will also fail to build, version_info[2]
> > is out of array bounds.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  meson.build | 2 +-
> >  src/meson.build | 6 +++---
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index d7062af..70dd318 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -2,7 +2,7 @@
> >  # project definition
> >  #
> >  project('spice-gtk', 'c',
> > - version : run_command('build-aux/git-version-gen',
> > '${MESON_SOURCE_ROOT}/.tarball-version', check : true).stdout().strip(),
> > + version : '0.36',
> >   license : 'LGPLv2.1',
> >   meson_version : '>= 0.49')
> >
> > diff --git a/src/meson.build b/src/meson.build
> > index d9614cb..c55db44 100644
> > --- a/src/meson.build
> > +++ b/src/meson.build
> > @@ -8,9 +8,9 @@ spice_gtk_include += [include_directories('.')]
> >  version_info = meson.project_version().split('.')
> >  major = '@0@'.format(version_info[0])
> >  minor = '@0@'.format(version_info[1])
> > -micro = version_info[2].split('-')[0]
> > -if micro == ''
> > -  micro = '0'
> > +micro = '0'
> > +if version_info.length() > 2
> > +  micro = version_info[2].split('-')[0]
> >  endif
> >  version_data = configuration_data()
> >  version_data.set('SPICE_GTK_MAJOR_VERSION', major)
> > --
> > 2.20.1.98.gecbdaf0899
> >
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk 02/12] meson: fix ninja dist, and building from tarball

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> meson doesn't handle git-version-gen correctly yet (see
> meson#688). Let's set the version manually for now.
> 

Why? Current code works, why removing it?

> And a tag version vX.X will also fail to build, version_info[2]
> is out of array bounds.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  meson.build | 2 +-
>  src/meson.build | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index d7062af..70dd318 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2,7 +2,7 @@
>  # project definition
>  #
>  project('spice-gtk', 'c',
> - version : run_command('build-aux/git-version-gen',
> '${MESON_SOURCE_ROOT}/.tarball-version', check : true).stdout().strip(),
> + version : '0.36',
>   license : 'LGPLv2.1',
>   meson_version : '>= 0.49')
>  
> diff --git a/src/meson.build b/src/meson.build
> index d9614cb..c55db44 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -8,9 +8,9 @@ spice_gtk_include += [include_directories('.')]
>  version_info = meson.project_version().split('.')
>  major = '@0@'.format(version_info[0])
>  minor = '@0@'.format(version_info[1])
> -micro = version_info[2].split('-')[0]
> -if micro == ''
> -  micro = '0'
> +micro = '0'
> +if version_info.length() > 2
> +  micro = version_info[2].split('-')[0]
>  endif
>  version_data = configuration_data()
>  version_data.set('SPICE_GTK_MAJOR_VERSION', major)
> --
> 2.20.1.98.gecbdaf0899
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 2/2] meson: mingw: add windows configuration

[Marc-André Lureau]

Hi

On Fri, Jan 18, 2019 at 3:08 PM Victor Toso  wrote:
>
> On Fri, Jan 18, 2019 at 02:42:24PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Jan 18, 2019 at 1:57 PM Victor Toso  wrote:
> > >
> > > From: Victor Toso 
> > >
> > > Using fedora 29 mingw paths. Not hardcoding prefix but keeping a
> > > comment that should be added in case one would like to install over
> > > what they have in the system :)
> > >
> > > Signed-off-by: Victor Toso 
> >
> > Not needed, F29 ships with its own meson mingw cross file and helper
> > mingw{32,64}-meson.
> >
> > See also "Drop autotools" .gitlab-ci diff.
>
> | (mkdir buildw && cd buildw && mingw64-meson)
>
> Heh, thanks. I tried to use mingw64-meson several times before,
> did not work for me. If I create && enter the folder, it works
> indeed.
>
> As I use meson builddir, I was trying mingw64-meson buildwin
> without success.

yes, I had the same problem. I guess its worth opening a bug for
mingw-filesystem

> Cheers,
>
> > > ---
> > >  meson_mingw.txt | 18 ++
> > >  1 file changed, 18 insertions(+)
> > >  create mode 100644 meson_mingw.txt
> > >
> > > diff --git a/meson_mingw.txt b/meson_mingw.txt
> > > new file mode 100644
> > > index 000..8a088fc
> > > --- /dev/null
> > > +++ b/meson_mingw.txt
> > > @@ -0,0 +1,18 @@
> > > +# Options for installing in a Fedora 29
> > > +# meson win-build --cross-file meson_mingw.txt \
> > > +# -Dprefix=/usr/x86_64-w64-mingw32/sys-root/mingw \
> > > +# -Dlibdir=lib
> > > +
> > > +[binaries]
> > > +c = '/usr/bin/x86_64-w64-mingw32-gcc'
> > > +cpp = '/usr/bin/x86_64-w64-mingw32-g++'
> > > +ar = '/usr/bin/x86_64-w64-mingw32-ar'
> > > +strip = '/usr/bin/x86_64-w64-mingw32-strip'
> > > +pkgconfig = '/usr/bin/x86_64-w64-mingw32-pkg-config'
> > > +#exe_wrapper = 'wine' # A command used to run generated executables.
> > > +
> > > +[host_machine]
> > > +system = 'windows'
> > > +cpu_family = 'x86'
> > > +cpu = 'x86_64'
> > > +endian = 'little'
> > > --
> > > 2.20.1
> > >
> > > ___
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> >
> >
> > --
> > Marc-André Lureau



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 2/2] meson: mingw: add windows configuration

[Victor Toso]

On Fri, Jan 18, 2019 at 02:42:24PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jan 18, 2019 at 1:57 PM Victor Toso  wrote:
> >
> > From: Victor Toso 
> >
> > Using fedora 29 mingw paths. Not hardcoding prefix but keeping a
> > comment that should be added in case one would like to install over
> > what they have in the system :)
> >
> > Signed-off-by: Victor Toso 
> 
> Not needed, F29 ships with its own meson mingw cross file and helper
> mingw{32,64}-meson.
> 
> See also "Drop autotools" .gitlab-ci diff.

| (mkdir buildw && cd buildw && mingw64-meson)

Heh, thanks. I tried to use mingw64-meson several times before,
did not work for me. If I create && enter the folder, it works
indeed.

As I use meson builddir, I was trying mingw64-meson buildwin
without success.

Cheers,

> > ---
> >  meson_mingw.txt | 18 ++
> >  1 file changed, 18 insertions(+)
> >  create mode 100644 meson_mingw.txt
> >
> > diff --git a/meson_mingw.txt b/meson_mingw.txt
> > new file mode 100644
> > index 000..8a088fc
> > --- /dev/null
> > +++ b/meson_mingw.txt
> > @@ -0,0 +1,18 @@
> > +# Options for installing in a Fedora 29
> > +# meson win-build --cross-file meson_mingw.txt \
> > +# -Dprefix=/usr/x86_64-w64-mingw32/sys-root/mingw \
> > +# -Dlibdir=lib
> > +
> > +[binaries]
> > +c = '/usr/bin/x86_64-w64-mingw32-gcc'
> > +cpp = '/usr/bin/x86_64-w64-mingw32-g++'
> > +ar = '/usr/bin/x86_64-w64-mingw32-ar'
> > +strip = '/usr/bin/x86_64-w64-mingw32-strip'
> > +pkgconfig = '/usr/bin/x86_64-w64-mingw32-pkg-config'
> > +#exe_wrapper = 'wine' # A command used to run generated executables.
> > +
> > +[host_machine]
> > +system = 'windows'
> > +cpu_family = 'x86'
> > +cpu = 'x86_64'
> > +endian = 'little'
> > --
> > 2.20.1
> >
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> 
> -- 
> Marc-André Lureau


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk 02/12] meson: fix ninja dist, and building from tarball

[Victor Toso]

Hi,

On Fri, Jan 18, 2019 at 04:16:07AM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> meson doesn't handle git-version-gen correctly yet (see
> meson#688). Let's set the version manually for now.
> 
> And a tag version vX.X will also fail to build, version_info[2]
> is out of array bounds.
> 
> Signed-off-by: Marc-André Lureau 
Acked-by: Victor Toso 
> ---
>  meson.build | 2 +-
>  src/meson.build | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index d7062af..70dd318 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2,7 +2,7 @@
>  # project definition
>  #
>  project('spice-gtk', 'c',
> - version : run_command('build-aux/git-version-gen', 
> '${MESON_SOURCE_ROOT}/.tarball-version', check : true).stdout().strip(),
> + version : '0.36',
>   license : 'LGPLv2.1',
>   meson_version : '>= 0.49')
>  
> diff --git a/src/meson.build b/src/meson.build
> index d9614cb..c55db44 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -8,9 +8,9 @@ spice_gtk_include += [include_directories('.')]
>  version_info = meson.project_version().split('.')
>  major = '@0@'.format(version_info[0])
>  minor = '@0@'.format(version_info[1])
> -micro = version_info[2].split('-')[0]
> -if micro == ''
> -  micro = '0'
> +micro = '0'
> +if version_info.length() > 2
> +  micro = version_info[2].split('-')[0]
>  endif
>  version_data = configuration_data()
>  version_data.set('SPICE_GTK_MAJOR_VERSION', major)
> -- 
> 2.20.1.98.gecbdaf0899
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 2/2] meson: mingw: add windows configuration

[Christophe Fergeau]

Hey,

On Fri, Jan 18, 2019 at 10:57:06AM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> Using fedora 29 mingw paths. Not hardcoding prefix but keeping a
> comment that should be added in case one would like to install over
> what they have in the system :)

This is the same as /usr/share/mingw/toolchain-mingw64.meson from
mingw64-filesystem-104-2.fc29.noarch, I don't think we need to duplicate
it, do we?

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 2/2] meson: mingw: add windows configuration

[Marc-André Lureau]

Hi

On Fri, Jan 18, 2019 at 1:57 PM Victor Toso  wrote:
>
> From: Victor Toso 
>
> Using fedora 29 mingw paths. Not hardcoding prefix but keeping a
> comment that should be added in case one would like to install over
> what they have in the system :)
>
> Signed-off-by: Victor Toso 

Not needed, F29 ships with its own meson mingw cross file and helper
mingw{32,64}-meson.

See also "Drop autotools" .gitlab-ci diff.


> ---
>  meson_mingw.txt | 18 ++
>  1 file changed, 18 insertions(+)
>  create mode 100644 meson_mingw.txt
>
> diff --git a/meson_mingw.txt b/meson_mingw.txt
> new file mode 100644
> index 000..8a088fc
> --- /dev/null
> +++ b/meson_mingw.txt
> @@ -0,0 +1,18 @@
> +# Options for installing in a Fedora 29
> +# meson win-build --cross-file meson_mingw.txt \
> +# -Dprefix=/usr/x86_64-w64-mingw32/sys-root/mingw \
> +# -Dlibdir=lib
> +
> +[binaries]
> +c = '/usr/bin/x86_64-w64-mingw32-gcc'
> +cpp = '/usr/bin/x86_64-w64-mingw32-g++'
> +ar = '/usr/bin/x86_64-w64-mingw32-ar'
> +strip = '/usr/bin/x86_64-w64-mingw32-strip'
> +pkgconfig = '/usr/bin/x86_64-w64-mingw32-pkg-config'
> +#exe_wrapper = 'wine' # A command used to run generated executables.
> +
> +[host_machine]
> +system = 'windows'
> +cpu_family = 'x86'
> +cpu = 'x86_64'
> +endian = 'little'
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 1/2] meson: fix micro on release tag

[Marc-André Lureau]

On Fri, Jan 18, 2019 at 1:57 PM Victor Toso  wrote:
>
> From: Victor Toso 
>
> On 0.36, there is no third value so we would get:
>
> src/meson.build:11:0: ERROR:  Index 2 out of bounds of array of size 2.
>
> Signed-off-by: Victor Toso 

see "meson: fix ninja dist, and building from tarball"

> ---
>  src/meson.build | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/meson.build b/src/meson.build
> index d9614cb..bf39464 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -8,10 +8,15 @@ spice_gtk_include += [include_directories('.')]
>  version_info = meson.project_version().split('.')
>  major = '@0@'.format(version_info[0])
>  minor = '@0@'.format(version_info[1])
> -micro = version_info[2].split('-')[0]
> -if micro == ''
> -  micro = '0'
> +if version_info.length() > 2
> +micro = version_info[2].split('-')[0]
> +if micro == ''
> +micro = '0'
> +endif
> +else
> +micro = '0'
>  endif
> +
>  version_data = configuration_data()
>  version_data.set('SPICE_GTK_MAJOR_VERSION', major)
>  version_data.set('SPICE_GTK_MINOR_VERSION', minor)
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 1/2] meson: fix micro on release tag

[Victor Toso]

Hi,

On Fri, Jan 18, 2019 at 05:13:44AM -0500, Frediano Ziglio wrote:
> > 
> > On Fri, Jan 18, 2019 at 10:57:05AM +0100, Victor Toso wrote:
> > > From: Victor Toso 
> > > 
> > > On 0.36, there is no third value so we would get:
> > > 
> > > src/meson.build:11:0: ERROR:  Index 2 out of bounds of array of size 
> > > 2.
> > > 
> > > Signed-off-by: Victor Toso 
> > > ---
> > >  src/meson.build | 11 ---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/meson.build b/src/meson.build
> > > index d9614cb..bf39464 100644
> > > --- a/src/meson.build
> > > +++ b/src/meson.build
> > > @@ -8,10 +8,15 @@ spice_gtk_include += [include_directories('.')]
> > >  version_info = meson.project_version().split('.')
> > >  major = '@0@'.format(version_info[0])
> > >  minor = '@0@'.format(version_info[1])
> > > -micro = version_info[2].split('-')[0]
> > > -if micro == ''
> > > -  micro = '0'
> > > +if version_info.length() > 2
> > > +micro = version_info[2].split('-')[0]
> > > +if micro == ''
> 
> I doubt there would be a version with x.y.-z syntax

:)

> > > +micro = '0'
> > > +endif
> > > +else
> > > +micro = '0'
> > 
> 
> Why not
> 
> micro = '0'
> if version_info.length() > 2
> micro = version_info[2].split('-')[0]
> endif

Just because of `if micro == ''` part.
I don't mind changing if you prefer this way.

> > Copy-paste issue, bad indentation. Fixed locally.
> > 
> > >  endif
> > > +
> > >  version_data = configuration_data()
> > >  version_data.set('SPICE_GTK_MAJOR_VERSION', major)
> > >  version_data.set('SPICE_GTK_MINOR_VERSION', minor)
> 
> Frediano


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 1/2] meson: fix micro on release tag

[Frediano Ziglio]

> > > From: Victor Toso 
> > > 
> > > On 0.36, there is no third value so we would get:
> > > 
> > > src/meson.build:11:0: ERROR:  Index 2 out of bounds of array of size
> > > 2.
> > > 
> > > Signed-off-by: Victor Toso 
> > > ---
> > >  src/meson.build | 11 ---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/meson.build b/src/meson.build
> > > index d9614cb..bf39464 100644
> > > --- a/src/meson.build
> > > +++ b/src/meson.build
> > > @@ -8,10 +8,15 @@ spice_gtk_include += [include_directories('.')]
> > >  version_info = meson.project_version().split('.')

Or here do something like

version_info = (meson.project_version() + '.0.0').split('.')

> > >  major = '@0@'.format(version_info[0])
> > >  minor = '@0@'.format(version_info[1])
> > > -micro = version_info[2].split('-')[0]
> > > -if micro == ''
> > > -  micro = '0'
> > > +if version_info.length() > 2
> > > +micro = version_info[2].split('-')[0]
> > > +if micro == ''
> 
> I doubt there would be a version with x.y.-z syntax
> 
> > > +micro = '0'
> > > +endif
> > > +else
> > > +micro = '0'
> > 
> 
> Why not
> 
> micro = '0'
> if version_info.length() > 2
> micro = version_info[2].split('-')[0]
> endif
> 
> > Copy-paste issue, bad indentation. Fixed locally.
> > 
> > >  endif
> > > +
> > >  version_data = configuration_data()
> > >  version_data.set('SPICE_GTK_MAJOR_VERSION', major)
> > >  version_data.set('SPICE_GTK_MINOR_VERSION', minor)
> 
> Frediano
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 1/2] meson: fix micro on release tag

[Frediano Ziglio]

> 
> On Fri, Jan 18, 2019 at 10:57:05AM +0100, Victor Toso wrote:
> > From: Victor Toso 
> > 
> > On 0.36, there is no third value so we would get:
> > 
> > src/meson.build:11:0: ERROR:  Index 2 out of bounds of array of size 2.
> > 
> > Signed-off-by: Victor Toso 
> > ---
> >  src/meson.build | 11 ---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/meson.build b/src/meson.build
> > index d9614cb..bf39464 100644
> > --- a/src/meson.build
> > +++ b/src/meson.build
> > @@ -8,10 +8,15 @@ spice_gtk_include += [include_directories('.')]
> >  version_info = meson.project_version().split('.')
> >  major = '@0@'.format(version_info[0])
> >  minor = '@0@'.format(version_info[1])
> > -micro = version_info[2].split('-')[0]
> > -if micro == ''
> > -  micro = '0'
> > +if version_info.length() > 2
> > +micro = version_info[2].split('-')[0]
> > +if micro == ''

I doubt there would be a version with x.y.-z syntax

> > +micro = '0'
> > +endif
> > +else
> > +micro = '0'
> 

Why not

micro = '0'
if version_info.length() > 2
micro = version_info[2].split('-')[0]
endif

> Copy-paste issue, bad indentation. Fixed locally.
> 
> >  endif
> > +
> >  version_data = configuration_data()
> >  version_data.set('SPICE_GTK_MAJOR_VERSION', major)
> >  version_data.set('SPICE_GTK_MINOR_VERSION', minor)

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 1/2] meson: fix micro on release tag

[Victor Toso]

On Fri, Jan 18, 2019 at 10:57:05AM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> On 0.36, there is no third value so we would get:
> 
> src/meson.build:11:0: ERROR:  Index 2 out of bounds of array of size 2.
> 
> Signed-off-by: Victor Toso 
> ---
>  src/meson.build | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/src/meson.build b/src/meson.build
> index d9614cb..bf39464 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -8,10 +8,15 @@ spice_gtk_include += [include_directories('.')]
>  version_info = meson.project_version().split('.')
>  major = '@0@'.format(version_info[0])
>  minor = '@0@'.format(version_info[1])
> -micro = version_info[2].split('-')[0]
> -if micro == ''
> -  micro = '0'
> +if version_info.length() > 2
> +micro = version_info[2].split('-')[0]
> +if micro == ''
> +micro = '0'
> +endif
> +else
> +micro = '0'

Copy-paste issue, bad indentation. Fixed locally.

>  endif
> +
>  version_data = configuration_data()
>  version_data.set('SPICE_GTK_MAJOR_VERSION', major)
>  version_data.set('SPICE_GTK_MINOR_VERSION', minor)
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk 1/2] meson: fix micro on release tag

[Victor Toso]

From: Victor Toso 

On 0.36, there is no third value so we would get:

src/meson.build:11:0: ERROR:  Index 2 out of bounds of array of size 2.

Signed-off-by: Victor Toso 
---
 src/meson.build | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/meson.build b/src/meson.build
index d9614cb..bf39464 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -8,10 +8,15 @@ spice_gtk_include += [include_directories('.')]
 version_info = meson.project_version().split('.')
 major = '@0@'.format(version_info[0])
 minor = '@0@'.format(version_info[1])
-micro = version_info[2].split('-')[0]
-if micro == ''
-  micro = '0'
+if version_info.length() > 2
+micro = version_info[2].split('-')[0]
+if micro == ''
+micro = '0'
+endif
+else
+micro = '0'
 endif
+
 version_data = configuration_data()
 version_data.set('SPICE_GTK_MAJOR_VERSION', major)
 version_data.set('SPICE_GTK_MINOR_VERSION', minor)
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk 2/2] meson: mingw: add windows configuration

[Victor Toso]

From: Victor Toso 

Using fedora 29 mingw paths. Not hardcoding prefix but keeping a
comment that should be added in case one would like to install over
what they have in the system :)

Signed-off-by: Victor Toso 
---
 meson_mingw.txt | 18 ++
 1 file changed, 18 insertions(+)
 create mode 100644 meson_mingw.txt

diff --git a/meson_mingw.txt b/meson_mingw.txt
new file mode 100644
index 000..8a088fc
--- /dev/null
+++ b/meson_mingw.txt
@@ -0,0 +1,18 @@
+# Options for installing in a Fedora 29
+# meson win-build --cross-file meson_mingw.txt \
+# -Dprefix=/usr/x86_64-w64-mingw32/sys-root/mingw \
+# -Dlibdir=lib
+
+[binaries]
+c = '/usr/bin/x86_64-w64-mingw32-gcc'
+cpp = '/usr/bin/x86_64-w64-mingw32-g++'
+ar = '/usr/bin/x86_64-w64-mingw32-ar'
+strip = '/usr/bin/x86_64-w64-mingw32-strip'
+pkgconfig = '/usr/bin/x86_64-w64-mingw32-pkg-config'
+#exe_wrapper = 'wine' # A command used to run generated executables.
+
+[host_machine]
+system = 'windows'
+cpu_family = 'x86'
+cpu = 'x86_64'
+endian = 'little'
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent 7/8 v3] Send the GraphicsDeviceInfo to the server

[Lukáš Hrázký]

On Thu, 2019-01-17 at 14:29 -0600, Jonathon Jongsma wrote:
> On Wed, 2019-01-16 at 13:52 +0100, Lukáš Hrázký wrote:
> > Adds serialization of the GraphicsDeviceInfo message and sends it to
> > the
> > server when it starts to stream.
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> >  src/spice-streaming-agent.cpp | 54 +--
> > 
> >  1 file changed, 45 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> > agent.cpp
> > index 3024d98..891e4cb 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -31,6 +31,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -93,6 +94,33 @@ public:
> >  }
> >  };
> >  
> > +class DeviceDisplayInfoMessage : public
> > OutboundMessage > STREAM_TYPE_DEVICE_DISPLAY_INFO>
> > +{
> > +public:
> > +DeviceDisplayInfoMessage(const DeviceDisplayInfo ) :
> > OutboundMessage(info) {}
> > +
> > +static size_t size(const DeviceDisplayInfo )
> > +{
> > +return sizeof(PayloadType) +
> > std::min(info.device_address.length(), 255lu) + 1;
> 
> Where does this 255 come from? Can we at least use a constant or
> something?

Oh, yes. I should have put that into a constant.

The same limit is also in the server and the vd_agent, Frediano
suggested to put it into spice-protocol. I'm not entirely sure, as it
looks a bit ad-hoc and I didn't find a good place in spice-protocol to
put it in, so I didn't do it. Practically it will also probably never
have any impact...

> > +}
> > +
> > +void write_message_body(StreamPort _port, const
> > DeviceDisplayInfo )
> > +{
> > +std::string device_address = info.device_address;
> > +if (device_address.length() > 255) {
> > +syslog(LOG_WARNING,
> > +   "device address of stream id %u is longer than
> > 255 bytes, trimming.", info.stream_id);
> > +device_address = device_address.substr(0, 255);
> > +}
> > +StreamMsgDeviceDisplayInfo strm_msg_info{};
> > +strm_msg_info.stream_id = info.stream_id;
> > +strm_msg_info.device_display_id = info.device_display_id;
> > +strm_msg_info.device_address_len = device_address.length() +
> > 1;
> > +stream_port.write(_msg_info, sizeof(strm_msg_info));
> > +stream_port.write(device_address.c_str(),
> > device_address.length() + 1);
> > +}
> > +};
> > +
> >  static bool streaming_requested = false;
> >  static bool quit_requested = false;
> >  static std::set client_codecs;
> > @@ -217,17 +245,25 @@ do_capture(StreamPort _port, FrameLog
> > _log)
> >  throw std::runtime_error("cannot find a suitable capture
> > system");
> >  }
> >  
> > +std::vector display_info;
> >  try {
> > -std::vector display_info = capture-
> > > get_device_display_info();
> > 
> > -syslog(LOG_DEBUG, "Got device info of %lu devices from
> > the plugin", display_info.size());
> > -for (const auto  : display_info) {
> > -syslog(LOG_DEBUG, "   id %u: device address %s,
> > device display id: %u",
> > -   info.id,
> > -   info.device_address.c_str(),
> > -   info.device_display_id);
> > -}
> > +display_info = capture->get_device_display_info();
> >  } catch (const Error ) {
> > -syslog(LOG_ERR, "Error while getting device info: %s",
> > e.what());
> > +syslog(LOG_ERR, "Error while getting device display
> > info: %s", e.what());
> > +}
> > +
> > +syslog(LOG_DEBUG, "Got device info of %zu devices from the
> > plugin", display_info.size());
> > +for (const auto  : display_info) {
> > +syslog(LOG_DEBUG, "   stream id %u: device address: %s,
> > device display id: %u",
> > +   info.stream_id,
> > +   info.device_address.c_str(),
> > +   info.device_display_id);
> > +}
> > +
> > +if (display_info.size() > 0) {
> > +stream_port.send(display_info[
> > 0]);
> 
> Shouldn't we have some handling here in case size() is greater than 1?
> at least print a warning or something?

You mean if there are more than one display/monitor in the info vector
to print a warning we have more than one display and only stream the
first one? I'm not sure why, but I can add it... 

> > +} else {
> > +syslog(LOG_ERR, "Empty device display info from the
> > plugin");
> >  }
> >  
> >  while (!quit_requested && streaming_requested) {
> 
> 
> Reviewed-by: Jonathon Jongsma 
> 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent 6/8 v3] Interface + implementation of getting device display info

[Lukáš Hrázký]

On Thu, 2019-01-17 at 14:09 -0600, Jonathon Jongsma wrote:
> A couple minor comments below.
> 
> On Wed, 2019-01-16 at 13:52 +0100, Lukáš Hrázký wrote:
> > Adds an interface method to the FrameCapture class to get the device
> > display info (device address and device display id) for each display
> > of
> > the graphics device that is captured.
> > 
> > Also adds functions to the API implementing this functionality for
> > X11
> > in variants with and without DRM (the non-DRM version is rather
> > limited
> > and may not work for more complex setups) as well as some helper
> > functions to make it easier for plugins to implement this and avoid
> > code
> > duplication.
> > 
> > Implements the new interface method for the two built-in plugins
> > (mjpeg-fallback and gst-plugin).
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> >  configure.ac  |   2 +
> >  include/spice-streaming-agent/Makefile.am |   2 +
> >  .../spice-streaming-agent/display-info.hpp|  52 +++
> >  .../spice-streaming-agent/frame-capture.hpp   |  13 +
> >  .../x11-display-info.hpp  |  57 
> >  src/Makefile.am   |   7 +
> >  src/display-info.cpp  | 100 ++
> >  src/gst-plugin.cpp|  14 +
> >  src/mjpeg-fallback.cpp|  17 +-
> >  src/spice-streaming-agent.cpp |  13 +
> >  src/unittests/Makefile.am |   6 +
> >  src/utils.cpp |  46 +++
> >  src/utils.hpp |   4 +
> >  src/x11-display-info.cpp  | 302
> > ++
> >  14 files changed, 633 insertions(+), 2 deletions(-)
> >  create mode 100644 include/spice-streaming-agent/display-info.hpp
> >  create mode 100644 include/spice-streaming-agent/x11-display-
> > info.hpp
> >  create mode 100644 src/display-info.cpp
> >  create mode 100644 src/utils.cpp
> >  create mode 100644 src/x11-display-info.cpp
> > 
> > diff --git a/configure.ac b/configure.ac
> > index e66e6d8..fd18efe 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -34,8 +34,10 @@ SPICE_PROTOCOL_MIN_VER=0.12.14
> >  PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >=
> > $SPICE_PROTOCOL_MIN_VER])
> >  AC_SUBST([SPICE_PROTOCOL_MIN_VER])
> >  
> > +PKG_CHECK_MODULES(DRM, libdrm)
> >  PKG_CHECK_MODULES(X11, x11)
> >  PKG_CHECK_MODULES(XFIXES, xfixes)
> > +PKG_CHECK_MODULES(XRANDR, xrandr)
> >  
> >  PKG_CHECK_MODULES(JPEG, libjpeg, , [
> >  AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > diff --git a/include/spice-streaming-agent/Makefile.am
> > b/include/spice-streaming-agent/Makefile.am
> > index bcd679b..96c1a57 100644
> > --- a/include/spice-streaming-agent/Makefile.am
> > +++ b/include/spice-streaming-agent/Makefile.am
> > @@ -1,8 +1,10 @@
> >  NULL =
> >  public_includedir = $(includedir)/spice-streaming-agent
> >  public_include_HEADERS = \
> > +   display-info.hpp \
> > error.hpp \
> > frame-capture.hpp \
> > plugin.hpp \
> > +   x11-display-info.hpp \
> > $(NULL)
> >  
> > diff --git a/include/spice-streaming-agent/display-info.hpp
> > b/include/spice-streaming-agent/display-info.hpp
> > new file mode 100644
> > index 000..f16212b
> > --- /dev/null
> > +++ b/include/spice-streaming-agent/display-info.hpp
> > @@ -0,0 +1,52 @@
> > +/* \copyright
> > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > + */
> > +
> > +#ifndef SPICE_STREAMING_AGENT_DISPLAY_INFO_HPP
> > +#define SPICE_STREAMING_AGENT_DISPLAY_INFO_HPP
> > +
> > +#include 
> > +#include 
> > +
> > +
> > +namespace spice __attribute__ ((visibility ("default"))) {
> > +namespace streaming_agent {
> > +
> > +/**
> > + * Lists graphics cards listed in the DRM sybsystem in
> > /sys/class/drm.
> > + * Throws an instance of Error in case of an I/O error.
> > + *
> > + * @return a vector of paths of all graphics cards present in
> > /sys/class/drm
> > + */
> > +std::vector list_cards();
> > +
> > +/**
> > + * Reads a single number in hex format from a file.
> > + * Throws an instance of Error in case of an I/O or parsing error.
> > + *
> > + * @param path the path to the file
> > + * @return the number parsed from the file
> > + */
> > +uint32_t read_hex_number_from_file(const std::string );
> > +
> > +/**
> > + * Resolves any symlinks and then extracts the PCI path from the
> > canonical path
> > + * to a card. Returns the path in the following format:
> > + * "pci//./.../."
> > + *
> > + *  is the PCI domain, followed by . of any
> > PCI bridges
> > + * in the chain leading to the device. The last . is
> > the
> > + * graphics device. All of , ,  are
> > hexadecimal numbers
> > + * with the following number of digits:
> > + *   : 4
> > + *   : 2
> > + *   : 1
> > + *
> > + * @param device_path the path to the card
> > + * @return the device address
> > + */
> > +std::string get_device_address(const std::string _path);
> > +
> > +}} // namespace 

Re: [Spice-devel] [PATCH spice 5/8 v3] Send the graphics device info from streaming agent to the vd_agent

[Lukáš Hrázký]

On Thu, 2019-01-17 at 13:10 -0600, Jonathon Jongsma wrote:
> On Wed, 2019-01-16 at 13:52 +0100, Lukáš Hrázký wrote:
> > Adds the graphics device info from the streaming device(s) to the
> > VDAgentGraphicsDeviceInfo message sent to the vd_agent.
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> >  server/red-stream-device.c | 18 +++
> >  server/red-stream-device.h |  7 +
> >  server/reds.c  | 63
> > --
> >  3 files changed, 86 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> > index 6c90d77d..e7ed9f76 100644
> > --- a/server/red-stream-device.c
> > +++ b/server/red-stream-device.c
> > @@ -342,6 +342,8 @@ handle_msg_device_display_info(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  dev->device_display_info.device_address,
> >  dev->device_display_info.device_display_id);
> >  
> > +reds_send_device_display_info(red_char_device_get_server(RED_CHA
> > R_DEVICE(dev)));
> > +
> >  return true;
> >  }
> >  
> > @@ -799,6 +801,22 @@ stream_device_init(StreamDevice *dev)
> >  dev->msg_len = sizeof(*dev->msg);
> >  }
> >  
> > +StreamDeviceDisplayInfo
> > *stream_device_get_device_display_info(StreamDevice *dev)
> > +{
> > +return >device_display_info;
> > +}
> > +
> > +int32_t stream_device_get_stream_channel_id(StreamDevice *dev)
> > +{
> > +if (dev->stream_channel == NULL) {
> > +return -1;
> > +}
> > +
> > +int32_t channel_id;
> > +g_object_get(dev->stream_channel, "id", _id, NULL);
> > +return channel_id;
> > +}
> > +
> 
> Did you consider using a similar interface to the one used for the QXL
> displays? e.g. _get_device_address() / _get_device_display_ids() / etc?
> 
> I know that the stream device currently (and probably always) only has
> a single display per channel, but part of me wishes the interfaces had
> some consistency.  Not a big issue, just curious.

IIRC I did have a similar interface while I had it handle info for
multiple streams, but after I realized I'm not sure how multiple
streams will be implemented and dumbed it down to a single info
message, I simlified the interface as well. The interface also depends
on that. It is an interface internal to the server, so it can be
changed when multi-monitor support is implemented for streaming...

> Acked-by: Jonathon Jongsma 

Thanks,
Lukas

> >  static StreamDevice *
> >  stream_device_new(SpiceCharDeviceInstance *sin, RedsState *reds)
> >  {
> > diff --git a/server/red-stream-device.h b/server/red-stream-device.h
> > index 996be016..d7ab5e41 100644
> > --- a/server/red-stream-device.h
> > +++ b/server/red-stream-device.h
> > @@ -56,6 +56,13 @@ StreamDevice *stream_device_connect(RedsState
> > *reds, SpiceCharDeviceInstance *si
> >   */
> >  void stream_device_create_channel(StreamDevice *dev);
> >  
> > +StreamDeviceDisplayInfo
> > *stream_device_get_device_display_info(StreamDevice *dev);
> > +
> > +/**
> > + * Returns -1 if the StreamDevice doesn't have a channel yet.
> > + */
> > +int32_t stream_device_get_stream_channel_id(StreamDevice *dev);
> > +
> >  G_END_DECLS
> >  
> >  #endif /* STREAM_DEVICE_H */
> > diff --git a/server/reds.c b/server/reds.c
> > index 88a82419..22e81d73 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -902,14 +902,33 @@ void reds_send_device_display_info(RedsState
> > *reds)
> >  }
> >  g_debug("Sending device display info to the agent:");
> >  
> > -size_t message_size = sizeof(VDAgentGraphicsDeviceInfo);
> >  QXLInstance *qxl;
> > +RedCharDevice *dev;
> > +
> > +size_t message_size = sizeof(VDAgentGraphicsDeviceInfo);
> > +
> > +// size for the qxl device info
> >  FOREACH_QXL_INSTANCE(reds, qxl) {
> >  message_size +=
> >  (sizeof(VDAgentDeviceDisplayInfo) +
> > strlen(red_qxl_get_device_address(qxl)) + 1) *
> >  red_qxl_get_monitors_count(qxl);
> >  }
> >  
> > +// size for the stream device info
> > +GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
> > +if (IS_STREAM_DEVICE(dev)) {
> > +size_t device_address_len =
> > +strlen(stream_device_get_device_display_info(STREAM_
> > DEVICE(dev))->device_address);
> > +
> > +if (device_address_len == 0) {
> > +// the device info wasn't set (yet), don't send it
> > +continue;
> > +}
> > +
> > +message_size += (sizeof(VDAgentDeviceDisplayInfo) +
> > device_address_len + 1);
> > +}
> > +}
> > +
> >  RedCharDeviceWriteBuffer *char_dev_buf =
> > vdagent_new_write_buffer(reds->agent_dev,
> >   VD_AGENT_GRAPHICS_DEVICE_IN
> > FO,
> >   message_size,
> > @@ -925,6 +944,8 @@ void reds_send_device_display_info(RedsState
> > *reds)
> >  graphics_device_info->count = 0;
> >  
> >  

Re: [Spice-devel] [PATCH spice-protocol 2/8 v3] Add the StreamMsgGraphicsDeviceInfo message

[Lukáš Hrázký]

Hi,

On Thu, 2019-01-17 at 11:23 -0600, Jonathon Jongsma wrote:
> Accidentally sent this on the v2 series, but I'll repeat it here:
> 
> Commit subject mentions the wrong type name?
> StreamMsgGraphicsDeviceInfo != StreamMsgDeviceDisplayInfo

You're right, I'll fix it.

> On Wed, 2019-01-16 at 13:52 +0100, Lukáš Hrázký wrote:
> > The message contains information about the graphics device and
> > monitor
> > belonging to a particular video stream (which maps to a channel) from
> > the streaming agent.
> > 
> > Signed-off-by: Lukáš Hrázký 
> > Acked-by: Frediano Ziglio 
> > ---
> >  spice/stream-device.h | 31 +++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/spice/stream-device.h b/spice/stream-device.h
> > index 6add42b..c70690a 100644
> > --- a/spice/stream-device.h
> > +++ b/spice/stream-device.h
> > @@ -90,6 +90,8 @@ typedef enum StreamMsgType {
> >  STREAM_TYPE_CURSOR_SET,
> >  /* guest cursor position */
> >  STREAM_TYPE_CURSOR_MOVE,
> > +/* the graphics device display information message (device
> > address and display id) */
> > +STREAM_TYPE_DEVICE_DISPLAY_INFO,
> >  } StreamMsgType;
> >  
> >  typedef enum StreamCapabilities {
> > @@ -140,6 +142,35 @@ typedef struct StreamMsgData {
> >  uint8_t data[0];
> >  } StreamMsgData;
> >  
> > +/* This message contains information about the graphics device and
> > monitor
> > + * belonging to a particular video stream (which maps to a channel)
> > from
> > + * the streaming agent.
> > + *
> > + * The device_address is the hardware address of the device (e.g.
> > PCI),
> > + * device_display_id is the id of the monitor on the device.
> > + *
> > + * The supported device address format is:
> > + * "pci//./.../."
> > + *
> > + * The "pci" identifies the rest of the string as a PCI address. It
> > is the only
> > + * supported address at the moment, other identifiers can be
> > introduced later.
> > + *  is the PCI domain, followed by . of any
> > PCI bridges
> > + * in the chain leading to the device. The last . is
> > the
> > + * graphics device. All of , ,  are
> > hexadecimal numbers
> > + * with the following number of digits:
> > + *   : 4
> > + *   : 2
> > + *   : 1
> > + *
> > + * Sent from the streaming agent to the server.
> > + */
> > +typedef struct StreamMsgDeviceDisplayInfo {
> > +uint32_t stream_id;
> > +uint32_t device_display_id;
> > +uint32_t device_address_len;
> > +uint8_t device_address[0];  // a zero-terminated string
> > +} StreamMsgDeviceDisplayInfo;
> > +
> >  /* Tell to stop current stream and possibly start a new one.
> >   * This message is sent by the host to the guest.
> >   * Allows to communicate the codecs supported by the clients.
> 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer

[Yuri Benditovich]

On Wed, Jan 16, 2019 at 6:11 PM Christophe Fergeau 
wrote:

> On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:
> > On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau 
> > wrote:
> > > In _SpiceUsbDeviceManagerPrivate, you replaced
> > > #ifndef G_OS_WIN32
> > > libusb_device *libdev;
> > > #endif
> > >
> > > with
> > >
> > > #ifndef G_OS_WIN32
> > > SpiceUsbBackendDevice *bdev;
> > > #endif
> > >
> > > The #ifdef is there because of this comment in
> > > spice_usb_device_manager_device_to_bdev:
> > >
> > >   /*
> > >* On win32 we need to do this the hard and slow way, by asking
> libusb to
> > >* re-enumerate all devices and then finding a matching device.
> > >* We cannot cache the libusb_device like we do under Linux since the
> > >* driver swap we do under windows invalidates the cached libdev.
> > >*/
> > >
> > > After your patch, spice_usb_device_manager_device_to_bdev is no longer
> > > at the right level of indirection imo, it looks up the 'bdev' when
> > > needed on Windows in usb-device-manager.c, but my understanding of that
> > > comment is that any libusb call within SpiceUsbBackendDevice should not
> > > use a cached libusb_device?
> > >
> > >
> > >
> > As fas as I understand, there is no change in the level of indirection.
> > Previously was:
> > On Linux: usb-device-manager receives libusb device on hotplug indication
> > and uses it until the device disappears.
> > On Windows: each time the usb-device-manager needs the libusb device it
> > takes fresh list of libusb devices and finds one according to known
> device
> > properties.
> >
> > Now:
> > On Linux: usb-device-manager receives backend device wrapping libusb
> device
> > on hotplug indication and uses it until the device disappears
> > On Windows: each time the usb-device-manager needs the backend device it
> > takes fresh list of new backend devices and finds one according to known
> > device properties.
> > So, the backend device is always fresh one and it always have fresh
> libusb
> > device.
> >
> > This can be simplified with removal of all these lookups in the
> > usb-device-manager (and I already illustrated how) but after this commit
> is
> > done.
> > If from your point of view this is the requirement to existing commit,
> > please let me know (then we will need to reevaluate our plans and our
> > ability to deliver the patches in reasonable time).
>
> I'll quote again the comment:
>
>/*
> * On win32 we need to do this the hard and slow way, by asking libusb
> to
> * re-enumerate all devices and then finding a matching device.
> * We cannot cache the libusb_device like we do under Linux since the
> * driver swap we do under windows invalidates the cached libdev.
> */
>
> It says we cannot cache a libusb_device because it can change behind our
> back, so we need to reenumerate all devices and lookup the right
> libusb_device before every call to libusb.
> Before your changes, we did not hold any long-lasting references to a
> libusb_device on Windows, so we were guaranteed to do that lookup before
> every call. After your changes, SpiceUsbBackendDevice is holding a
> long-lasting reference to a libusb_device, but does not care about the
> win32 issue. Only usb-device-manager is making sure the libusb_device
> it's going to use is valid. This does not make sense to me to leak this
> implementation detail to usb-device-manager. SpiceUsbBackend should hide
> it, otherwise its API is going to be error-prone.
>
> When you say this can be simplified by removing this lookup, are you
> confirming that this comment I quoted above is obsolete?
>

This comment, IMO, is wrong and permanent enumeration is not mandatory.
None of existing objects can disappear if it is is referenced.
But the object must be used with proper libusb context.

Changes in existing patch are intentionally minimal to allow comparison
between
previous and existing code. After the current patch is merged I will be
able to solve
the problem of persistency as I should be, I believe (together with
unification of the
structure holding device properties). But mixing all these things together
discards
the current patch and I can't predict the time frame of next round.


> Christophe
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel