[PATCH] modesetting: code refactor for PRIME sync
Alex Goins
agoins at nvidia.com
Fri Aug 24 16:50:04 UTC 2018
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 at 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
>
>
More information about the xorg-devel
mailing list