[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged
On Thu, Nov 03, 2016 at 06:42:38PM +, Emil Velikov wrote:
> Hi guys,
>
> On 2 November 2016 at 16:31, Christophe Fergeau
> wrote:
> > 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.
> >
> Kernel coding style advises against both typedefs and CamelCase. We do
> have a few "offenders" but it's better to not add more.
>
> Ftw when in doubt do search/grep through the document - I don't thinks
> there's many people who've read the thing in one go :-)
Ah thanks, I'll get rid of the CamelCase!
Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL:
[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged
Hi guys,
On 2 November 2016 at 16:31, Christophe Fergeau wrote:
> 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.
>
Kernel coding style advises against both typedefs and CamelCase. We do
have a few "offenders" but it's better to not add more.
Ftw when in doubt do search/grep through the document - I don't thinks
there's many people who've read the thing in one go :-)
Thanks
Emil
[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged
On Wed, Nov 02, 2016 at 05:31:28PM +0100, Christophe Fergeau wrote:
> >
> > > + 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 ;)
At first I changed the commit to exclude flags and surface_id from these
checks, but on second though, I'd keep them the same way as the rest,
this way if these can get changed to a different value outside of
copy_rom, this code will still work as intended.
Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL:
[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged
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 *)>rom->client_monitors_config,
> > @@ -88,17 +99,42 @@ static int
> > qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
> > >rom->client_monitors_config.heads[i];
> > struct qxl_head *client_head =
> > >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:
[Spice-devel] [drm/qxl 5/6] qxl: Don't notify userspace when monitors config is unchanged
>
> When the QXL driver receives a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
> interrupt,
> we currently always notify userspace that there was some hotplug event.
>
> However, gnome-shell/mutter is reacting to this event by attempting a
> resolution change, which it does by issueing drmModeRmFB, drmModeAddFB,
> and then drmModeSetCrtc. This has the side-effect of causing
> qxl_crtc_mode_set() to tell the QXL virtual hardware that a primary
> surface was destroyed and created. After going through QEMU and then the
> remote SPICE client, a new identical monitors config message will be
> sent, resulting in a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt to
> be emitted, and the same scenario occurring again.
>
> As destroying/creating the primary surface causes a visible screen
> flicker, this makes the guest hard to use (
> https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ).
>
> This commit checks if the screen configuration we received is the same
> one as the current one, and does not notify userspace about it if that's
> the case.
>
> Signed-off-by: Christophe Fergeau
Great message!
> ---
> drivers/gpu/drm/qxl/qxl_display.c | 65
> +--
> 1 file changed, 55 insertions(+), 10 deletions(-)
>
> 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.
> static int qxl_display_copy_rom_client_monitors_config(struct qxl_device
> *qdev)
why not returning MonitorsConfigCopyStatus ?
> {
> 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 *)>rom->client_monitors_config,
> @@ -70,7 +77,7 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
> qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
> sizeof(qdev->rom->client_monitors_config),
> qdev->rom->client_monitors_config_crc);
> - return 1;
> + return MONITORS_CONFIG_BAD_CRC;
> }
> if (num_monitors > qdev->monitors_config->max_allowed) {
> DRM_DEBUG_KMS("client monitors list will be truncated: %d <
> %d\n",
> @@ -79,6 +86,10 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
> } else {
> num_monitors = qdev->rom->client_monitors_config.count;
> }
> + if (qdev->client_monitors_config
> + && (num_monitors != qdev->client_monitors_config->count)) {
> + changed = true;
> + }
> qxl_alloc_client_monitors_config(qdev, num_monitors);
> /* we copy max from the client but it isn't used */
> qdev->client_monitors_config->max_allowed =
> @@ -88,17 +99,42 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
> >rom->client_monitors_config.heads[i];
> struct qxl_head *client_head =
> >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) {
> +
