Re: [PATCH xserver 1/2] animcur: Fix transitions between animated cursors
Adam Jackson writes: >> Should check for a NULL cursor here. > > It does, hidden in the macro: > > render/animcur.c:#define IsAnimCur(c) ((c) && ((c)->bits == > &animCursorBits)) Cool. In which case, the patch is Reviewed-by: Keith Packard -- -keith signature.asc Description: PGP signature ___ 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 1/2] animcur: Fix transitions between animated cursors
On Tue, 2018-01-09 at 14:06 -0800, Keith Packard wrote:
> Adam Jackson writes:
>
> > +static void
> > +AnimCurCancelTimer(DeviceIntPtr pDev)
> > +{
> > +CursorPtr cur = pDev->spriteInfo->anim.pCursor;
> > +
> > +if (IsAnimCur(cur))
> > +TimerCancel(GetAnimCur(cur)->timer);
> > +}
> > +
>
> Should check for a NULL cursor here.
It does, hidden in the macro:
render/animcur.c:#define IsAnimCur(c) ((c) && ((c)->bits ==
&animCursorBits))
- ajax
___
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 1/2] animcur: Fix transitions between animated cursors
Adam Jackson writes:
> +static void
> +AnimCurCancelTimer(DeviceIntPtr pDev)
> +{
> +CursorPtr cur = pDev->spriteInfo->anim.pCursor;
> +
> +if (IsAnimCur(cur))
> +TimerCancel(GetAnimCur(cur)->timer);
> +}
> +
Should check for a NULL cursor here.
Otherwise, this lgtm.
--
-keith
signature.asc
Description: PGP signature
___
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 2/2] randr: Use RRCrtcGetScanoutSize() in ProcRRSetScreenSize()
Alex Goins writes: > RRCrtcGetScanoutSize() adds a significant amount of complexity compared to > simply checking the rotation and swapping width and height, since it operates > by > applying the transformation matrix crtc->transform to the mode. There's also an implicit assumption that when a transform is applied, there will be a shadow buffer and so the scanout buffer need not be large enough to hold the whole image. I don't know if that's useful in practice, but that is why there are two different paths here. -- -keith signature.asc Description: PGP signature ___ 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] xfree86: add default modes for 16:9 and 16:10
Improve the user experience for users with wide screens by adding standard 16:9 and 16:10 modes to extramodes, as suggested previously (https://lists.x.org/archives/xorg-devel/2016-February/048866.html). Tested successfully on my laptop. Feedback welcome. See also https://bugs.freedesktop.org/show_bug.cgi?id=37858. Signed-off-by: Martin Wilck --- hw/xfree86/common/extramodes | 142 +++ 1 file changed, 142 insertions(+) diff --git a/hw/xfree86/common/extramodes b/hw/xfree86/common/extramodes index 450502670286..5a446938250f 100644 --- a/hw/xfree86/common/extramodes +++ b/hw/xfree86/common/extramodes @@ -25,3 +25,145 @@ Modeline "2048x1536" 340.48 2048 2216 2440 2832 1536 1537 1540 1603 -hsync +vs # 2048x1536 @ 85Hz (VESA GTF) hsync: 137.0kHz Modeline "2048x1536" 388.04 2048 2216 2440 2832 1536 1537 1540 1612 -hsync +vsync +### 16:9 modelines generated by cvt + +# 640x360 59.32 Hz (CVT 0.23M9-R) hsync: 22.19 kHz; pclk: 17.75 MHz +Modeline "640x360R" 17.75 640 688 720 800 360 363 368 374 +hsync -vsync + +# 640x360 59.84 Hz (CVT 0.23M9) hsync: 22.50 kHz; pclk: 18.00 MHz +Modeline "640x360" 18.00 640 664 720 800 360 363 368 376 -hsync +vsync + +# 720x405 58.99 Hz (CVT 0.29M9-R) hsync: 24.72 kHz; pclk: 21.75 MHz +Modeline "720x405R" 21.75 720 768 800 880 405 408 413 419 +hsync -vsync + +# 720x405 59.51 Hz (CVT 0.29M9) hsync: 25.11 kHz; pclk: 22.50 MHz +Modeline "720x405" 22.50 720 744 808 896 405 408 413 422 -hsync +vsync + +# 864x486 59.57 Hz (CVT 0.42M9-R) hsync: 29.79 kHz; pclk: 30.50 MHz +Modeline "864x486R" 30.50 864 912 944 1024 486 489 494 500 +hsync -vsync + +# 864x486 59.92 Hz (CVT 0.42M9) hsync: 30.32 kHz; pclk: 32.50 MHz +Modeline "864x486" 32.50 864 888 968 1072 486 489 494 506 -hsync +vsync + +# 960x540 59.82 Hz (CVT 0.52M9-R) hsync: 33.26 kHz; pclk: 37.25 MHz +Modeline "960x540R" 37.25 960 1008 1040 1120 540 543 548 556 +hsync -vsync + +# 960x540 59.63 Hz (CVT 0.52M9) hsync: 33.51 kHz; pclk: 40.75 MHz +Modeline "960x540" 40.75 960 992 1088 1216 540 543 548 562 -hsync +vsync + +# 1024x576 59.82 Hz (CVT 0.59M9-R) hsync: 35.47 kHz; pclk: 42.00 MHz +Modeline "1024x576R" 42.00 1024 1072 1104 1184 576 579 584 593 +hsync -vsync + +# 1024x576 59.90 Hz (CVT 0.59M9) hsync: 35.88 kHz; pclk: 46.50 MHz +Modeline "1024x576" 46.50 1024 1064 1160 1296 576 579 584 599 -hsync +vsync + +# 1280x720 59.74 Hz (CVT 0.92M9-R) hsync: 44.27 kHz; pclk: 63.75 MHz +Modeline "1280x720R" 63.75 1280 1328 1360 1440 720 723 728 741 +hsync -vsync + +# 1280x720 59.86 Hz (CVT 0.92M9) hsync: 44.77 kHz; pclk: 74.50 MHz +Modeline "1280x720" 74.50 1280 1344 1472 1664 720 723 728 748 -hsync +vsync + +# 1368x768 59.85 Hz (CVT) hsync: 47.28 kHz; pclk: 72.25 MHz +Modeline "1368x768R" 72.25 1368 1416 1448 1528 768 771 781 790 +hsync -vsync + +# 1368x768 59.88 Hz (CVT) hsync: 47.79 kHz; pclk: 85.25 MHz +Modeline "1368x768" 85.25 1368 1440 1576 1784 768 771 781 798 -hsync +vsync + +# 1600x900 59.82 Hz (CVT 1.44M9-R) hsync: 55.40 kHz; pclk: 97.50 MHz +Modeline "1600x900R" 97.50 1600 1648 1680 1760 900 903 908 926 +hsync -vsync + +# 1600x900 59.95 Hz (CVT 1.44M9) hsync: 55.99 kHz; pclk: 118.25 MHz +Modeline "1600x900" 118.25 1600 1696 1856 2112 900 903 908 934 -hsync +vsync + +# 1920x1080 59.93 Hz (CVT 2.07M9-R) hsync: 66.59 kHz; pclk: 138.50 MHz +Modeline "1920x1080R" 138.50 1920 1968 2000 2080 1080 1083 1088 +hsync -vsync + +# 1920x1080 59.96 Hz (CVT 2.07M9) hsync: 67.16 kHz; pclk: 173.00 MHz +Modeline "1920x1080" 173.00 1920 2048 2248 2576 1080 1083 1088 1120 -hsync +vsync + +# 2048x1152 59.91 Hz (CVT 2.36M9-R) hsync: 70.99 kHz; pclk: 156.75 MHz +Modeline "2048x1152R" 156.75 2048 2096 2128 2208 1152 1155 1160 1185 +hsync -vsync + +# 2048x1152 59.90 Hz (CVT 2.36M9) hsync: 71.58 kHz; pclk: 197.00 MHz +Modeline "2048x1152" 197.00 2048 2184 2400 2752 1152 1155 1160 1195 -hsync +vsync + +# 2560x1440 59.95 Hz (CVT 3.69M9-R) hsync: 88.79 kHz; pclk: 241.50 MHz +Modeline "2560x1440R" 241.50 2560 2608 2640 2720 1440 1443 1448 1481 +hsync -vsync + +# 2560x1440 59.96 Hz (CVT 3.69M9) hsync: 89.52 kHz; pclk: 312.25 MHz +Modeline "2560x1440" 312.25 2560 2752 3024 3488 1440 1443 1448 1493 -hsync +vsync + +# 2880x1620 59.97 Hz (CVT 4.67M9-R) hsync: 99.92 kHz; pclk: 303.75 MHz +Modeline "2880x1620R" 303.75 2880 2928 2960 3040 1620 1623 1628 1666 +hsync -vsync + +# 2880x1620 59.96 Hz (CVT 4.67M9) hsync: 100.67 kHz; pclk: 396.25 MHz +Modeline "2880x1620" 396.25 2880 3096 3408 3936 1620 1623 1628 1679 -hsync +vsync + +# 3200x1800 59.94 Hz (CVT 5.76M9-R) hsync: 111.01 kHz; pclk: 373.00 MHz +Modeline "3200x1800R" 373.00 3200 3248 3280 3360 1800 1803 1808 1852 +hsync -vsync + +# 3200x1800 59.96 Hz (CVT 5.76M9) hsync: 111.82 kHz; pclk: 492.00 MHz +Modeline "3200x1800" 492.00 3200 3456 3800 4400 1800 1803 1808 1865 -hsync +vsync + +# 3840x2160 59.97 Hz (CVT 8.29M9-R) hsync: 133.25 kHz; pclk: 533.00 MHz +Mo
Re: [PATCH xserver 1/2] animcur: Fix transitions between animated cursors
On Tue, 2018-01-09 at 10:39 -0800, Aaron Plattner wrote: > I re-verified the crash and verified that this series fixes it, so you > can add > Tested-by: Aaron Plattner Merged, thanks! remote: I: patch #196638 updated using rev de60245e05c0d2528d4ff42557a044387e53315c. remote: I: patch #196639 updated using rev 4d82a150b2ee29c1025408cdb9ece255452a81bd. remote: I: 2 patch(es) updated to state Accepted. To ssh://git.freedesktop.org/git/xorg/xserver a09fbe6c82..4d82a150b2 master -> master - ajax ___ 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 2/2] randr: Use RRCrtcGetScanoutSize() in ProcRRSetScreenSize()
On Tue, 9 Jan 2018, Michel Dänzer wrote:
> On 2018-01-09 03:44 AM, Alex Goins wrote:
> > Previously, ProcRRSetScreenSize() manually computed the dimensions of a
> > CRTC's
> > viewport in order to check that it does not extend beyond the bounds of the
> > new
> > screen size. It did this incorrectly, leading to bugs.
> >
> > A previous patch "randr: Fix rotation check in ProcRRSetScreenSize()" fixed
> > the
> > issue by directly correcting the calculation, but to avoid future bugs of
> > this
> > class, this patch uses RRCrtcGetScanoutSize() to do the calculation,
> > implicitly
> > accounting for all possible transforms.
> >
> > There is existing precedent for this method in ProcRRSetCrtcConfig(), where
> > RRModeGetScanoutSize() is used directly due to the transform having not yet
> > been
> > applied but the check is otherwise the same.
> >
> > Signed-off-by: Alex Goins
> > ---
> > randr/rrscreen.c | 13 -
> > 1 file changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/randr/rrscreen.c b/randr/rrscreen.c
> > index f484383..7321eef 100644
> > --- a/randr/rrscreen.c
> > +++ b/randr/rrscreen.c
> > @@ -265,17 +265,12 @@ ProcRRSetScreenSize(ClientPtr client)
> > }
> > for (i = 0; i < pScrPriv->numCrtcs; i++) {
> > RRCrtcPtr crtc = pScrPriv->crtcs[i];
> > -RRModePtr mode = crtc->mode;
> >
> > -if (mode) {
> > -int source_width = mode->mode.width;
> > -int source_height = mode->mode.height;
> > -Rotation rotation = crtc->rotation;
> > +if (crtc->mode) {
> > +int source_width;
> > +int source_height;
> >
> > -if (rotation & (RR_Rotate_90 | RR_Rotate_270)) {
> > -source_width = mode->mode.height;
> > -source_height = mode->mode.width;
> > -}
> > +RRCrtcGetScanoutSize(crtc, &source_width, &source_height);
> >
> > if (crtc->x + source_width > stuff->width ||
> > crtc->y + source_height > stuff->height)
> >
>
> Reviewed-by: Michel Dänzer
>
> Is there any reason to have patch 1 separately?
The idea was just to preserve the direct fix in the git history, in case some
issue crops up with RRCrtcGetScanoutSize(), or there was some objection to its
use.
RRCrtcGetScanoutSize() adds a significant amount of complexity compared to
simply checking the rotation and swapping width and height, since it operates by
applying the transformation matrix crtc->transform to the mode.
> P.S. Looks like there are similar issues in randr/rrcrtc.c, in
> crtc_to_box and rrCheckPixmapBounding.
True, looks like those should be fixed as well.
Thanks,
Alex
> --
> Earthling Michel Dänzer | http://www.amd.com
> Libre software enthusiast | Mesa and X developer
> ___
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 1/2] animcur: Fix transitions between animated cursors
On Tue, 2018-01-09 at 10:08 -0800, Aaron Plattner wrote:
> On 01/09/2018 08:51 AM, Adam Jackson wrote:
>
> > @@ -165,8 +174,10 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr
> > pScreen, CursorPtr pCursor)
> > if (pCursor != pDev->spriteInfo->anim.pCursor) {
> > AnimCurPtr ac = GetAnimCur(pCursor);
> >
> > -ret = (*pScreen->DisplayCursor)
> > -(pDev, pScreen, ac->elts[0].pCursor);
> > +AnimCurCancelTimer(pDev);
> > +ret = (*pScreen->DisplayCursor) (pDev, pScreen,
> > + ac->elts[0].pCursor);
> > +
>
> This is a slight change in behavior if DisplayCursor fails since it will
> cancel the previous timer whereas before it wouldn't. Are there any
> weird cases where failing to change the cursor is expected and canceling
> animation of the previous cursor would be a problem?
Mm, not really. Practically speaking ->DisplayCursor is always
miPointerDisplayCursor, which only fails for keyboard devices. Which I
guess means the second patch is more paranoid than it needs to be and
unwinding would probably go fine on BadAlloc. Meh.
> Assuming not, both changes
> Reviewed-by: Aaron Plattner
>
> I'll try to put together a build to test these today.
Cool, thanks for helping dig into this.
- ajax
___
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 1/2] animcur: Fix transitions between animated cursors
On 01/09/2018 10:08 AM, Aaron Plattner wrote:
> On 01/09/2018 08:51 AM, Adam Jackson wrote:
>> We weren't cancelling the old timer when changing cursors, making things
>> go all crashy. Logically we could always cancel the timer first, but
>> then we'd have to call TimerSet to re-arm ourselves, and GetTimeInMillis
>> is potentially expensive.
>>
>> Reported-by:
>> https://devtalk.nvidia.com/default/topic/1028172/linux/titan-v-ubuntu-16-04lts-and-387-34-driver-crashes-badly/post/5230967/#5230967
>> Signed-off-by: Adam Jackson
>> ---
>> render/animcur.c | 25 +++--
>> 1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/render/animcur.c b/render/animcur.c
>> index 058bc1b323..797029443c 100644
>> --- a/render/animcur.c
>> +++ b/render/animcur.c
>> @@ -151,11 +151,20 @@ AnimCurTimerNotify(OsTimerPtr timer, CARD32 now, void
>> *arg)
>> return ac->elts[elt].delay;
>> }
>>
>> +static void
>> +AnimCurCancelTimer(DeviceIntPtr pDev)
>> +{
>> +CursorPtr cur = pDev->spriteInfo->anim.pCursor;
>> +
>> +if (IsAnimCur(cur))
>> +TimerCancel(GetAnimCur(cur)->timer);
>> +}
>> +
>> static Bool
>> AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr
>> pCursor)
>> {
>> AnimCurScreenPtr as = GetAnimCurScreen(pScreen);
>> -Bool ret;
>> +Bool ret = TRUE;
>>
>> if (IsFloating(pDev))
>> return FALSE;
>> @@ -165,8 +174,10 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr
>> pScreen, CursorPtr pCursor)
>> if (pCursor != pDev->spriteInfo->anim.pCursor) {
>> AnimCurPtr ac = GetAnimCur(pCursor);
>>
>> -ret = (*pScreen->DisplayCursor)
>> -(pDev, pScreen, ac->elts[0].pCursor);
>> +AnimCurCancelTimer(pDev);
>> +ret = (*pScreen->DisplayCursor) (pDev, pScreen,
>> + ac->elts[0].pCursor);
>> +
>
> This is a slight change in behavior if DisplayCursor fails since it will
> cancel the previous timer whereas before it wouldn't. Are there any
> weird cases where failing to change the cursor is expected and canceling
> animation of the previous cursor would be a problem?
>
> Assuming not, both changes
> Reviewed-by: Aaron Plattner
>
> I'll try to put together a build to test these today.
I re-verified the crash and verified that this series fixes it, so you
can add
Tested-by: Aaron Plattner
>> if (ret) {
>> pDev->spriteInfo->anim.elt = 0;
>> pDev->spriteInfo->anim.pCursor = pCursor;
>> @@ -176,15 +187,9 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr
>> pScreen, CursorPtr pCursor)
>> AnimCurTimerNotify, pDev);
>> }
>> }
>> -else
>> -ret = TRUE;
>> }
>> else {
>> -CursorPtr old = pDev->spriteInfo->anim.pCursor;
>> -
>> -if (old && IsAnimCur(old))
>> -TimerCancel(GetAnimCur(old)->timer);
>> -
>> +AnimCurCancelTimer(pDev);
>> pDev->spriteInfo->anim.pCursor = 0;
>> pDev->spriteInfo->anim.pScreen = 0;
>> ret = (*pScreen->DisplayCursor) (pDev, pScreen, pCursor);
>>
>
___
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 1/2] animcur: Fix transitions between animated cursors
On 01/09/2018 08:51 AM, Adam Jackson wrote:
> We weren't cancelling the old timer when changing cursors, making things
> go all crashy. Logically we could always cancel the timer first, but
> then we'd have to call TimerSet to re-arm ourselves, and GetTimeInMillis
> is potentially expensive.
>
> Reported-by:
> https://devtalk.nvidia.com/default/topic/1028172/linux/titan-v-ubuntu-16-04lts-and-387-34-driver-crashes-badly/post/5230967/#5230967
> Signed-off-by: Adam Jackson
> ---
> render/animcur.c | 25 +++--
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/render/animcur.c b/render/animcur.c
> index 058bc1b323..797029443c 100644
> --- a/render/animcur.c
> +++ b/render/animcur.c
> @@ -151,11 +151,20 @@ AnimCurTimerNotify(OsTimerPtr timer, CARD32 now, void
> *arg)
> return ac->elts[elt].delay;
> }
>
> +static void
> +AnimCurCancelTimer(DeviceIntPtr pDev)
> +{
> +CursorPtr cur = pDev->spriteInfo->anim.pCursor;
> +
> +if (IsAnimCur(cur))
> +TimerCancel(GetAnimCur(cur)->timer);
> +}
> +
> static Bool
> AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor)
> {
> AnimCurScreenPtr as = GetAnimCurScreen(pScreen);
> -Bool ret;
> +Bool ret = TRUE;
>
> if (IsFloating(pDev))
> return FALSE;
> @@ -165,8 +174,10 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr
> pScreen, CursorPtr pCursor)
> if (pCursor != pDev->spriteInfo->anim.pCursor) {
> AnimCurPtr ac = GetAnimCur(pCursor);
>
> -ret = (*pScreen->DisplayCursor)
> -(pDev, pScreen, ac->elts[0].pCursor);
> +AnimCurCancelTimer(pDev);
> +ret = (*pScreen->DisplayCursor) (pDev, pScreen,
> + ac->elts[0].pCursor);
> +
This is a slight change in behavior if DisplayCursor fails since it will
cancel the previous timer whereas before it wouldn't. Are there any
weird cases where failing to change the cursor is expected and canceling
animation of the previous cursor would be a problem?
Assuming not, both changes
Reviewed-by: Aaron Plattner
I'll try to put together a build to test these today.
> if (ret) {
> pDev->spriteInfo->anim.elt = 0;
> pDev->spriteInfo->anim.pCursor = pCursor;
> @@ -176,15 +187,9 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr
> pScreen, CursorPtr pCursor)
> AnimCurTimerNotify, pDev);
> }
> }
> -else
> -ret = TRUE;
> }
> else {
> -CursorPtr old = pDev->spriteInfo->anim.pCursor;
> -
> -if (old && IsAnimCur(old))
> -TimerCancel(GetAnimCur(old)->timer);
> -
> +AnimCurCancelTimer(pDev);
> pDev->spriteInfo->anim.pCursor = 0;
> pDev->spriteInfo->anim.pScreen = 0;
> ret = (*pScreen->DisplayCursor) (pDev, pScreen, pCursor);
>
___
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] animcur: Move timer into pDev->spriteInfo->anim
On 01/09/2018 08:49 AM, Adam Jackson wrote:
> On Mon, 2018-01-08 at 15:02 -0800, Aaron Plattner wrote:
>> Commit 094a63d56fbfb9e23210cc9ac538fb198af37cee moved the timer that handles
>> animated cursors from the per-screen AnimCurScreenRec into the per-cursor
>> AnimCurRec. However, the timer that runs takes the DeviceIntPtr as its
>> argument,
>> and looks up which screen the cursor is supposed to be on from that.
>>
>> This leads to a problem when a device changes from one animated cursor to
>> another: The timer for the first cursor is not canceled. Then, when the
>> device
>> transitions to a static cursor, the second timer is canceled
>> pDev->spriteInfo->anim.pScreen is cleared. Finally, the first timer fires and
>> AnimCurTimerNotify crashes because pScreen is NULL.
>>
>> Since there's only ever supposed to be one timer pending for a given device,
>> move the timer into pDev->spriteInfo->anim.pTimer. This timer is canceled
>> when
>> transitioning to a static cursor, and re-armed when transitioning from one
>> animated cursor to another.
>>
>> Signed-off-by: Aaron Plattner
>> Reported-by:
>> https://devtalk.nvidia.com/default/topic/1028172/linux/titan-v-ubuntu-16-04lts-and-387-34-driver-crashes-badly/post/5230967/#5230967
>> ---
>> I'm not convinced that I fully understood how these timers worked with
>> multiple
>> X screens before 094a63d56fbf, so hopefully this is an acceptable fix. It
>> seems
>> to work in my single-X-screen testing.
>
> I think what you've got gets multiple screens right. I had considered
> just always cancelling the timer first, but that would mean re-arming
> the timer constantly too, and the less we can call TimerSet (and thus
> GetTimeInMillis) the better.
>
> There's one corner case I think this gets wrong:
>
>> @@ -325,13 +321,11 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas,
>> int ncursor,
>> pCursor->id = cid;
>>
>> ac = GetAnimCur(pCursor);
>> -ac->timer = TimerSet(NULL, 0, 0, AnimCurTimerNotify, NULL);
>
> This bit matters. If TimerSet's first argument is NULL it will
> allocate, and that can fail. If the TimerSet in AnimCurDisplayCursor
> would fail, we would need to unwind the work we just did to change the
> sprite, which could itself fail. That's too hairy to want to deal with,
> hence allocating up front. (Granted I didn't _check_ whether that
> allocation succeeded...)
Yeah, that's a good point.
> But then the other problem is:
>
>> --- a/include/inputstr.h
>> +++ b/include/inputstr.h
>> @@ -514,6 +514,7 @@ typedef struct _SpriteInfoRec {
>> struct {
>> CursorPtr pCursor;
>> ScreenPtr pScreen;
>> +OsTimerPtr pTimer;
>> int elt;
>> } anim;
>> } SpriteInfoRec, *SpriteInfoPtr;
>
> That's an ABI header, so I can't cherry-pick this fix to 1.19.
I'd be surprised if anyone outside the server used these fields, but
you're right, better safe than sorry.
> I think we can just cancel the old timer when changing animcurs.
> Patches to follow in a moment.
Agreed.
> - ajax
>
___
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 2/2] animcur: Handle allocation failure for the animation timer
Signed-off-by: Adam Jackson
---
render/animcur.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/render/animcur.c b/render/animcur.c
index 797029443c..b5d222bc6e 100644
--- a/render/animcur.c
+++ b/render/animcur.c
@@ -299,7 +299,7 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, int
ncursor,
CursorPtr *ppCursor, ClientPtr client, XID cid)
{
CursorPtr pCursor;
-int rc, i;
+int rc = BadAlloc, i;
AnimCurPtr ac;
for (i = 0; i < screenInfo.numScreens; i++)
@@ -314,7 +314,7 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, int
ncursor,
sizeof(AnimCurRec) +
ncursor * sizeof(AnimCurElt), 1);
if (!pCursor)
-return BadAlloc;
+return rc;
dixInitPrivates(pCursor, pCursor + 1, PRIVATE_CURSOR);
pCursor->bits = &animCursorBits;
pCursor->refcnt = 1;
@@ -333,8 +333,10 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, int
ncursor,
ac->timer = TimerSet(NULL, 0, 0, AnimCurTimerNotify, NULL);
/* security creation/labeling check */
-rc = XaceHook(XACE_RESOURCE_ACCESS, client, cid, RT_CURSOR, pCursor,
- RT_NONE, NULL, DixCreateAccess);
+if (ac->timer)
+rc = XaceHook(XACE_RESOURCE_ACCESS, client, cid, RT_CURSOR, pCursor,
+ RT_NONE, NULL, DixCreateAccess);
+
if (rc != Success) {
TimerFree(ac->timer);
dixFiniPrivates(pCursor, PRIVATE_CURSOR);
--
2.14.3
___
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 1/2] animcur: Fix transitions between animated cursors
We weren't cancelling the old timer when changing cursors, making things
go all crashy. Logically we could always cancel the timer first, but
then we'd have to call TimerSet to re-arm ourselves, and GetTimeInMillis
is potentially expensive.
Reported-by:
https://devtalk.nvidia.com/default/topic/1028172/linux/titan-v-ubuntu-16-04lts-and-387-34-driver-crashes-badly/post/5230967/#5230967
Signed-off-by: Adam Jackson
---
render/animcur.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/render/animcur.c b/render/animcur.c
index 058bc1b323..797029443c 100644
--- a/render/animcur.c
+++ b/render/animcur.c
@@ -151,11 +151,20 @@ AnimCurTimerNotify(OsTimerPtr timer, CARD32 now, void
*arg)
return ac->elts[elt].delay;
}
+static void
+AnimCurCancelTimer(DeviceIntPtr pDev)
+{
+CursorPtr cur = pDev->spriteInfo->anim.pCursor;
+
+if (IsAnimCur(cur))
+TimerCancel(GetAnimCur(cur)->timer);
+}
+
static Bool
AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor)
{
AnimCurScreenPtr as = GetAnimCurScreen(pScreen);
-Bool ret;
+Bool ret = TRUE;
if (IsFloating(pDev))
return FALSE;
@@ -165,8 +174,10 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr pScreen,
CursorPtr pCursor)
if (pCursor != pDev->spriteInfo->anim.pCursor) {
AnimCurPtr ac = GetAnimCur(pCursor);
-ret = (*pScreen->DisplayCursor)
-(pDev, pScreen, ac->elts[0].pCursor);
+AnimCurCancelTimer(pDev);
+ret = (*pScreen->DisplayCursor) (pDev, pScreen,
+ ac->elts[0].pCursor);
+
if (ret) {
pDev->spriteInfo->anim.elt = 0;
pDev->spriteInfo->anim.pCursor = pCursor;
@@ -176,15 +187,9 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr pScreen,
CursorPtr pCursor)
AnimCurTimerNotify, pDev);
}
}
-else
-ret = TRUE;
}
else {
-CursorPtr old = pDev->spriteInfo->anim.pCursor;
-
-if (old && IsAnimCur(old))
-TimerCancel(GetAnimCur(old)->timer);
-
+AnimCurCancelTimer(pDev);
pDev->spriteInfo->anim.pCursor = 0;
pDev->spriteInfo->anim.pScreen = 0;
ret = (*pScreen->DisplayCursor) (pDev, pScreen, pCursor);
--
2.14.3
___
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] animcur: Move timer into pDev->spriteInfo->anim
On Mon, 2018-01-08 at 15:02 -0800, Aaron Plattner wrote:
> Commit 094a63d56fbfb9e23210cc9ac538fb198af37cee moved the timer that handles
> animated cursors from the per-screen AnimCurScreenRec into the per-cursor
> AnimCurRec. However, the timer that runs takes the DeviceIntPtr as its
> argument,
> and looks up which screen the cursor is supposed to be on from that.
>
> This leads to a problem when a device changes from one animated cursor to
> another: The timer for the first cursor is not canceled. Then, when the device
> transitions to a static cursor, the second timer is canceled
> pDev->spriteInfo->anim.pScreen is cleared. Finally, the first timer fires and
> AnimCurTimerNotify crashes because pScreen is NULL.
>
> Since there's only ever supposed to be one timer pending for a given device,
> move the timer into pDev->spriteInfo->anim.pTimer. This timer is canceled when
> transitioning to a static cursor, and re-armed when transitioning from one
> animated cursor to another.
>
> Signed-off-by: Aaron Plattner
> Reported-by:
> https://devtalk.nvidia.com/default/topic/1028172/linux/titan-v-ubuntu-16-04lts-and-387-34-driver-crashes-badly/post/5230967/#5230967
> ---
> I'm not convinced that I fully understood how these timers worked with
> multiple
> X screens before 094a63d56fbf, so hopefully this is an acceptable fix. It
> seems
> to work in my single-X-screen testing.
I think what you've got gets multiple screens right. I had considered
just always cancelling the timer first, but that would mean re-arming
the timer constantly too, and the less we can call TimerSet (and thus
GetTimeInMillis) the better.
There's one corner case I think this gets wrong:
> @@ -325,13 +321,11 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas,
> int ncursor,
> pCursor->id = cid;
>
> ac = GetAnimCur(pCursor);
> -ac->timer = TimerSet(NULL, 0, 0, AnimCurTimerNotify, NULL);
This bit matters. If TimerSet's first argument is NULL it will
allocate, and that can fail. If the TimerSet in AnimCurDisplayCursor
would fail, we would need to unwind the work we just did to change the
sprite, which could itself fail. That's too hairy to want to deal with,
hence allocating up front. (Granted I didn't _check_ whether that
allocation succeeded...)
But then the other problem is:
> --- a/include/inputstr.h
> +++ b/include/inputstr.h
> @@ -514,6 +514,7 @@ typedef struct _SpriteInfoRec {
> struct {
> CursorPtr pCursor;
> ScreenPtr pScreen;
> +OsTimerPtr pTimer;
> int elt;
> } anim;
> } SpriteInfoRec, *SpriteInfoPtr;
That's an ABI header, so I can't cherry-pick this fix to 1.19.
I think we can just cancel the old timer when changing animcurs.
Patches to follow in a moment.
- ajax
___
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 2/2] randr: Use RRCrtcGetScanoutSize() in ProcRRSetScreenSize()
On 2018-01-09 03:44 AM, Alex Goins wrote:
> Previously, ProcRRSetScreenSize() manually computed the dimensions of a CRTC's
> viewport in order to check that it does not extend beyond the bounds of the
> new
> screen size. It did this incorrectly, leading to bugs.
>
> A previous patch "randr: Fix rotation check in ProcRRSetScreenSize()" fixed
> the
> issue by directly correcting the calculation, but to avoid future bugs of this
> class, this patch uses RRCrtcGetScanoutSize() to do the calculation,
> implicitly
> accounting for all possible transforms.
>
> There is existing precedent for this method in ProcRRSetCrtcConfig(), where
> RRModeGetScanoutSize() is used directly due to the transform having not yet
> been
> applied but the check is otherwise the same.
>
> Signed-off-by: Alex Goins
> ---
> randr/rrscreen.c | 13 -
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/randr/rrscreen.c b/randr/rrscreen.c
> index f484383..7321eef 100644
> --- a/randr/rrscreen.c
> +++ b/randr/rrscreen.c
> @@ -265,17 +265,12 @@ ProcRRSetScreenSize(ClientPtr client)
> }
> for (i = 0; i < pScrPriv->numCrtcs; i++) {
> RRCrtcPtr crtc = pScrPriv->crtcs[i];
> -RRModePtr mode = crtc->mode;
>
> -if (mode) {
> -int source_width = mode->mode.width;
> -int source_height = mode->mode.height;
> -Rotation rotation = crtc->rotation;
> +if (crtc->mode) {
> +int source_width;
> +int source_height;
>
> -if (rotation & (RR_Rotate_90 | RR_Rotate_270)) {
> -source_width = mode->mode.height;
> -source_height = mode->mode.width;
> -}
> +RRCrtcGetScanoutSize(crtc, &source_width, &source_height);
>
> if (crtc->x + source_width > stuff->width ||
> crtc->y + source_height > stuff->height)
>
Reviewed-by: Michel Dänzer
Is there any reason to have patch 1 separately?
P.S. Looks like there are similar issues in randr/rrcrtc.c, in
crtc_to_box and rrCheckPixmapBounding.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
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
