On Sun, Dec 6, 2015 at 10:48 PM, Ben Skeggs <skeg...@gmail.com> 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 <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