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 <[email protected]> > > 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 > > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
