Re: [PATCH] Take shift in crtc positions for ZaphodHeads configs into account.

[Mario Kleiner <mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Ping? Could we merge this patch? Nobody seems to have objections, it 
works since months on my test machines and an identical patch got 
reviewed and merged into xf86-video-modesetting, where it works without 
trouble, so it should be safe to apply.

thanks,
-mario

On 08/06/2015 07:34 PM, Ilia Mirkin wrote:
> I don't understand this patch (what are all these masks? how are they
> used?), and don't want to invest the time required to do so.
>
> However Mario is probably the sole serious user of ZaphodHeads, and if
> it fixes issues for him, probably fixes issues for others who try and
> give up with ZaphodHeads. Any objections if I just push this out?
>
> On Sat, Jun 27, 2015 at 8:33 PM, Mario Kleiner
> <mario.kleiner.de@gmail.com> wrote:
>> In multi-x-screen ZaphodHeads configurations, there isn't a
>> one-to-one mapping of kernel provided drmmode crtc index
>> to the index of the corresponding xf86Crtc inside the
>> xf86CrtcConfig crtc array anymore, ie. for kernel provided
>> drmmode->mode_res->crtcs[i], the i'th crtc won't correspond
>> to the xf86Crtc in the i'th slot of the x-screens xf86CrtcConfig
>> anymore, once ZaphodHeads has only selected a subset of all crtcs
>> of a graphics card for a given x-screen, instead of all crtcs.
>>
>> This breaks the mapping of bit positions in the bit masks returned
>> in kencoder->possible_crtcs and kencoder->possible_clones. A 1 bit
>> in position i of those masks allows use of the kernels i'th crtc for
>> the given kencoder. The X-Servers dix code checks those bit masks
>> for valid xf86Output -> xf86Crtc assignments, assuming that the i'th
>> slot xf86CrtcConfigPtr config->crtc[i] corresponds to bit i in the
>> xf86Output->possibe_crtcs bitmask, and bails if the bitmask doesn't
>> allow the specified assignment of crtc to output. If ZaphodHeads
>> breaks the assumption of bit i <-> crtc slot i this ends in failure.
>>
>> Take this shift of crtc index positions wrt. encoder bitmask bit
>> positions into account by bit-shifting positions accordingly when
>> assigning encoder->possible_crtcs to output->possible_crtcs, so
>> the proper indices match up again for validation by the dix.
>>
>> This problem wasn't apparent last year when testing the ZaphodHeads
>> support on some Kepler cards, as apparently the encoder->possible_crtcs
>> bitmasks returned for those cards by the kernel just had all 4
>> lsb bits set for all tested encoders/output, so each of the cards 4
>> crtcs could go with each output and things worked by chance.
>>
>> The current code breaks, e.g., on 2010 MacBookPro with nv50, where
>> one crtc is hardwired to the internal lvds panel, and one crtc
>> is hardwired to the external DP connector, resulting in a failure
>> where dual-display on single-x-screen works fine, but assigning
>> each output to a separate x-screen via ZaphodHeads fails due to
>> the mismatched encoder->possible_crtcs bitmasks.
>>
>> This patch fixes the problem.
>>
>> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
>> ---
>>   src/drmmode_display.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>> index c30cb3a..3679482 100644
>> --- a/src/drmmode_display.c
>> +++ b/src/drmmode_display.c
>> @@ -1214,7 +1214,7 @@ drmmode_zaphod_match(ScrnInfoPtr pScrn, const char *s, char *output_name)
>>   }
>>
>>   static unsigned int
>> -drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int num)
>> +drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int num, int crtcshift)
>>   {
>>          NVPtr pNv = NVPTR(pScrn);
>>          xf86OutputPtr output;
>> @@ -1296,8 +1296,8 @@ drmmode_output_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int num)
>>          output->subpixel_order = subpixel_conv_table[koutput->subpixel];
>>          output->driver_private = drmmode_output;
>>
>> -       output->possible_crtcs = kencoder->possible_crtcs;
>> -       output->possible_clones = kencoder->possible_clones;
>> +       output->possible_crtcs = kencoder->possible_crtcs >> crtcshift;
>> +       output->possible_clones = kencoder->possible_clones >> crtcshift;
>>
>>          output->interlaceAllowed = true;
>>          output->doubleScanAllowed = true;
>> @@ -1421,6 +1421,7 @@ Bool drmmode_pre_init(ScrnInfoPtr pScrn, int fd, int cpp)
>>          NVEntPtr pNVEnt = NVEntPriv(pScrn);
>>          int i;
>>          unsigned int crtcs_needed = 0;
>> +       int crtcshift;
>>
>>          drmmode = xnfalloc(sizeof *drmmode);
>>          drmmode->fd = fd;
>> @@ -1444,8 +1445,9 @@ Bool drmmode_pre_init(ScrnInfoPtr pScrn, int fd, int cpp)
>>          }
>>
>>          xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Initializing outputs ...\n");
>> +       crtcshift = ffs(pNVEnt->assigned_crtcs ^ 0xffffffff) - 1;
>
> Mario, any objections if I touch this up as
>
> ~pNVEnc->assigned_crtcs? XOR with ~0 is not a pattern I've seen a lot.
>
>>          for (i = 0; i < drmmode->mode_res->count_connectors; i++)
>> -               crtcs_needed += drmmode_output_init(pScrn, drmmode, i);
>> +               crtcs_needed += drmmode_output_init(pScrn, drmmode, i, crtcshift);
>>
>>          xf86DrvMsg(pScrn->scrnIndex, X_INFO,
>>                     "%d crtcs needed for screen.\n", crtcs_needed);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau