[PATCH 2/2] randr: cleanup provider properly

[Knut Petersen]

On 11.01.2013 00:02, Peter Hutterer wrote:
> On Wed, Jan 09, 2013 at 01:14:55PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <airlied at redhat.com>
>>
>> So in the cold plug server shutdown case, we reap the resources
>> before we call CloseScreen handlers, so the config->randr_provider
>> is a dangling pointer when the xf86CrtcCloseScreen handler is called,
>>
>> however in the hot screen unplug case, we can't rely on automatically
>> reaped resources, so we need to clean up the provider in the xf86CrtcCloseScreen
>> case.
>>
>> This patch provides a cleanup callback from the randr provider removal
>> into the DDX so it can cleanup properly, this then gets called by the automatic
>> code for cold plug, or if hot unplug it gets called explicitly.
>>
>> Fixes a number of random server crashes on shutdown
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=58174
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=891140
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
> seems correct, as far as I understand this code
>
> Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>

Tested here, fixes bug 58174. Definitely needed in master asap.

Tested-by: Knut Petersen <knut.petersen at t-online.de>
>
> Cheers,
>     Peter
>
>
>> ---
>>   hw/xfree86/modes/xf86Crtc.c    | 12 ++----------
>>   hw/xfree86/modes/xf86RandR12.c | 22 ++++++++++++++++++++++
>>   randr/randrstr.h               |  6 ++++++
>>   randr/rrprovider.c             |  2 ++
>>   4 files changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
>> index 45764fc..1905b18 100644
>> --- a/hw/xfree86/modes/xf86Crtc.c
>> +++ b/hw/xfree86/modes/xf86Crtc.c
>> @@ -743,16 +743,8 @@ xf86CrtcCloseScreen(ScreenPtr screen)
>>       }
>>       /* detach any providers */
>>       if (config->randr_provider) {
>> -        if (config->randr_provider->offload_sink) {
>> -            DetachOffloadGPU(screen);
>> -            config->randr_provider->offload_sink = NULL;
>> -        }
>> -        else if (config->randr_provider->output_source) {
>> -            DetachOutputGPU(screen);
>> -            config->randr_provider->output_source = NULL;
>> -        }
>> -        else if (screen->current_master)
>> -            DetachUnboundGPU(screen);
>> +        RRProviderDestroy(config->randr_provider);
>> +        config->randr_provider = NULL;
>>       }
>>       return TRUE;
>>   }
>> diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
>> index 3530abf..01fc9c5 100644
>> --- a/hw/xfree86/modes/xf86RandR12.c
>> +++ b/hw/xfree86/modes/xf86RandR12.c
>> @@ -1885,6 +1885,27 @@ xf86RandR13ConstrainCursorHarder(DeviceIntPtr dev, ScreenPtr screen, int mode, i
>>       }
>>   }
>>   
>> +static void
>> +xf86RandR14ProviderDestroy(ScreenPtr screen, RRProviderPtr provider)
>> +{
>> +    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
>> +    xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
>> +
>> +    if (config->randr_provider == provider) {
>> +        if (config->randr_provider->offload_sink) {
>> +            DetachOffloadGPU(screen);
>> +            config->randr_provider->offload_sink = NULL;
>> +        }
>> +        else if (config->randr_provider->output_source) {
>> +            DetachOutputGPU(screen);
>> +            config->randr_provider->output_source = NULL;
>> +        }
>> +        else if (screen->current_master)
>> +            DetachUnboundGPU(screen);
>> +    }
>> +    config->randr_provider = NULL;
>> +}
>> +
>>   static Bool
>>   xf86RandR12Init12(ScreenPtr pScreen)
>>   {
>> @@ -1914,6 +1935,7 @@ xf86RandR12Init12(ScreenPtr pScreen)
>>       rp->rrProviderSetProperty = xf86RandR14ProviderSetProperty;
>>       rp->rrProviderGetProperty = xf86RandR14ProviderGetProperty;
>>       rp->rrCrtcSetScanoutPixmap = xf86CrtcSetScanoutPixmap;
>> +    rp->rrProviderDestroy = xf86RandR14ProviderDestroy;
>>   
>>       pScrn->PointerMoved = xf86RandR12PointerMoved;
>>       pScrn->ChangeGamma = xf86RandR12ChangeGamma;
>> diff --git a/randr/randrstr.h b/randr/randrstr.h
>> index a16302f..cc3ab1d 100644
>> --- a/randr/randrstr.h
>> +++ b/randr/randrstr.h
>> @@ -232,6 +232,9 @@ typedef Bool (*RRProviderSetOffloadSinkProcPtr)(ScreenPtr pScreen,
>>                                            RRProviderPtr offload_sink);
>>   
>>   
>> +typedef void (*RRProviderDestroyProcPtr)(ScreenPtr pScreen,
>> +                                         RRProviderPtr provider);
>> +
>>   /* These are for 1.0 compatibility */
>>   
>>   typedef struct _rrRefresh {
>> @@ -330,6 +333,9 @@ typedef struct _rrScrPriv {
>>       Bool discontiguous;
>>   
>>       RRProviderPtr provider;
>> +
>> +    RRProviderDestroyProcPtr rrProviderDestroy;
>> +
>>   } rrScrPrivRec, *rrScrPrivPtr;
>>   
>>   extern _X_EXPORT DevPrivateKeyRec rrPrivKeyRec;
>> diff --git a/randr/rrprovider.c b/randr/rrprovider.c
>> index c4ed515..b321e62 100644
>> --- a/randr/rrprovider.c
>> +++ b/randr/rrprovider.c
>> @@ -389,6 +389,8 @@ RRProviderDestroyResource (pointer value, XID pid)
>>       {
>>           rrScrPriv(pScreen);
>>   
>> +        if (pScrPriv->rrProviderDestroy)
>> +            (*pScrPriv->rrProviderDestroy)(pScreen, provider);
>>           pScrPriv->provider = NULL;
>>       }
>>       free(provider);
>> -- 
>> 1.8.1
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>