* [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-19 15:55 Colin King
2017-09-19 20:05 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Colin King @ 2017-09-19 15:55 UTC (permalink / raw)
To: fred gao, Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, David Airlie, intel-gvt-dev, intel-gfx, dri-devel
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
An earlier fix changed the return type from find_bb_size however the
integer return is being assigned to a unsigned int so the -ve error
check will never be detected. Make bb_size an int to fix this.
Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
index 2c0ccbb817dc..f41cbf664b69 100644
--- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
+++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
@@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
struct intel_shadow_bb_entry *entry_obj;
struct intel_vgpu *vgpu = s->vgpu;
unsigned long gma = 0;
- uint32_t bb_size;
+ int bb_size;
void *dst = NULL;
int ret = 0;
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-19 15:55 [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly Colin King
@ 2017-09-19 20:05 ` Patchwork
2017-09-19 21:46 ` [PATCH][drm-next] " Zhenyu Wang
2017-09-19 23:24 ` ✓ Fi.CI.IGT: success for " Patchwork
2 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-09-19 20:05 UTC (permalink / raw)
To: Colin King; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/gvt: ensure -ve return value is handled correctly
URL : https://patchwork.freedesktop.org/series/30604/
State : success
== Summary ==
Series 30604v1 drm/i915/gvt: ensure -ve return value is handled correctly
https://patchwork.freedesktop.org/api/1.0/series/30604/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> INCOMPLETE (fi-kbl-7500u) fdo#102850
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
pass -> FAIL (fi-snb-2600) fdo#100215
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-b:
incomplete -> DMESG-WARN (fi-cfl-s) fdo#102294
Test drv_module_reload:
Subgroup basic-reload:
pass -> DMESG-WARN (fi-glk-1) fdo#102777
fdo#102850 https://bugs.freedesktop.org/show_bug.cgi?id=102850
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fdo#102777 https://bugs.freedesktop.org/show_bug.cgi?id=102777
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:440s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:468s
fi-blb-e6850 total:289 pass:224 dwarn:1 dfail:0 fail:0 skip:64 time:422s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:514s
fi-bwr-2160 total:289 pass:184 dwarn:0 dfail:0 fail:0 skip:105 time:279s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:495s
fi-byt-j1900 total:289 pass:254 dwarn:1 dfail:0 fail:0 skip:34 time:490s
fi-byt-n2820 total:289 pass:250 dwarn:1 dfail:0 fail:0 skip:38 time:490s
fi-cfl-s total:289 pass:222 dwarn:35 dfail:0 fail:0 skip:32 time:543s
fi-elk-e7500 total:289 pass:230 dwarn:0 dfail:0 fail:0 skip:59 time:416s
fi-glk-1 total:289 pass:259 dwarn:1 dfail:0 fail:0 skip:29 time:567s
fi-hsw-4770 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:427s
fi-hsw-4770r total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:403s
fi-ilk-650 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:434s
fi-ivb-3520m total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:490s
fi-ivb-3770 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:466s
fi-kbl-7500u total:118 pass:100 dwarn:1 dfail:0 fail:0 skip:16
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:572s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:587s
fi-pnv-d510 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:542s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:453s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:748s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:490s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:476s
fi-snb-2520m total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:572s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:1 skip:39 time:412s
bf6ecf6d25c1c45e576643b7d7a65e8b1e6b4f01 drm-tip: 2017y-09m-19d-17h-23m-04s UTC integration manifest
85130ca1c516 drm/i915/gvt: ensure -ve return value is handled correctly
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5752/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-19 15:55 [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly Colin King
2017-09-19 20:05 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-09-19 21:46 ` Zhenyu Wang
2017-09-20 2:35 ` Joe Perches
2017-09-19 23:24 ` ✓ Fi.CI.IGT: success for " Patchwork
2 siblings, 1 reply; 11+ messages in thread
From: Zhenyu Wang @ 2017-09-19 21:46 UTC (permalink / raw)
To: Colin King
Cc: fred gao, David Airlie, intel-gfx, kernel-janitors, linux-kernel,
dri-devel, Rodrigo Vivi, intel-gvt-dev
[-- Attachment #1.1: Type: text/plain, Size: 1295 bytes --]
On 2017.09.19 16:55:34 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> An earlier fix changed the return type from find_bb_size however the
> integer return is being assigned to a unsigned int so the -ve error
> check will never be detected. Make bb_size an int to fix this.
>
> Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
>
> Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 2c0ccbb817dc..f41cbf664b69 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> struct intel_shadow_bb_entry *entry_obj;
> struct intel_vgpu *vgpu = s->vgpu;
> unsigned long gma = 0;
> - uint32_t bb_size;
> + int bb_size;
> void *dst = NULL;
> int ret = 0;
>
Applied this, thanks!
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-19 15:55 [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly Colin King
2017-09-19 20:05 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-09-19 21:46 ` [PATCH][drm-next] " Zhenyu Wang
@ 2017-09-19 23:24 ` Patchwork
2 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-09-19 23:24 UTC (permalink / raw)
To: Colin King; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/gvt: ensure -ve return value is handled correctly
URL : https://patchwork.freedesktop.org/series/30604/
State : success
== Summary ==
Test perf:
Subgroup polling:
fail -> PASS (shard-hsw) fdo#102252 +1
Test kms_flip:
Subgroup rcs-wf_vblank-vs-dpms:
dmesg-warn -> PASS (shard-hsw)
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
shard-hsw total:2317 pass:1245 dwarn:3 dfail:0 fail:12 skip:1057 time:9651s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5752/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-19 21:46 ` [PATCH][drm-next] " Zhenyu Wang
@ 2017-09-20 2:35 ` Joe Perches
2017-09-20 22:44 ` Zhenyu Wang
0 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2017-09-20 2:35 UTC (permalink / raw)
To: Zhenyu Wang, Colin King
Cc: fred gao, intel-gfx, Joonas Lahtinen, kernel-janitors,
linux-kernel, dri-devel, Rodrigo Vivi, intel-gvt-dev, Zhi Wang
On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > An earlier fix changed the return type from find_bb_size however the
> > integer return is being assigned to a unsigned int so the -ve error
> > check will never be detected. Make bb_size an int to fix this.
> >
> > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> >
> > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > index 2c0ccbb817dc..f41cbf664b69 100644
> > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > struct intel_shadow_bb_entry *entry_obj;
> > struct intel_vgpu *vgpu = s->vgpu;
> > unsigned long gma = 0;
> > - uint32_t bb_size;
> > + int bb_size;
> > void *dst = NULL;
> > int ret = 0;
> >
>
> Applied this, thanks!
Is it possible for bb_size to be both >= 2g and valid?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-20 2:35 ` Joe Perches
@ 2017-09-20 22:44 ` Zhenyu Wang
2017-09-21 14:31 ` Joonas Lahtinen
0 siblings, 1 reply; 11+ messages in thread
From: Zhenyu Wang @ 2017-09-20 22:44 UTC (permalink / raw)
To: Joe Perches
Cc: fred gao, intel-gfx, Joonas Lahtinen, kernel-janitors,
linux-kernel, dri-devel, Rodrigo Vivi, Colin King, intel-gvt-dev,
Zhi Wang
[-- Attachment #1.1: Type: text/plain, Size: 1685 bytes --]
On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > An earlier fix changed the return type from find_bb_size however the
> > > integer return is being assigned to a unsigned int so the -ve error
> > > check will never be detected. Make bb_size an int to fix this.
> > >
> > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > >
> > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > struct intel_shadow_bb_entry *entry_obj;
> > > struct intel_vgpu *vgpu = s->vgpu;
> > > unsigned long gma = 0;
> > > - uint32_t bb_size;
> > > + int bb_size;
> > > void *dst = NULL;
> > > int ret = 0;
> > >
> >
> > Applied this, thanks!
>
> Is it possible for bb_size to be both >= 2g and valid?
Never be possible in practise and if really that big I think something
is already insane indeed.
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-20 22:44 ` Zhenyu Wang
@ 2017-09-21 14:31 ` Joonas Lahtinen
2017-09-21 16:17 ` Wang, Zhi A
0 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2017-09-21 14:31 UTC (permalink / raw)
To: Zhenyu Wang, Joe Perches
Cc: fred gao, intel-gfx, kernel-janitors, linux-kernel, dri-devel,
Rodrigo Vivi, Colin King, intel-gvt-dev, Zhi Wang
On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > An earlier fix changed the return type from find_bb_size however the
> > > > integer return is being assigned to a unsigned int so the -ve error
> > > > check will never be detected. Make bb_size an int to fix this.
> > > >
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > > >
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > - uint32_t bb_size;
> > > > + int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >
> > >
> > > Applied this, thanks!
> >
> > Is it possible for bb_size to be both >= 2g and valid?
>
> Never be possible in practise and if really that big I think something
> is already insane indeed.
It's good idea to document these assumptions as WARN_ON's. In i915, if
the value is completely internal to kernel, we're using GEM_BUG_ON for
these so that our CI will notice breakage. If it's not a driver
internal value only, a WARN_ON is the appropriate action.
Otherwise the information is lost and the next person reading the code
will have the same question in mind.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-21 14:31 ` Joonas Lahtinen
@ 2017-09-21 16:17 ` Wang, Zhi A
2017-09-22 11:11 ` Joonas Lahtinen
0 siblings, 1 reply; 11+ messages in thread
From: Wang, Zhi A @ 2017-09-21 16:17 UTC (permalink / raw)
To: Joonas Lahtinen, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx@lists.freedesktop.org,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, Vivi, Rodrigo, Colin King,
intel-gvt-dev@lists.freedesktop.org
Hi Joonas:
Thanks for the introduction. I have been thinking about the possibility of introducing GEM_BUG_ON into GVT-g recently and investigating on it. I'm just a bit confused about the usage between GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly is disabled in a production kernel. In the case of i915, I'm sure it will be enabled in CI test so that it can catch broken code path. Looking into GVT-g, the similar scenario is we enable it in QA test.
Let's say GEM_BUG_ON can do its work very well in QA test but QA test is not fully covered all the condition, then something might be still broken when it comes to the production kernel for user and GEM_BUG_ON will be disabled and will not catch that, I guess.
That's my confusion which scratched my mind during the investigation: If GEM_BUG_ON is not always working, then it looks WARN_ON should always be used.... Expected to learn more about the story behind. :)
Thanks,
Zhi.
-----Original Message-----
From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of Joonas Lahtinen
Sent: Thursday, September 21, 2017 5:32 PM
To: Zhenyu Wang <zhenyuw@linux.intel.com>; Joe Perches <joe@perches.com>
Cc: Gao, Fred <fred.gao@intel.com>; David Airlie <airlied@linux.ie>; intel-gfx@lists.freedesktop.org; kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org; Jani Nikula <jani.nikula@linux.intel.com>; dri-devel@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Colin King <colin.king@canonical.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
Subject: Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > An earlier fix changed the return type from find_bb_size however
> > > > the integer return is being assigned to a unsigned int so the
> > > > -ve error check will never be detected. Make bb_size an int to fix this.
> > > >
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against
> > > > 0")
> > > >
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for
> > > > perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > - uint32_t bb_size;
> > > > + int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >
> > >
> > > Applied this, thanks!
> >
> > Is it possible for bb_size to be both >= 2g and valid?
>
> Never be possible in practise and if really that big I think something
> is already insane indeed.
It's good idea to document these assumptions as WARN_ON's. In i915, if the value is completely internal to kernel, we're using GEM_BUG_ON for these so that our CI will notice breakage. If it's not a driver internal value only, a WARN_ON is the appropriate action.
Otherwise the information is lost and the next person reading the code will have the same question in mind.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
intel-gvt-dev mailing list
intel-gvt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-21 16:17 ` Wang, Zhi A
@ 2017-09-22 11:11 ` Joonas Lahtinen
2017-09-22 17:50 ` Wang, Zhi A
0 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2017-09-22 11:11 UTC (permalink / raw)
To: Wang, Zhi A, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx@lists.freedesktop.org,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, Vivi, Rodrigo, Colin King,
intel-gvt-dev@lists.freedesktop.org
On Thu, 2017-09-21 at 16:17 +0000, Wang, Zhi A wrote:
> Hi Joonas:
>
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is basically there to catch things that we do not expect
ever to happen within the driver. So we often list the function
preconditions as GEM_BUG_ON. It's there for the same reason as the
lockdep_assert_held and KASAN. It's sometimes heavy checks that we
really want to run when functionally validating kernel.
GEM_BUG_ON became to existence because adding checks for obvious
conditions at the critical command submission path GEM is not
sustainable for performance in production.
The expectation is that each GEM_BUG_ON has a testcase in I-G-T that
has the potential to hit it if driver was modified not to respect those
preconditions. So once our testest passes, we can disable the
GEM_BUG_ONs and be confident of the internal driver quality and get the
release performance.
WARN_ON is mostly used for the cases when the hardware is behaving
differently than we expect. We can't remove them as we don't have all
the hardware in the world to test, but we try to exercise them too
through I-G-Ts. The test will often be the subtest that was written to
reproduce the problem with our expectations of hardware in case of
hangs and other bugs. After we've corrected the driver behaviour, or
got a hardware W/A assigned, we keep the test and add a WARN_ON to make
sure there will be no regression back to the same situation.
This is at least what should happen, given time constraints, there may
be variations.
User behaving unexpectedly should never result in WARN_ON (or even
worse, BUG_ON), should always just be debug messages displayed (not to
trigger the CI) and errors propagated back to user:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values
Bare BUG_ON should only be used when there's the danger of corrupting
system memory or filesystems, so from graphics driver, that's not very
often. Controlled propagation of errors and maybe WARN_ON is always
preferred if possible.
> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
>
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
>
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used.... Expected to learn more about the story behind. :)
So if the saying is some object is "never going to be bigger than 2G",
there should be either:
1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by
trying to allocate a huge object for example, and should get rejection
as -EINVAL
2. Test to see if the object is bigger, and propagate back the error if
it is. Either resulting in user reported error if the origin of the
object is outside of kernel <-> hardware. Or a WARN_ON if it's strange
hardware or kernel driver behavior.
You should choose depending on how often your function gets called, and
how critical the execution time is.
Hopefully this clarified things.
Regards, Joonas
--
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] 11+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-22 11:11 ` Joonas Lahtinen
@ 2017-09-22 17:50 ` Wang, Zhi A
2017-09-25 9:32 ` Joonas Lahtinen
0 siblings, 1 reply; 11+ messages in thread
From: Wang, Zhi A @ 2017-09-22 17:50 UTC (permalink / raw)
To: Joonas Lahtinen, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx@lists.freedesktop.org,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, Vivi, Rodrigo, Colin King,
intel-gvt-dev@lists.freedesktop.org
Thanks for the reply. Learned a lot. :)
GEM_BUG_ON is new to me since it wasn't there at the beginning of GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the code and some of them should be GEM_BUG_ON now.
Now I can figure out those differences. We can discuss with our QA to see if they would like to enable I915_GEM_DEBUG and then we can move to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :) Thank you so much. Have a great weekend.
Thanks,
Zhi.
-----Original Message-----
From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com]
Sent: Friday, September 22, 2017 2:11 PM
To: Wang, Zhi A <zhi.a.wang@intel.com>; Zhenyu Wang <zhenyuw@linux.intel.com>; Joe Perches <joe@perches.com>
Cc: Gao, Fred <fred.gao@intel.com>; David Airlie <airlied@linux.ie>; intel-gfx@lists.freedesktop.org; kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org; Jani Nikula <jani.nikula@linux.intel.com>; dri-devel@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Colin King <colin.king@canonical.com>; intel-gvt-dev@lists.freedesktop.org
Subject: Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
On Thu, 2017-09-21 at 16:17 +0000, Wang, Zhi A wrote:
> Hi Joonas:
>
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is basically there to catch things that we do not expect ever to happen within the driver. So we often list the function preconditions as GEM_BUG_ON. It's there for the same reason as the lockdep_assert_held and KASAN. It's sometimes heavy checks that we really want to run when functionally validating kernel.
GEM_BUG_ON became to existence because adding checks for obvious conditions at the critical command submission path GEM is not sustainable for performance in production.
The expectation is that each GEM_BUG_ON has a testcase in I-G-T that has the potential to hit it if driver was modified not to respect those preconditions. So once our testest passes, we can disable the GEM_BUG_ONs and be confident of the internal driver quality and get the release performance.
WARN_ON is mostly used for the cases when the hardware is behaving differently than we expect. We can't remove them as we don't have all the hardware in the world to test, but we try to exercise them too through I-G-Ts. The test will often be the subtest that was written to reproduce the problem with our expectations of hardware in case of hangs and other bugs. After we've corrected the driver behaviour, or got a hardware W/A assigned, we keep the test and add a WARN_ON to make sure there will be no regression back to the same situation.
This is at least what should happen, given time constraints, there may be variations.
User behaving unexpectedly should never result in WARN_ON (or even worse, BUG_ON), should always just be debug messages displayed (not to trigger the CI) and errors propagated back to user:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values
Bare BUG_ON should only be used when there's the danger of corrupting system memory or filesystems, so from graphics driver, that's not very often. Controlled propagation of errors and maybe WARN_ON is always preferred if possible.
> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
>
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
>
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used.... Expected to learn more about the story behind. :)
So if the saying is some object is "never going to be bigger than 2G", there should be either:
1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by trying to allocate a huge object for example, and should get rejection as -EINVAL
2. Test to see if the object is bigger, and propagate back the error if it is. Either resulting in user reported error if the origin of the object is outside of kernel <-> hardware. Or a WARN_ON if it's strange hardware or kernel driver behavior.
You should choose depending on how often your function gets called, and how critical the execution time is.
Hopefully this clarified things.
Regards, Joonas
--
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] 11+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-22 17:50 ` Wang, Zhi A
@ 2017-09-25 9:32 ` Joonas Lahtinen
0 siblings, 0 replies; 11+ messages in thread
From: Joonas Lahtinen @ 2017-09-25 9:32 UTC (permalink / raw)
To: Wang, Zhi A, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx@lists.freedesktop.org,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
Jani Nikula, dri-devel@lists.freedesktop.org, Vivi, Rodrigo,
Colin King, intel-gvt-dev@lists.freedesktop.org
On Fri, 2017-09-22 at 17:50 +0000, Wang, Zhi A wrote:
> Thanks for the reply. Learned a lot. :)
>
> GEM_BUG_ON is new to me since it wasn't there at the beginning of
> GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the
> code and some of them should be GEM_BUG_ON now.
>
> Now I can figure out those differences. We can discuss with our QA to
> see if they would like to enable I915_GEM_DEBUG and then we can move
> to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :)
> Thank you so much. Have a great weekend.
GVT_BUG_ON is probably the way to go :)
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-09-25 9:32 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-19 15:55 [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly Colin King
2017-09-19 20:05 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-09-19 21:46 ` [PATCH][drm-next] " Zhenyu Wang
2017-09-20 2:35 ` Joe Perches
2017-09-20 22:44 ` Zhenyu Wang
2017-09-21 14:31 ` Joonas Lahtinen
2017-09-21 16:17 ` Wang, Zhi A
2017-09-22 11:11 ` Joonas Lahtinen
2017-09-22 17:50 ` Wang, Zhi A
2017-09-25 9:32 ` Joonas Lahtinen
2017-09-19 23:24 ` ✓ Fi.CI.IGT: success for " Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox