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

[Mario Kleiner]

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
 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 
---
  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 ^ 0x) - 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", 

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

[Mario Kleiner]

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...@gmail.com wrote:

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

[Ilia Mirkin]

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...@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...@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 ^ 0x) - 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

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

[Mario Kleiner]

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...@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 ^ 0x) - 1;
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