Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-12-28 Thread samuel.pitoiset 



On 12/11/2015 21:21, Samuel Pitoiset wrote:



On 11/12/2015 08:51 PM, Samuel Pitoiset wrote:

Hi Emil,

On 11/10/2015 04:35 PM, Emil Velikov wrote:

Hi Samuel,

Sorry about this I thought I already replied :-\

On 29 October 2015 at 22:22, Samuel Pitoiset
 wrote:

On 10/27/2015 02:01 PM, samuel.pitoiset wrote:

On 27/10/2015 12:52, Emil Velikov wrote:


On 27 October 2015 at 10:50, samuel.pitoiset

wrote:


On 27/10/2015 11:37, Emil Velikov wrote:


On 22 October 2015 at 00:16, Julien Isorce

wrote:


The real fix is in nouveau_drm_winsys.c by setting dev to 0.
Which means dev's ownership has been passed to previous call.
Other changes are there to be consistent with what the
screen_create functions already do on errors.

Encountered this crash because nvc0_screen_create sometimes fails
with:
nvc0_screen_create:717 - Error allocating PGRAPH context for
M2MF: -16
Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354

Signed-off-by: Julien Isorce 
---
src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 0330164..9b8ddac 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device
*dev)
   unsigned oclass = 0;
   int ret, i;

-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
  return NULL;
+   }


Imho having these in screen_create() seems like the wrong 'layer'.
Shouldn't one call nouveau_device_dev() from within
nouveau_drm_screen_unref
and explicitly call the latter if the calloc() (here and in
nv50/nvc0)
fails ?



We can't do that because nouveau_drm_screen_unref() needs a valid
nouveau_screen
object and in this case it is NULL.


Ouch I was under the impression that we've brought back the
concept of
winsys in nouveau with the hash_table patches. Seems like we haven't
:(

If we are to do so (split things just like the radeon/amdgpu winsys)
then we can kill two birds with one stone. The missing
device_del() on
calloc failure as well as other error paths in nvxx_screen_create().



Okay, I'll have a look at how radeon/amdgpu split those things.



Well, this doesn't seem to be "trivial" to do it properly actually.
This is on my todolist (but not with a top priority) so, if someone
else want to send a patch for this stuff, feel free to do it. :)


On the contrary - it's pretty trivial 99% of the work is either code
movement or sed job.
On the other hand, it's might not turn out to be stable material
(rather large diff). So if please a comment or two (something
resembling my suggestion) and get feel free to push it.

Roughly how many things do you have in your mesa todo list prior to
nouveau_winsys ?


Lot of things, mostly related to performance counters! ;)
Fixing a segfault when something else has failed doesn't sound like to
be a top priority for me. But... I agree this should be fixed, I'll have
a look this month.


I meant, this segfault only happens when nvXX_screen_create() fails,
it's pretty rare, and it's not a critical issue. :)


This issue has probably been fixed along with 
323d4da372298900ce02293a682ba563ac29f4cb (nouveau: fix screen creation 
failure paths).


If someone get a chance to test it, please report if it works.
Thanks.







Cheers,
Emil


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-11-12 Thread Samuel Pitoiset 

Hi Emil,

On 11/10/2015 04:35 PM, Emil Velikov wrote:

Hi Samuel,

Sorry about this I thought I already replied :-\

On 29 October 2015 at 22:22, Samuel Pitoiset  wrote:

On 10/27/2015 02:01 PM, samuel.pitoiset wrote:

On 27/10/2015 12:52, Emil Velikov wrote:


On 27 October 2015 at 10:50, samuel.pitoiset 
wrote:


On 27/10/2015 11:37, Emil Velikov wrote:


On 22 October 2015 at 00:16, Julien Isorce 
wrote:


The real fix is in nouveau_drm_winsys.c by setting dev to 0.
Which means dev's ownership has been passed to previous call.
Other changes are there to be consistent with what the
screen_create functions already do on errors.

Encountered this crash because nvc0_screen_create sometimes fails
with:
nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354

Signed-off-by: Julien Isorce 
---
src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 0330164..9b8ddac 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
   unsigned oclass = 0;
   int ret, i;

-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
  return NULL;
+   }


