Re: [PATCH xserver 3/3] ramdac: Handle master and slave cursors independently

[Alex Goins]

> On 09/11/17 02:34 AM, Alex Goins wrote:
> > On Wed, 8 Nov 2017, Michel Dänzer wrote:

> I'm asking this to avoid ending up in a similar situation as with the
> PRIME synchronization functionality, where it seems like the modesetting
> driver still can't use it with itself as the peer.

The modesetting driver has always been able to use PRIME Sync with itself as the
peer. See changes b83dede9 and 80e64dae, source and sink implementations,
respectively. Both went in at the same time.

-Alex___
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: [RFC xserver v3 00/11] DRI3 v1.2: DMA fences

[Louis-Francis Ratté-Boulianne]

Hi,

On Thu, 2017-11-09 at 11:49 +0100, Nicolai Hähnle wrote:
> Will this also support the wait-fence part of PresentPixmap?

Yes, that has been implemented in the current patchset. If the present
driver supports it (e.g. modesetting/glamor), it can handle the fence
itself. If not, the Present core is gonna poll the DMA fence fd.

I hope I understood correctly your question,

--
Louis-Francis

> Having an explicit wait-fence in PresentPixmap could potentially
> help 
> with OpenGL driver multi-threading. We currently have to sync with
> the 
> driver thread before sending PresentPixmap, to ensure that the
> necessary 
> command submissions have completed in the kernel and an implicit
> fence 
> added to the buffer.
> 
> It would be better if we could just create a syncobj in the 
> application/API thread, add that as a wait-fence for the present,
> and 
> hand it off to the driver thread to be signaled once previous GL 
> commands have completed.
> 
> Cheers,
> Nicolai
> 
> 
> On 06.11.2017 22:42, Louis-Francis Ratté-Boulianne wrote:
> > Hello,
> > 
> > This patchset implements of what we would like to see become DRI3
> > v1.2,
> > that is the implementation of DMA fences.
> > For some context, please see previous submissions:
> > 
> > https://lists.x.org/archives/xorg-devel/2017-August/054439.html
> > https://lists.x.org/archives/xorg-devel/2017-September/054770.html
> > 
> > The main change in this iteration is:
> > 
> >   - Make SetTriggered and Reset return a result instead of adding
> > a new 'type' field for fences.
> > 
> > Here are the repositories:
> > 
> > https://gitlab.collabora.com/lfrb/dri3proto/commits/rfc/2017-10/x11
> > -fences
> > https://gitlab.collabora.com/lfrb/presentproto/commits/rfc/2017-11/
> > x11-fences
> > https://gitlab.collabora.com/lfrb/xserver/commits/rfc/2017-11/x11-f
> > ences
> > 
> > Thanks,
> > 
> > Louis-Francis
> > 
> > ___
> > 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
> > 
> 
> 
___
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] glx: Only flush indirect contexts in MakeCurrent

[Adam Jackson]

If the context is direct none of the GL commands were issued by this
process, the server couldn't flush them even if it wanted to.

Signed-off-by: Adam Jackson 
---
 glx/glxcmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index 241abc6a5..421129a4d 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -626,7 +626,7 @@ DoMakeCurrent(__GLXclientState * cl,
 /*
  ** Flush the previous context if needed.
  */
-Bool need_flush = GL_TRUE;
+Bool need_flush = prevglxc->isDirect;
 #ifdef GLX_CONTEXT_RELEASE_BEHAVIOR_ARB
 if (prevglxc->releaseBehavior == GLX_CONTEXT_RELEASE_BEHAVIOR_NONE_ARB)
 need_flush = GL_FALSE;
-- 
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 3/3] ramdac: Handle master and slave cursors independently

[Michel Dänzer]

On 09/11/17 03:06 PM, Hans de Goede wrote:
> On 09-11-17 02:34, Alex Goins wrote:
>> On Wed, 8 Nov 2017, Hans de Goede wrote:
>>
 and that would be preferable to using the server's software cursor (due
 to the fact that it's unsynchronzied by OpenGL rendering to the root
 window, it can cause corruption with certain compositors).
>>>
>>> Frankly, that sounds like an issue with your direct rendering
>>> infrastructure. We used to have the same issue with DRI1, but no more
>>> with DRI2/DRI3 (we still have an intermittent cursor flickering issue
>>> though, but not corruption).
>>
>> Really? I observe the same issue using mesa + modesetting + glamor +
>> i915 +
>> mutter. Dragging a window around with software cursor produces a
>> square of
>> corruption around the cursor.
>>
>> In any case, I would like some means to bring back the pre-7b634067
>> functionality. In situations such as using UDL as a slave, it appears
>> as a
>> regression. An opt-in mechanism for drivers that support master-driven
>> cursor is
>> acceptable, I think. I'll follow up with a new patch set implementing the
>> feedback so far if you agree.
> 
> I'll leave further discussion of this between you and Michel Däzner. FWIW
> I do believe that Michel's suggestion to implement the master-driven cursor
> overlay in some generic place so it works with all the drivers is a better
> solution then allowing drivers to opt-out.

