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 <skeg...@gmail.com> wrote: > From: Ben Skeggs <bske...@redhat.com> > > 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 <bske...@redhat.com> > --- > 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(&dev->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, &screen->channel); > if (ret) > return ret; > - screen->drm = nouveau_drm(&dev->object); > - screen->device = dev; > > ret = nouveau_client_new(screen->device, &screen->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 &screen->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 = &screen->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 = &screen->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 = &screen->base.base; > + pscreen->destroy = nv50_screen_destroy; > > ret = nouveau_screen_init(&screen->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 &screen->base; > > fail: > - nv50_screen_destroy(pscreen); > - return NULL; > + screen->base.base.context_create = NULL; > + return &screen->base; > } > > int > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c > b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c > index e45031a..4897ebe 100644 > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c > @@ -617,8 +617,7 @@ nvc0_screen_resize_tls_area(struct nvc0_screen *screen, > #define FAIL_SCREEN_INIT(str, err) \ > do { \ > NOUVEAU_ERR(str, err); \ > - nvc0_screen_destroy(pscreen); \ > - return NULL; \ > + goto fail; \ > } while(0) > > struct nouveau_screen * > @@ -650,6 +649,7 @@ nvc0_screen_create(struct nouveau_device *dev) > if (!screen) > return NULL; > pscreen = &screen->base.base; > + pscreen->destroy = nvc0_screen_destroy; > > ret = nouveau_screen_init(&screen->base, dev); > if (ret) { > @@ -672,7 +672,6 @@ nvc0_screen_create(struct nouveau_device *dev) > screen->base.vidmem_bindings = 0; > } > > - pscreen->destroy = nvc0_screen_destroy; > pscreen->context_create = nvc0_create; > pscreen->is_format_supported = nvc0_screen_is_format_supported; > pscreen->get_param = nvc0_screen_get_param; > @@ -1065,8 +1064,8 @@ nvc0_screen_create(struct nouveau_device *dev) > return &screen->base; > > fail: > - nvc0_screen_destroy(pscreen); > - return NULL; > + screen->base.base.context_create = NULL; > + return &screen->base; > } > > int > diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c > b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c > index e117dfc..456530d 100644 > --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c > +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c > @@ -59,7 +59,7 @@ nouveau_drm_screen_create(int fd) > { > struct nouveau_device *dev = NULL; > struct nouveau_screen *(*init)(struct nouveau_device *); > - struct nouveau_screen *screen; > + struct nouveau_screen *screen = NULL; > int ret, dupfd = -1; > > pipe_mutex_lock(nouveau_screen_mutex); > @@ -117,7 +117,7 @@ nouveau_drm_screen_create(int fd) > } > > screen = init(dev); > - if (!screen) > + if (!screen || !screen->base.context_create) > goto err; > > /* Use dupfd in hash table, to avoid errors if the original fd gets > @@ -130,10 +130,14 @@ nouveau_drm_screen_create(int fd) > return &screen->base; > > err: > - if (dev) > - nouveau_device_del(&dev); > - else if (dupfd >= 0) > - close(dupfd); > + if (screen) { > + screen->base.destroy(&screen->base); > + } else { > + if (dev) > + nouveau_device_del(&dev); > + else if (dupfd >= 0) > + close(dupfd); > + } > pipe_mutex_unlock(nouveau_screen_mutex); > return NULL; > } > -- > 2.6.3 > > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/nouveau _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/nouveau