[PATCH] drm/i915: In render_state_init reset obj in teardown path

[PATCH] drm/i915: In render_state_init reset obj in teardown path

[Matthew Auld]

The teardown path in render_state_init leaves so->obj != NULL.

Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_render_state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 71611bf..10f3cf0 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -70,6 +70,7 @@ static int render_state_init(struct render_state *so, struct drm_device *dev)
 
 free_gem:
 	drm_gem_object_unreference(&so->obj->base);
+	so->obj = NULL;
 	return ret;
 }
 
-- 
2.4.11

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: In render_state_init reset obj in teardown path

[Dave Gordon]

On 26/04/16 10:21, Matthew Auld wrote:
> The teardown path in render_state_init leaves so->obj != NULL.
>
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_render_state.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 71611bf..10f3cf0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -70,6 +70,7 @@ static int render_state_init(struct render_state *so, struct drm_device *dev)
>
>   free_gem:
>   	drm_gem_object_unreference(&so->obj->base);
> +	so->obj = NULL;
>   	return ret;
>   }

It doesn't actually matter, because 'so' is pointing to a local object a 
few frames up the callstack. But I don't think this function is entitled 
to assume that; it should leave everything in a consistent state so 
there aren't any dangling pointers to objects that have been freed lying 
around - someday the argument may turn into a per-engine static or some 
other long-lived thing. So,

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: In render_state_init reset obj in teardown path

[Patchwork]

== Series Details ==

Series: drm/i915: In render_state_init reset obj in teardown path
URL   : https://patchwork.freedesktop.org/series/6314/
State : failure

== Summary ==

Series 6314v1 drm/i915: In render_state_init reset obj in teardown path
http://patchwork.freedesktop.org/api/1.0/series/6314/revisions/1/mbox/

Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> FAIL       (hsw-gt2)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                pass       -> SKIP       (hsw-brixbox)

bdw-nuci7        total:200  pass:188  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:200  pass:175  dwarn:0   dfail:0   fail:0   skip:25 
bsw-nuc-2        total:199  pass:158  dwarn:0   dfail:0   fail:0   skip:41 
byt-nuc          total:199  pass:158  dwarn:0   dfail:0   fail:0   skip:41 
hsw-brixbox      total:200  pass:173  dwarn:0   dfail:0   fail:0   skip:27 
hsw-gt2          total:200  pass:177  dwarn:0   dfail:0   fail:2   skip:21 
ilk-hp8440p      total:200  pass:139  dwarn:0   dfail:0   fail:0   skip:61 
ivb-t430s        total:200  pass:169  dwarn:0   dfail:0   fail:0   skip:31 
skl-i7k-2        total:200  pass:173  dwarn:0   dfail:0   fail:0   skip:27 
skl-nuci5        total:200  pass:189  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:200  pass:158  dwarn:0   dfail:0   fail:0   skip:42 
snb-x220t        total:200  pass:158  dwarn:0   dfail:0   fail:1   skip:41 

Results at /archive/results/CI_IGT_test/Patchwork_2069/

f028bc7dba69ef340ff5139e06fb332a62bc8455 drm-intel-nightly: 2016y-04m-26d-10h-05m-40s UTC integration manifest
09b3bbe drm/i915: In render_state_init reset obj in teardown path

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: In render_state_init reset obj in teardown path

[Joonas Lahtinen]

On ti, 2016-04-26 at 11:04 +0100, Dave Gordon wrote:
> On 26/04/16 10:21, Matthew Auld wrote:
> > 
> > The teardown path in render_state_init leaves so->obj != NULL.
> > 
> > Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_render_state.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> > index 71611bf..10f3cf0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> > @@ -70,6 +70,7 @@ static int render_state_init(struct render_state *so, struct drm_device *dev)
> > 
> >   free_gem:
> >   	drm_gem_object_unreference(&so->obj->base);
> > +	so->obj = NULL;

This is object_init style function, if it fails, the contents of "so"
is expected to be uninitialized, and should only be freed or attempted
to re-initialize by caller, never inspected, so no need for this.

See gen8_ppgtt_init for an example, it would be rather cubersome to
undo all assignments on error.

In short, I do not see this as a necessary step.

Regards, Joonas

> >   	return ret;
> >   }
> It doesn't actually matter, because 'so' is pointing to a local object a 
> few frames up the callstack. But I don't think this function is entitled 
> to assume that; it should leave everything in a consistent state so 
> there aren't any dangling pointers to objects that have been freed lying 
> around - someday the argument may turn into a per-engine static or some 
> other long-lived thing. So,
> 
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx