modesetting: code refactor for PRIME sync

Submitted by Qu, Jim on Aug. 24, 2018, 3:30 a.m.

Details

Message ID 1535081458-30808-1-git-send-email-Jim.Qu@amd.com
State New
Headers show
Series "modesetting: code refactor for PRIME sync" ( rev: 1 ) in X.org

Not browsing as part of any series.

Commit Message

Qu, Jim Aug. 24, 2018, 3:30 a.m.
The PRIME sync on modesetting assume that all two GPUs are used
modesetting driver on the hybrid system. The X will be crashed
on the system with other DDX driver, such as amdgpu.

show the log like:

randr: falling back to unsynchronized pixmap sharing
(EE)
(EE) Backtrace:
(EE) 0: /usr/lib/xorg/Xorg (xorg_backtrace+0x4e)
(EE) 1: /usr/lib/xorg/Xorg (0x55cb0151a000+0x1b5ce9)
(EE) 2: /lib/x86_64-linux-gnu/libpthread.so.0 (0x7f1587a1d000+0x11390)
(EE)
(EE) Segmentation fault at address 0x0
(EE)

The issue is that modesetting as the master, and amdgpu as the slave.
Thus, when the master attempts to access pSlavePixPriv in ms_dirty_update(),
problems result due to the fact that it's accessing AMD's 'ppriv' using the
modesetting structure definition.

Apart from fixing crash issue, the patch also try to resolve the dependence
between master interface and slave interface, that say driver can not assume
that the other also use modesetting.

To make the logic cleanly, the slave always input master pixmap to master
interfaces, master can refer the slave pixmap in master driver if it need.
there is an exception, master->StartPixmapTracking()/StopPixmapTracking,
the backend PixmapStartDirtyTracking()/PixmapStopDirtyTracking() are used by
other vendors, so it can be refined in next step.

Signed-off-by: Jim Qu <Jim.Qu@amd.com>

Change-Id: I4398ff85bb69848cacda1d20db960283bcc526b5
---
 dix/pixmap.c                                     |  1 +
 fb/fbpixmap.c                                    |  1 +
 hw/xfree86/drivers/modesetting/driver.c          | 54 ++++++++++++------------
 hw/xfree86/drivers/modesetting/drmmode_display.c |  4 +-
 include/pixmapstr.h                              |  1 +
 randr/rrcrtc.c                                   | 10 ++---
 6 files changed, 38 insertions(+), 33 deletions(-)

Patch hide | download patch | download mbox

diff --git a/dix/pixmap.c b/dix/pixmap.c
index 81ac5e2..ee5ad73 100644
--- a/dix/pixmap.c
+++ b/dix/pixmap.c
@@ -162,6 +162,7 @@  PixmapPtr PixmapShareToSlave(PixmapPtr pixmap, ScreenPtr slave)
     pixmap->refcnt++;
 
     spix->master_pixmap = pixmap;
