[PATCH 2/2] randr: cleanup provider properly

[Peter Hutterer]

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>

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