Unfortunately, that's not really possible, because at least
xf86-video-intel doesn't use PixmapSyncDirtyHelper. So even if that is
made to handle this transparently, there needs to be an opt-in mechanism.


-- 
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 3/3] ramdac: Handle master and slave cursors independently

[Hans de Goede]

Hi,

On 09-11-17 02:34, Alex Goins wrote:

Thanks both for the quick feedback. Replies inline.

On Wed, 8 Nov 2017, Hans de Goede wrote:


https://lists.x.org/archives/xorg-devel/2016-September/050973.html implies
that
xf86CursorScreenPtr is part of the ABI. Mark xf86CursorPriv.h as such.


I'm not sure if exporting this is a good idea. I assume you are just
after xf86CursorScreenPtr->SWCursor ?  If so it is probably a better
idea to add a helper function taking pScreen which gets that and add
that function to the ABI.


That works for me, this is just based on the past advice for drivers to lookup
xf86CursorScreenPtr to tell if we are using a SW cursor, which sets a precedent
of it being considered ABI.


I understand, but I now realize that was partially bad advice, IMHO
we really don't want to grow the ABI, if at all possible we want to
shrink it. In my mind exporting a function is less likely to lead to
future ABI breaks then exporting the contents of a struct, hence
my preference for the helper function.


+Bool
+xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr
infoPtr)
+{
+Bool use_hw_cursor = FALSE;
+
+input_lock();
+
+if (xf86CheckSlavesHWCursor_locked(pScreen, cursor)) {
+use_hw_cursor = TRUE;
+goto unlock;


This goto seems wrong, what if the secondary GPU has outputs which
can do a hw-cursor, but the primary GPU also has active video outputs
and the primary GPU does not support a hw-cursor ?

I think you always need to check the master supports a hw-cursor.


+/* if there are no active slaves that support HW cursor, or using the
HW
+ * cursor on them failed, use the master */
+if (ScreenPriv->SlaveHWCursor) {
+xf86SetSlavesCursor_locked(pScreen, NullCursor, x, y);
+ScreenPriv->SlaveHWCursor = FALSE;
+}
+ret = xf86ScreenSetCursor(pScreen, pCurs, x, y);
  out:
   input_unlock();



Again what if both the primary and secondary GPU have active video outputs,
then you need to update the cursor on both. You seem te be thinking about
something like the XPS15 here, used with the nvidia driver forcing the
nvidia GPU to become the primary GPU, so the primary will not have any
video-outputs. But when using for example a Latitude E6430 in optimus
mode with nouveau, the intel GPU will be the primary, driving the LCD
panel and the nvidia GPU will be the secondary driving the HDMI output,
so you need to set the cursor on both.


You're right, there is a possibility of combined native and PRIME outputs. In
fact, I typically test on a desktop system with iGPU Multi-Monitor, where this
possibility is especially clear.

I overlooked the case of native + PRIME where the PRIME slave supports HW
cursor. Native + PRIME with non-HW cursor slave works, but the former doesn't.


Ok.



On Wed, 8 Nov 2017, Michel Dänzer wrote:


Change 7b634067 added HW cursor support for PRIME by removing the
pixmap_dirty_list check from xf86CursorSetCursor() and making the requisite
cursor functions set/check the cursor both on master and slave.

However, before this change, drivers that did not make use of pixmap_dirty_list
to implement PRIME master were able to pass this check. That may have been a
bug, but in effect it allowed hardware cursor to be enabled with PRIME, where
the master composites the cursor into the shared pixmap.

Naturally, the slave driving an actual hardware cursor is preferable to the
master compositing a cursor into the shared pixmap, but there are certain
situations where the slave cannot drive a hardware cursor (certain DRM drivers,
or when used with a transform). In these cases the master may still be capable
of compositing a cursor,


How and where exactly would this "compositing the cursor into the shared
pixmap" happen? Looks like this is just left for the master screen
driver to handle? If so, there probably needs to be a mechanism for the
master screen driver to opt into this.


Yes, it's just left for the master screen the handle as if it were a hardware
cursor.

Prior to change 7b634067 this was the existing behavior of the NVIDIA driver +
PRIME outputs, due to the fact that the check to exclude HW cursor support on
PRIME outputs relied on pScreen->pixmap_dirty_list not being empty. With NVIDIA
as the master, X would successfully set a cursor on the master despite PRIME
being in use, and the master would use that to composite its own cursor into the
shared pixmap.

Change 7b634067 had the side effect of preventing this configuration, and the
intent here is to re-enable it as an alternative fallback to the server's SW
cursor, while maintaining the possibility of slave-driven HW cursor in
configurations that support it.


and that would be preferable to using the server's software cursor (due
to the fact that it's unsynchronzied by OpenGL rendering to the root
window, it can cause corruption with certain compositors).


Frankly, that sounds like an issue with your direct rendering
infrastructure. We used to 

Re: [PATCH xserver 3/3] ramdac: Handle master and slave cursors independently

[Michel Dänzer]

On 09/11/17 02:34 AM, Alex Goins wrote:
> On Wed, 8 Nov 2017, Michel Dänzer wrote:
> 
>>> Change 7b634067 added HW cursor support for PRIME by removing the
>>> pixmap_dirty_list check from xf86CursorSetCursor() and making the requisite
>>> cursor functions set/check the cursor both on master and slave.
>>>
>>> However, before this change, drivers that did not make use of 
>>> pixmap_dirty_list
>>> to implement PRIME master were able to pass this check. That may have been a
>>> bug, but in effect it allowed hardware cursor to be enabled with PRIME, 
>>> where
>>> the master composites the cursor into the shared pixmap.
>>>
>>> Naturally, the slave driving an actual hardware cursor is preferable to the
>>> master compositing a cursor into the shared pixmap, but there are certain
>>> situations where the slave cannot drive a hardware cursor (certain DRM 
>>> drivers,
>>> or when used with a transform). In these cases the master may still be 
>>> capable
>>> of compositing a cursor,
>>
>> How and where exactly would this "compositing the cursor into the shared
>> pixmap" happen? Looks like this is just left for the master screen
>> driver to handle? If so, there probably needs to be a mechanism for the
>> master screen driver to opt into this.
> 
> Yes, it's just left for the master screen the handle as if it were a hardware
> cursor.

Please provide an implementation of this as part of the series. In order
of my preference:

1. Make PixmapSyncDirtyHelper handle it automagically.

2. Add a helper function that drivers can call after
   PixmapSyncDirtyHelper.

I'm asking this to avoid ending up in a similar situation as with the
PRIME synchronization functionality, where it seems like the modesetting
driver still can't use it with itself as the peer.


>>> and that would be preferable to using the server's software cursor (due
>>> to the fact that it's unsynchronzied by OpenGL rendering to the root
>>> window, it can cause corruption with certain compositors).
>>
>> Frankly, that sounds like an issue with your direct rendering
>> infrastructure. We used to have the same issue with DRI1, but no more
>> with DRI2/DRI3 (we still have an intermittent cursor flickering issue
>> though, but not corruption).
> 
> Really? I observe the same issue using mesa + modesetting + glamor + i915 +
> mutter. Dragging a window around with software cursor produces a square of
> corruption around the cursor.

