* [PATCH] drm/i915: tidy up gen8_init_scratch
@ 2016-04-27 12:19 Matthew Auld
2016-04-27 12:30 ` Mika Kuoppala
2016-04-27 12:49 ` ✗ Fi.CI.BAT: failure for drm/i915: tidy up gen8_init_scratch (rev2) Patchwork
0 siblings, 2 replies; 10+ messages in thread
From: Matthew Auld @ 2016-04-27 12:19 UTC (permalink / raw)
To: intel-gfx
Prefer a goto teardown path to do all the required cleanup.
v2:
(Joonas Lahtinen)
- remove NULL assignments
- rename goto labels
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0d666b3..87947ec 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -866,6 +866,7 @@ static void gen8_free_page_tables(struct drm_device *dev,
static int gen8_init_scratch(struct i915_address_space *vm)
{
struct drm_device *dev = vm->dev;
+ int ret;
vm->scratch_page = alloc_scratch_page(dev);
if (IS_ERR(vm->scratch_page))
@@ -873,24 +874,21 @@ static int gen8_init_scratch(struct i915_address_space *vm)
vm->scratch_pt = alloc_pt(dev);
if (IS_ERR(vm->scratch_pt)) {
- free_scratch_page(dev, vm->scratch_page);
- return PTR_ERR(vm->scratch_pt);
+ ret = PTR_ERR(vm->scratch_pt);
+ goto free_scratch_page;
}
vm->scratch_pd = alloc_pd(dev);
if (IS_ERR(vm->scratch_pd)) {
- free_pt(dev, vm->scratch_pt);
- free_scratch_page(dev, vm->scratch_page);
- return PTR_ERR(vm->scratch_pd);
+ ret = PTR_ERR(vm->scratch_pd);
+ goto free_pt;
}
if (USES_FULL_48BIT_PPGTT(dev)) {
vm->scratch_pdp = alloc_pdp(dev);
if (IS_ERR(vm->scratch_pdp)) {
- free_pd(dev, vm->scratch_pd);
- free_pt(dev, vm->scratch_pt);
- free_scratch_page(dev, vm->scratch_page);
- return PTR_ERR(vm->scratch_pdp);
+ ret = PTR_ERR(vm->scratch_pdp);
+ goto free_pd;
}
}
@@ -900,6 +898,15 @@ static int gen8_init_scratch(struct i915_address_space *vm)
gen8_initialize_pdp(vm, vm->scratch_pdp);
return 0;
+
+free_pd:
+ free_pd(dev, vm->scratch_pd);
+free_pt:
+ free_pt(dev, vm->scratch_pt);
+free_scratch_page:
+ free_scratch_page(dev, vm->scratch_page);
+
+ return ret;
}
static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
--
2.4.11
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: tidy up gen8_init_scratch
2016-04-27 12:19 [PATCH] drm/i915: tidy up gen8_init_scratch Matthew Auld
@ 2016-04-27 12:30 ` Mika Kuoppala
2016-04-27 13:10 ` Joonas Lahtinen
2016-04-27 12:49 ` ✗ Fi.CI.BAT: failure for drm/i915: tidy up gen8_init_scratch (rev2) Patchwork
1 sibling, 1 reply; 10+ messages in thread
From: Mika Kuoppala @ 2016-04-27 12:30 UTC (permalink / raw)
To: Matthew Auld, intel-gfx
Matthew Auld <matthew.auld@intel.com> writes:
> [ text/plain ]
> Prefer a goto teardown path to do all the required cleanup.
>
> v2:
> (Joonas Lahtinen)
> - remove NULL assignments
> - rename goto labels
>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0d666b3..87947ec 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -866,6 +866,7 @@ static void gen8_free_page_tables(struct drm_device *dev,
> static int gen8_init_scratch(struct i915_address_space *vm)
> {
> struct drm_device *dev = vm->dev;
> + int ret;
>
> vm->scratch_page = alloc_scratch_page(dev);
> if (IS_ERR(vm->scratch_page))
> @@ -873,24 +874,21 @@ static int gen8_init_scratch(struct i915_address_space *vm)
>
> vm->scratch_pt = alloc_pt(dev);
> if (IS_ERR(vm->scratch_pt)) {
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pt);
> + ret = PTR_ERR(vm->scratch_pt);
> + goto free_scratch_page;
> }
>
> vm->scratch_pd = alloc_pd(dev);
> if (IS_ERR(vm->scratch_pd)) {
> - free_pt(dev, vm->scratch_pt);
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pd);
> + ret = PTR_ERR(vm->scratch_pd);
> + goto free_pt;
> }
>
> if (USES_FULL_48BIT_PPGTT(dev)) {
> vm->scratch_pdp = alloc_pdp(dev);
> if (IS_ERR(vm->scratch_pdp)) {
> - free_pd(dev, vm->scratch_pd);
> - free_pt(dev, vm->scratch_pt);
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pdp);
> + ret = PTR_ERR(vm->scratch_pdp);
> + goto free_pd;
> }
> }
>
> @@ -900,6 +898,15 @@ static int gen8_init_scratch(struct i915_address_space *vm)
> gen8_initialize_pdp(vm, vm->scratch_pdp);
>
> return 0;
> +
> +free_pd:
> + free_pd(dev, vm->scratch_pd);
> +free_pt:
> + free_pt(dev, vm->scratch_pt);
> +free_scratch_page:
> + free_scratch_page(dev, vm->scratch_page);
> +
> + return ret;
> }
>
> static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
> --
> 2.4.11
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: tidy up gen8_init_scratch
2016-04-27 12:30 ` Mika Kuoppala
@ 2016-04-27 13:10 ` Joonas Lahtinen
0 siblings, 0 replies; 10+ messages in thread
From: Joonas Lahtinen @ 2016-04-27 13:10 UTC (permalink / raw)
To: Mika Kuoppala, Matthew Auld, intel-gfx
On ke, 2016-04-27 at 15:30 +0300, Mika Kuoppala wrote:
> Matthew Auld <matthew.auld@intel.com> writes:
>
> >
> > [ text/plain ]
> > Prefer a goto teardown path to do all the required cleanup.
> >
> > v2:
> > (Joonas Lahtinen)
> > - remove NULL assignments
> > - rename goto labels
> >
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Dave Gordon <david.s.gordon@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >
> > ---
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++++++---------
> > 1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 0d666b3..87947ec 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -866,6 +866,7 @@ static void gen8_free_page_tables(struct drm_device *dev,
> > static int gen8_init_scratch(struct i915_address_space *vm)
> > {
> > struct drm_device *dev = vm->dev;
> > + int ret;
> >
> > vm->scratch_page = alloc_scratch_page(dev);
> > if (IS_ERR(vm->scratch_page))
> > @@ -873,24 +874,21 @@ static int gen8_init_scratch(struct i915_address_space *vm)
> >
> > vm->scratch_pt = alloc_pt(dev);
> > if (IS_ERR(vm->scratch_pt)) {
> > - free_scratch_page(dev, vm->scratch_page);
> > - return PTR_ERR(vm->scratch_pt);
> > + ret = PTR_ERR(vm->scratch_pt);
> > + goto free_scratch_page;
> > }
> >
> > vm->scratch_pd = alloc_pd(dev);
> > if (IS_ERR(vm->scratch_pd)) {
> > - free_pt(dev, vm->scratch_pt);
> > - free_scratch_page(dev, vm->scratch_page);
> > - return PTR_ERR(vm->scratch_pd);
> > + ret = PTR_ERR(vm->scratch_pd);
> > + goto free_pt;
> > }
> >
> > if (USES_FULL_48BIT_PPGTT(dev)) {
> > vm->scratch_pdp = alloc_pdp(dev);
> > if (IS_ERR(vm->scratch_pdp)) {
> > - free_pd(dev, vm->scratch_pd);
> > - free_pt(dev, vm->scratch_pt);
> > - free_scratch_page(dev, vm->scratch_page);
> > - return PTR_ERR(vm->scratch_pdp);
> > + ret = PTR_ERR(vm->scratch_pdp);
> > + goto free_pd;
> > }
> > }
> >
> > @@ -900,6 +898,15 @@ static int gen8_init_scratch(struct i915_address_space *vm)
> > gen8_initialize_pdp(vm, vm->scratch_pdp);
> >
> > return 0;
> > +
> > +free_pd:
> > + free_pd(dev, vm->scratch_pd);
> > +free_pt:
> > + free_pt(dev, vm->scratch_pt);
> > +free_scratch_page:
> > + free_scratch_page(dev, vm->scratch_page);
> > +
> > + return ret;
> > }
> >
> > static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
--
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: tidy up gen8_init_scratch (rev2)
2016-04-27 12:19 [PATCH] drm/i915: tidy up gen8_init_scratch Matthew Auld
2016-04-27 12:30 ` Mika Kuoppala
@ 2016-04-27 12:49 ` Patchwork
2016-04-27 13:06 ` Mika Kuoppala
2016-04-27 13:14 ` Joonas Lahtinen
1 sibling, 2 replies; 10+ messages in thread
From: Patchwork @ 2016-04-27 12:49 UTC (permalink / raw)
To: Matthew Auld; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: tidy up gen8_init_scratch (rev2)
URL : https://patchwork.freedesktop.org/series/6315/
State : failure
== Summary ==
Series 6315v2 drm/i915: tidy up gen8_init_scratch
http://patchwork.freedesktop.org/api/1.0/series/6315/revisions/2/mbox/
Test drv_module_reload_basic:
dmesg-warn -> PASS (snb-x220t)
Test gem_exec_suspend:
Subgroup basic-s3:
fail -> PASS (snb-x220t)
Test kms_flip:
Subgroup basic-flip-vs-wf_vblank:
pass -> FAIL (snb-x220t)
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
hsw-brixbox total:200 pass:174 dwarn:0 dfail:0 fail:0 skip:26
hsw-gt2 total:200 pass:178 dwarn:0 dfail:0 fail:1 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:157 dwarn:0 dfail:0 fail:2 skip:41
Results at /archive/results/CI_IGT_test/Patchwork_2089/
4fa405ab5848b76c8568c7fb771d389a6695108c drm-intel-nightly: 2016y-04m-27d-10h-47m-35s UTC integration manifest
6107550 drm/i915: tidy up gen8_init_scratch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: ✗ Fi.CI.BAT: failure for drm/i915: tidy up gen8_init_scratch (rev2)
2016-04-27 12:49 ` ✗ Fi.CI.BAT: failure for drm/i915: tidy up gen8_init_scratch (rev2) Patchwork
@ 2016-04-27 13:06 ` Mika Kuoppala
2016-04-27 13:14 ` Joonas Lahtinen
1 sibling, 0 replies; 10+ messages in thread
From: Mika Kuoppala @ 2016-04-27 13:06 UTC (permalink / raw)
To: Patchwork, Matthew Auld; +Cc: intel-gfx
Patchwork <patchwork@emeril.freedesktop.org> writes:
> [ text/plain ]
> == Series Details ==
>
> Series: drm/i915: tidy up gen8_init_scratch (rev2)
> URL : https://patchwork.freedesktop.org/series/6315/
> State : failure
>
> == Summary ==
>
> Series 6315v2 drm/i915: tidy up gen8_init_scratch
> http://patchwork.freedesktop.org/api/1.0/series/6315/revisions/2/mbox/
>
> Test drv_module_reload_basic:
> dmesg-warn -> PASS (snb-x220t)
> Test gem_exec_suspend:
> Subgroup basic-s3:
> fail -> PASS (snb-x220t)
> Test kms_flip:
> Subgroup basic-flip-vs-wf_vblank:
> pass -> FAIL (snb-x220t)
https://bugs.freedesktop.org/show_bug.cgi?id=94294
>
> 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
> hsw-brixbox total:200 pass:174 dwarn:0 dfail:0 fail:0 skip:26
> hsw-gt2 total:200 pass:178 dwarn:0 dfail:0 fail:1 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:157 dwarn:0 dfail:0 fail:2 skip:41
>
> Results at /archive/results/CI_IGT_test/Patchwork_2089/
>
> 4fa405ab5848b76c8568c7fb771d389a6695108c drm-intel-nightly: 2016y-04m-27d-10h-47m-35s UTC integration manifest
> 6107550 drm/i915: tidy up gen8_init_scratch
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ✗ Fi.CI.BAT: failure for drm/i915: tidy up gen8_init_scratch (rev2)
2016-04-27 12:49 ` ✗ Fi.CI.BAT: failure for drm/i915: tidy up gen8_init_scratch (rev2) Patchwork
2016-04-27 13:06 ` Mika Kuoppala
@ 2016-04-27 13:14 ` Joonas Lahtinen
1 sibling, 0 replies; 10+ messages in thread
From: Joonas Lahtinen @ 2016-04-27 13:14 UTC (permalink / raw)
To: intel-gfx, Matthew Auld
On ke, 2016-04-27 at 12:49 +0000, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915: tidy up gen8_init_scratch (rev2)
> URL : https://patchwork.freedesktop.org/series/6315/
> State : failure
>
> == Summary ==
>
> Series 6315v2 drm/i915: tidy up gen8_init_scratch
> http://patchwork.freedesktop.org/api/1.0/series/6315/revisions/2/mbox/
>
> Test drv_module_reload_basic:
> dmesg-warn -> PASS (snb-x220t)
> Test gem_exec_suspend:
> Subgroup basic-s3:
> fail -> PASS (snb-x220t)
> Test kms_flip:
> Subgroup basic-flip-vs-wf_vblank:
> pass -> FAIL (snb-x220t)
>
Already reported;
(kms_flip:6253) CRITICAL: Test assertion failure function check_state, file kms_flip.c:698:
(kms_flip:6253) CRITICAL: Failed assertion: fabs((((double) diff.tv_usec) - usec_interflip) / usec_interflip) <= 0.005
(kms_flip:6253) CRITICAL: Last errno: 25, Inappropriate ioctl for device
(kms_flip:6253) CRITICAL: inter-vblank ts jitter: 0s, 183886usec
Subtest basic-flip-vs-wf_vblank failed.
https://bugs.freedesktop.org/show_bug.cgi?id=94294
So merging in.
Thanks for the patch.
> 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
> hsw-brixbox total:200 pass:174 dwarn:0 dfail:0 fail:0 skip:26
> hsw-gt2 total:200 pass:178 dwarn:0 dfail:0 fail:1 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:157 dwarn:0 dfail:0 fail:2 skip:41
>
> Results at /archive/results/CI_IGT_test/Patchwork_2089/
>
> 4fa405ab5848b76c8568c7fb771d389a6695108c drm-intel-nightly: 2016y-04m-27d-10h-47m-35s UTC integration manifest
> 6107550 drm/i915: tidy up gen8_init_scratch
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915: tidy up gen8_init_scratch
@ 2016-04-26 9:27 Matthew Auld
2016-04-27 7:39 ` Joonas Lahtinen
2016-04-27 10:15 ` Dave Gordon
0 siblings, 2 replies; 10+ messages in thread
From: Matthew Auld @ 2016-04-26 9:27 UTC (permalink / raw)
To: intel-gfx
Use goto teardown path and also ensure we reset any struct members which
would otherwise contain an error encoded pointer, and could be mistaken
for a valid address.
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0d666b3..a723b74 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -866,31 +866,31 @@ static void gen8_free_page_tables(struct drm_device *dev,
static int gen8_init_scratch(struct i915_address_space *vm)
{
struct drm_device *dev = vm->dev;
+ int ret;
vm->scratch_page = alloc_scratch_page(dev);
- if (IS_ERR(vm->scratch_page))
- return PTR_ERR(vm->scratch_page);
+ if (IS_ERR(vm->scratch_page)) {
+ ret = PTR_ERR(vm->scratch_page);
+ goto fail_scratch;
+ }
vm->scratch_pt = alloc_pt(dev);
if (IS_ERR(vm->scratch_pt)) {
- free_scratch_page(dev, vm->scratch_page);
- return PTR_ERR(vm->scratch_pt);
+ ret = PTR_ERR(vm->scratch_pt);
+ goto fail_pt;
}
vm->scratch_pd = alloc_pd(dev);
if (IS_ERR(vm->scratch_pd)) {
- free_pt(dev, vm->scratch_pt);
- free_scratch_page(dev, vm->scratch_page);
- return PTR_ERR(vm->scratch_pd);
+ ret = PTR_ERR(vm->scratch_pd);
+ goto fail_pd;
}
if (USES_FULL_48BIT_PPGTT(dev)) {
vm->scratch_pdp = alloc_pdp(dev);
if (IS_ERR(vm->scratch_pdp)) {
- free_pd(dev, vm->scratch_pd);
- free_pt(dev, vm->scratch_pt);
- free_scratch_page(dev, vm->scratch_page);
- return PTR_ERR(vm->scratch_pdp);
+ ret = PTR_ERR(vm->scratch_pdp);
+ goto fail_pdp;
}
}
@@ -900,6 +900,20 @@ static int gen8_init_scratch(struct i915_address_space *vm)
gen8_initialize_pdp(vm, vm->scratch_pdp);
return 0;
+
+fail_pdp:
+ vm->scratch_pdp = NULL;
+ free_pd(dev, vm->scratch_pd);
+fail_pd:
+ vm->scratch_pd = NULL;
+ free_pt(dev, vm->scratch_pt);
+fail_pt:
+ vm->scratch_pt = NULL;
+ free_scratch_page(dev, vm->scratch_page);
+fail_scratch:
+ vm->scratch_page = NULL;
+
+ return ret;
}
static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
--
2.4.11
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: tidy up gen8_init_scratch
2016-04-26 9:27 [PATCH] drm/i915: tidy up gen8_init_scratch Matthew Auld
@ 2016-04-27 7:39 ` Joonas Lahtinen
2016-04-27 10:15 ` Dave Gordon
1 sibling, 0 replies; 10+ messages in thread
From: Joonas Lahtinen @ 2016-04-27 7:39 UTC (permalink / raw)
To: Matthew Auld, intel-gfx
On ti, 2016-04-26 at 10:27 +0100, Matthew Auld wrote:
> Use goto teardown path and also ensure we reset any struct members which
> would otherwise contain an error encoded pointer, and could be mistaken
> for a valid address.
>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
As this goes through the whole function, good idea to check with git
blame for the function author and CC (in this case Mika, added as CC).
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 36 +++++++++++++++++++++++++-----------
> 1 file changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0d666b3..a723b74 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -866,31 +866,31 @@ static void gen8_free_page_tables(struct drm_device *dev,
> static int gen8_init_scratch(struct i915_address_space *vm)
> {
> struct drm_device *dev = vm->dev;
> + int ret;
>
> vm->scratch_page = alloc_scratch_page(dev);
> - if (IS_ERR(vm->scratch_page))
> - return PTR_ERR(vm->scratch_page);
> + if (IS_ERR(vm->scratch_page)) {
> + ret = PTR_ERR(vm->scratch_page);
> + goto fail_scratch;
> + }
>
> vm->scratch_pt = alloc_pt(dev);
> if (IS_ERR(vm->scratch_pt)) {
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pt);
> + ret = PTR_ERR(vm->scratch_pt);
> + goto fail_pt;
> }
>
> vm->scratch_pd = alloc_pd(dev);
> if (IS_ERR(vm->scratch_pd)) {
> - free_pt(dev, vm->scratch_pt);
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pd);
> + ret = PTR_ERR(vm->scratch_pd);
> + goto fail_pd;
> }
>
> if (USES_FULL_48BIT_PPGTT(dev)) {
> vm->scratch_pdp = alloc_pdp(dev);
> if (IS_ERR(vm->scratch_pdp)) {
> - free_pd(dev, vm->scratch_pd);
> - free_pt(dev, vm->scratch_pt);
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pdp);
> + ret = PTR_ERR(vm->scratch_pdp);
> + goto fail_pdp;
> }
> }
>
> @@ -900,6 +900,20 @@ static int gen8_init_scratch(struct i915_address_space *vm)
> gen8_initialize_pdp(vm, vm->scratch_pdp);
>
> return 0;
> +
> +fail_pdp:
> + vm->scratch_pdp = NULL;
I don't think assigning to NULL is necessary. If this function fails,
it'll lead to driver init failure.
> + free_pd(dev, vm->scratch_pd);
> +fail_pd:
> + vm->scratch_pd = NULL;
> + free_pt(dev, vm->scratch_pt);
> +fail_pt:
> + vm->scratch_pt = NULL;
> + free_scratch_page(dev, vm->scratch_page);
> +fail_scratch:
> + vm->scratch_page = NULL;
> +
Same comment as in the last patch, related to labels, the label should
describe the code under goto section, not the code/situation that jumps
to it.
Regards, Joonas
> + return ret;
> }
>
> static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
--
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: tidy up gen8_init_scratch
2016-04-26 9:27 [PATCH] drm/i915: tidy up gen8_init_scratch Matthew Auld
2016-04-27 7:39 ` Joonas Lahtinen
@ 2016-04-27 10:15 ` Dave Gordon
2016-04-27 11:26 ` Mika Kuoppala
1 sibling, 1 reply; 10+ messages in thread
From: Dave Gordon @ 2016-04-27 10:15 UTC (permalink / raw)
To: intel-gfx, Matthew Auld
On 26/04/16 10:27, Matthew Auld wrote:
> Use goto teardown path and also ensure we reset any struct members which
> would otherwise contain an error encoded pointer, and could be mistaken
> for a valid address.
>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 36 +++++++++++++++++++++++++-----------
> 1 file changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0d666b3..a723b74 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -866,31 +866,31 @@ static void gen8_free_page_tables(struct drm_device *dev,
> static int gen8_init_scratch(struct i915_address_space *vm)
> {
> struct drm_device *dev = vm->dev;
> + int ret;
>
> vm->scratch_page = alloc_scratch_page(dev);
> - if (IS_ERR(vm->scratch_page))
> - return PTR_ERR(vm->scratch_page);
> + if (IS_ERR(vm->scratch_page)) {
> + ret = PTR_ERR(vm->scratch_page);
> + goto fail_scratch;
> + }
>
> vm->scratch_pt = alloc_pt(dev);
> if (IS_ERR(vm->scratch_pt)) {
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pt);
> + ret = PTR_ERR(vm->scratch_pt);
> + goto fail_pt;
> }
>
> vm->scratch_pd = alloc_pd(dev);
> if (IS_ERR(vm->scratch_pd)) {
> - free_pt(dev, vm->scratch_pt);
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pd);
> + ret = PTR_ERR(vm->scratch_pd);
> + goto fail_pd;
> }
>
> if (USES_FULL_48BIT_PPGTT(dev)) {
> vm->scratch_pdp = alloc_pdp(dev);
> if (IS_ERR(vm->scratch_pdp)) {
> - free_pd(dev, vm->scratch_pd);
> - free_pt(dev, vm->scratch_pt);
> - free_scratch_page(dev, vm->scratch_page);
> - return PTR_ERR(vm->scratch_pdp);
> + ret = PTR_ERR(vm->scratch_pdp);
> + goto fail_pdp;
> }
> }
>
> @@ -900,6 +900,20 @@ static int gen8_init_scratch(struct i915_address_space *vm)
> gen8_initialize_pdp(vm, vm->scratch_pdp);
>
> return 0;
> +
> +fail_pdp:
> + vm->scratch_pdp = NULL;
> + free_pd(dev, vm->scratch_pd);
> +fail_pd:
> + vm->scratch_pd = NULL;
> + free_pt(dev, vm->scratch_pt);
> +fail_pt:
> + vm->scratch_pt = NULL;
> + free_scratch_page(dev, vm->scratch_page);
> +fail_scratch:
> + vm->scratch_page = NULL;
> +
> + return ret;
> }
>
> static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
Well, I agree it's a good idea not to assign non-NULL pointer values to
members of an OUT parameter, but perhaps consider a strategy of
assigning to a local, checking the result, and storing in the parameter
block only after we know that it's a good pointer. Maybe even keep ALL
the temporary pointers local and only assign *any* of them when we know
we can assign *all* of them and return success. That way there's no
cleanup of the result block required in the error path, only freeing of
still-local values.
BTW, as it appears that this function can be called more than once in
the lifetime of the driver, do we know that all these pointers will be
NULL on entry? Even on the second and subsequent calls?
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: tidy up gen8_init_scratch
2016-04-27 10:15 ` Dave Gordon
@ 2016-04-27 11:26 ` Mika Kuoppala
0 siblings, 0 replies; 10+ messages in thread
From: Mika Kuoppala @ 2016-04-27 11:26 UTC (permalink / raw)
To: Dave Gordon, intel-gfx, Matthew Auld
Dave Gordon <david.s.gordon@intel.com> writes:
> [ text/plain ]
> On 26/04/16 10:27, Matthew Auld wrote:
>> Use goto teardown path and also ensure we reset any struct members which
>> would otherwise contain an error encoded pointer, and could be mistaken
>> for a valid address.
>>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem_gtt.c | 36 +++++++++++++++++++++++++-----------
>> 1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 0d666b3..a723b74 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -866,31 +866,31 @@ static void gen8_free_page_tables(struct drm_device *dev,
>> static int gen8_init_scratch(struct i915_address_space *vm)
>> {
>> struct drm_device *dev = vm->dev;
>> + int ret;
>>
>> vm->scratch_page = alloc_scratch_page(dev);
>> - if (IS_ERR(vm->scratch_page))
>> - return PTR_ERR(vm->scratch_page);
>> + if (IS_ERR(vm->scratch_page)) {
>> + ret = PTR_ERR(vm->scratch_page);
>> + goto fail_scratch;
>> + }
>>
>> vm->scratch_pt = alloc_pt(dev);
>> if (IS_ERR(vm->scratch_pt)) {
>> - free_scratch_page(dev, vm->scratch_page);
>> - return PTR_ERR(vm->scratch_pt);
>> + ret = PTR_ERR(vm->scratch_pt);
>> + goto fail_pt;
>> }
>>
>> vm->scratch_pd = alloc_pd(dev);
>> if (IS_ERR(vm->scratch_pd)) {
>> - free_pt(dev, vm->scratch_pt);
>> - free_scratch_page(dev, vm->scratch_page);
>> - return PTR_ERR(vm->scratch_pd);
>> + ret = PTR_ERR(vm->scratch_pd);
>> + goto fail_pd;
>> }
>>
>> if (USES_FULL_48BIT_PPGTT(dev)) {
>> vm->scratch_pdp = alloc_pdp(dev);
>> if (IS_ERR(vm->scratch_pdp)) {
>> - free_pd(dev, vm->scratch_pd);
>> - free_pt(dev, vm->scratch_pt);
>> - free_scratch_page(dev, vm->scratch_page);
>> - return PTR_ERR(vm->scratch_pdp);
>> + ret = PTR_ERR(vm->scratch_pdp);
>> + goto fail_pdp;
>> }
>> }
>>
>> @@ -900,6 +900,20 @@ static int gen8_init_scratch(struct i915_address_space *vm)
>> gen8_initialize_pdp(vm, vm->scratch_pdp);
>>
>> return 0;
>> +
>> +fail_pdp:
>> + vm->scratch_pdp = NULL;
>> + free_pd(dev, vm->scratch_pd);
>> +fail_pd:
>> + vm->scratch_pd = NULL;
>> + free_pt(dev, vm->scratch_pt);
>> +fail_pt:
>> + vm->scratch_pt = NULL;
>> + free_scratch_page(dev, vm->scratch_page);
>> +fail_scratch:
>> + vm->scratch_page = NULL;
>> +
>> + return ret;
>> }
>>
>> static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
>
> Well, I agree it's a good idea not to assign non-NULL pointer values to
> members of an OUT parameter, but perhaps consider a strategy of
> assigning to a local, checking the result, and storing in the parameter
> block only after we know that it's a good pointer. Maybe even keep ALL
> the temporary pointers local and only assign *any* of them when we know
> we can assign *all* of them and return success. That way there's no
> cleanup of the result block required in the error path, only freeing of
> still-local values.
Local would be clean but then it would be burden on the hot path.
Lets just incur the, now useless, writes on the error path.
>
> BTW, as it appears that this function can be called more than once in
> the lifetime of the driver, do we know that all these pointers will be
> NULL on entry? Even on the second and subsequent calls?
>
The defensive programming of is good, but in this case we
kzalloc the ppgtt, which contains the struct we are initializing.
If we are unsuccessful, we free the ppgtt. And the function is static
so there are no external users.
That said, the value of separating the error handling into
a goto teardown is valuable for readability.
Change the goto labels as Joonas suggested and I am
ok with this.
Thanks,
-Mika
-
> .Dave.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-04-27 13:13 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-27 12:19 [PATCH] drm/i915: tidy up gen8_init_scratch Matthew Auld
2016-04-27 12:30 ` Mika Kuoppala
2016-04-27 13:10 ` Joonas Lahtinen
2016-04-27 12:49 ` ✗ Fi.CI.BAT: failure for drm/i915: tidy up gen8_init_scratch (rev2) Patchwork
2016-04-27 13:06 ` Mika Kuoppala
2016-04-27 13:14 ` Joonas Lahtinen
-- strict thread matches above, loose matches on Subject: below --
2016-04-26 9:27 [PATCH] drm/i915: tidy up gen8_init_scratch Matthew Auld
2016-04-27 7:39 ` Joonas Lahtinen
2016-04-27 10:15 ` Dave Gordon
2016-04-27 11:26 ` Mika Kuoppala
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.