[PATCH xserver] modesetting: unifdef MODESETTING_OUTPUT_SLAVE_SUPPORT
Commit c7e8d4a6ee9542f56cd241cf7a960fb8223a6b22 had already unifdef MODESETTING_OUTPUT_SLAVE_SUPPORT but commit 9257b1252da9092ddc676fec9aabe2b33dfad272 didn't notice that. Signed-off-by: Nikhil Mahale <nmah...@nvidia.com> --- hw/xfree86/drivers/modesetting/drmmode_display.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 61a0e27..6e755e9 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -1701,10 +1701,8 @@ drmmode_create_name(ScrnInfoPtr pScrn, drmModeConnectorPtr koutput, char *name, fallback: if (koutput->connector_type >= MS_ARRAY_SIZE(output_names)) snprintf(name, 32, "Unknown%d-%d", koutput->connector_type, koutput->connector_type_id); -#ifdef MODESETTING_OUTPUT_SLAVE_SUPPORT else if (pScrn->is_gpu) snprintf(name, 32, "%s-%d-%d", output_names[koutput->connector_type], pScrn->scrnIndex - GPU_SCREEN_OFFSET + 1, koutput->connector_type_id); -#endif else snprintf(name, 32, "%s-%d", output_names[koutput->connector_type], koutput->connector_type_id); } -- 2.6.6 ___ 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
Re: [PATCH xserver] randr: Do not check the screen size bound for gpu screens
> > OTOH this may indeed be a server bug, but your fix is not the right > one, I wonder why we are doing a RRSetScreenSize for slave GPU-s at > all. Since we already check that the Screen is big enough in > ProcRRSetCrtcConfig? We are looking for answer of this question, may be Dave or Ajax knows about this. Thanks, Nikhil Mahale On 08/31/2016 01:40 PM, Timo Aaltonen wrote: > > Any update on this? > > On 20.06.2016 20:27, Hans de Goede wrote: >> Hi, >> >> On 20-06-16 18:32, Nikhil Mahale wrote: >>> 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 | 2560x1440| 1920x1080 | failed | >>> --- >> >> Ah I see now, this will only happen when the eDP is a slave output, >> I was thinking that the HDMI would be a slave output, but that does >> not matter, this problem will happen independ of the HDMI being a normal >> or a slave output, as long as the eDP is a slave output. >> >>> $ xrandr --output eDP --mode 1920x1080 --pos 0x0 >>> $ xrandr --output HDMI --mode 2560x1440 --pos 0x0 >>
Re: [PATCH xserver] randr: Do not check the screen size bound for gpu screens
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 <nmah...@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;
[PATCH xserver] randr: Adjust master's last set time with slaves
In prime configurations master's last set time may not be latest
and greatest, adjust it with slaves last set time, pick up greatest
one. Otherwise xserver may end with events which has
lastSetTime < lastConfigTime even if that's not
the case and confuse xrandr client.
Signed-off-by: Nikhil Mahale <nmah...@nvidia.com>
---
randr/randr.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/randr/randr.c b/randr/randr.c
index 3aabb19..549f26c 100644
--- a/randr/randr.c
+++ b/randr/randr.c
@@ -584,6 +584,15 @@ RRTellChanged(ScreenPtr pScreen)
mastersp = pScrPriv;
}
+xorg_list_for_each_entry(iter, >output_slave_list, output_head) {
+pSlaveScrPriv = rrGetScrPriv(iter);
+
+if (CompareTimeStamps(mastersp->lastSetTime,
+ pSlaveScrPriv->lastSetTime) == EARLIER) {
+mastersp->lastSetTime = pSlaveScrPriv->lastSetTime;
+}
+}
+
if (mastersp->changed) {
UpdateCurrentTimeIf();
if (mastersp->configChanged) {
--
2.8.2
___
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
Re: [PATCH xserver] randr: Adjust master's last set time with slaves
On 05/27/2016 01:40 AM, Hans de Goede wrote:
Hi,
On 26-05-16 18:20, Nikhil Mahale wrote:
In prime configurations master's last set time may not be latest
and greatest, adjust it with slaves last set time, pick up greatest
one. Otherwise xserver may end with events which has
lastSetTime < lastConfigTime even if that's not
the case and confuse xrandr client.
Thanks, looks good to me now:
Acked-by: Hans de Goede <hdego...@redhat.com>
Sorry, I did forgot to add signoff line. Please let me add signoff line
in commit msg.
Regards,
Hans
---
randr/randr.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/randr/randr.c b/randr/randr.c
index 3aabb19..549f26c 100644
--- a/randr/randr.c
+++ b/randr/randr.c
@@ -584,6 +584,15 @@ RRTellChanged(ScreenPtr pScreen)
mastersp = pScrPriv;
}
+xorg_list_for_each_entry(iter, >output_slave_list,
output_head) {
+pSlaveScrPriv = rrGetScrPriv(iter);
+
+if (CompareTimeStamps(mastersp->lastSetTime,
+ pSlaveScrPriv->lastSetTime) == EARLIER) {
+mastersp->lastSetTime = pSlaveScrPriv->lastSetTime;
+}
+}
+
if (mastersp->changed) {
UpdateCurrentTimeIf();
if (mastersp->configChanged) {
___
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
[PATCH xserver] randr: Adjust master's last set time with slaves
In prime configurations master's last set time may not be latest
and greatest, adjust it with slaves last set time, pick up greatest
one. Otherwise xserver may end with events which has
lastSetTime < lastConfigTime even if that's not
the case and confuse xrandr client.
---
randr/randr.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/randr/randr.c b/randr/randr.c
index 3aabb19..549f26c 100644
--- a/randr/randr.c
+++ b/randr/randr.c
@@ -584,6 +584,15 @@ RRTellChanged(ScreenPtr pScreen)
mastersp = pScrPriv;
}
+xorg_list_for_each_entry(iter, >output_slave_list, output_head) {
+pSlaveScrPriv = rrGetScrPriv(iter);
+
+if (CompareTimeStamps(mastersp->lastSetTime,
+ pSlaveScrPriv->lastSetTime) == EARLIER) {
+mastersp->lastSetTime = pSlaveScrPriv->lastSetTime;
+}
+}
+
if (mastersp->changed) {
UpdateCurrentTimeIf();
if (mastersp->configChanged) {
--
2.8.2
---
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.
---
___
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
Re: [PATCH xserver] randr: Do not check the screen size bound for gpu screens
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 <nmah...@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. --- ___ 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
[PATCH xserver] randr: Adjust master's last set time with slaves
In prime configurations master's last set time may not be latest
and greatest, adjust it with slaves last set time, pick up greatest
one. Otherwise xserver may end with events which has
lastSetTime < lastConfigTime even if that's not
the case and confuse xrandr client.
Signed-off-by: Nikhil Mahale <nmah...@nvidia.com>
---
randr/randr.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/randr/randr.c b/randr/randr.c
index 3aabb19..c07def1 100644
--- a/randr/randr.c
+++ b/randr/randr.c
@@ -568,6 +568,7 @@ RRSetChanged(ScreenPtr pScreen)
void
RRTellChanged(ScreenPtr pScreen)
{
+ScreenPtr slave;
ScreenPtr master;
rrScrPriv(pScreen);
rrScrPrivPtr mastersp;
@@ -584,6 +585,15 @@ RRTellChanged(ScreenPtr pScreen)
mastersp = pScrPriv;
}
+xorg_list_for_each_entry(slave, >output_slave_list, output_head) {
+pSlaveScrPriv = rrGetScrPriv(slave);
+
+if (CompareTimeStamps(mastersp->lastSetTime,
+ pSlaveScrPriv->lastSetTime) == EARLIER) {
+mastersp->lastSetTime = pSlaveScrPriv->lastSetTime;
+}
+}
+
if (mastersp->changed) {
UpdateCurrentTimeIf();
if (mastersp->configChanged) {
--
2.8.2
---
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.
---
___
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
[PATCH xserver] randr: Do not check the screen size bound for gpu screens
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 <nmah...@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, , _width,
_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;
--
2.8.2
---
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.
---
___
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
Re: [PATCH] os: Fix timer race conditions
On Saturday, January 24, 2015 05:07:49 PM Aaron Plattner wrote: On 01/23/2015 10:42 AM, Keith Packard wrote: * PGP Signed by an unknown key Nikhil Mahale nmah...@nvidia.com writes: On Monday, December 29, 2014 02:35:53 PM Keith Packard wrote: Old Signed by an unknown key Nikhil Mahale nmah...@nvidia.com writes: ok, here is updated patch. This patch generated a host of warnings when the type of 'timers' changed to a volatile pointer. Here's an additional patch that gets rid of the warnings; it should be reviewed (and fixed, as needed), then squashed with the original patch. oh, Thanks for this catch. I haven't seen an updated patch that squashes these two together? Sorry, I didn't realize you were still waiting for that. I just sent a reviewed squashed v2. hey sorry, I also didn't realize you were still waiting for that. Thanks Aaron for taking care of this. Thanks, Nikhil Mahale -- Aaron ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://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. --- ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] os: Fix timer race conditions
On Monday, December 29, 2014 02:30:06 PM Keith Packard wrote: * PGP Signed by an unknown key Keith, any chance of getting this in for 1.17? I know you expressed reservations about it, but the current code definitely corrupts the timer list and is affecting a bunch of people, so this patch is certainly better than nothing. Yeah, it's an ugly little bit of code, and still seems prone to racing when the timer list is modified from a timer callback. Sorry, how modification of timer list from callback causes race condition? OsTimerRec is static to waitFor.c and before any modification in timer list we are blocking os signals. Also before callback we remove timer from timer list first. This reduces the window, but doesn't close it. But, as you say, it seems better than the current code. --- 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. --- ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] os: Fix timer race conditions
On Monday, December 29, 2014 02:35:53 PM Keith Packard wrote: * PGP Signed by an unknown key Nikhil Mahale nmah...@nvidia.com writes: ok, here is updated patch. This patch generated a host of warnings when the type of 'timers' changed to a volatile pointer. Here's an additional patch that gets rid of the warnings; it should be reviewed (and fixed, as needed), then squashed with the original patch. oh, Thanks for this catch. --- 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. --- ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] os: Fix timer race conditions
On Monday, November 24, 2014 10:52:37 PM Julien Cristau wrote:
On Fri, Nov 14, 2014 at 23:52:55 +0530, Nikhil Mahale wrote:
From 40f5100cb73aad8f90ba2c926dd08c4e15789de0 Mon Sep 17 00:00:00 2001
From: Nikhil Mahale nmah...@nvidia.com
Date: Tue, 11 Nov 2014 17:44:28 +0530
Subject: [PATCH] os: Fix timer race conditions
Fixing following kind of race-conditions -
WaitForSomething()
// timers - timer-1 - timer-2 - null
while (timers (int) (timers-expires - now) = 0)
// prototype - DoTimer(OsTimerPtr timer, CARD32 now,
OsTimerPtr *prev)
DoTimer(timers, now, timers)
OsBlockSignals(); OS Signal comes just before
blocking it,
timer-1 handler gets called.
// timer-1 gets served and
scheduled again;
// timers - timer-2 -
timer-1 - null
*prev = timer-next;
timer-next = NULL; // timers - null
// timers list gets corrupted here and timer-2 gets
removed from list.
https://bugs.freedesktop.org/show_bug.cgi?id=86288
Signed-off-by: Nikhil Mahale nmah...@nvidia.com
---
os/WaitFor.c | 31 +--
1 file changed, 21 insertions(+), 10 deletions(-)
As far as I can tell after this change DoTimer is always called with
signals disabled, so it doesn't need to disable them itself?
ok, here is updated patch.
Cheers,
Julien
---
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.
---
From 5c797ce9cdb596107f9ebcdf8345ca234c587b22 Mon Sep 17 00:00:00 2001
From: Nikhil Mahale nmah...@nvidia.com
Date: Tue, 11 Nov 2014 17:44:28 +0530
Subject: [PATCH] os: Fix timer race conditions
Fixing following kind of race-conditions -
WaitForSomething()
|
// timers - timer-1 - timer-2 - null
while (timers (int) (timers-expires - now) = 0)
// prototype - DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev)
DoTimer(timers, now, timers)
|
|
OsBlockSignals(); OS Signal comes just before blocking it,
timer-1 handler gets called.
// timer-1 gets served and scheduled again;
// timers - timer-2 - timer-1 - null
*prev = timer-next;
timer-next = NULL; // timers - null
// timers list gets corrupted here and timer-2 gets removed from list.
https://bugs.freedesktop.org/show_bug.cgi?id=86288
Signed-off-by: Nikhil Mahale nmah...@nvidia.com
---
os/WaitFor.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/os/WaitFor.c b/os/WaitFor.c
index 39fedd9..e768356 100644
--- a/os/WaitFor.c
+++ b/os/WaitFor.c
@@ -123,7 +123,7 @@ struct _OsTimerRec {
static void DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev);
static void CheckAllTimers(void);
-static OsTimerPtr timers = NULL;
+static volatile OsTimerPtr timers = NULL;
/*
* WaitForSomething:
@@ -263,11 +263,14 @@ WaitForSomething(int *pClientsReady)
if ((int) (timers-expires - now) = 0)
expired = 1;
-while (timers (int) (timers-expires - now) = 0)
-DoTimer(timers, now, timers);
+if (expired) {
+OsBlockSignals();
+while (timers (int) (timers-expires - now) = 0)
+DoTimer(timers, now, timers);
+OsReleaseSignals();
-if (expired)
return 0;
+}
}
}
else {
@@ -281,11 +284,14 @@ WaitForSomething(int *pClientsReady)
if ((int) (timers-expires - now) = 0)
expired = 1;
-while
[PATCH] os: Fix timer race conditions
From 40f5100cb73aad8f90ba2c926dd08c4e15789de0 Mon Sep 17 00:00:00 2001
From: Nikhil Mahale nmah...@nvidia.com
Date: Tue, 11 Nov 2014 17:44:28 +0530
Subject: [PATCH] os: Fix timer race conditions
Fixing following kind of race-conditions -
WaitForSomething()
|
// timers - timer-1 - timer-2 - null
while (timers (int) (timers-expires - now) = 0)
// prototype - DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev)
DoTimer(timers, now, timers)
|
|
OsBlockSignals(); OS Signal comes just before blocking it,
timer-1 handler gets called.
// timer-1 gets served and scheduled again;
// timers - timer-2 - timer-1 - null
*prev = timer-next;
timer-next = NULL; // timers - null
// timers list gets corrupted here and timer-2 gets removed from list.
https://bugs.freedesktop.org/show_bug.cgi?id=86288
Signed-off-by: Nikhil Mahale nmah...@nvidia.com
---
os/WaitFor.c | 31 +--
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/os/WaitFor.c b/os/WaitFor.c
index 39fedd9..6e54222 100644
--- a/os/WaitFor.c
+++ b/os/WaitFor.c
@@ -123,7 +123,7 @@ struct _OsTimerRec {
static void DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev);
static void CheckAllTimers(void);
-static OsTimerPtr timers = NULL;
+static volatile OsTimerPtr timers = NULL;
/*
* WaitForSomething:
@@ -263,11 +263,14 @@ WaitForSomething(int *pClientsReady)
if ((int) (timers-expires - now) = 0)
expired = 1;
-while (timers (int) (timers-expires - now) = 0)
-DoTimer(timers, now, timers);
+if (expired) {
+OsBlockSignals();
+while (timers (int) (timers-expires - now) = 0)
+DoTimer(timers, now, timers);
+OsReleaseSignals();
-if (expired)
return 0;
+}
}
}
else {
@@ -281,11 +284,14 @@ WaitForSomething(int *pClientsReady)
if ((int) (timers-expires - now) = 0)
expired = 1;
-while (timers (int) (timers-expires - now) = 0)
-DoTimer(timers, now, timers);
+if (expired) {
+OsBlockSignals();
+while (timers (int) (timers-expires - now) = 0)
+DoTimer(timers, now, timers);
+OsReleaseSignals();
-if (expired)
return 0;
+}
}
}
if (someReady)
@@ -408,10 +414,11 @@ DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev)
OsBlockSignals();
*prev = timer-next;
timer-next = NULL;
+OsReleaseSignals();
+
newTime = (*timer-callback) (timer, now, timer-arg);
if (newTime)
TimerSet(timer, 0, newTime, timer-callback, timer-arg);
-OsReleaseSignals();
}
OsTimerPtr
@@ -515,8 +522,12 @@ TimerCheck(void)
{
CARD32 now = GetTimeInMillis();
-while (timers (int) (timers-expires - now) = 0)
-DoTimer(timers, now, timers);
+if (timers (int) (timers-expires - now) = 0) {
+OsBlockSignals();
+while (timers (int) (timers-expires - now) = 0)
+DoTimer(timers, now, timers);
+OsReleaseSignals();
+}
}
void
--
1.8.1.5
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] xrandr: Fix bug in calling crtc_set_transform()
Compare pending transformation with currently pending transformation on
server,
instead of comparing it with current transformation on server.
Signed-off-by: Nikhil Mahale nmah...@nvidia.com
---
xrandr.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/xrandr.c b/xrandr.c
index 366f6dc..4097a52 100644
--- a/xrandr.c
+++ b/xrandr.c
@@ -324,7 +324,10 @@ struct _crtc {
Rotation rotation;
output_t **outputs;
intnoutput;
-transform_tcurrent_transform, pending_transform;
+/* current and pending transformation on server */
+transform_tcurrent_transform, current_pending_transform;
+/* pending transformation on client */
+transform_t pending_transform;
};
struct _output_prop {
@@ -1331,11 +1334,17 @@ get_crtcs (void)
attr-currentFilter,
attr-currentParams,
attr-currentNparams);
+set_transform (crtcs[c].current_pending_transform,
+ attr-pendingTransform,
+ attr-pendingFilter,
+ attr-pendingParams,
+ attr-pendingNparams);
XFree (attr);
}
else
{
init_transform (crtcs[c].current_transform);
+copy_transform (crtcs[c].current_pending_transform,
crtcs[c].current_transform);
}
copy_transform (crtcs[c].pending_transform,
crtcs[c].current_transform);
}
@@ -1568,7 +1577,7 @@ crtc_apply (crtc_t *crtc)
s = RRSetConfigSuccess;
else
{
- if (!equal_transform (crtc-current_transform,
crtc-pending_transform))
+ if (!equal_transform (crtc-current_pending_transform,
crtc-pending_transform))
crtc_set_transform (crtc, crtc-pending_transform);
s = XRRSetCrtcConfig (dpy, res, crtc-crtc.xid, CurrentTime,
crtc-x, crtc-y, mode, crtc-rotation,
--
1.8.1.5
---
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.
---
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH randr] randr: Applying current transformation on Crtc
Screen size should be large enough to accommodate all crtcs,
Applying current rotation/transformation, and find out bounds of Crtc.
Not translating bounds by crtc-[xy], because that translation already present
in transform matrix.
Signed-off-by: Nikhil Mahale nmah...@nvidia.com
---
randr/rrscreen.c| 41 -
render/matrix.c |6 ++
render/picturestr.h |3 +++
3 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/randr/rrscreen.c b/randr/rrscreen.c
index 39340cc..120eadd 100644
--- a/randr/rrscreen.c
+++ b/randr/rrscreen.c
@@ -232,6 +232,34 @@ ProcRRGetScreenSizeRange(ClientPtr client)
return Success;
}
+static BoxRec
+rrGetModeBounds(RRModePtr mode, Rotation rotation,
+PictTransformPtr transform)
+{
+BoxRec box = {0, 0, 0, 0};
+int source_width;
+int source_height;
+
+
+if (mode == NULL) {
+return box;
+}
+
+source_width = mode-mode.width;
+source_height = mode-mode.height;
+if (rotation == RR_Rotate_90 || rotation == RR_Rotate_270) {
+source_width = mode-mode.height;
+source_height = mode-mode.width;
+}
+
+box.x2 = source_width;
+box.y2 = source_height;
+
+PictureTransformBounds(transform, box);
+
+return box;
+}
+
int
ProcRRSetScreenSize(ClientPtr client)
{
@@ -265,17 +293,12 @@ ProcRRSetScreenSize(ClientPtr client)
RRModePtr mode = crtc-mode;
if (mode) {
-int source_width = mode-mode.width;
-int source_height = mode-mode.height;
-Rotation rotation = crtc-rotation;
+BoxRec bounds;
-if (rotation == RR_Rotate_90 || rotation == RR_Rotate_270) {
-source_width = mode-mode.height;
-source_height = mode-mode.width;
-}
+bounds = rrGetModeBounds(mode, crtc-rotation, crtc-transform);
-if (crtc-x + source_width stuff-width ||
-crtc-y + source_height stuff-height)
+if (bounds.x2 stuff-width || bounds.y2 stuff-height ||
+bounds.x1 0 || bounds.y1 0)
return BadMatch;
}
}
diff --git a/render/matrix.c b/render/matrix.c
index 83cd66c..aec15d9 100644
--- a/render/matrix.c
+++ b/render/matrix.c
@@ -80,6 +80,12 @@ PictureTransformPoint(PictTransformPtr transform,
PictVectorPtr vector)
}
Bool
+PictureTransformBounds(PictTransformPtr transform, BoxRec *box)
+{
+return pixman_transform_bounds(transform, box);
+}
+
+Bool
PictureTransformPoint3d(PictTransformPtr transform, PictVectorPtr vector)
{
return pixman_transform_point_3d(transform, vector);
diff --git a/render/picturestr.h b/render/picturestr.h
index dc00f41..51a1b4b 100644
--- a/render/picturestr.h
+++ b/render/picturestr.h
@@ -609,6 +609,9 @@ extern _X_EXPORT Bool
PictureTransformPoint(PictTransformPtr transform, PictVectorPtr vector);
extern _X_EXPORT Bool
+PictureTransformBounds(PictTransformPtr transform, BoxRec *box);
+
+extern _X_EXPORT Bool
PictureTransformPoint3d(PictTransformPtr transform, PictVectorPtr vector);
#endif /* _PICTURESTR_H_ */
--
1.7.6.5
---
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.
---
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