That's a modesetting driver bug. It allows page flipping with SW cursor,
but that can't work correctly. At least xf86-video-amdgpu/ati don't have
this issue.


-- 
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: [RFC xserver v3 00/11] DRI3 v1.2: DMA fences

[Nicolai Hähnle]

Will this also support the wait-fence part of PresentPixmap?

Having an explicit wait-fence in PresentPixmap could potentially help 
with OpenGL driver multi-threading. We currently have to sync with the 
driver thread before sending PresentPixmap, to ensure that the necessary 
command submissions have completed in the kernel and an implicit fence 
added to the buffer.


It would be better if we could just create a syncobj in the 
application/API thread, add that as a wait-fence for the present, and 
hand it off to the driver thread to be signaled once previous GL 
commands have completed.


Cheers,
Nicolai


On 06.11.2017 22:42, Louis-Francis Ratté-Boulianne wrote:

Hello,

This patchset implements of what we would like to see become DRI3 v1.2,
that is the implementation of DMA fences.
For some context, please see previous submissions:

https://lists.x.org/archives/xorg-devel/2017-August/054439.html
https://lists.x.org/archives/xorg-devel/2017-September/054770.html

The main change in this iteration is:

  - Make SetTriggered and Reset return a result instead of adding
a new 'type' field for fences.

Here are the repositories:

https://gitlab.collabora.com/lfrb/dri3proto/commits/rfc/2017-10/x11-fences
https://gitlab.collabora.com/lfrb/presentproto/commits/rfc/2017-11/x11-fences
https://gitlab.collabora.com/lfrb/xserver/commits/rfc/2017-11/x11-fences

Thanks,

Louis-Francis

___
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




--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
___
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 3/3] randr: free crtc->outputs on destroy

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta 
---
 randr/rrcrtc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
index c904fa035..39af679b2 100644
--- a/randr/rrcrtc.c
+++ b/randr/rrcrtc.c
@@ -878,6 +878,7 @@ RRCrtcDestroyResource(void *value, XID pid)
 free(crtc->gammaRed);
 if (crtc->mode)
 RRModeDestroy(crtc->mode);
