Re: [PATCH 1/2] server: allows to set maximum monitors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> Hey,
> 
> 
> Looks good to me, a few comments below. A bit worried that the guest
> does not really know about the maximum number of monitors and about
> truncating monitors info behind its back, but I don't have actual issues
> to point out... (well, with an UMS driver, you can use xrandr to enable
> more monitors than available, the guest OS will see them, but you won't
> be able to display them with remote-viewer).
> So ACK from me with the smaller issues fixed
> 
> Christophe
> 
> On Fri, Jun 12, 2015 at 03:17:37PM +0100, Frediano Ziglio wrote:
> > spice-server will attempt to limit number of monitors.
> > Guest machine can send monitor list it accepts. Limiting the number sent
> > by guest will limit the number of monitors client will try to enable.
> > The guest usually see client monitors enabled and start using it so
> > not seeing client monitor won't try to enable more monitor.
> > In this case the additional monitor guest can support will always be
> > seen as heads with no attached monitors.
> > This allows limiting monitors number without changing guest drivers.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> >  server/red_dispatcher.c  | 11 +++++++++++
> >  server/red_dispatcher.h  |  1 +
> >  server/red_worker.c      | 12 +++++++-----
> >  server/spice-qxl.h       |  3 +++
> >  server/spice-server.syms |  5 +++++
> >  5 files changed, 27 insertions(+), 5 deletions(-)
> > 
> > diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> > index f5f3e52..3838bbb 100644
> > --- a/server/red_dispatcher.c
> > +++ b/server/red_dispatcher.c
> > @@ -66,6 +66,7 @@ struct RedDispatcher {
> >      Ring async_commands;
> >      pthread_mutex_t  async_lock;
> >      QXLDevSurfaceCreate surface_create;
> > +    unsigned max_monitors;
> 
> 'unsigned int' (applies throughout this patch)
> 

Never saw a compiler complaining about it. Anyway, if is a coding style rule I'll update it.

> >  };
> >  
> >  extern uint32_t streaming_video;
> > @@ -693,6 +694,7 @@ static void
> > red_dispatcher_monitors_config_async(RedDispatcher *dispatcher,
> >      payload.base.cmd = async_command_alloc(dispatcher, message, cookie);
> >      payload.monitors_config = monitors_config;
> >      payload.group_id = group_id;
> > +    payload.max_monitors = dispatcher->max_monitors;
> >  
> >      dispatcher_send_message(&dispatcher->dispatcher, message, &payload);
> >  }
> > @@ -987,6 +989,13 @@ void spice_qxl_monitors_config_async(QXLInstance
> > *instance, QXLPHYSICAL monitors
> >  }
> >  
> >  SPICE_GNUC_VISIBLE
> > +void spice_qxl_set_monitors_config_limit(QXLInstance *instance,
> > +                                         unsigned max_monitors)
> 
> set_max_monitors() maybe ? But the name you used is fine too.
> unsigned int rather than unsigned.
> 

Ok, I like shorter names.