Imho having these in screen_create() seems like the wrong 'layer'.
Shouldn't one call nouveau_device_dev() from within
nouveau_drm_screen_unref
and explicitly call the latter if the calloc() (here and in
nv50/nvc0)
fails ?



We can't do that because nouveau_drm_screen_unref() needs a valid
nouveau_screen
object and in this case it is NULL.


Ouch I was under the impression that we've brought back the concept of
winsys in nouveau with the hash_table patches. Seems like we haven't
:(

If we are to do so (split things just like the radeon/amdgpu winsys)
then we can kill two birds with one stone. The missing device_del() on
calloc failure as well as other error paths in nvxx_screen_create().



Okay, I'll have a look at how radeon/amdgpu split those things.



Well, this doesn't seem to be "trivial" to do it properly actually.
This is on my todolist (but not with a top priority) so, if someone
else want to send a patch for this stuff, feel free to do it. :)


On the contrary - it's pretty trivial 99% of the work is either code
movement or sed job.
On the other hand, it's might not turn out to be stable material
(rather large diff). So if please a comment or two (something
resembling my suggestion) and get feel free to push it.

Roughly how many things do you have in your mesa todo list prior to
nouveau_winsys ?


Lot of things, mostly related to performance counters! ;)
Fixing a segfault when something else has failed doesn't sound like to 
be a top priority for me. But... I agree this should be fixed, I'll have 
a look this month.




Cheers,
Emil


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-11-12 Thread Samuel Pitoiset 



On 11/12/2015 08:51 PM, Samuel Pitoiset wrote:

Hi Emil,

On 11/10/2015 04:35 PM, Emil Velikov wrote:

Hi Samuel,

Sorry about this I thought I already replied :-\

On 29 October 2015 at 22:22, Samuel Pitoiset
 wrote:

On 10/27/2015 02:01 PM, samuel.pitoiset wrote:

On 27/10/2015 12:52, Emil Velikov wrote:


On 27 October 2015 at 10:50, samuel.pitoiset

wrote:


On 27/10/2015 11:37, Emil Velikov wrote:


On 22 October 2015 at 00:16, Julien Isorce 
wrote:


The real fix is in nouveau_drm_winsys.c by setting dev to 0.
Which means dev's ownership has been passed to previous call.
Other changes are there to be consistent with what the
screen_create functions already do on errors.

Encountered this crash because nvc0_screen_create sometimes fails
with:
nvc0_screen_create:717 - Error allocating PGRAPH context for
M2MF: -16
Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354

Signed-off-by: Julien Isorce 
---
src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 0330164..9b8ddac 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
   unsigned oclass = 0;
   int ret, i;

-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
  return NULL;
+   }


Imho having these in screen_create() seems like the wrong 'layer'.
Shouldn't one call nouveau_device_dev() from within
nouveau_drm_screen_unref
and explicitly call the latter if the calloc() (here and in
nv50/nvc0)
fails ?



We can't do that because nouveau_drm_screen_unref() needs a valid
nouveau_screen
object and in this case it is NULL.


Ouch I was under the impression that we've brought back the concept of
winsys in nouveau with the hash_table patches. Seems like we haven't
:(

If we are to do so (split things just like the radeon/amdgpu winsys)
then we can kill two birds with one stone. The missing device_del() on
calloc failure as well as other error paths in nvxx_screen_create().



Okay, I'll have a look at how radeon/amdgpu split those things.



Well, this doesn't seem to be "trivial" to do it properly actually.
This is on my todolist (but not with a top priority) so, if someone
else want to send a patch for this stuff, feel free to do it. :)


On the contrary - it's pretty trivial 99% of the work is either code
movement or sed job.
On the other hand, it's might not turn out to be stable material
(rather large diff). So if please a comment or two (something
resembling my suggestion) and get feel free to push it.

Roughly how many things do you have in your mesa todo list prior to
nouveau_winsys ?


Lot of things, mostly related to performance counters! ;)
Fixing a segfault when something else has failed doesn't sound like to
be a top priority for me. But... I agree this should be fixed, I'll have
a look this month.


I meant, this segfault only happens when nvXX_screen_create() fails, 
it's pretty rare, and it's not a critical issue. :)






Cheers,
Emil


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-11-10 Thread Emil Velikov 
Hi Samuel,

Sorry about this I thought I already replied :-\

