On 03/23/2018 01:24 PM, Victor Toso wrote:
Hi,
On Tue, Mar 20, 2018 at 05:24:42PM +0200, Uri Lublin wrote:
This was true for virtual graphic cards, but not true for
device-assigned graphic cards.
For example NVIDIA M2000 has 8.
Still we currently pick only a single crtc for each output
(but looping over them to find an active one)
Signed-off-by: Uri Lublin <uril@xxxxxxxxxx>
I got a [-Werror=maybe-uninitialized] here:
src/vdagent/x11-randr.c: In function ‘get_current_mon_config’:
src/vdagent/x11-randr.c:686:46: error: ‘crtc’ may be used uninitialized in this function
mon_config->monitors[i].x = crtc->x;
~~~~^~~
Quite weird, it does not realize that we checked ncrtc != 0
before, which means that crtc_from_id() will be called at least
once!
A suggestion patch to squash is attached with the suggestions I'm
giving inline.
Thanks, I'll take it.
---
src/vdagent/x11-randr.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
index aade5ca..0c196de 100644
--- a/src/vdagent/x11-randr.c
+++ b/src/vdagent/x11-randr.c
@@ -58,6 +58,8 @@ static XRRCrtcInfo *crtc_from_id(struct vdagent_x11 *x11, int id)
{
int i;
+ if (id == 0)
+ return NULL;
Brackets! Not that important but I get that for new code we
should try to use brackets.
Yes, I'll add them.
Thanks,
Uri.
Out of curiosity:
* Number of if without brackets: 600
# grep -rniI "if.*(.*" * | grep * -v "{" | wc
* Number of if with brackets: 409
$ grep -rniI "if.*(.*{" * | wc
So, without brackets is actually majority in vd_agent :)
for (i = 0 ; i < x11->randr.res->ncrtc ; ++i) {
if (id == x11->randr.res->crtcs[i]) {
return x11->randr.crtcs[i];
@@ -650,7 +652,7 @@ static int config_size(int num_of_monitors)
static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
{
- int i, num_of_monitors = 0;
+ int i, j, num_of_monitors = 0;
I would move both j and *crtc inside the for block and initialize
crtc to NULL to avoid the warning.
XRRModeInfo *mode;
XRRCrtcInfo *crtc;
XRRScreenResources *res = x11->randr.res;
@@ -665,10 +667,15 @@ static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
for (i = 0 ; i < res->noutput; i++) {
if (x11->randr.outputs[i]->ncrtc == 0)
continue; /* Monitor disabled, already zero-ed by calloc */
- if (x11->randr.outputs[i]->ncrtc != 1)
- goto error;
+ if (x11->randr.outputs[i]->crtc == 0)
+ continue; /* Monitor disabled */
- crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[0]);
+ for (j=0; j<x11->randr.outputs[i]->ncrtc; j++) {
+ crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[j]);
+ if (crtc) {
+ break;
+ }
You can add the crtc check in the for (...; crtc != NULL && ;...)
as well.
Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>
+ }
if (!crtc)
goto error;
--
2.14.3
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel