| Message ID | 1463720446-22166-1-git-send-email-nmahale@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
"randr: Do not check the screen size bound for gpu screens"
( rev:
1
)
in
X.org (DEPRECATED - USE GITLAB) |
diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c index 566a3db..82db9a8 100644 --- a/randr/rrcrtc.c +++ b/randr/rrcrtc.c @@ -1176,27 +1176,20 @@ ProcRRSetCrtcConfig(ClientPtr client) #ifdef RANDR_12_INTERFACE /* + * For gpu screen, CrtcSet set/adjust the master screen size along + * with mode. + * * Check screen size bounds if the DDX provides a 1.2 interface * for setting screen size. Else, assume the CrtcSet sets * the size along with the mode. If the driver supports transforms, * then it must allow crtcs to display a subset of the screen, so * only do this check for drivers without transform support. */ - if (pScrPriv->rrScreenSetSize && !crtc->transforms) { + if (!pScreen->isGPU && pScrPriv->rrScreenSetSize && !crtc->transforms) { int source_width; int source_height; PictTransform transform; struct pixman_f_transform f_transform, f_inverse; - int width, height; - - if (pScreen->isGPU) { - width = pScreen->current_master->width; - height = pScreen->current_master->height; - } - else { - width = pScreen->width; - height = pScreen->height; - } RRTransformCompute(stuff->x, stuff->y, mode->mode.width, mode->mode.height, @@ -1206,13 +1199,13 @@ ProcRRSetCrtcConfig(ClientPtr client) RRModeGetScanoutSize(mode, &transform, &source_width, &source_height); - if (stuff->x + source_width > width) { + if (stuff->x + source_width > pScreen->width) { client->errorValue = stuff->x; free(outputs); return BadValue; } - if (stuff->y + source_height > height) { + if (stuff->y + source_height > pScreen->height) { client->errorValue = stuff->y; free(outputs); return BadValue;
On 20.05.2016 08:00, Nikhil Mahale wrote: > For gpu screen, CrtcSet set/adjust the master screen size along > mode in following callstack - > > ProcRRSetCrtcConfig() > | > -> RRCrtcSet() > | > -> rrCheckPixmapBounding() > | > -> pScrPriv->rrScreenSetSize() > > Checking screen size bound for gpus screen cause some configurations > to fails, e.g > > $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \ > --mode 2560x1440 --pos 0x0 > > Here xrandr utility first sets screen size to 2560x1440 which > gets resized to 1920x1080 on RRSetCrtcConfig request for eDP, > and then RRSetCrtcConfig request for HDMI fails because > of failure of screen bound check. > > Signed-off-by: Nikhil Mahale <nmahale@nvidia.com> I've tried to come up with a test that would hang/crash the xserver reliably, but failed. It usually takes a number of cycles through mirrored/extended/external-only modes, or switching between external-only and mirrored. Anyway, the impact is that on intel+nvidia hybrid the server can crash or hang or just fail to set the mode without these two patches.
On 05/26/2016 01:31 PM, Timo Aaltonen wrote: > On 20.05.2016 08:00, Nikhil Mahale wrote: >> For gpu screen, CrtcSet set/adjust the master screen size along >> mode in following callstack - >> >> ProcRRSetCrtcConfig() >> | >> -> RRCrtcSet() >> | >> -> rrCheckPixmapBounding() >> | >> -> pScrPriv->rrScreenSetSize() >> >> Checking screen size bound for gpus screen cause some configurations >> to fails, e.g >> >> $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \ >> --mode 2560x1440 --pos 0x0 >> >> Here xrandr utility first sets screen size to 2560x1440 which >> gets resized to 1920x1080 on RRSetCrtcConfig request for eDP, >> and then RRSetCrtcConfig request for HDMI fails because >> of failure of screen bound check. >> >> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com> > > I've tried to come up with a test that would hang/crash the xserver > reliably, but failed. It usually takes a number of cycles through > mirrored/extended/external-only modes, or switching between > external-only and mirrored. Anyway, the impact is that on intel+nvidia > hybrid the server can crash or hang or just fail to set the mode without > these two patches. > Sorry Timo, I don't think I understand what you want to say. This patch was not intended to fix hang/crash, it simple fixes modeset failure for the use cases like I mentioned in description of patch. The second "[PATCH xserver] randr: Adjust master's last set time with slaves" fixes XRandr's client confusion about lastConfigTimes and lastSetTime, I seen client was repeatedly sending modeset request because each time it gets lastSetTime < lastConfigTime. Both the patches only impacting prime configurations. Thanks, Nikhil Mahale ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
On 26.05.2016 11:21, Nikhil Mahale wrote: > On 05/26/2016 01:31 PM, Timo Aaltonen wrote: >> On 20.05.2016 08:00, Nikhil Mahale wrote: >>> For gpu screen, CrtcSet set/adjust the master screen size along >>> mode in following callstack - >>> >>> ProcRRSetCrtcConfig() >>> | >>> -> RRCrtcSet() >>> | >>> -> rrCheckPixmapBounding() >>> | >>> -> pScrPriv->rrScreenSetSize() >>> >>> Checking screen size bound for gpus screen cause some configurations >>> to fails, e.g >>> >>> $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \ >>> --mode 2560x1440 --pos 0x0 >>> >>> Here xrandr utility first sets screen size to 2560x1440 which >>> gets resized to 1920x1080 on RRSetCrtcConfig request for eDP, >>> and then RRSetCrtcConfig request for HDMI fails because >>> of failure of screen bound check. >>> >>> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com> >> >> I've tried to come up with a test that would hang/crash the xserver >> reliably, but failed. It usually takes a number of cycles through >> mirrored/extended/external-only modes, or switching between >> external-only and mirrored. Anyway, the impact is that on intel+nvidia >> hybrid the server can crash or hang or just fail to set the mode without >> these two patches. >> > > Sorry Timo, I don't think I understand what you want to say. This patch > was not intended to fix hang/crash, it simple fixes modeset failure for > the use cases like I mentioned in description of patch. > > The second "[PATCH xserver] randr: Adjust master's last set time with > slaves" fixes XRandr's client confusion about lastConfigTimes and > lastSetTime, I seen client was repeatedly sending modeset request > because each time it gets lastSetTime < lastConfigTime. > > Both the patches only impacting prime configurations. It was for whoever reviews these. The server certainly can crash without the patches with prime setup, according to my tests..
Hi, On 20-05-16 07:00, Nikhil Mahale wrote: > For gpu screen, CrtcSet set/adjust the master screen size along > mode in following callstack - > > ProcRRSetCrtcConfig() > | > -> RRCrtcSet() > | > -> rrCheckPixmapBounding() > | > -> pScrPriv->rrScreenSetSize() > > Checking screen size bound for gpus screen cause some configurations > to fails, e.g > > $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \ > --mode 2560x1440 --pos 0x0 > > Here xrandr utility first sets screen size to 2560x1440 which > gets resized to 1920x1080 on RRSetCrtcConfig request for eDP, > and then RRSetCrtcConfig request for HDMI fails because > of failure of screen bound check. Hmm, but one has the same problem when not using clone mode, then the master screen size must be made big enough to hold both crtc-s, so how come this has never been a problem then ? I've a feeling that in that case we end up doing things in finer grained steps. What if you do: $ xrandr --output eDP --mode 1920x1080 --pos 0x0 $ xrandr --output HDMI --mode 2560x1440 --pos 0x0 ? Does that work ? If that works, which I would expect it will, then this should be fixed in the xrandr utility instead IMHO. Just circumventing the screen size check for slave outputs seem to go against the xrandr-1.2 spec to me. Also I guess one can reproduce the same problem without using slave-outputs, in which case your patch will not help. Regards, Hans > > Signed-off-by: Nikhil Mahale <nmahale@nvidia.com> > --- > randr/rrcrtc.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c > index 566a3db..82db9a8 100644 > --- a/randr/rrcrtc.c > +++ b/randr/rrcrtc.c > @@ -1176,27 +1176,20 @@ ProcRRSetCrtcConfig(ClientPtr client) > > #ifdef RANDR_12_INTERFACE > /* > + * For gpu screen, CrtcSet set/adjust the master screen size along > + * with mode. > + * > * Check screen size bounds if the DDX provides a 1.2 interface > * for setting screen size. Else, assume the CrtcSet sets > * the size along with the mode. If the driver supports transforms, > * then it must allow crtcs to display a subset of the screen, so > * only do this check for drivers without transform support. > */ > - if (pScrPriv->rrScreenSetSize && !crtc->transforms) { > + if (!pScreen->isGPU && pScrPriv->rrScreenSetSize && !crtc->transforms) { > int source_width; > int source_height; > PictTransform transform; > struct pixman_f_transform f_transform, f_inverse; > - int width, height; > - > - if (pScreen->isGPU) { > - width = pScreen->current_master->width; > - height = pScreen->current_master->height; > - } > - else { > - width = pScreen->width; > - height = pScreen->height; > - } > > RRTransformCompute(stuff->x, stuff->y, > mode->mode.width, mode->mode.height, > @@ -1206,13 +1199,13 @@ ProcRRSetCrtcConfig(ClientPtr client) > > RRModeGetScanoutSize(mode, &transform, &source_width, > &source_height); > - if (stuff->x + source_width > width) { > + if (stuff->x + source_width > pScreen->width) { > client->errorValue = stuff->x; > free(outputs); > return BadValue; > } > > - if (stuff->y + source_height > height) { > + if (stuff->y + source_height > pScreen->height) { > client->errorValue = stuff->y; > free(outputs); > return BadValue; >
Sorry for late reply, Hans. See inline - On 06/13/2016 10:36 PM, Hans de Goede wrote: > Hi, > > On 20-05-16 07:00, Nikhil Mahale wrote: >> For gpu screen, CrtcSet set/adjust the master screen size along >> mode in following callstack - >> >> ProcRRSetCrtcConfig() >> | >> -> RRCrtcSet() >> | >> -> rrCheckPixmapBounding() >> | >> -> pScrPriv->rrScreenSetSize() >> >> Checking screen size bound for gpus screen cause some configurations >> to fails, e.g >> >> $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \ >> --mode 2560x1440 --pos 0x0 >> >> Here xrandr utility first sets screen size to 2560x1440 which >> gets resized to 1920x1080 on RRSetCrtcConfig request for eDP, >> and then RRSetCrtcConfig request for HDMI fails because >> of failure of screen bound check. > > Hmm, but one has the same problem when not using clone mode, > then the master screen size must be made big enough to hold > both crtc-s, so how come this has never been a problem then ? > > I've a feeling that in that case we end up doing things in > finer grained steps. What if you do: > > $ xrandr --output eDP --mode 1920x1080 --pos 0x0 > $ xrandr --output HDMI --mode 2560x1440 --pos 0x0 > > ? Does that work ? If that works, which I would expect it will, > then this should be fixed in the xrandr utility instead IMHO. > > Just circumventing the screen size check for slave outputs > seem to go against the xrandr-1.2 spec to me. > > Also I guess one can reproduce the same problem without using > slave-outputs, in which case your patch will not help. > I don't think we could reproduce this problem without using slave- output, because rrScreenSetSize() path which I explained in commit message is not there, isn't it? Let me try to explain differece between both sequence of commands, please correct me if required - $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \ --mode 2560x1440 --pos 0x0 ----------------------------------------------------------------------- | Cmd | Screen size | eDP | HDMI | ----------------------------------------------------------------------- | RRSetScreenSize | 2560x1440 | - | - | ----------------------------------------------------------------------- | RRSetCrtcConfig | 1920x1080 | 1920x1080 | - | | | changed from 2560x1440 | | | | | because of rrScreenSetSize() | | | | | path explained in commit | | | | | msg. ....(A) | | | ----------------------------------------------------------------------- | RRSetCrtcConfig | 1920x1080 | 1920x1080 | failed | ----------------------------------------------------------------------- $ xrandr --output eDP --mode 1920x1080 --pos 0x0 $ xrandr --output HDMI --mode 2560x1440 --pos 0x0 ------------------------------------------------------------------------ | Cmd | Screen size | eDP | HDMI | ------------------------------------------------------------------------ | RRSetScreenSize | 1920x1080 | - | - | ------------------------------------------------------------------------ | RRSetCrtcConfig | 1920x1080 | 1920x1080 | - | ------------------------------------------------------------------------ | RRSetScreenSize | 2560x1440 | 1920x1080 | - | ------------------------------------------------------------------------ | RRSetCrtcConfig | 2560x1440 | 1920x1080 | 2560x1440 | ------------------------------------------------------------------------ Case (A) is not there in second sequence of xrand commands, so you could not reproduce problem. Thanks, Nikhil Mahale > > Regards, > > Hans > > >> >> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com> >> --- >> randr/rrcrtc.c | 19 ++++++------------- >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c >> index 566a3db..82db9a8 100644 >> --- a/randr/rrcrtc.c >> +++ b/randr/rrcrtc.c >> @@ -1176,27 +1176,20 @@ ProcRRSetCrtcConfig(ClientPtr client) >> >> #ifdef RANDR_12_INTERFACE >> /* >> + * For gpu screen, CrtcSet set/adjust the master screen size >> along >> + * with mode. >> + * >> * Check screen size bounds if the DDX provides a 1.2 interface >> * for setting screen size. Else, assume the CrtcSet sets >> * the size along with the mode. If the driver supports >> transforms, >> * then it must allow crtcs to display a subset of the >> screen, so >> * only do this check for drivers without transform support. >> */ >> - if (pScrPriv->rrScreenSetSize && !crtc->transforms) { >> + if (!pScreen->isGPU && pScrPriv->rrScreenSetSize && >> !crtc->transforms) { >> int source_width; >> int source_height; >> PictTransform transform; >> struct pixman_f_transform f_transform, f_inverse; >> - int width, height; >> - >> - if (pScreen->isGPU) { >> - width = pScreen->current_master->width; >> - height = pScreen->current_master->height; >> - } >> - else { >> - width = pScreen->width; >> - height = pScreen->height; >> - } >> >> RRTransformCompute(stuff->x, stuff->y, >> mode->mode.width, mode->mode.height, >> @@ -1206,13 +1199,13 @@ ProcRRSetCrtcConfig(ClientPtr client) >> >> RRModeGetScanoutSize(mode, &transform, &source_width, >> &source_height); >> - if (stuff->x + source_width > width) { >> + if (stuff->x + source_width > pScreen->width) { >> client->errorValue = stuff->x; >> free(outputs); >> return BadValue; >> } >> >> - if (stuff->y + source_height > height) { >> + if (stuff->y + source_height > pScreen->height) { >> client->errorValue = stuff->y; >> free(outputs); >> return BadValue; >> > _______________________________________________ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
For gpu screen, CrtcSet set/adjust the master screen size along mode in following callstack - ProcRRSetCrtcConfig() | -> RRCrtcSet() | -> rrCheckPixmapBounding() | -> pScrPriv->rrScreenSetSize() Checking screen size bound for gpus screen cause some configurations to fails, e.g $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \ --mode 2560x1440 --pos 0x0 Here xrandr utility first sets screen size to 2560x1440 which gets resized to 1920x1080 on RRSetCrtcConfig request for eDP, and then RRSetCrtcConfig request for HDMI fails because of failure of screen bound check. Signed-off-by: Nikhil Mahale <nmahale@nvidia.com> --- randr/rrcrtc.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)