Re: [Nouveau] [mesa v2 5/9] nouveau: fix screen creation failure paths

[Ilia Mirkin]

This all seems very roundabout... Can't we do this in a somewhat
consistent way with the device being cleaned up in one place or
another but not both?

On Thu, Nov 26, 2015 at 8:04 PM, Ben Skeggs  wrote:
> From: Ben Skeggs 
>
> The winsys layer would attempt to cleanup the nouveau_device if screen
> init failed, however, in most paths the pipe driver would have already
> destroyed it, resulting in accesses to freed memory etc.
>
> This commit fixes the problem by allowing the winsys to detect whether
> the pipe driver's destroy function needs to be called or not.
>
> Signed-off-by: Ben Skeggs 
> ---
>  src/gallium/drivers/nouveau/nouveau_screen.c|  8 ++--
>  src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 19 ++-
>  src/gallium/drivers/nouveau/nv50/nv50_screen.c  |  6 +++---
>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c  |  9 -
>  src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 16 ++--
>  5 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c 
> b/src/gallium/drivers/nouveau/nouveau_screen.c
> index a012579..3cdcc20 100644
> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
> @@ -147,6 +147,12 @@ nouveau_screen_init(struct nouveau_screen *screen, 
> struct nouveau_device *dev)
> if (nv_dbg)
>nouveau_mesa_debug = atoi(nv_dbg);
>
> +   /* These must be set before any failure is possible, as the cleanup
> +* paths assume they're responsible for deleting them.
> +*/
> +   screen->drm = nouveau_drm(>object);
> +   screen->device = dev;
> +
> /*
>  * this is initialized to 1 in nouveau_drm_screen_create after screen
>  * is fully constructed and added to the global screen list.
> @@ -175,8 +181,6 @@ nouveau_screen_init(struct nouveau_screen *screen, struct 
> nouveau_device *dev)
>  data, size, >channel);
> if (ret)
>return ret;
> -   screen->drm = nouveau_drm(>object);
> -   screen->device = dev;
>
> ret = nouveau_client_new(screen->device, >client);
> if (ret)
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> index ea29811..854f70c 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> @@ -413,23 +413,20 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
>  #define FAIL_SCREEN_INIT(str, err)\
> do {   \
>NOUVEAU_ERR(str, err);  \
> -  nv30_screen_destroy(pscreen);   \
> -  return NULL;\
> +  screen->base.base.context_create = NULL;\
> +  return >base;   \
> } while(0)
>
>  struct nouveau_screen *
>  nv30_screen_create(struct nouveau_device *dev)
>  {
> -   struct nv30_screen *screen = CALLOC_STRUCT(nv30_screen);
> +   struct nv30_screen *screen;
> struct pipe_screen *pscreen;
> struct nouveau_pushbuf *push;
> struct nv04_fifo *fifo;
> unsigned oclass = 0;
> int ret, i;
>
> -   if (!screen)
> -  return NULL;
> -
> switch (dev->chipset & 0xf0) {
> case 0x30:
>if (RANKINE_0397_CHIPSET & (1 << (dev->chipset & 0x0f)))
> @@ -458,10 +455,16 @@ nv30_screen_create(struct nouveau_device *dev)
>
> if (!oclass) {
>NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
> -  FREE(screen);
>return NULL;
> }
>
> +   screen = CALLOC_STRUCT(nv30_screen);
> +   if (!screen)
> +  return NULL;
> +
> +   pscreen = >base.base;
> +   pscreen->destroy = nv30_screen_destroy;
> +
> /*
>  * Some modern apps try to use msaa without keeping in mind the
>  * restrictions on videomem of older cards. Resulting in dmesg saying:
> @@ -479,8 +482,6 @@ nv30_screen_create(struct nouveau_device *dev)
> if (screen->max_sample_count > 4)
>screen->max_sample_count = 4;
>
> -   pscreen = >base.base;
> -   pscreen->destroy = nv30_screen_destroy;
> pscreen->get_param = nv30_screen_get_param;
> pscreen->get_paramf = nv30_screen_get_paramf;
> pscreen->get_shader_param = nv30_screen_get_shader_param;
> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c 
> b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> index 82b9e93..46c812b 100644
> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> @@ -762,6 +762,7 @@ nv50_screen_create(struct nouveau_device *dev)
> if (!screen)
>return NULL;
> pscreen = >base.base;
> +   pscreen->destroy = nv50_screen_destroy;
>
> ret = nouveau_screen_init(>base, dev);
> if (ret) {
> @@ -782,7 +783,6 @@ nv50_screen_create(struct nouveau_device *dev)
>
> chan = screen->base.channel;
>

Re: [Nouveau] [mesa v2 5/9] nouveau: fix screen creation failure paths

[Ben Skeggs]

On 12/07/2015 01:40 PM, Ilia Mirkin wrote:
> This all seems very roundabout... Can't we do this in a somewhat
> consistent way with the device being cleaned up in one place or
> another but not both?
That would be lovely, but not possible.  It has to be cleaned up by the
pipe screen destroy() function, as that's the normal exit path.  If the
pipe driver creation path fails before it's got a struct, then the
winsys will have to clean up after itself instead.

> 
> On Thu, Nov 26, 2015 at 8:04 PM, Ben Skeggs  wrote:
>> From: Ben Skeggs 
>>
>> The winsys layer would attempt to cleanup the nouveau_device if screen
>> init failed, however, in most paths the pipe driver would have already
>> destroyed it, resulting in accesses to freed memory etc.
>>
>> This commit fixes the problem by allowing the winsys to detect whether
>> the pipe driver's destroy function needs to be called or not.
>>
>> Signed-off-by: Ben Skeggs 
>> ---
>>  src/gallium/drivers/nouveau/nouveau_screen.c|  8 ++--
>>  src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 19 ++-
>>  src/gallium/drivers/nouveau/nv50/nv50_screen.c  |  6 +++---
>>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c  |  9 -
>>  src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 16 ++--
>>  5 files changed, 33 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c 
>> b/src/gallium/drivers/nouveau/nouveau_screen.c
>> index a012579..3cdcc20 100644
>> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
>> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
>> @@ -147,6 +147,12 @@ nouveau_screen_init(struct nouveau_screen *screen, 
>> struct nouveau_device *dev)
>> if (nv_dbg)
>>nouveau_mesa_debug = atoi(nv_dbg);
>>
>> +   /* These must be set before any failure is possible, as the cleanup
>> +* paths assume they're responsible for deleting them.
>> +*/
>> +   screen->drm = nouveau_drm(>object);
>> +   screen->device = dev;
>> +
>> /*
>>  * this is initialized to 1 in nouveau_drm_screen_create after screen
>>  * is fully constructed and added to the global screen list.
>> @@ -175,8 +181,6 @@ nouveau_screen_init(struct nouveau_screen *screen, 
>> struct nouveau_device *dev)
>>  data, size, >channel);
>> if (ret)
>>return ret;
>> -   screen->drm = nouveau_drm(>object);
>> -   screen->device = dev;
>>
>> ret = nouveau_client_new(screen->device, >client);
>> if (ret)
>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> index ea29811..854f70c 100644
>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> @@ -413,23 +413,20 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
>>  #define FAIL_SCREEN_INIT(str, err)\
>> do {   \
>>NOUVEAU_ERR(str, err);  \
>> -  nv30_screen_destroy(pscreen);   \
>> -  return NULL;\
>> +  screen->base.base.context_create = NULL;\
>> +  return >base;   \
>> } while(0)
>>
>>  struct nouveau_screen *
>>  nv30_screen_create(struct nouveau_device *dev)
>>  {
>> -   struct nv30_screen *screen = CALLOC_STRUCT(nv30_screen);
>> +   struct nv30_screen *screen;
>> struct pipe_screen *pscreen;
>> struct nouveau_pushbuf *push;
>> struct nv04_fifo *fifo;
>> unsigned oclass = 0;
>> int ret, i;
>>
>> -   if (!screen)
>> -  return NULL;
>> -
>> switch (dev->chipset & 0xf0) {
>> case 0x30:
>>if (RANKINE_0397_CHIPSET & (1 << (dev->chipset & 0x0f)))
>> @@ -458,10 +455,16 @@ nv30_screen_create(struct nouveau_device *dev)
>>
>> if (!oclass) {
>>NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
>> -  FREE(screen);
>>return NULL;
>> }
>>
>> +   screen = CALLOC_STRUCT(nv30_screen);
>> +   if (!screen)
>> +  return NULL;
>> +
>> +   pscreen = >base.base;
>> +   pscreen->destroy = nv30_screen_destroy;
>> +
>> /*
>>  * Some modern apps try to use msaa without keeping in mind the
>>  * restrictions on videomem of older cards. Resulting in dmesg saying:
>> @@ -479,8 +482,6 @@ nv30_screen_create(struct nouveau_device *dev)
>> if (screen->max_sample_count > 4)
>>screen->max_sample_count = 4;
>>
>> -   pscreen = >base.base;
>> -   pscreen->destroy = nv30_screen_destroy;
>> pscreen->get_param = nv30_screen_get_param;
>> pscreen->get_paramf = nv30_screen_get_paramf;
>> pscreen->get_shader_param = nv30_screen_get_shader_param;
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c 
>> b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> index 82b9e93..46c812b 100644
>> --- 

Re: [Nouveau] [mesa v2 5/9] nouveau: fix screen creation failure paths

[Ilia Mirkin]

On Sun, Dec 6, 2015 at 10:48 PM, Ben Skeggs  wrote:
> On 12/07/2015 01:40 PM, Ilia Mirkin wrote:
>> This all seems very roundabout... Can't we do this in a somewhat
>> consistent way with the device being cleaned up in one place or
>> another but not both?
> That would be lovely, but not possible.  It has to be cleaned up by the
> pipe screen destroy() function, as that's the normal exit path.  If the
> pipe driver creation path fails before it's got a struct, then the
> winsys will have to clean up after itself instead.

Couldn't you make the pipe_screen_create() function destroy the device
in all failure paths?

>
>>
>> On Thu, Nov 26, 2015 at 8:04 PM, Ben Skeggs  wrote:
>>> From: Ben Skeggs 
>>>
>>> The winsys layer would attempt to cleanup the nouveau_device if screen
>>> init failed, however, in most paths the pipe driver would have already
>>> destroyed it, resulting in accesses to freed memory etc.
>>>
>>> This commit fixes the problem by allowing the winsys to detect whether
>>> the pipe driver's destroy function needs to be called or not.
>>>
>>> Signed-off-by: Ben Skeggs 
>>> ---
>>>  src/gallium/drivers/nouveau/nouveau_screen.c|  8 ++--
>>>  src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 19 
>>> ++-
>>>  src/gallium/drivers/nouveau/nv50/nv50_screen.c  |  6 +++---
>>>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c  |  9 -
>>>  src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 16 ++--
>>>  5 files changed, 33 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c 
>>> b/src/gallium/drivers/nouveau/nouveau_screen.c
>>> index a012579..3cdcc20 100644
>>> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
>>> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
>>> @@ -147,6 +147,12 @@ nouveau_screen_init(struct nouveau_screen *screen, 
>>> struct nouveau_device *dev)
>>> if (nv_dbg)
>>>nouveau_mesa_debug = atoi(nv_dbg);
>>>
>>> +   /* These must be set before any failure is possible, as the cleanup
>>> +* paths assume they're responsible for deleting them.
>>> +*/
>>> +   screen->drm = nouveau_drm(>object);
>>> +   screen->device = dev;
>>> +
>>> /*
>>>  * this is initialized to 1 in nouveau_drm_screen_create after screen
>>>  * is fully constructed and added to the global screen list.
>>> @@ -175,8 +181,6 @@ nouveau_screen_init(struct nouveau_screen *screen, 
>>> struct nouveau_device *dev)
>>>  data, size, >channel);
>>> if (ret)
>>>return ret;
>>> -   screen->drm = nouveau_drm(>object);
>>> -   screen->device = dev;
>>>
>>> ret = nouveau_client_new(screen->device, >client);
>>> if (ret)
>>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
>>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> index ea29811..854f70c 100644
>>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> @@ -413,23 +413,20 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
>>>  #define FAIL_SCREEN_INIT(str, err)\
>>> do {   \
>>>NOUVEAU_ERR(str, err);  \
>>> -  nv30_screen_destroy(pscreen);   \
>>> -  return NULL;\
>>> +  screen->base.base.context_create = NULL;\
>>> +  return >base;   \
>>> } while(0)
>>>
>>>  struct nouveau_screen *
>>>  nv30_screen_create(struct nouveau_device *dev)
>>>  {
>>> -   struct nv30_screen *screen = CALLOC_STRUCT(nv30_screen);
>>> +   struct nv30_screen *screen;
>>> struct pipe_screen *pscreen;
>>> struct nouveau_pushbuf *push;
>>> struct nv04_fifo *fifo;
>>> unsigned oclass = 0;
>>> int ret, i;
>>>
>>> -   if (!screen)
>>> -  return NULL;
>>> -
>>> switch (dev->chipset & 0xf0) {
>>> case 0x30:
>>>if (RANKINE_0397_CHIPSET & (1 << (dev->chipset & 0x0f)))
>>> @@ -458,10 +455,16 @@ nv30_screen_create(struct nouveau_device *dev)
>>>
>>> if (!oclass) {
>>>NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
>>> -  FREE(screen);
>>>return NULL;
>>> }
>>>
>>> +   screen = CALLOC_STRUCT(nv30_screen);
>>> +   if (!screen)
>>> +  return NULL;
>>> +
>>> +   pscreen = >base.base;
>>> +   pscreen->destroy = nv30_screen_destroy;
>>> +
>>> /*
>>>  * Some modern apps try to use msaa without keeping in mind the
>>>  * restrictions on videomem of older cards. Resulting in dmesg saying:
>>> @@ -479,8 +482,6 @@ nv30_screen_create(struct nouveau_device *dev)
>>> if (screen->max_sample_count > 4)
>>>screen->max_sample_count = 4;
>>>
>>> -   pscreen = >base.base;
>>> -   pscreen->destroy = nv30_screen_destroy;
>>> pscreen->get_param = nv30_screen_get_param;
>>> 

Re: [Nouveau] [mesa v2 5/9] nouveau: fix screen creation failure paths

[Ben Skeggs]

On 12/07/2015 01:53 PM, Ilia Mirkin wrote:
> On Sun, Dec 6, 2015 at 10:48 PM, Ben Skeggs  wrote:
>> On 12/07/2015 01:40 PM, Ilia Mirkin wrote:
>>> This all seems very roundabout... Can't we do this in a somewhat
>>> consistent way with the device being cleaned up in one place or
>>> another but not both?
>> That would be lovely, but not possible.  It has to be cleaned up by the
>> pipe screen destroy() function, as that's the normal exit path.  If the
>> pipe driver creation path fails before it's got a struct, then the
>> winsys will have to clean up after itself instead.
> 
> Couldn't you make the pipe_screen_create() function destroy the device
> in all failure paths?
Probably.  I guess that's slightly nicer, I'll re-spin them shortly.

> 
>>
>>>
>>> On Thu, Nov 26, 2015 at 8:04 PM, Ben Skeggs  wrote:
 From: Ben Skeggs 

 The winsys layer would attempt to cleanup the nouveau_device if screen
 init failed, however, in most paths the pipe driver would have already
 destroyed it, resulting in accesses to freed memory etc.

 This commit fixes the problem by allowing the winsys to detect whether
 the pipe driver's destroy function needs to be called or not.

 Signed-off-by: Ben Skeggs 
 ---
  src/gallium/drivers/nouveau/nouveau_screen.c|  8 ++--
  src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 19 
 ++-
  src/gallium/drivers/nouveau/nv50/nv50_screen.c  |  6 +++---
  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c  |  9 -
  src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 16 ++--
  5 files changed, 33 insertions(+), 25 deletions(-)

 diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c 
 b/src/gallium/drivers/nouveau/nouveau_screen.c
 index a012579..3cdcc20 100644
 --- a/src/gallium/drivers/nouveau/nouveau_screen.c
 +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
 @@ -147,6 +147,12 @@ nouveau_screen_init(struct nouveau_screen *screen, 
 struct nouveau_device *dev)
 if (nv_dbg)
nouveau_mesa_debug = atoi(nv_dbg);

 +   /* These must be set before any failure is possible, as the cleanup
 +* paths assume they're responsible for deleting them.
 +*/
 +   screen->drm = nouveau_drm(>object);
 +   screen->device = dev;
 +
 /*
  * this is initialized to 1 in nouveau_drm_screen_create after screen
  * is fully constructed and added to the global screen list.
 @@ -175,8 +181,6 @@ nouveau_screen_init(struct nouveau_screen *screen, 
 struct nouveau_device *dev)
  data, size, >channel);
 if (ret)
return ret;
 -   screen->drm = nouveau_drm(>object);
 -   screen->device = dev;

 ret = nouveau_client_new(screen->device, >client);
 if (ret)
 diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
 b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
 index ea29811..854f70c 100644
 --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
 +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
 @@ -413,23 +413,20 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
  #define FAIL_SCREEN_INIT(str, err)\
 do {   \
NOUVEAU_ERR(str, err);  \
 -  nv30_screen_destroy(pscreen);   \
 -  return NULL;\
 +  screen->base.base.context_create = NULL;\
 +  return >base;   \
 } while(0)

  struct nouveau_screen *
  nv30_screen_create(struct nouveau_device *dev)
  {
 -   struct nv30_screen *screen = CALLOC_STRUCT(nv30_screen);
 +   struct nv30_screen *screen;
 struct pipe_screen *pscreen;
 struct nouveau_pushbuf *push;
 struct nv04_fifo *fifo;
 unsigned oclass = 0;
 int ret, i;

 -   if (!screen)
 -  return NULL;
 -
 switch (dev->chipset & 0xf0) {
 case 0x30:
if (RANKINE_0397_CHIPSET & (1 << (dev->chipset & 0x0f)))
 @@ -458,10 +455,16 @@ nv30_screen_create(struct nouveau_device *dev)

 if (!oclass) {
NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
 -  FREE(screen);
return NULL;
 }

 +   screen = CALLOC_STRUCT(nv30_screen);
 +   if (!screen)
 +  return NULL;
 +
 +   pscreen = >base.base;
 +   pscreen->destroy = nv30_screen_destroy;
 +
 /*
  * Some modern apps try to use msaa without keeping in mind the
  * restrictions on videomem of older cards. Resulting in dmesg saying:
 @@ -479,8 +482,6 @@ nv30_screen_create(struct nouveau_device *dev)

[Nouveau] [mesa v2 5/9] nouveau: fix screen creation failure paths

[Ben Skeggs]

From: Ben Skeggs 

The winsys layer would attempt to cleanup the nouveau_device if screen
init failed, however, in most paths the pipe driver would have already
destroyed it, resulting in accesses to freed memory etc.

This commit fixes the problem by allowing the winsys to detect whether
the pipe driver's destroy function needs to be called or not.

Signed-off-by: Ben Skeggs 
---
 src/gallium/drivers/nouveau/nouveau_screen.c|  8 ++--
 src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 19 ++-
 src/gallium/drivers/nouveau/nv50/nv50_screen.c  |  6 +++---
 src/gallium/drivers/nouveau/nvc0/nvc0_screen.c  |  9 -
 src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 16 ++--
 5 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c 
b/src/gallium/drivers/nouveau/nouveau_screen.c
index a012579..3cdcc20 100644
--- a/src/gallium/drivers/nouveau/nouveau_screen.c
+++ b/src/gallium/drivers/nouveau/nouveau_screen.c
@@ -147,6 +147,12 @@ nouveau_screen_init(struct nouveau_screen *screen, struct 
nouveau_device *dev)
if (nv_dbg)
   nouveau_mesa_debug = atoi(nv_dbg);
 
+   /* These must be set before any failure is possible, as the cleanup
+* paths assume they're responsible for deleting them.
+*/
+   screen->drm = nouveau_drm(>object);
+   screen->device = dev;
+
/*
 * this is initialized to 1 in nouveau_drm_screen_create after screen
 * is fully constructed and added to the global screen list.
@@ -175,8 +181,6 @@ nouveau_screen_init(struct nouveau_screen *screen, struct 
nouveau_device *dev)
 data, size, >channel);
if (ret)
   return ret;
-   screen->drm = nouveau_drm(>object);
-   screen->device = dev;
 
ret = nouveau_client_new(screen->device, >client);
if (ret)
diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index ea29811..854f70c 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -413,23 +413,20 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
 #define FAIL_SCREEN_INIT(str, err)\
do {   \
   NOUVEAU_ERR(str, err);  \
-  nv30_screen_destroy(pscreen);   \
-  return NULL;\
+  screen->base.base.context_create = NULL;\
+  return >base;   \
} while(0)
 
 struct nouveau_screen *
 nv30_screen_create(struct nouveau_device *dev)
 {
-   struct nv30_screen *screen = CALLOC_STRUCT(nv30_screen);
+   struct nv30_screen *screen;
struct pipe_screen *pscreen;
struct nouveau_pushbuf *push;
struct nv04_fifo *fifo;
unsigned oclass = 0;
int ret, i;
 
-   if (!screen)
-  return NULL;
-
switch (dev->chipset & 0xf0) {
case 0x30:
   if (RANKINE_0397_CHIPSET & (1 << (dev->chipset & 0x0f)))
@@ -458,10 +455,16 @@ nv30_screen_create(struct nouveau_device *dev)
 
if (!oclass) {
   NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
-  FREE(screen);
   return NULL;
}
 
+   screen = CALLOC_STRUCT(nv30_screen);
+   if (!screen)
+  return NULL;
+
+   pscreen = >base.base;
+   pscreen->destroy = nv30_screen_destroy;
+
/*
 * Some modern apps try to use msaa without keeping in mind the
 * restrictions on videomem of older cards. Resulting in dmesg saying:
@@ -479,8 +482,6 @@ nv30_screen_create(struct nouveau_device *dev)
if (screen->max_sample_count > 4)
   screen->max_sample_count = 4;
 
-   pscreen = >base.base;
-   pscreen->destroy = nv30_screen_destroy;
pscreen->get_param = nv30_screen_get_param;
pscreen->get_paramf = nv30_screen_get_paramf;
pscreen->get_shader_param = nv30_screen_get_shader_param;
diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c 
b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
index 82b9e93..46c812b 100644
--- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
+++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
@@ -762,6 +762,7 @@ nv50_screen_create(struct nouveau_device *dev)
if (!screen)
   return NULL;
pscreen = >base.base;
+   pscreen->destroy = nv50_screen_destroy;
 
ret = nouveau_screen_init(>base, dev);
if (ret) {
@@ -782,7 +783,6 @@ nv50_screen_create(struct nouveau_device *dev)
 
chan = screen->base.channel;
 
-   pscreen->destroy = nv50_screen_destroy;
pscreen->context_create = nv50_create;
pscreen->is_format_supported = nv50_screen_is_format_supported;
pscreen->get_param = nv50_screen_get_param;
@@ -964,8 +964,8 @@ nv50_screen_create(struct nouveau_device *dev)
return >base;
 
 fail:
-   nv50_screen_destroy(pscreen);
-   return NULL;
+   screen->base.base.context_create = NULL;
+   return >base;
 }
 
 int
diff --git