Hi Samuel, Sorry about this I thought I already replied :-\
On 29 October 2015 at 22:22, Samuel Pitoiset <samuel.pitoi...@gmail.com> 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 <samuel.pitoi...@gmail.com> >>> wrote: >>>> >>>> On 27/10/2015 11:37, Emil Velikov wrote: >>>>> >>>>> On 22 October 2015 at 00:16, Julien Isorce <julien.iso...@gmail.com> >>>>> 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 <j.iso...@samsung.com> >>>>>> --- >>>>>> 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
