<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> Hi AlexG, I just has a question. Did you suppose the output master device(dGPU) is equivalent to master screen, output slave(iGPU) device is equivalent to slave screen in modesetting driver under PRIME(not reverse) case? Thanks JimQu <div class="moz-cite-prefix">On 2018年08月18日 02:55, Alex Goins wrote: </div> <blockquote type="cite" cite="mid:alpine.DEB.2.20.1808171314480.26533@agoins-DiGiTS"> <pre wrap="">I think this patch is along the right lines, but misses part of the fix. Although I don't have access to a modesetting<->modesetting PRIME output slaving setup to test this theory right now, I think this will break it. Have you tested that this path is exercised? It is true that SharedPixmapNotifyDamage() is a function implemented by the slave that should only be called by the master, and that ppriv->notify_on_damage is supposed to be set on master's ppriv. The current implementation is incorrect, as you've found, but it still worked just fine for modesetting<->modesetting PRIME output slaving. Why is that? The control flow should be as follows (and only applies to PRIME Sync): -- 1) Slave calls master->PresentSharedPixmap(), and it fails (see drmmode_SharedPixmapPresent). 2) Slave calls master->RequestSharedPixmapNotifyDamage(pSlavePix) to request for the master to call slave->SharedPixmapNotifyDamage(pSlavePix) when the backing master pixmap receives damage. If the master is the modesetting driver, the master sets pMasterPixPriv->notify_on_damage = TRUE to know that it needs to notify the slave when damage is received. 3) When the master receives damage, if pMasterPixPriv->notify_on_damage == TRUE, it calls slave->SharedPixmapNotifyDamage(pSlavePix) and then sets pMasterPixPriv->notify_on_damage = FALSE. -- There are currently errors in both step 2 and step 3. In step 2, the modesetting driver as master currently sets pSlavePixPriv->notify_on_damage = TRUE instead of pMasterPixPriv->notify_on_damage = TRUE. In step 3, the modesetting driver as master checks pSlavePixPriv->notify_on_damage == TRUE, and if it passes, calls pSlave->SharedPixmapNotifyDamage(pSlavePix). This currently works on modesetting<->modesetting PRIME because while both are wrong, they are consistently wrong, and on modesetting<->modesetting PRIME setups, pSlavePixPriv is accessible to both master and slave. All of the attributes are being stored in and accessed from pSlavePixPriv, whereas the master-related ones should actually be stored in pMasterPixPriv. My understanding of your setup is that you have modesetting as the master, and AMD as the slave. Thus, when the master attempts to access pSlavePixPriv, problems result due to the fact that it's accessing AMD's 'ppriv' using the modesetting structure definition. Your patch fixes step 3 by getting 'ppriv' from the master pixmap (pMasterPixPriv as above), rather than the slave. The !screen->isGPU check is probably good to have, but it's probably redundant because pixmap_dirty_list should be empty for GPU screens. I think that is all fine. The patch also needs to fix step 2, again by getting 'ppriv' from the master pixmap rather than the slave, but this time for msRequestSharedPixmapNotifyDamage(). Without that, pMasterPixPriv->notify_on_damage will always be FALSE, breaking modesetting<->modesetting PRIME. Moreover, it could cause more problems like the original bug if a non-modesetting slave were to call master->RequestSharedPixmapNotifyDamage(pSlavePix). I was also confused as to why slave->SharedPixmapNotifyDamage() is being called at all on your setup, as 'randr: falling back to unsynchronized pixmap sharing' indicates that PRIME Sync is disabled which should mean that ppriv->notify_on_damage should always be FALSE. I think that's just because ppriv is currently that of the AMD slave, so it's reading some random part of AMD's ppriv using modesetting's structure definition. I think if you fix msRequestSharedPixmapNotifyDamage() as described above, this patch should be good to go, but as it stands, it will likely introduce a regression in modesetting<->modesetting PRIME. Please fix that, then verify that modesetting<->modesetting PRIME (with PRIME Sync) still works. I can also verify it myself once I have access to a test setup that will work with it. Thanks, Alex On Wed, 8 Aug 2018, Alex Deucher wrote: </pre> <blockquote type="cite"> <pre wrap="">On Sun, Aug 5, 2018 at 10:40 PM, Jim Qu <a class="moz-txt-link-rfc2396E" href="mailto:Jim.Qu@amd.com"><Jim.Qu@amd.com></a> wrote: </pre> <blockquote type="cite"> <pre wrap="">On some Intel iGPU + AMD dGPU platform, when connect extern display from dGPU, X will crash, 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) There is NULL pointer accessing on ent->slave_dst->drawable.pScreen-> SharedPixmapNotifyDamage. On the platform, since the dGPU is GPU device, so that the iGPU is output master device. SharedPixmapNotifyDamage() should be called when current device is output master. Change-Id: I8fa6922a4f75b5e068970fc4d362f778052379f2 Signed-off-by: Jim Qu <a class="moz-txt-link-rfc2396E" href="mailto:Jim.Qu@amd.com"><Jim.Qu@amd.com></a> </pre> </blockquote> <pre wrap=""> Reviewed-by: Alex Deucher <a class="moz-txt-link-rfc2396E" href="mailto:alexander.deucher@amd.com"><alexander.deucher@amd.com></a> </pre> <blockquote type="cite"> <pre wrap="">--- hw/xfree86/drivers/modesetting/driver.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c index 9362370..37fafb1 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); -- 2.7.4 _______________________________________________ <a class="moz-txt-link-abbreviated" href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>: X.Org development Archives: <a class="moz-txt-link-freetext" href="http://lists.x.org/archives/xorg-devel">http://lists.x.org/archives/xorg-devel</a> Info: <a class="moz-txt-link-freetext" href="https://lists.x.org/mailman/listinfo/xorg-devel">https://lists.x.org/mailman/listinfo/xorg-devel</a> </pre> </blockquote> <pre wrap="">_______________________________________________ <a class="moz-txt-link-abbreviated" href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>: X.Org development Archives: <a class="moz-txt-link-freetext" href="http://lists.x.org/archives/xorg-devel">http://lists.x.org/archives/xorg-devel</a> Info: <a class="moz-txt-link-freetext" href="https://lists.x.org/mailman/listinfo/xorg-devel">https://lists.x.org/mailman/listinfo/xorg-devel</a> </pre> </blockquote> </blockquote> </body> </html>