modesetting: Fix X crash in ms_dirty_update()

Submitted by Qu, Jim on Aug. 3, 2018, 1:27 p.m.

Details

Message ID 1533302862-12996-1-git-send-email-Jim.Qu@amd.com
State New
Series "modesetting: Fix X crash in ms_dirty_update()"
Headers show

Commit Message

Qu, Jim Aug. 3, 2018, 1:27 p.m.
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 slave.

Change-Id: Id633e29c126670ee64ff1f5f79d489e5068cd439
Signed-off-by: Jim Qu <Jim.Qu@amd.com>
---
 hw/xfree86/drivers/modesetting/driver.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
index 9362370..6022315 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -640,20 +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);
 
-            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);
-            }
-
-            /* Requested manual updating */
-            if (ppriv->defer_dirty_update)
-                continue;
+                    ent->slave_dst->drawable.pScreen->
+                        SharedPixmapNotifyDamage(ent->slave_dst);
+                }
 
+                /* Requested manual updating */
+                if (ppriv->defer_dirty_update)
+                    continue;
+            }
             redisplay_dirty(screen, ent, timeout);
             DamageEmpty(ent->damage);
         }

Comments

jimqu Aug. 3, 2018, 1:38 p.m.
Sorry, it poped an error when sent the first patch mail, so sent it again.

Hi Adam,

For urgent issue, I just made the patch first. please help review.

Thanks

JimQu

On 2018年08月03日 21:27, Jim Qu wrote:
> 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 slave.
>
> Change-Id: Id633e29c126670ee64ff1f5f79d489e5068cd439
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> ---
>   hw/xfree86/drivers/modesetting/driver.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
> index 9362370..6022315 100644
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -640,20 +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);
>   
> -            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);
> -            }
> -
> -            /* Requested manual updating */
> -            if (ppriv->defer_dirty_update)
> -                continue;
> +                    ent->slave_dst->drawable.pScreen->
> +                        SharedPixmapNotifyDamage(ent->slave_dst);
> +                }
>   
> +                /* Requested manual updating */
> +                if (ppriv->defer_dirty_update)
> +                    continue;
> +            }
>               redisplay_dirty(screen, ent, timeout);
>               DamageEmpty(ent->damage);
>           }
Michel Dänzer Aug. 3, 2018, 3:08 p.m.
On 2018-08-03 03:27 PM, Jim Qu wrote:
> 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 slave.
> 
> Change-Id: Id633e29c126670ee64ff1f5f79d489e5068cd439
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> ---
>  hw/xfree86/drivers/modesetting/driver.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
> index 9362370..6022315 100644
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -640,20 +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);
>  
> -            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);
> -            }
> -
> -            /* Requested manual updating */
> -            if (ppriv->defer_dirty_update)
> -                continue;
> +                    ent->slave_dst->drawable.pScreen->
> +                        SharedPixmapNotifyDamage(ent->slave_dst);
> +                }
>  
> +                /* Requested manual updating */
> +                if (ppriv->defer_dirty_update)
> +                    continue;
> +            }

I don't think this is right. E.g. why would the slave driver call its
own SharedPixmapNotifyDamage hook? Also, ppriv->defer_dirty_update is
only set to TRUE by hooks which are only called for the master screen.

So, I think the start of the new block needs to be something like:

            if (!screen->isGPU) {
                msPixmapPrivPtr ppriv =
                    msGetPixmapPriv(&ms->drmmode,
ent->slave_dst->master_pixmap);

and msStartFlippingPixmapTracking and msStopFlippingPixmapTracking also
need to use ->master_pixmap instead of the slave pixmaps.


Not sure about the defer_dirty_update related code, that might only make
sense in the slave.


The more I look at this PRIME synchronization related code, the more I
wonder if it was ever tested with master and slave screens using
different drivers...
jimqu Aug. 4, 2018, 12:29 p.m.
在 2018/8/3 23:08, Michel Dänzer 写道:
> On 2018-08-03 03:27 PM, Jim Qu wrote:
>> 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 slave.
>>
>> Change-Id: Id633e29c126670ee64ff1f5f79d489e5068cd439
>> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
>> ---
>>   hw/xfree86/drivers/modesetting/driver.c | 23 ++++++++++++-----------
>>   1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
>> index 9362370..6022315 100644
>> --- a/hw/xfree86/drivers/modesetting/driver.c
>> +++ b/hw/xfree86/drivers/modesetting/driver.c
>> @@ -640,20 +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);
>>   
>> -            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);
>> -            }
>> -
>> -            /* Requested manual updating */
>> -            if (ppriv->defer_dirty_update)
>> -                continue;
>> +                    ent->slave_dst->drawable.pScreen->
>> +                        SharedPixmapNotifyDamage(ent->slave_dst);
>> +                }
>>   
>> +                /* Requested manual updating */
>> +                if (ppriv->defer_dirty_update)
>> +                    continue;
>> +            }
> I don't think this is right. E.g. why would the slave driver call its
> own SharedPixmapNotifyDamage hook? Also, ppriv->defer_dirty_update is
> only set to TRUE by hooks which are only called for the master screen.
>
> So, I think the start of the new block needs to be something like:
>
>              if (!screen->isGPU) {
>                  msPixmapPrivPtr ppriv =
>                      msGetPixmapPriv(&ms->drmmode,
> ent->slave_dst->master_pixmap);

Indeed, you are right, current modsetting driver should be as output 
source(master screen), and it call slave's SharedPixmapNotifyDamage() to 
present shared buffer. I will update in the next version.

> and msStartFlippingPixmapTracking and msStopFlippingPixmapTracking also
> need to use ->master_pixmap instead of the slave pixmaps.
>
>
> Not sure about the defer_dirty_update related code, that might only make
> sense in the slave.
>
>
> The more I look at this PRIME synchronization related code, the more I
> wonder if it was ever tested with master and slave screens using
> different drivers...

Yeah,  Maybe , the master hook PresentSharedPixmap() also need master 
pixmap.

Thanks
JimQu
>
>