> > +{
> > +    instance->st->dispatcher->max_monitors = MAX(1u, max_monitors);
> > +}
> > +
> > +SPICE_GNUC_VISIBLE
> >  void spice_qxl_driver_unload(QXLInstance *instance)
> >  {
> >      red_dispatcher_driver_unload(instance->st->dispatcher);
> > @@ -1110,6 +1119,8 @@ void red_dispatcher_init(QXLInstance *qxl)
> >      red_dispatcher->base.destroy_surface_wait =
> >      qxl_worker_destroy_surface_wait;
> >      red_dispatcher->base.loadvm_commands = qxl_worker_loadvm_commands;
> >  
> > +    red_dispatcher->max_monitors = UINT16_MAX;
> > +
> 
> Why UINT16_MAX ?
> 

16 bit are used for same information in different structure. But UINT_MAX will do the same.

> >      qxl->st->qif->get_init_info(qxl, &init_info);
> >  
> >      init_data.memslot_id_bits = init_info.memslot_id_bits;
> > diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
> > index 907b7c7..70b8a62 100644
> > --- a/server/red_dispatcher.h
> > +++ b/server/red_dispatcher.h
> > @@ -200,6 +200,7 @@ typedef struct RedWorkerMessageMonitorsConfigAsync {
> >      RedWorkerMessageAsync base;
> >      QXLPHYSICAL monitors_config;
> >      int group_id;
> > +    unsigned int max_monitors;
> >  } RedWorkerMessageMonitorsConfigAsync;
> >  
> >  typedef struct RedWorkerMessageDriverUnload {
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 5deb30b..564bfba 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -11234,11 +11234,13 @@ static inline void
> > red_monitors_config_item_add(DisplayChannelClient *dcc)
> >  }
> >  
> >  static void worker_update_monitors_config(RedWorker *worker,
> > -                                          QXLMonitorsConfig
> > *dev_monitors_config)
> > +                                          QXLMonitorsConfig
> > *dev_monitors_config,
> > +                                          unsigned max_monitors)
> >  {
> >      int heads_size;
> >      MonitorsConfig *monitors_config;
> >      int i;
> > +    unsigned count = MIN(dev_monitors_config->count, max_monitors);
> >  
> >      monitors_config_decref(worker->monitors_config);
> >  
> > @@ -11252,13 +11254,13 @@ static void
> > worker_update_monitors_config(RedWorker *worker,
> >                      dev_monitors_config->heads[i].width,
> >                      dev_monitors_config->heads[i].height);
> >      }
> > -    heads_size = dev_monitors_config->count * sizeof(QXLHead);
> > +    heads_size = count * sizeof(QXLHead);
> >      worker->monitors_config = monitors_config =
> >          spice_malloc(sizeof(*monitors_config) + heads_size);
> >      monitors_config->refs = 1;
> >      monitors_config->worker = worker;
> > -    monitors_config->count = dev_monitors_config->count;
> > -    monitors_config->max_allowed = dev_monitors_config->max_allowed;
> > +    monitors_config->count = count;
> > +    monitors_config->max_allowed = MIN(dev_monitors_config->max_allowed,
> > max_monitors);
> >      memcpy(monitors_config->heads, dev_monitors_config->heads,
> >      heads_size);
> >  }
> >  
> > @@ -11668,7 +11670,7 @@ static void handle_dev_monitors_config_async(void
> > *opaque, void *payload)
> >                        dev_monitors_config->max_allowed);
> >          return;
> >      }
> > -    worker_update_monitors_config(worker, dev_monitors_config);
> > +    worker_update_monitors_config(worker, dev_monitors_config,
> > msg->max_monitors);
> >      red_worker_push_monitors_config(worker);
> >  }
> >  
> > diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> > index 31ff742..eed4b54 100644
> > --- a/server/spice-qxl.h
> > +++ b/server/spice-qxl.h
> > @@ -97,6 +97,9 @@ void spice_qxl_monitors_config_async(QXLInstance
> > *instance, QXLPHYSICAL monitors
> >                                       int group_id, uint64_t cookie);
> >  /* since spice 0.12.3 */
> >  void spice_qxl_driver_unload(QXLInstance *instance);
> > +/* since spice 0.12.6 */
> > +void spice_qxl_set_monitors_config_limit(QXLInstance *instance,
> > +                                         unsigned max_monitors);
> >  
> >  typedef struct QXLDrawArea {
> >      uint8_t *buf;
> > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > index 2193811..eba3c83 100644
> > --- a/server/spice-server.syms
> > +++ b/server/spice-server.syms
> > @@ -153,3 +153,8 @@ global:
> >      spice_server_get_best_record_rate;
> >      spice_server_set_record_rate;
> >  } SPICE_SERVER_0.12.4;
> > +
> > +SPICE_SERVER_0.12.6 {
> > +global:
> > +    spice_qxl_set_monitors_config_limit;
> > +} SPICE_SERVER_0.12.5;
> > --
> > 2.1.0
> > 

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]