Re: [Mesa-dev] [PATCH] nouveau: fix double free when screen_create fails
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(&dev); 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
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(&dev); 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
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(&dev); 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
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(&dev); >> 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
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(&dev); 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
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(&dev); 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
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(&dev); >>> 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
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(&dev); 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
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(&dev); >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
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(&dev); >> 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(&dev); >> 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(&dev); >> return NULL; >> + } >> pscreen = &screen->base.base; >>ret = nouveau_screen_init(&screen->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
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(&dev); 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(&dev); 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(&dev); return NULL; + } pscreen = &screen->base.base; ret = nouveau_screen_init(&screen->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
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(&dev); 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(&dev); 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(&dev); return NULL; + } pscreen = &screen->base.base; ret = nouveau_screen_init(&screen->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