+free(crtc->outputs);
 free(crtc);
 return 1;
 }
-- 
2.14.1.439.g647b9b4702

___
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/3] randr: always realloc crtcs and outputs

[Giuseppe Bilotta]

When the last crtc (resp. output) is destroyed, the rrScrPriv crtcs
(resp. outputs) fields do not get cleared, which can lead to a situation
where the private's numCrtcs (resp. numOutputs) field is zero, but the
associated memory is still allocated. Just checking if numCrtcs (resp.
numOutputs) is zero is thus not a good criteria to determine whetehr to
use a realloc or a malloc.

Since crtcs (resp. outputs) are NULL-initialized anyway, relying on
numCrtcs (resp. numOutputs) is actually unnecessary, because
reallocation of a NULL ptr is equivalent to a malloc anyway.

Therefore, just use realloc() unconditionally, and ensure that the
fields are properly initialized.

Signed-off-by: Giuseppe Bilotta 
---
 randr/rrcrtc.c   | 9 +++--
 randr/rroutput.c | 9 +++--
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
index 2eb9fbdc8..c904fa035 100644
--- a/randr/rrcrtc.c
+++ b/randr/rrcrtc.c
@@ -66,13 +66,10 @@ RRCrtcCreate(ScreenPtr pScreen, void *devPrivate)
 pScrPriv = rrGetScrPriv(pScreen);
 
 /* make space for the crtc pointer */
-if (pScrPriv->numCrtcs)
-crtcs = reallocarray(pScrPriv->crtcs,
- pScrPriv->numCrtcs + 1, sizeof(RRCrtcPtr));
-else
-crtcs = malloc(sizeof(RRCrtcPtr));
+crtcs = reallocarray(pScrPriv->crtcs,
+pScrPriv->numCrtcs + 1, sizeof(RRCrtcPtr));
 if (!crtcs)
-return FALSE;
+return NULL;
 pScrPriv->crtcs = crtcs;
 
 crtc = calloc(1, sizeof(RRCrtcRec));
diff --git a/randr/rroutput.c b/randr/rroutput.c
index 647f19a52..90fe4e6c1 100644
--- a/randr/rroutput.c
+++ b/randr/rroutput.c
@@ -71,13 +71,10 @@ RROutputCreate(ScreenPtr pScreen,
 
 pScrPriv = rrGetScrPriv(pScreen);
 
-if (pScrPriv->numOutputs)
-outputs = reallocarray(pScrPriv->outputs,
-   pScrPriv->numOutputs + 1, sizeof(RROutputPtr));
-else
-outputs = malloc(sizeof(RROutputPtr));
+outputs = reallocarray(pScrPriv->outputs,
+   pScrPriv->numOutputs + 1, sizeof(RROutputPtr));
 if (!outputs)
-return FALSE;
+return NULL;
 
 pScrPriv->outputs = outputs;
 
-- 
2.14.1.439.g647b9b4702

___
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/3] randr: rrGetScreenResources: initialize memory

[Giuseppe Bilotta]

Similarly to bb766ef11227bd8c71ac65845d1930edd0eda40d, ensure that the
extra padding is set to 0.

Signed-off-by: Giuseppe Bilotta 
---
 randr/rrscreen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/randr/rrscreen.c b/randr/rrscreen.c
index d6c499580..0c70b28dd 100644
--- a/randr/rrscreen.c
+++ b/randr/rrscreen.c
@@ -558,7 +558,7 @@ rrGetScreenResources(ClientPtr client, Bool query)
 
 extraLen = rep.length << 2;
 if (extraLen) {
-extra = malloc(extraLen);
+extra = calloc(1, extraLen);
 if (!extra) {
 free(modes);
 return BadAlloc;
-- 
2.14.1.439.g647b9b4702

___
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 0/3] randr valgrind cleanups

[Giuseppe Bilotta]

These three small patches replace and complement my previous “randr: properly
cleanup on crtc and output destroy”.

The first one cleans up another bit of unitialized memory being sent over the
wire (similar to the previous ProcRRGtOutputInfo patch) which I didn't spot 
earlier,
and the other two fix the leaks related to dynamic creation and destruction of 
crtcs
and outputs (hopefull correctly, this time ;-))

Giuseppe Bilotta (3):
  randr: rrGetScreenResources: initialize memory
  randr: always realloc crtcs and outputs
  randr: free crtc->outputs on destroy

 randr/rrcrtc.c   | 10 --
 randr/rroutput.c |  9 +++--
 randr/rrscreen.c |  2 +-
 3 files changed, 8 insertions(+), 13 deletions(-)

-- 
2.14.1.439.g647b9b4702

___
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