On 29 October 2015 at 22:22, Samuel Pitoiset  wrote:
> On 10/27/2015 02:01 PM, samuel.pitoiset wrote:
>> On 27/10/2015 12:52, Emil Velikov wrote:
>>>
>>> On 27 October 2015 at 10:50, samuel.pitoiset 
>>> wrote:

 On 27/10/2015 11:37, Emil Velikov wrote:
>
> On 22 October 2015 at 00:16, Julien Isorce 
> wrote:
>>
>> The real fix is in nouveau_drm_winsys.c by setting dev to 0.
>> Which means dev's ownership has been passed to previous call.
>> Other changes are there to be consistent with what the
>> screen_create functions already do on errors.
>>
>> Encountered this crash because nvc0_screen_create sometimes fails
>> with:
>> nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
>> Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354
>>
>> Signed-off-by: Julien Isorce 
>> ---
>>src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
>>src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
>>src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
>>3 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> index 0330164..9b8ddac 100644
>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> @@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
>>   unsigned oclass = 0;
>>   int ret, i;
>>
>> -   if (!screen)
>> +   if (!screen) {
>> +  nouveau_device_del();
>>  return NULL;
>> +   }
>>
> Imho having these in screen_create() seems like the wrong 'layer'.
> Shouldn't one call nouveau_device_dev() from within
> nouveau_drm_screen_unref
>and explicitly call the latter if the calloc() (here and in
> nv50/nvc0)
> fails ?


 We can't do that because nouveau_drm_screen_unref() needs a valid
 nouveau_screen
 object and in this case it is NULL.

>>> Ouch I was under the impression that we've brought back the concept of
>>> winsys in nouveau with the hash_table patches. Seems like we haven't
>>> :(
>>>
>>> If we are to do so (split things just like the radeon/amdgpu winsys)
>>> then we can kill two birds with one stone. The missing device_del() on
>>> calloc failure as well as other error paths in nvxx_screen_create().
>>
>>
>> Okay, I'll have a look at how radeon/amdgpu split those things.
>
>
> Well, this doesn't seem to be "trivial" to do it properly actually.
> This is on my todolist (but not with a top priority) so, if someone
> else want to send a patch for this stuff, feel free to do it. :)
>
On the contrary - it's pretty trivial 99% of the work is either code
movement or sed job.
On the other hand, it's might not turn out to be stable material
(rather large diff). So if please a comment or two (something
resembling my suggestion) and get feel free to push it.

Roughly how many things do you have in your mesa todo list prior to
nouveau_winsys ?

Cheers,
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-10-29 Thread Samuel Pitoiset 



On 10/27/2015 02:01 PM, samuel.pitoiset wrote:



On 27/10/2015 12:52, Emil Velikov wrote:
On 27 October 2015 at 10:50, samuel.pitoiset 
 wrote:

On 27/10/2015 11:37, Emil Velikov wrote:

On 22 October 2015 at 00:16, Julien Isorce 
wrote:

The real fix is in nouveau_drm_winsys.c by setting dev to 0.
Which means dev's ownership has been passed to previous call.
Other changes are there to be consistent with what the
screen_create functions already do on errors.

Encountered this crash because nvc0_screen_create sometimes fails 
with:
nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: 
-16

Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354

Signed-off-by: Julien Isorce 
---
   src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
   src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
   src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
   3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 0330164..9b8ddac 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
  unsigned oclass = 0;
  int ret, i;

-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
 return NULL;
+   }


Imho having these in screen_create() seems like the wrong 'layer'.
Shouldn't one call nouveau_device_dev() from within
nouveau_drm_screen_unref
   and explicitly call the latter if the calloc() (here and in 
nv50/nvc0)

fails ?


We can't do that because nouveau_drm_screen_unref() needs a valid
nouveau_screen
object and in this case it is NULL.


Ouch I was under the impression that we've brought back the concept of
winsys in nouveau with the hash_table patches. Seems like we haven't
:(

If we are to do so (split things just like the radeon/amdgpu winsys)
then we can kill two birds with one stone. The missing device_del() on
calloc failure as well as other error paths in nvxx_screen_create().


Okay, I'll have a look at how radeon/amdgpu split those things.


Well, this doesn't seem to be "trivial" to do it properly actually.
This is on my todolist (but not with a top priority) so, if someone
else want to send a patch for this stuff, feel free to do it. :)





