[Nouveau] [PATCH] Take shift in crtc positions for ZaphodHeads configs into account.
Mario Kleiner
mario.kleiner.de at gmail.com
Thu Aug 6 11:44:37 PDT 2015
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.
>
I can't blame you for that :). The confused commit message reflects my
confusion when i was trying to figure out how this (didn't) work on some
few NVidia cards, while working on most of them ;) - Took me three
separate attempts to find out the why, because the error messages from
server and kernel send you in all kinds of random directions of what the
cause of the failure could be.
Anyway. The kencoder->possible_crtcs mask encodes which hardware crtcs
can go with the given kencoder. i'th bit set == i'th crtc usable for
that encoder. The mask comes from nouveau-kms which gets it from the
VBIOS DCB table. Most cards seem to just have 1 bits everywhere, so any
crtc can go with any encoder. Some cards like the one in my Laptop have
hard-wired encoder->crtc, so there can be 0 bits.
During crtc enumeration when initializing an X-Screen, the driver fills
the x-screens xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
crtc config array with enumerated crtc's which it gets from the kernel,
always starting at slot 0 of the config->crtc[] array for each x-screen.
On a single-x-screen setup, this means that gpu hardware crtc i ends in
config->crtc[i], so config->crtc[i] corresponds to bit i in the
possible_crtcs bitmask.
Some setup code inside the server assumes that this 1-1 correspondence
between bits in possible_crtcs and config->crtc slots is always true.
On a ZaphodHeads setup this isn't the case, e.g., if you have a
dual-head setup and assign one output and crtc to x-screen 0 and the
other output and crtc to x-screen 1, then crtc 0 will end in
config->crtc[0] of x-screen 0, and crtc 1 will end in config->crtc[0] of
x-screen 1, as filling of the config->crtc[] arrays happens separately
for each x-screen, always starting at slot 0.
So now you'd have hw crtc 1, which corresponds to bit 1 in
kencoder->possible_crtcs ending in slot 0 of x-screen 1, instead of slot
1 as would be the case on a non-Zaphod setup.
During init, nouveau calls the x-server function
xf86InitialConfiguration(pScrn, TRUE), which calls xf86PickCrtcs(),
which has this validation/matching code (hw/xfree86/modes/xf86Crtc.c)
/*
* Select a crtc for this output and
* then attempt to configure the remaining
* outputs
*/
for (c = 0; c < config->num_crtc; c++) {
if ((output->possible_crtcs & (1 << c)) == 0)
continue;
crtc = config->crtc[c];
-> It assumes bit c == slot c in config->crtc[c], therefore it fails to
find a proper setup if that correspondence is broken on a ZaphodHeads
setup. It matches the wrong bit position in the possible_crtc mask. If
you get lucky (on many Nvidia cards) and the wrong bit position has a 1
bit, then it works by accident. If you get unlucky like on my laptopt it
bails out, aborting the server with the most confusing/misleading
sequence of error messages in xorg.log and dmesg you could imagine.
So the patch tries to bit-shift the output->possible_crtcs mask when
assigning from the kernels kencoder->possible_crtcs, to restore the 1-1
correspondence between hw crtcs and slots in pScrn->config->crtc[] to
make the servers matching code happy.
The crtcshift is the shift to apply to do this. pNVEnt->assigned_crtcs
is a bit mask which marks already in use hw crtcs. Searching for the
first 0 bit in that mask via that ffs() statement gives you the index of
the first free hw crtc, the one which will end up in the
pScrn->config->crtc[0] slot for the to-be-setup x-screen, or iow. how
many bits you need to shift the output->possible_crtcs bitmask to
restore a 1-1 correspondence from the perspective of the server.
On a single x-screen setup, that ffs() statment always gives you a
crtcshift of 0 as pNVEnt->assigned_crtcs is all zero at server
startup/regen, and that routine is called only one for a gpu, so the
patch turns into a no-op for the regular case.
> 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?
Alex Deucher just reviewed and r-b'd the same patch for
xf86-video-modesetting's ZaphodHeads implementation (on xorg-devel), so
assuming he followed my convoluted commit message, that could count as a
vote for the correctness of this one.
Atm. only nouveau and modesetting need the patch. intel and
radeon/amdgpu and other ddx'en don't need it because their kms drivers
always report a kencoder->possible_crtcs bitmask with as many 1 bits as
there are hw crtcs - any encoder seems to go with any crtc. nouveau-kms
seems to be the only one that does something clever with that bitmask,
so needs a bit of special treatment.
>
> On Sat, Jun 27, 2015 at 8:33 PM, Mario Kleiner
> <mario.kleiner.de at 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 at 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.
>
Sure, fix it up, bit-wise not is much simpler. Thanks!
-mario
>> 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau
More information about the Nouveau
mailing list