[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

Hey,

On Mon, Oct 31, 2016 at 08:00:09AM -0400, Frediano Ziglio wrote:
> > diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> > b/drivers/gpu/drm/qxl/qxl_display.c
> > index 156b7de..edb90f6 100644
> > --- a/drivers/gpu/drm/qxl/qxl_display.c
> > +++ b/drivers/gpu/drm/qxl/qxl_display.c
> > @@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct
> > qxl_device *qdev, unsigned c
> >  	qdev->client_monitors_config->count = count;
> >  }
> >  
> > +enum MonitorsConfigCopyStatus {
> > +	MONITORS_CONFIG_COPIED,
> > +	MONITORS_CONFIG_UNCHANGED,
> > +	MONITORS_CONFIG_BAD_CRC,
> > +};
> > +
> 
> I don't remember exactly kernel style, a 
> 
> typedef enum {
> 	MONITORS_CONFIG_COPIED,
> 	MONITORS_CONFIG_UNCHANGED,
> 	MONITORS_CONFIG_BAD_CRC,
> } MonitorsConfigCopyStatus;
> 
> could make following code shorter.

A git grep enum in qxl/ returns a dozen results, none of these using
typedef, I guess I just followed that style.

> 
> >  static int qxl_display_copy_rom_client_monitors_config(struct qxl_device
> >  *qdev)
> 
> why not returning MonitorsConfigCopyStatus ?

No idea ;) I'll change the patch.

> 
> >  {
> >  	int i;
> >  	int num_monitors;
> >  	uint32_t crc;
> > +	bool changed = false;
> >  
> 
> using a "MonitorsConfigCopyStatus res = MONITORS_CONFIG_UNCHANGED" here
> could make return statement shorter.
> 
> >  	num_monitors = qdev->rom->client_monitors_config.count;
> >  	crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config,
> > @@ -88,17 +99,42 @@ static int
> > qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
> >  			&qdev->rom->client_monitors_config.heads[i];
> >  		struct qxl_head *client_head =
> >  			&qdev->client_monitors_config->heads[i];
> > -		client_head->x = c_rect->left;
> > -		client_head->y = c_rect->top;
> > -		client_head->width = c_rect->right - c_rect->left;
> > -		client_head->height = c_rect->bottom - c_rect->top;
> > -		client_head->surface_id = 0;
> > -		client_head->id = i;
> > -		client_head->flags = 0;
> > +		if (client_head->x != c_rect->left) {
> > +			client_head->x = c_rect->left;
> > +			changed = true;
> > +		}
> > +		if (client_head->y != c_rect->top) {
> > +			client_head->y = c_rect->top;
> > +			changed = true;
> > +		}
> > +		if (client_head->width != c_rect->right - c_rect->left) {
> > +			client_head->width = c_rect->right - c_rect->left;
> > +			changed = true;
> > +		}
> > +		if (client_head->height != c_rect->bottom - c_rect->top) {
> > +			client_head->height = c_rect->bottom - c_rect->top;
> > +			changed = true;
> > +		}
> > +		if (client_head->surface_id != 0) {
> > +			client_head->surface_id = 0;
> > +			changed = true;
> > +		}
> > +		if (client_head->id != i) {
> > +			client_head->id = i;
> > +			changed = true;
> > +		}
> 
> quite similar code... I would write a macro but I'm a too macro fun :)
> Would be something like this
> 
> 	if (client_head->id != i)
> 		res = MONITORS_CONFIG_COPIED;
> 	client_head->id = i;
> 
> make sense?

I'm not a big macro fan, especially if they have side effects, so I
preferred to keep things explicit, even though I am annoyed by the
repetitive code too /o\


> 
> > +		if (client_head->flags != 0) {
> > +			client_head->flags = 0;
> > +			changed = true;
> > +		}
> 
> why testing flags change if is always 0 ?

Yeah, same for surface_id above actually. Just mechanically changed
everything ;)

> 
> Usually I would write something like
> 
> 	for (;;) {
> 		status = qxl_display_copy_rom_client_monitors_config(qdev);
> 		if (status != MONITORS_CONFIG_BAD_CRC)
> 			break;
> 		qxl_io_log(qdev, "failed crc check for client_monitors_config,"
> 				 " retrying\n");
> 	}
> 
> or
> 
> 	while ((status = qxl_display_copy_rom_client_monitors_config(qdev))
> 		==  MONITORS_CONFIG_BAD_CRC) {
> 		qxl_io_log(qdev, "failed crc check for client_monitors_config,"
> 				 " retrying\n");
> 	}
> 
> (just style and probably indentation is even wrong)

Same as above, I don't like either, first one obscures the loop exit
condition, and second one makes the assignment/test harder to read.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161102/0ebe12cf/attachment.sig>