+    pixmap->slave_pixmap = spix;
 
     ret = slave->SetSharedPixmapBacking(spix, handle);
     if (ret == FALSE) {
diff --git a/fb/fbpixmap.c b/fb/fbpixmap.c
index a524200..982af3f 100644
--- a/fb/fbpixmap.c
+++ b/fb/fbpixmap.c
@@ -69,6 +69,7 @@  fbCreatePixmap(ScreenPtr pScreen, int width, int height, int depth,
     pPixmap->refcnt = 1;
     pPixmap->devPrivate.ptr = (void *) ((char *) pPixmap + base + adjust);
     pPixmap->master_pixmap = NULL;
+    pPixmap->slave_pixmap = NULL;
 
 #ifdef FB_DEBUG
     pPixmap->devPrivate.ptr =
diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
index 9362370..bccdb20 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -640,19 +640,21 @@  ms_dirty_update(ScreenPtr screen, int *timeout)
     xorg_list_for_each_entry(ent, &screen->pixmap_dirty_list, ent) {
         region = DamageRegion(ent->damage);
         if (RegionNotEmpty(region)) {
-            msPixmapPrivPtr ppriv =
-                msGetPixmapPriv(&ms->drmmode, ent->slave_dst);
+            if (!screen->isGPU) {
+                msPixmapPrivPtr ppriv =
+                    msGetPixmapPriv(&ms->drmmode, ent->slave_dst->master_pixmap);
 
-            if (ppriv->notify_on_damage) {
-                ppriv->notify_on_damage = FALSE;
+                if (ppriv->notify_on_damage) {
+                    ppriv->notify_on_damage = FALSE;
 
-                ent->slave_dst->drawable.pScreen->
-                    SharedPixmapNotifyDamage(ent->slave_dst);
-            }
+                    ent->slave_dst->drawable.pScreen->
+                        SharedPixmapNotifyDamage(ent->slave_dst);
+                }
 
-            /* Requested manual updating */
-            if (ppriv->defer_dirty_update)
-                continue;
+                /* Requested manual updating */
+                if (ppriv->defer_dirty_update)
+                    continue;
+            }
 
             redisplay_dirty(screen, ent, timeout);
             DamageEmpty(ent->damage);
@@ -1244,32 +1246,32 @@  msDisableSharedPixmapFlipping(RRCrtcPtr crtc)
 
 static Bool
 msStartFlippingPixmapTracking(RRCrtcPtr crtc, DrawablePtr src,
-                              PixmapPtr slave_dst1, PixmapPtr slave_dst2,
+                              PixmapPtr master1, PixmapPtr master2,
                               int x, int y, int dst_x, int dst_y,
                               Rotation rotation)
 {
     ScreenPtr pScreen = src->pScreen;
     modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
 
-    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, slave_dst1),
-                    ppriv2 = msGetPixmapPriv(&ms->drmmode, slave_dst2);
+    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, master1),
+                    ppriv2 = msGetPixmapPriv(&ms->drmmode, master2);
 
-    if (!PixmapStartDirtyTracking(src, slave_dst1, x, y,
+    if (!PixmapStartDirtyTracking(src, master1->slave_pixmap, x, y,
                                   dst_x, dst_y, rotation)) {
         return FALSE;
     }
 
-    if (!PixmapStartDirtyTracking(src, slave_dst2, x, y,
+    if (!PixmapStartDirtyTracking(src, master2->slave_pixmap, x, y,
                                   dst_x, dst_y, rotation)) {
-        PixmapStopDirtyTracking(src, slave_dst1);
+        PixmapStopDirtyTracking(src, master1->slave_pixmap);
         return FALSE;
     }
 
     ppriv1->slave_src = src;
     ppriv2->slave_src = src;
 
-    ppriv1->dirty = ms_dirty_get_ent(pScreen, slave_dst1);
-    ppriv2->dirty = ms_dirty_get_ent(pScreen, slave_dst2);
+    ppriv1->dirty = ms_dirty_get_ent(pScreen, master1->slave_pixmap);
+    ppriv2->dirty = ms_dirty_get_ent(pScreen, master2->slave_pixmap);
 
     ppriv1->defer_dirty_update = TRUE;
     ppriv2->defer_dirty_update = TRUE;
@@ -1278,12 +1280,12 @@  msStartFlippingPixmapTracking(RRCrtcPtr crtc, DrawablePtr src,
 }
 
 static Bool
-msPresentSharedPixmap(PixmapPtr slave_dst)
+msPresentSharedPixmap(PixmapPtr master)
 {
-    ScreenPtr pScreen = slave_dst->drawable.pScreen;
+    ScreenPtr pScreen = master->drawable.pScreen;
     modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
 
-    msPixmapPrivPtr ppriv = msGetPixmapPriv(&ms->drmmode, slave_dst);
+    msPixmapPrivPtr ppriv = msGetPixmapPriv(&ms->drmmode, master);
 
     RegionPtr region = DamageRegion(ppriv->dirty->damage);
 
@@ -1299,18 +1301,18 @@  msPresentSharedPixmap(PixmapPtr slave_dst)
 
 static Bool
 msStopFlippingPixmapTracking(DrawablePtr src,
-                             PixmapPtr slave_dst1, PixmapPtr slave_dst2)
+                             PixmapPtr master1, PixmapPtr master2)
 {
     ScreenPtr pScreen = src->pScreen;
     modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
 
-    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, slave_dst1),
-                    ppriv2 = msGetPixmapPriv(&ms->drmmode, slave_dst2);
+    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, master1),
+                    ppriv2 = msGetPixmapPriv(&ms->drmmode, master2);
 
     Bool ret = TRUE;
 
-    ret &= PixmapStopDirtyTracking(src, slave_dst1);
-    ret &= PixmapStopDirtyTracking(src, slave_dst2);
+    ret &= PixmapStopDirtyTracking(src, master1->slave_pixmap);
+    ret &= PixmapStopDirtyTracking(src, master2->slave_pixmap);
 
     if (ret) {
         ppriv1->slave_src = NULL;
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
index f6f2e9f..5525383 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -1073,7 +1073,7 @@  drmmode_SharedPixmapPresent(PixmapPtr ppix, xf86CrtcPtr crtc,
 {
     ScreenPtr master = crtc->randr_crtc->pScreen->current_master;
 
-    if (master->PresentSharedPixmap(ppix)) {
+    if (master->PresentSharedPixmap(ppix->master_pixmap)) {
         /* Success, queue flip to back target */
         if (drmmode_SharedPixmapFlip(ppix, crtc, drmmode))
             return TRUE;
@@ -1091,7 +1091,7 @@  drmmode_SharedPixmapPresent(PixmapPtr ppix, xf86CrtcPtr crtc,
         /* Set flag first in case we are immediately notified */
         ppriv->wait_for_damage = TRUE;
 
-        if (master->RequestSharedPixmapNotifyDamage(ppix))
+        if (master->RequestSharedPixmapNotifyDamage(ppix->master_pixmap))
             return TRUE;
         else
             ppriv->wait_for_damage = FALSE;
diff --git a/include/pixmapstr.h b/include/pixmapstr.h
index de50101..d572ae3 100644
--- a/include/pixmapstr.h
+++ b/include/pixmapstr.h
@@ -85,6 +85,7 @@  typedef struct _Pixmap {
     unsigned usage_hint;        /* see CREATE_PIXMAP_USAGE_* */
 
     PixmapPtr master_pixmap;    /* pointer to master copy of pixmap for pixmap sharing */
+    PixmapPtr slave_pixmap;  /* pointer to slave copy of pixmap for pixmap sharing */
 } PixmapRec;
 
 typedef struct _PixmapDirtyUpdate {
diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
index 5d90262..f7463ff 100644
--- a/randr/rrcrtc.c
+++ b/randr/rrcrtc.c
@@ -402,8 +402,8 @@  RRCrtcDetachScanoutPixmap(RRCrtcPtr crtc)
             pScrPriv->rrDisableSharedPixmapFlipping(crtc);
 
             master->StopFlippingPixmapTracking(mrootdraw,
-                                               crtc->scanout_pixmap,
-                                               crtc->scanout_pixmap_back);
+                                               crtc->scanout_pixmap->master_pixmap,
+                                               crtc->scanout_pixmap_back->master_pixmap);
 
             rrDestroySharedPixmap(crtc, crtc->scanout_pixmap_back);
             crtc->scanout_pixmap_back = NULL;
@@ -561,15 +561,15 @@  rrSetupPixmapSharing(RRCrtcPtr crtc, int width, int height,
 
         if (!pMasterScrPriv->rrStartFlippingPixmapTracking(crtc,
                                                            mrootdraw,
-                                                           spix_front,
-                                                           spix_back,
+                                                           spix_front->master_pixmap,
+                                                           spix_back->master_pixmap,
                                                            x, y, 0, 0,
                                                            rotation)) {
             pSlaveScrPriv->rrDisableSharedPixmapFlipping(crtc);
             goto fail;
         }
 
-        master->PresentSharedPixmap(spix_front);
+        master->PresentSharedPixmap(spix_front->master_pixmap);
 
         return TRUE;
 

Comments

Inline

On Fri, 24 Aug 2018, Jim Qu wrote:

> The PRIME sync on modesetting assume that all two GPUs are used
> modesetting driver on the hybrid system. The X will be crashed
> on the system with other DDX driver, such as amdgpu.

It only makes this assumption when the other DDX driver is the slave. It works
just fine as-is if the other DDX driver is the master (in my testing when
developing this, I was testing 'NVIDIA master / modesetting(i915) slave' and
'modesetting(Nouveau) master / modesetting(i915) slave', so the bug wasn't
exposed.

> show the log like:
> 
> randr: falling back to unsynchronized pixmap sharing
> (EE)
> (EE) Backtrace:
> (EE) 0: /usr/lib/xorg/Xorg (xorg_backtrace+0x4e)
> (EE) 1: /usr/lib/xorg/Xorg (0x55cb0151a000+0x1b5ce9)
> (EE) 2: /lib/x86_64-linux-gnu/libpthread.so.0 (0x7f1587a1d000+0x11390)
> (EE)
> (EE) Segmentation fault at address 0x0
> (EE)
> 
> The issue is that modesetting as the master, and amdgpu as the slave.
> Thus, when the master attempts to access pSlavePixPriv in ms_dirty_update(),
> problems result due to the fact that it's accessing AMD's 'ppriv' using the
> modesetting structure definition.
> 
> Apart from fixing crash issue, the patch also try to resolve the dependence
> between master interface and slave interface, that say driver can not assume
> that the other also use modesetting.
> 
> To make the logic cleanly, the slave always input master pixmap to master
> interfaces, master can refer the slave pixmap in master driver if it need.
> there is an exception, master->StartPixmapTracking()/StopPixmapTracking,
> the backend PixmapStartDirtyTracking()/PixmapStopDirtyTracking() are used by
> other vendors, so it can be refined in next step.
> 
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> 
> Change-Id: I4398ff85bb69848cacda1d20db960283bcc526b5
> ---
>  dix/pixmap.c                                     |  1 +
>  fb/fbpixmap.c                                    |  1 +
>  hw/xfree86/drivers/modesetting/driver.c          | 54 ++++++++++++------------
>  hw/xfree86/drivers/modesetting/drmmode_display.c |  4 +-
>  include/pixmapstr.h                              |  1 +
>  randr/rrcrtc.c                                   | 10 ++---
>  6 files changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/dix/pixmap.c b/dix/pixmap.c
> index 81ac5e2..ee5ad73 100644
> --- a/dix/pixmap.c
> +++ b/dix/pixmap.c
> @@ -162,6 +162,7 @@ PixmapPtr PixmapShareToSlave(PixmapPtr pixmap, ScreenPtr slave)
>      pixmap->refcnt++;
>  
>      spix->master_pixmap = pixmap;
> +    pixmap->slave_pixmap = spix;
>  
>      ret = slave->SetSharedPixmapBacking(spix, handle);
>      if (ret == FALSE) {
> diff --git a/fb/fbpixmap.c b/fb/fbpixmap.c
> index a524200..982af3f 100644
> --- a/fb/fbpixmap.c
> +++ b/fb/fbpixmap.c
> @@ -69,6 +69,7 @@ fbCreatePixmap(ScreenPtr pScreen, int width, int height, int depth,
>      pPixmap->refcnt = 1;
>      pPixmap->devPrivate.ptr = (void *) ((char *) pPixmap + base + adjust);
>      pPixmap->master_pixmap = NULL;
> +    pPixmap->slave_pixmap = NULL;
>  
>  #ifdef FB_DEBUG
>      pPixmap->devPrivate.ptr =
> diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
> index 9362370..bccdb20 100644
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -640,19 +640,21 @@ ms_dirty_update(ScreenPtr screen, int *timeout)
>      xorg_list_for_each_entry(ent, &screen->pixmap_dirty_list, ent) {
>          region = DamageRegion(ent->damage);
>          if (RegionNotEmpty(region)) {
> -            msPixmapPrivPtr ppriv =
> -                msGetPixmapPriv(&ms->drmmode, ent->slave_dst);
> +            if (!screen->isGPU) {
> +                msPixmapPrivPtr ppriv =
> +                    msGetPixmapPriv(&ms->drmmode, ent->slave_dst->master_pixmap);
>  
> -            if (ppriv->notify_on_damage) {
> -                ppriv->notify_on_damage = FALSE;
> +                if (ppriv->notify_on_damage) {
> +                    ppriv->notify_on_damage = FALSE;
>  
> -                ent->slave_dst->drawable.pScreen->
> -                    SharedPixmapNotifyDamage(ent->slave_dst);
> -            }
> +                    ent->slave_dst->drawable.pScreen->
> +                        SharedPixmapNotifyDamage(ent->slave_dst);
> +                }
>  
> -            /* Requested manual updating */
> -            if (ppriv->defer_dirty_update)
> -                continue;
> +                /* Requested manual updating */
> +                if (ppriv->defer_dirty_update)
> +                    continue;
> +            }
>  
>              redisplay_dirty(screen, ent, timeout);
>              DamageEmpty(ent->damage);
> @@ -1244,32 +1246,32 @@ msDisableSharedPixmapFlipping(RRCrtcPtr crtc)
>  
>  static Bool
>  msStartFlippingPixmapTracking(RRCrtcPtr crtc, DrawablePtr src,
> -                              PixmapPtr slave_dst1, PixmapPtr slave_dst2,
> +                              PixmapPtr master1, PixmapPtr master2,
>                                int x, int y, int dst_x, int dst_y,

This is an ABI break.

>  {
>      ScreenPtr pScreen = src->pScreen;
>      modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
>  
> -    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, slave_dst1),
> -                    ppriv2 = msGetPixmapPriv(&ms->drmmode, slave_dst2);
> +    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, master1),
> +                    ppriv2 = msGetPixmapPriv(&ms->drmmode, master2);
>  
> -    if (!PixmapStartDirtyTracking(src, slave_dst1, x, y,
> +    if (!PixmapStartDirtyTracking(src, master1->slave_pixmap, x, y,
>                                    dst_x, dst_y, rotation)) {
>          return FALSE;
>      }
>  
> -    if (!PixmapStartDirtyTracking(src, slave_dst2, x, y,
> +    if (!PixmapStartDirtyTracking(src, master2->slave_pixmap, x, y,
>                                    dst_x, dst_y, rotation)) {
> -        PixmapStopDirtyTracking(src, slave_dst1);
> +        PixmapStopDirtyTracking(src, master1->slave_pixmap);
>          return FALSE;
>      }
>  
>      ppriv1->slave_src = src;
>      ppriv2->slave_src = src;
>  
> -    ppriv1->dirty = ms_dirty_get_ent(pScreen, slave_dst1);
> -    ppriv2->dirty = ms_dirty_get_ent(pScreen, slave_dst2);
> +    ppriv1->dirty = ms_dirty_get_ent(pScreen, master1->slave_pixmap);
> +    ppriv2->dirty = ms_dirty_get_ent(pScreen, master2->slave_pixmap);
>  
>      ppriv1->defer_dirty_update = TRUE;
>      ppriv2->defer_dirty_update = TRUE;
> @@ -1278,12 +1280,12 @@ msStartFlippingPixmapTracking(RRCrtcPtr crtc, DrawablePtr src,
>  }
>  
>  static Bool
> -msPresentSharedPixmap(PixmapPtr slave_dst)
> +msPresentSharedPixmap(PixmapPtr master)
>  {

Same here. This interface is part of the ABI.

> -    ScreenPtr pScreen = slave_dst->drawable.pScreen;
> +    ScreenPtr pScreen = master->drawable.pScreen;
>      modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
>  
> -    msPixmapPrivPtr ppriv = msGetPixmapPriv(&ms->drmmode, slave_dst);
> +    msPixmapPrivPtr ppriv = msGetPixmapPriv(&ms->drmmode, master);
>  
>      RegionPtr region = DamageRegion(ppriv->dirty->damage);
>  
> @@ -1299,18 +1301,18 @@ msPresentSharedPixmap(PixmapPtr slave_dst)
>  
>  static Bool
>  msStopFlippingPixmapTracking(DrawablePtr src,
> -                             PixmapPtr slave_dst1, PixmapPtr slave_dst2)
> +                             PixmapPtr master1, PixmapPtr master2)

Also an ABI break.

>  {
>      ScreenPtr pScreen = src->pScreen;
>      modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
>  
> -    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, slave_dst1),
> -                    ppriv2 = msGetPixmapPriv(&ms->drmmode, slave_dst2);
> +    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, master1),
> +                    ppriv2 = msGetPixmapPriv(&ms->drmmode, master2);
>  
>      Bool ret = TRUE;
>  
> -    ret &= PixmapStopDirtyTracking(src, slave_dst1);
> -    ret &= PixmapStopDirtyTracking(src, slave_dst2);
> +    ret &= PixmapStopDirtyTracking(src, master1->slave_pixmap);
> +    ret &= PixmapStopDirtyTracking(src, master2->slave_pixmap);
>  
>      if (ret) {
>          ppriv1->slave_src = NULL;
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index f6f2e9f..5525383 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -1073,7 +1073,7 @@ drmmode_SharedPixmapPresent(PixmapPtr ppix, xf86CrtcPtr crtc,
>  {
>      ScreenPtr master = crtc->randr_crtc->pScreen->current_master;
>  
> -    if (master->PresentSharedPixmap(ppix)) {
> +    if (master->PresentSharedPixmap(ppix->master_pixmap)) {
>          /* Success, queue flip to back target */
>          if (drmmode_SharedPixmapFlip(ppix, crtc, drmmode))
>              return TRUE;
> @@ -1091,7 +1091,7 @@ drmmode_SharedPixmapPresent(PixmapPtr ppix, xf86CrtcPtr crtc,
>          /* Set flag first in case we are immediately notified */
>          ppriv->wait_for_damage = TRUE;
>  
> -        if (master->RequestSharedPixmapNotifyDamage(ppix))
> +        if (master->RequestSharedPixmapNotifyDamage(ppix->master_pixmap))

ABI break.

These might make more logical sense, but changing them will require bumping the
ABI. Moreover, they follow the precedent of the non-sync
{Start/Stop}PixmapTracking, so if we change these, we should probably change
those too.

Thanks,
Alex

>              return TRUE;
>          else
>              ppriv->wait_for_damage = FALSE;
> diff --git a/include/pixmapstr.h b/include/pixmapstr.h
> index de50101..d572ae3 100644
> --- a/include/pixmapstr.h
> +++ b/include/pixmapstr.h
> @@ -85,6 +85,7 @@ typedef struct _Pixmap {
>      unsigned usage_hint;        /* see CREATE_PIXMAP_USAGE_* */
>  
>      PixmapPtr master_pixmap;    /* pointer to master copy of pixmap for pixmap sharing */
> +    PixmapPtr slave_pixmap;  /* pointer to slave copy of pixmap for pixmap sharing */
>  } PixmapRec;
>  
>  typedef struct _PixmapDirtyUpdate {
> diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
> index 5d90262..f7463ff 100644
> --- a/randr/rrcrtc.c
> +++ b/randr/rrcrtc.c
> @@ -402,8 +402,8 @@ RRCrtcDetachScanoutPixmap(RRCrtcPtr crtc)
>              pScrPriv->rrDisableSharedPixmapFlipping(crtc);
>  
>              master->StopFlippingPixmapTracking(mrootdraw,
> -                                               crtc->scanout_pixmap,
> -                                               crtc->scanout_pixmap_back);
> +                                               crtc->scanout_pixmap->master_pixmap,
> +                                               crtc->scanout_pixmap_back->master_pixmap);
>  
>              rrDestroySharedPixmap(crtc, crtc->scanout_pixmap_back);
>              crtc->scanout_pixmap_back = NULL;
> @@ -561,15 +561,15 @@ rrSetupPixmapSharing(RRCrtcPtr crtc, int width, int height,
>  
>          if (!pMasterScrPriv->rrStartFlippingPixmapTracking(crtc,
>                                                             mrootdraw,
> -                                                           spix_front,
> -                                                           spix_back,
> +                                                           spix_front->master_pixmap,
> +                                                           spix_back->master_pixmap,
>                                                             x, y, 0, 0,
>                                                             rotation)) {
>              pSlaveScrPriv->rrDisableSharedPixmapFlipping(crtc);
>              goto fail;
>          }
>  
> -        master->PresentSharedPixmap(spix_front);
> +        master->PresentSharedPixmap(spix_front->master_pixmap);
>  
>          return TRUE;
>  
> -- 
> 2.7.4
> 
>
在 2018/8/25 0:50, Alex Goins 写道:
> Inline
>
> On Fri, 24 Aug 2018, Jim Qu wrote:
>
>> The PRIME sync on modesetting assume that all two GPUs are used
>> modesetting driver on the hybrid system. The X will be crashed
>> on the system with other DDX driver, such as amdgpu.
> It only makes this assumption when the other DDX driver is the slave. It works
> just fine as-is if the other DDX driver is the master (in my testing when
> developing this, I was testing 'NVIDIA master / modesetting(i915) slave' and
> 'modesetting(Nouveau) master / modesetting(i915) slave', so the bug wasn't
> exposed.

OK, I will update the comments.

>> show the log like:
>>
>> randr: falling back to unsynchronized pixmap sharing
>> (EE)
>> (EE) Backtrace:
>> (EE) 0: /usr/lib/xorg/Xorg (xorg_backtrace+0x4e)
>> (EE) 1: /usr/lib/xorg/Xorg (0x55cb0151a000+0x1b5ce9)
>> (EE) 2: /lib/x86_64-linux-gnu/libpthread.so.0 (0x7f1587a1d000+0x11390)
>> (EE)
>> (EE) Segmentation fault at address 0x0
>> (EE)
>>
>> The issue is that modesetting as the master, and amdgpu as the slave.
>> Thus, when the master attempts to access pSlavePixPriv in ms_dirty_update(),
>> problems result due to the fact that it's accessing AMD's 'ppriv' using the
>> modesetting structure definition.
>>
>> Apart from fixing crash issue, the patch also try to resolve the dependence
>> between master interface and slave interface, that say driver can not assume
>> that the other also use modesetting.
>>
>> To make the logic cleanly, the slave always input master pixmap to master
>> interfaces, master can refer the slave pixmap in master driver if it need.
>> there is an exception, master->StartPixmapTracking()/StopPixmapTracking,
>> the backend PixmapStartDirtyTracking()/PixmapStopDirtyTracking() are used by
>> other vendors, so it can be refined in next step.
>>
>> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
>>
>> Change-Id: I4398ff85bb69848cacda1d20db960283bcc526b5
>> ---
>>   dix/pixmap.c                                     |  1 +
>>   fb/fbpixmap.c                                    |  1 +
>>   hw/xfree86/drivers/modesetting/driver.c          | 54 ++++++++++++------------
>>   hw/xfree86/drivers/modesetting/drmmode_display.c |  4 +-
>>   include/pixmapstr.h                              |  1 +
>>   randr/rrcrtc.c                                   | 10 ++---
>>   6 files changed, 38 insertions(+), 33 deletions(-)
>>
>> diff --git a/dix/pixmap.c b/dix/pixmap.c
>> index 81ac5e2..ee5ad73 100644
>> --- a/dix/pixmap.c
>> +++ b/dix/pixmap.c
>> @@ -162,6 +162,7 @@ PixmapPtr PixmapShareToSlave(PixmapPtr pixmap, ScreenPtr slave)
>>       pixmap->refcnt++;
>>   
>>       spix->master_pixmap = pixmap;
>> +    pixmap->slave_pixmap = spix;
>>   
>>       ret = slave->SetSharedPixmapBacking(spix, handle);
>>       if (ret == FALSE) {
>> diff --git a/fb/fbpixmap.c b/fb/fbpixmap.c
>> index a524200..982af3f 100644
>> --- a/fb/fbpixmap.c
>> +++ b/fb/fbpixmap.c
>> @@ -69,6 +69,7 @@ fbCreatePixmap(ScreenPtr pScreen, int width, int height, int depth,
>>       pPixmap->refcnt = 1;
>>       pPixmap->devPrivate.ptr = (void *) ((char *) pPixmap + base + adjust);
>>       pPixmap->master_pixmap = NULL;
>> +    pPixmap->slave_pixmap = NULL;
>>   
>>   #ifdef FB_DEBUG
>>       pPixmap->devPrivate.ptr =
>> diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
>> index 9362370..bccdb20 100644
>> --- a/hw/xfree86/drivers/modesetting/driver.c
>> +++ b/hw/xfree86/drivers/modesetting/driver.c
>> @@ -640,19 +640,21 @@ ms_dirty_update(ScreenPtr screen, int *timeout)
>>       xorg_list_for_each_entry(ent, &screen->pixmap_dirty_list, ent) {
>>           region = DamageRegion(ent->damage);
>>           if (RegionNotEmpty(region)) {
>> -            msPixmapPrivPtr ppriv =
>> -                msGetPixmapPriv(&ms->drmmode, ent->slave_dst);
>> +            if (!screen->isGPU) {
>> +                msPixmapPrivPtr ppriv =
>> +                    msGetPixmapPriv(&ms->drmmode, ent->slave_dst->master_pixmap);
>>   
>> -            if (ppriv->notify_on_damage) {
>> -                ppriv->notify_on_damage = FALSE;
>> +                if (ppriv->notify_on_damage) {
>> +                    ppriv->notify_on_damage = FALSE;
>>   
>> -                ent->slave_dst->drawable.pScreen->
>> -                    SharedPixmapNotifyDamage(ent->slave_dst);
>> -            }
>> +                    ent->slave_dst->drawable.pScreen->
>> +                        SharedPixmapNotifyDamage(ent->slave_dst);
>> +                }
>>   
>> -            /* Requested manual updating */
>> -            if (ppriv->defer_dirty_update)
>> -                continue;
>> +                /* Requested manual updating */
>> +                if (ppriv->defer_dirty_update)
>> +                    continue;
>> +            }
>>   
>>               redisplay_dirty(screen, ent, timeout);
>>               DamageEmpty(ent->damage);
>> @@ -1244,32 +1246,32 @@ msDisableSharedPixmapFlipping(RRCrtcPtr crtc)
>>   
>>   static Bool
>>   msStartFlippingPixmapTracking(RRCrtcPtr crtc, DrawablePtr src,
>> -                              PixmapPtr slave_dst1, PixmapPtr slave_dst2,
>> +                              PixmapPtr master1, PixmapPtr master2,
>>                                 int x, int y, int dst_x, int dst_y,
> This is an ABI break.
>
>>   {
>>       ScreenPtr pScreen = src->pScreen;
>>       modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
>>   
>> -    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, slave_dst1),
>> -                    ppriv2 = msGetPixmapPriv(&ms->drmmode, slave_dst2);
>> +    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, master1),
>> +                    ppriv2 = msGetPixmapPriv(&ms->drmmode, master2);
>>   
>> -    if (!PixmapStartDirtyTracking(src, slave_dst1, x, y,
>> +    if (!PixmapStartDirtyTracking(src, master1->slave_pixmap, x, y,
>>                                     dst_x, dst_y, rotation)) {
>>           return FALSE;
>>       }
>>   
>> -    if (!PixmapStartDirtyTracking(src, slave_dst2, x, y,
>> +    if (!PixmapStartDirtyTracking(src, master2->slave_pixmap, x, y,
>>                                     dst_x, dst_y, rotation)) {
>> -        PixmapStopDirtyTracking(src, slave_dst1);
>> +        PixmapStopDirtyTracking(src, master1->slave_pixmap);
>>           return FALSE;
>>       }
>>   
>>       ppriv1->slave_src = src;
>>       ppriv2->slave_src = src;
>>   
>> -    ppriv1->dirty = ms_dirty_get_ent(pScreen, slave_dst1);
>> -    ppriv2->dirty = ms_dirty_get_ent(pScreen, slave_dst2);
>> +    ppriv1->dirty = ms_dirty_get_ent(pScreen, master1->slave_pixmap);
>> +    ppriv2->dirty = ms_dirty_get_ent(pScreen, master2->slave_pixmap);
>>   
>>       ppriv1->defer_dirty_update = TRUE;
>>       ppriv2->defer_dirty_update = TRUE;
>> @@ -1278,12 +1280,12 @@ msStartFlippingPixmapTracking(RRCrtcPtr crtc, DrawablePtr src,
>>   }
>>   
>>   static Bool
>> -msPresentSharedPixmap(PixmapPtr slave_dst)
>> +msPresentSharedPixmap(PixmapPtr master)
>>   {
> Same here. This interface is part of the ABI.
>
>> -    ScreenPtr pScreen = slave_dst->drawable.pScreen;
>> +    ScreenPtr pScreen = master->drawable.pScreen;
>>       modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
>>   
>> -    msPixmapPrivPtr ppriv = msGetPixmapPriv(&ms->drmmode, slave_dst);
>> +    msPixmapPrivPtr ppriv = msGetPixmapPriv(&ms->drmmode, master);
>>   
>>       RegionPtr region = DamageRegion(ppriv->dirty->damage);
>>   
>> @@ -1299,18 +1301,18 @@ msPresentSharedPixmap(PixmapPtr slave_dst)
>>   
>>   static Bool
>>   msStopFlippingPixmapTracking(DrawablePtr src,
>> -                             PixmapPtr slave_dst1, PixmapPtr slave_dst2)
>> +                             PixmapPtr master1, PixmapPtr master2)
> Also an ABI break.
>
>>   {
>>       ScreenPtr pScreen = src->pScreen;
>>       modesettingPtr ms = modesettingPTR(xf86ScreenToScrn(pScreen));
>>   
>> -    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, slave_dst1),
>> -                    ppriv2 = msGetPixmapPriv(&ms->drmmode, slave_dst2);
>> +    msPixmapPrivPtr ppriv1 = msGetPixmapPriv(&ms->drmmode, master1),
>> +                    ppriv2 = msGetPixmapPriv(&ms->drmmode, master2);
>>   
>>       Bool ret = TRUE;
>>   
>> -    ret &= PixmapStopDirtyTracking(src, slave_dst1);
>> -    ret &= PixmapStopDirtyTracking(src, slave_dst2);
>> +    ret &= PixmapStopDirtyTracking(src, master1->slave_pixmap);
>> +    ret &= PixmapStopDirtyTracking(src, master2->slave_pixmap);
>>   
>>       if (ret) {
>>           ppriv1->slave_src = NULL;
>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> index f6f2e9f..5525383 100644
>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> @@ -1073,7 +1073,7 @@ drmmode_SharedPixmapPresent(PixmapPtr ppix, xf86CrtcPtr crtc,
>>   {
>>       ScreenPtr master = crtc->randr_crtc->pScreen->current_master;
>>   
>> -    if (master->PresentSharedPixmap(ppix)) {
>> +    if (master->PresentSharedPixmap(ppix->master_pixmap)) {
>>           /* Success, queue flip to back target */
>>           if (drmmode_SharedPixmapFlip(ppix, crtc, drmmode))
>>               return TRUE;
>> @@ -1091,7 +1091,7 @@ drmmode_SharedPixmapPresent(PixmapPtr ppix, xf86CrtcPtr crtc,
>>           /* Set flag first in case we are immediately notified */
>>           ppriv->wait_for_damage = TRUE;
>>   
>> -        if (master->RequestSharedPixmapNotifyDamage(ppix))
>> +        if (master->RequestSharedPixmapNotifyDamage(ppix->master_pixmap))
> ABI break.
>
> These might make more logical sense, but changing them will require bumping the
> ABI. Moreover, they follow the precedent of the non-sync
> {Start/Stop}PixmapTracking, so if we change these, we should probably change
> those too.
>
> Thanks,
> Alex

Mm, it looks like I think more things but ignore the ABI issue. The 
thing I need to do should keep current ABI, but just fix the modesetting 
issue.

Thanks
JimQu
>>               return TRUE;
>>           else
>>               ppriv->wait_for_damage = FALSE;
>> diff --git a/include/pixmapstr.h b/include/pixmapstr.h
>> index de50101..d572ae3 100644
>> --- a/include/pixmapstr.h
>> +++ b/include/pixmapstr.h
>> @@ -85,6 +85,7 @@ typedef struct _Pixmap {
>>       unsigned usage_hint;        /* see CREATE_PIXMAP_USAGE_* */
>>   
>>       PixmapPtr master_pixmap;    /* pointer to master copy of pixmap for pixmap sharing */
>> +    PixmapPtr slave_pixmap;  /* pointer to slave copy of pixmap for pixmap sharing */
>>   } PixmapRec;
>>   
>>   typedef struct _PixmapDirtyUpdate {
>> diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
>> index 5d90262..f7463ff 100644
>> --- a/randr/rrcrtc.c
>> +++ b/randr/rrcrtc.c
>> @@ -402,8 +402,8 @@ RRCrtcDetachScanoutPixmap(RRCrtcPtr crtc)
>>               pScrPriv->rrDisableSharedPixmapFlipping(crtc);
>>   
>>               master->StopFlippingPixmapTracking(mrootdraw,
>> -                                               crtc->scanout_pixmap,
>> -                                               crtc->scanout_pixmap_back);
>> +                                               crtc->scanout_pixmap->master_pixmap,
>> +                                               crtc->scanout_pixmap_back->master_pixmap);
>>   
>>               rrDestroySharedPixmap(crtc, crtc->scanout_pixmap_back);
>>               crtc->scanout_pixmap_back = NULL;
>> @@ -561,15 +561,15 @@ rrSetupPixmapSharing(RRCrtcPtr crtc, int width, int height,
>>   
>>           if (!pMasterScrPriv->rrStartFlippingPixmapTracking(crtc,
>>                                                              mrootdraw,
>> -                                                           spix_front,
>> -                                                           spix_back,
>> +                                                           spix_front->master_pixmap,
>> +                                                           spix_back->master_pixmap,
>>                                                              x, y, 0, 0,
>>                                                              rotation)) {
>>               pSlaveScrPriv->rrDisableSharedPixmapFlipping(crtc);
>>               goto fail;
>>           }
>>   
>> -        master->PresentSharedPixmap(spix_front);
>> +        master->PresentSharedPixmap(spix_front->master_pixmap);
>>   
>>           return TRUE;
>>   
>> -- 
>> 2.7.4
>>
>>