[Spice-devel] [QXL PATCH 1/2] Make output name numbering 0-based

[Jonathon Jongsma]

On Fri, 2018-10-26 at 03:30 -0400, Frediano Ziglio wrote:
> I think you mean "Make output name numbering 1-based".
> 
> > The QXL driver names its outputs starting at 0 (e.g. Virtual-0,
> > Virtual-1, etc). This code was presumably copy/pasted from a
> > different
> > driver, and is not necessary for the QXL driver. Other drivers
> > simply
> > use the kernel connector_type_id which starts at 1. For example,
> > the
> > modsetting driver changed from 0-based names to 1-based names for
> > the
> 
> typo: modesetting
> 
> > same reason in xserver commit 139e36dd.
> > 
> > This will help to make it easier to identify which xrandr outputs
> > belong
> > to which drm connector without requiring as many driver-specific
> > special-cases.
> > 
> > This change might effect custom xorg configurations that references
> > a
> > specific output name. But the same change was made in modesetting
> > driver
> > despite that possibility.
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> > ---
> >  src/qxl_drmmode.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/src/qxl_drmmode.c b/src/qxl_drmmode.c
> > index a2f84b1..a814859 100644
> > --- a/src/qxl_drmmode.c
> > +++ b/src/qxl_drmmode.c
> > @@ -765,8 +765,7 @@ drmmode_output_init(ScrnInfoPtr pScrn,
> > drmmode_ptr
> > drmmode, int num)
> >  		}
> >  	}
> >  
> > -	/* need to do smart conversion here for compat with non-kms ATI
> > driver */
> > -	snprintf(name, 32, "%s-%d", output_names[koutput-
> > >connector_type],
> > koutput->connector_type_id - 1);
> > +	snprintf(name, 32, "%s-%d", output_names[koutput-
> > >connector_type],
> > koutput->connector_type_id);
> >  	
> >  
> >  	output = xf86OutputCreate (pScrn, &drmmode_output_funcs, name);
> 
> Otherwise looks good.
> 
> However is it possible that you have to handle both cases (0-index
> and
> 1-index) due to different packages installations. Is not guaranteed
> that
> you'll have the new Xorg driver installed so this code instead of
> making
> it easier will make more complicated.

Yes, this change will theoretically make it slightly more complicated.
But the impact is pretty small I think. For example, in the case that
I'm working on right now (finding the xrandr output that matches a drm
output), the impact might be:

Without this patch:
 *    Loop through the xrandr outputs and see if any of them is named
      'Virtual-0'.
 * If there is such an output, increment the counter before comparing
      against the drm output name

With this patch:
 *    The algorithm above still works.

If you instead chose to handle this case by looking exclusively at the
vendor_id/device_id of the device, then that does get more difficult
after this change. But it's pretty minor IMO.

Jonathon