I agree that it's not really an elegant fix but we don't really have 
the

choice actually.
In my opinion, this is not that bad.


I never said it's "bad" just the wrong place for the fix. Or in other
words - if we're to fix things might as well do it properly :-)


Sure, I agree. :)



-Emil




___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-10-27 Thread Julien Isorce 
On 25 October 2015 at 21:56, Samuel Pitoiset 
wrote:

>
>
> On 10/22/2015 01:16 AM, Julien Isorce wrote:
>
>> The real fix is in nouveau_drm_winsys.c by setting dev to 0.
>> Which means dev's ownership has been passed to previous call.
>> Other changes are there to be consistent with what the
>> screen_create functions already do on errors.
>>
>
> This actually happens because nouveau_device_del() is (sometimes) called
> twice
> when nvXX_screen_create() fails.
>
> I don't really like this solution but I don't have a better one for now,
> I'll think about
> that in the next few days. :)
>

Yeah and it is certainly hard to maintain. Ideally it should take ownership
of the device only on success. I'll send another patch to compare with the
other way around.


>
> Note that you forgot to call nouveau_device_del() in nvc0_screen_create().


Ah right, I missed it on the first return, thx.


>
>
>
>> Encountered this crash because nvc0_screen_create sometimes fails with:
>> nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
>> Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354
>>
>> Signed-off-by: Julien Isorce 
>> ---
>>   src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
>>   src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
>>   src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
>>   3 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> index 0330164..9b8ddac 100644
>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> @@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
>>  unsigned oclass = 0;
>>  int ret, i;
>>   -   if (!screen)
>> +   if (!screen) {
>> +  nouveau_device_del();
>> return NULL;
>> +   }
>>switch (dev->chipset & 0xf0) {
>>  case 0x30:
>> @@ -456,6 +458,7 @@ nv30_screen_create(struct nouveau_device *dev)
>>if (!oclass) {
>> NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
>> +  nouveau_device_del();
>> FREE(screen);
>> return NULL;
>>  }
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> index ec51d00..e9604d5 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> @@ -711,8 +711,10 @@ nv50_screen_create(struct nouveau_device *dev)
>>  int ret;
>>screen = CALLOC_STRUCT(nv50_screen);
>> -   if (!screen)
>> +   if (!screen) {
>> +  nouveau_device_del();
>> return NULL;
>> +   }
>>  pscreen = >base.base;
>>ret = nouveau_screen_init(>base, dev);
>> diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> index c6603e3..bd1d761 100644
>> --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> @@ -117,6 +117,8 @@ nouveau_drm_screen_create(int fd)
>> }
>> screen = (struct nouveau_screen*)init(dev);
>> +   /* Previous init func took ownership of dev */
>> +   dev = 0;
>> if (!screen)
>> goto err;
>>
>>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-10-27 Thread Emil Velikov 
On 22 October 2015 at 00:16, Julien Isorce  wrote:
> The real fix is in nouveau_drm_winsys.c by setting dev to 0.
> Which means dev's ownership has been passed to previous call.
> Other changes are there to be consistent with what the
> screen_create functions already do on errors.
>
> Encountered this crash because nvc0_screen_create sometimes fails with:
> nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
> Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354
>
> Signed-off-by: Julien Isorce 
> ---
>  src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
>  src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
>  src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
>  3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> index 0330164..9b8ddac 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> @@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
> unsigned oclass = 0;
> int ret, i;
>
> -   if (!screen)
> +   if (!screen) {
> +  nouveau_device_del();
>return NULL;
> +   }
>
Imho having these in screen_create() seems like the wrong 'layer'.
Shouldn't one call nouveau_device_dev() from within
nouveau_drm_screen_unref
 and explicitly call the latter if the calloc() (here and in nv50/nvc0) fails ?

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-10-27 Thread samuel.pitoiset 



On 27/10/2015 11:37, Emil Velikov wrote:

On 22 October 2015 at 00:16, Julien Isorce  wrote:

The real fix is in nouveau_drm_winsys.c by setting dev to 0.
Which means dev's ownership has been passed to previous call.
Other changes are there to be consistent with what the
screen_create functions already do on errors.

Encountered this crash because nvc0_screen_create sometimes fails with:
nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354

