[PATCH] modesetting: code refactor for PRIME sync
jimqu
jimqu at amd.com
Sat Aug 25 03:12:21 UTC 2018
在 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 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
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
>>
>>
More information about the xorg-devel
mailing list