Signed-off-by: Julien Isorce 
---
  src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
  src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
  src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
  3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 0330164..9b8ddac 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
 unsigned oclass = 0;
 int ret, i;

-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
return NULL;
+   }


Imho having these in screen_create() seems like the wrong 'layer'.
Shouldn't one call nouveau_device_dev() from within
nouveau_drm_screen_unref
  and explicitly call the latter if the calloc() (here and in nv50/nvc0) fails ?


We can't do that because nouveau_drm_screen_unref() needs a valid 
nouveau_screen

object and in this case it is NULL.

I agree that it's not really an elegant fix but we don't really have the 
choice actually.

In my opinion, this is not that bad.



-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-10-27 Thread samuel.pitoiset 



On 27/10/2015 12:52, Emil Velikov wrote:

On 27 October 2015 at 10:50, samuel.pitoiset  wrote:

On 27/10/2015 11:37, Emil Velikov wrote:

On 22 October 2015 at 00:16, Julien Isorce 
wrote:

The real fix is in nouveau_drm_winsys.c by setting dev to 0.
Which means dev's ownership has been passed to previous call.
Other changes are there to be consistent with what the
screen_create functions already do on errors.

Encountered this crash because nvc0_screen_create sometimes fails with:
nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354

Signed-off-by: Julien Isorce 
---
   src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
   src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
   src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
   3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 0330164..9b8ddac 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
  unsigned oclass = 0;
  int ret, i;

-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
 return NULL;
+   }


Imho having these in screen_create() seems like the wrong 'layer'.
Shouldn't one call nouveau_device_dev() from within
nouveau_drm_screen_unref
   and explicitly call the latter if the calloc() (here and in nv50/nvc0)
fails ?


We can't do that because nouveau_drm_screen_unref() needs a valid
nouveau_screen
object and in this case it is NULL.


Ouch I was under the impression that we've brought back the concept of
winsys in nouveau with the hash_table patches. Seems like we haven't
:(

If we are to do so (split things just like the radeon/amdgpu winsys)
then we can kill two birds with one stone. The missing device_del() on
calloc failure as well as other error paths in nvxx_screen_create().


Okay, I'll have a look at how radeon/amdgpu split those things.




I agree that it's not really an elegant fix but we don't really have the
choice actually.
In my opinion, this is not that bad.


I never said it's "bad" just the wrong place for the fix. Or in other
words - if we're to fix things might as well do it properly :-)


Sure, I agree. :)



-Emil


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-10-27 Thread Emil Velikov 
On 27 October 2015 at 10:50, samuel.pitoiset  wrote:
> On 27/10/2015 11:37, Emil Velikov wrote:
>>
>> On 22 October 2015 at 00:16, Julien Isorce 
>> wrote:
>>>
>>> The real fix is in nouveau_drm_winsys.c by setting dev to 0.
>>> Which means dev's ownership has been passed to previous call.
>>> Other changes are there to be consistent with what the
>>> screen_create functions already do on errors.
>>>
>>> Encountered this crash because nvc0_screen_create sometimes fails with:
>>> nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
>>> Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354
>>>
>>> Signed-off-by: Julien Isorce 
>>> ---
>>>   src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
>>>   src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
>>>   src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
>>>   3 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> index 0330164..9b8ddac 100644
>>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> @@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
>>>  unsigned oclass = 0;
>>>  int ret, i;
>>>
>>> -   if (!screen)
>>> +   if (!screen) {
>>> +  nouveau_device_del();
>>> return NULL;
>>> +   }
>>>
>> Imho having these in screen_create() seems like the wrong 'layer'.
>> Shouldn't one call nouveau_device_dev() from within
>> nouveau_drm_screen_unref
>>   and explicitly call the latter if the calloc() (here and in nv50/nvc0)
>> fails ?
>
>
> We can't do that because nouveau_drm_screen_unref() needs a valid
> nouveau_screen
> object and in this case it is NULL.
>
Ouch I was under the impression that we've brought back the concept of
winsys in nouveau with the hash_table patches. Seems like we haven't
:(

If we are to do so (split things just like the radeon/amdgpu winsys)
then we can kill two birds with one stone. The missing device_del() on
calloc failure as well as other error paths in nvxx_screen_create().

> I agree that it's not really an elegant fix but we don't really have the
> choice actually.
> In my opinion, this is not that bad.
>
I never said it's "bad" just the wrong place for the fix. Or in other
words - if we're to fix things might as well do it properly :-)

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-10-25 Thread Samuel Pitoiset 



On 10/22/2015 01:16 AM, Julien Isorce wrote:

The real fix is in nouveau_drm_winsys.c by setting dev to 0.
Which means dev's ownership has been passed to previous call.
Other changes are there to be consistent with what the
screen_create functions already do on errors.


This actually happens because nouveau_device_del() is (sometimes) called 
twice

when nvXX_screen_create() fails.

I don't really like this solution but I don't have a better one for now, 
I'll think about

that in the next few days. :)

Note that you forgot to call nouveau_device_del() in nvc0_screen_create().



Encountered this crash because nvc0_screen_create sometimes fails with:
nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354

Signed-off-by: Julien Isorce 
---
  src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
  src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
  src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
  3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 0330164..9b8ddac 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
 unsigned oclass = 0;
 int ret, i;
  
-   if (!screen)

+   if (!screen) {
+  nouveau_device_del();
return NULL;
+   }
  
 switch (dev->chipset & 0xf0) {

 case 0x30:
@@ -456,6 +458,7 @@ nv30_screen_create(struct nouveau_device *dev)
  
 if (!oclass) {

NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
+  nouveau_device_del();
FREE(screen);
return NULL;
 }
diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c 
b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
index ec51d00..e9604d5 100644
--- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
+++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
@@ -711,8 +711,10 @@ nv50_screen_create(struct nouveau_device *dev)
 int ret;
  
 screen = CALLOC_STRUCT(nv50_screen);

-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
return NULL;
+   }
 pscreen = >base.base;
  
 ret = nouveau_screen_init(>base, dev);

diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c 
b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
index c6603e3..bd1d761 100644
--- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
+++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
@@ -117,6 +117,8 @@ nouveau_drm_screen_create(int fd)
}
  
  	screen = (struct nouveau_screen*)init(dev);

+   /* Previous init func took ownership of dev */
+   dev = 0;
if (!screen)
goto err;
  


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails

2015-10-21 Thread Julien Isorce 
The real fix is in nouveau_drm_winsys.c by setting dev to 0.
Which means dev's ownership has been passed to previous call.
Other changes are there to be consistent with what the
screen_create functions already do on errors.

Encountered this crash because nvc0_screen_create sometimes fails with:
nvc0_screen_create:717 - Error allocating PGRAPH context for M2MF: -16
Also see: https://bugs.freedesktop.org/show_bug.cgi?id=70354

Signed-off-by: Julien Isorce 
---
 src/gallium/drivers/nouveau/nv30/nv30_screen.c  | 5 -
 src/gallium/drivers/nouveau/nv50/nv50_screen.c  | 4 +++-
 src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 2 ++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 0330164..9b8ddac 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -425,8 +425,10 @@ nv30_screen_create(struct nouveau_device *dev)
unsigned oclass = 0;
int ret, i;
 
-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
   return NULL;
+   }
 
switch (dev->chipset & 0xf0) {
case 0x30:
@@ -456,6 +458,7 @@ nv30_screen_create(struct nouveau_device *dev)
 
if (!oclass) {
   NOUVEAU_ERR("unknown 3d class for 0x%02x\n", dev->chipset);
+  nouveau_device_del();
   FREE(screen);
   return NULL;
}
diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c 
b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
index ec51d00..e9604d5 100644
--- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
+++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
@@ -711,8 +711,10 @@ nv50_screen_create(struct nouveau_device *dev)
int ret;
 
screen = CALLOC_STRUCT(nv50_screen);
-   if (!screen)
+   if (!screen) {
+  nouveau_device_del();
   return NULL;
+   }
pscreen = >base.base;
 
ret = nouveau_screen_init(>base, dev);
diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c 
b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
index c6603e3..bd1d761 100644
--- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
+++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
@@ -117,6 +117,8 @@ nouveau_drm_screen_create(int fd)
}
 
screen = (struct nouveau_screen*)init(dev);
+   /* Previous init func took ownership of dev */
+   dev = 0;
if (!screen)
goto err;
 
-- 
1.9.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev