* RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-21 16:17 ` Wang, Zhi A
0 siblings, 0 replies; 28+ 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,
Jani Nikula, 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
^ permalink raw reply [flat|nested] 28+ 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
0 siblings, 0 replies; 28+ 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] 28+ 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
-1 siblings, 0 replies; 28+ 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-22 11:11 ` Joonas Lahtinen
0 siblings, 0 replies; 28+ 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,
Jani Nikula, 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-22 11:11 ` Joonas Lahtinen
0 siblings, 0 replies; 28+ 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] 28+ 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
-1 siblings, 0 replies; 28+ 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
VGhhbmtzIGZvciB0aGUgcmVwbHkuIExlYXJuZWQgYSBsb3QuIDopDQoNCkdFTV9CVUdfT04gaXMg
bmV3IHRvIG1lIHNpbmNlIGl0IHdhc24ndCB0aGVyZSBhdCB0aGUgYmVnaW5uaW5nIG9mIEdWVC1n
IHVwc3RyZWFtLiBJdCBzaG93ZWQgdXAgbGF0ZXIuIFNvIEkgbGVmdCBhIGxvdCBvZiBXQVJOX09O
IGluIHRoZSBjb2RlIGFuZCBzb21lIG9mIHRoZW0gc2hvdWxkIGJlIEdFTV9CVUdfT04gbm93Lg0K
DQpOb3cgSSBjYW4gZmlndXJlIG91dCB0aG9zZSBkaWZmZXJlbmNlcy4gV2UgY2FuIGRpc2N1c3Mg
d2l0aCBvdXIgUUEgdG8gc2VlIGlmIHRoZXkgd291bGQgbGlrZSB0byBlbmFibGUgSTkxNV9HRU1f
REVCVUcgYW5kIHRoZW4gd2UgY2FuIG1vdmUgdG8gR0VNX0JVR19PTiBhbHNvLCBvciBtYXliZSB3
ZSBjYW4gaGF2ZSBhIGRlZGljYXRlZCBHVlRfQlVHX09OLiA6KSBUaGFuayB5b3Ugc28gbXVjaC4g
SGF2ZSBhIGdyZWF0IHdlZWtlbmQuDQoNClRoYW5rcywNClpoaS4NCg0KLS0tLS1PcmlnaW5hbCBN
ZXNzYWdlLS0tLS0NCkZyb206IEpvb25hcyBMYWh0aW5lbiBbbWFpbHRvOmpvb25hcy5sYWh0aW5l
bkBsaW51eC5pbnRlbC5jb21dIA0KU2VudDogRnJpZGF5LCBTZXB0ZW1iZXIgMjIsIDIwMTcgMjox
MSBQTQ0KVG86IFdhbmcsIFpoaSBBIDx6aGkuYS53YW5nQGludGVsLmNvbT47IFpoZW55dSBXYW5n
IDx6aGVueXV3QGxpbnV4LmludGVsLmNvbT47IEpvZSBQZXJjaGVzIDxqb2VAcGVyY2hlcy5jb20+
DQpDYzogR2FvLCBGcmVkIDxmcmVkLmdhb0BpbnRlbC5jb20+OyBEYXZpZCBBaXJsaWUgPGFpcmxp
ZWRAbGludXguaWU+OyBpbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnOyBrZXJuZWwtamFu
aXRvcnNAdmdlci5rZXJuZWwub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBKYW5p
IE5pa3VsYSA8amFuaS5uaWt1bGFAbGludXguaW50ZWwuY29tPjsgZHJpLWRldmVsQGxpc3RzLmZy
ZWVkZXNrdG9wLm9yZzsgVml2aSwgUm9kcmlnbyA8cm9kcmlnby52aXZpQGludGVsLmNvbT47IENv
bGluIEtpbmcgPGNvbGluLmtpbmdAY2Fub25pY2FsLmNvbT47IGludGVsLWd2dC1kZXZAbGlzdHMu
ZnJlZWRlc2t0b3Aub3JnDQpTdWJqZWN0OiBSZTogW1BBVENIXVtkcm0tbmV4dF0gZHJtL2k5MTUv
Z3Z0OiBlbnN1cmUgLXZlIHJldHVybiB2YWx1ZSBpcyBoYW5kbGVkIGNvcnJlY3RseQ0KDQpPbiBU
aHUsIDIwMTctMDktMjEgYXQgMTY6MTcgKzAwMDAsIFdhbmcsIFpoaSBBIHdyb3RlOg0KPiBIaSBK
b29uYXM6DQo+IA0KPiBUaGFua3MgZm9yIHRoZSBpbnRyb2R1Y3Rpb24uIEkgaGF2ZSBiZWVuIHRo
aW5raW5nIGFib3V0IHRoZSANCj4gcG9zc2liaWxpdHkgb2YgaW50cm9kdWNpbmcgR0VNX0JVR19P
TiBpbnRvIEdWVC1nIHJlY2VudGx5IGFuZCANCj4gaW52ZXN0aWdhdGluZyBvbiBpdC4gSSdtIGp1
c3QgYSBiaXQgY29uZnVzZWQgYWJvdXQgdGhlIHVzYWdlIGJldHdlZW4gDQo+IEdFTV9CVUdfT04g
YW5kIFdBUk5fT04uDQoNCkdFTV9CVUdfT04gaXMgYmFzaWNhbGx5IHRoZXJlIHRvIGNhdGNoIHRo
aW5ncyB0aGF0IHdlIGRvIG5vdCBleHBlY3QgZXZlciB0byBoYXBwZW4gd2l0aGluIHRoZSBkcml2
ZXIuIFNvIHdlIG9mdGVuIGxpc3QgdGhlIGZ1bmN0aW9uIHByZWNvbmRpdGlvbnMgYXMgR0VNX0JV
R19PTi4gSXQncyB0aGVyZSBmb3IgdGhlIHNhbWUgcmVhc29uIGFzIHRoZSBsb2NrZGVwX2Fzc2Vy
dF9oZWxkIGFuZCBLQVNBTi4gSXQncyBzb21ldGltZXMgaGVhdnkgY2hlY2tzIHRoYXQgd2UgcmVh
bGx5IHdhbnQgdG8gcnVuIHdoZW4gZnVuY3Rpb25hbGx5IHZhbGlkYXRpbmcga2VybmVsLg0KDQpH
RU1fQlVHX09OIGJlY2FtZSB0byBleGlzdGVuY2UgYmVjYXVzZSBhZGRpbmcgY2hlY2tzIGZvciBv
YnZpb3VzIGNvbmRpdGlvbnMgYXQgdGhlIGNyaXRpY2FsIGNvbW1hbmQgc3VibWlzc2lvbiBwYXRo
IEdFTSBpcyBub3Qgc3VzdGFpbmFibGUgZm9yIHBlcmZvcm1hbmNlIGluIHByb2R1Y3Rpb24uDQoN
ClRoZSBleHBlY3RhdGlvbiBpcyB0aGF0IGVhY2ggR0VNX0JVR19PTiBoYXMgYSB0ZXN0Y2FzZSBp
biBJLUctVCB0aGF0IGhhcyB0aGUgcG90ZW50aWFsIHRvIGhpdCBpdCBpZiBkcml2ZXIgd2FzIG1v
ZGlmaWVkIG5vdCB0byByZXNwZWN0IHRob3NlIHByZWNvbmRpdGlvbnMuIFNvIG9uY2Ugb3VyIHRl
c3Rlc3QgcGFzc2VzLCB3ZSBjYW4gZGlzYWJsZSB0aGUgR0VNX0JVR19PTnMgYW5kIGJlIGNvbmZp
ZGVudCBvZiB0aGUgaW50ZXJuYWwgZHJpdmVyIHF1YWxpdHkgYW5kIGdldCB0aGUgcmVsZWFzZSBw
ZXJmb3JtYW5jZS4NCg0KV0FSTl9PTiBpcyBtb3N0bHkgdXNlZCBmb3IgdGhlIGNhc2VzIHdoZW4g
dGhlIGhhcmR3YXJlIGlzIGJlaGF2aW5nIGRpZmZlcmVudGx5IHRoYW4gd2UgZXhwZWN0LiBXZSBj
YW4ndCByZW1vdmUgdGhlbSBhcyB3ZSBkb24ndCBoYXZlIGFsbCB0aGUgaGFyZHdhcmUgaW4gdGhl
IHdvcmxkIHRvIHRlc3QsIGJ1dCB3ZSB0cnkgdG8gZXhlcmNpc2UgdGhlbSB0b28gdGhyb3VnaCBJ
LUctVHMuIFRoZSB0ZXN0IHdpbGwgb2Z0ZW4gYmUgdGhlIHN1YnRlc3QgdGhhdCB3YXMgd3JpdHRl
biB0byByZXByb2R1Y2UgdGhlIHByb2JsZW0gd2l0aCBvdXIgZXhwZWN0YXRpb25zIG9mIGhhcmR3
YXJlIGluIGNhc2Ugb2YgaGFuZ3MgYW5kIG90aGVyIGJ1Z3MuIEFmdGVyIHdlJ3ZlIGNvcnJlY3Rl
ZCB0aGUgZHJpdmVyIGJlaGF2aW91ciwgb3IgZ290IGEgaGFyZHdhcmUgVy9BIGFzc2lnbmVkLCB3
ZSBrZWVwIHRoZSB0ZXN0IGFuZCBhZGQgYSBXQVJOX09OIHRvIG1ha2Ugc3VyZSB0aGVyZSB3aWxs
IGJlIG5vIHJlZ3Jlc3Npb24gYmFjayB0byB0aGUgc2FtZSBzaXR1YXRpb24uDQoNClRoaXMgaXMg
YXQgbGVhc3Qgd2hhdCBzaG91bGQgaGFwcGVuLCBnaXZlbiB0aW1lIGNvbnN0cmFpbnRzLCB0aGVy
ZSBtYXkgYmUgdmFyaWF0aW9ucy4NCg0KVXNlciBiZWhhdmluZyB1bmV4cGVjdGVkbHkgc2hvdWxk
IG5ldmVyIHJlc3VsdCBpbiBXQVJOX09OIChvciBldmVuIHdvcnNlLCBCVUdfT04pLCBzaG91bGQg
YWx3YXlzIGp1c3QgYmUgZGVidWcgbWVzc2FnZXMgZGlzcGxheWVkIChub3QgdG8gdHJpZ2dlciB0
aGUgQ0kpIGFuZCBlcnJvcnMgcHJvcGFnYXRlZCBiYWNrIHRvIHVzZXI6DQoNCmh0dHBzOi8vMDEu
b3JnL2xpbnV4Z3JhcGhpY3MvZ2Z4LWRvY3MvZHJtL2dwdS9kcm0tdWFwaS5odG1sI3JlY29tbWVu
ZGVkDQotaW9jdGwtcmV0dXJuLXZhbHVlcw0KDQpCYXJlIEJVR19PTiBzaG91bGQgb25seSBiZSB1
c2VkIHdoZW4gdGhlcmUncyB0aGUgZGFuZ2VyIG9mIGNvcnJ1cHRpbmcgc3lzdGVtIG1lbW9yeSBv
ciBmaWxlc3lzdGVtcywgc28gZnJvbSBncmFwaGljcyBkcml2ZXIsIHRoYXQncyBub3QgdmVyeSBv
ZnRlbi4gQ29udHJvbGxlZCBwcm9wYWdhdGlvbiBvZiBlcnJvcnMgYW5kIG1heWJlIFdBUk5fT04g
aXMgYWx3YXlzIHByZWZlcnJlZCBpZiBwb3NzaWJsZS4NCg0KDQo+IEdFTV9CVUdfT04gaXMgb25s
eSBlbmFibGVkIHdoZW4ga2VybmVsIGRlYnVnIGlzIGVuYWJsZWQsIHdoaWNoIG1vc3RseSANCj4g
aXMgZGlzYWJsZWQgaW4gYSBwcm9kdWN0aW9uIGtlcm5lbC4gSW4gdGhlIGNhc2Ugb2YgaTkxNSwg
SSdtIHN1cmUgaXQgDQo+IHdpbGwgYmUgZW5hYmxlZCBpbiBDSSB0ZXN0IHNvIHRoYXQgaXQgY2Fu
IGNhdGNoIGJyb2tlbiBjb2RlIHBhdGguDQo+IExvb2tpbmcgaW50byBHVlQtZywgdGhlIHNpbWls
YXIgc2NlbmFyaW8gaXMgd2UgZW5hYmxlIGl0IGluIFFBIHRlc3QuDQo+IA0KPiBMZXQncyBzYXkg
R0VNX0JVR19PTiBjYW4gZG8gaXRzIHdvcmsgdmVyeSB3ZWxsIGluIFFBIHRlc3QgYnV0IFFBIHRl
c3QgDQo+IGlzIG5vdCBmdWxseSBjb3ZlcmVkIGFsbCB0aGUgY29uZGl0aW9uLCB0aGVuIHNvbWV0
aGluZyBtaWdodCBiZSBzdGlsbCANCj4gYnJva2VuIHdoZW4gaXQgY29tZXMgdG8gdGhlIHByb2R1
Y3Rpb24ga2VybmVsIGZvciB1c2VyIGFuZCBHRU1fQlVHX09OIA0KPiB3aWxsIGJlIGRpc2FibGVk
IGFuZCB3aWxsIG5vdCBjYXRjaCB0aGF0LCBJIGd1ZXNzLg0KPiANCj4gVGhhdCdzIG15IGNvbmZ1
c2lvbiB3aGljaCBzY3JhdGNoZWQgbXkgbWluZCBkdXJpbmcgdGhlIGludmVzdGlnYXRpb246DQo+
IElmIEdFTV9CVUdfT04gaXMgbm90IGFsd2F5cyB3b3JraW5nLCB0aGVuIGl0IGxvb2tzIFdBUk5f
T04gc2hvdWxkIA0KPiBhbHdheXMgYmUgdXNlZC4uLi4gRXhwZWN0ZWQgdG8gbGVhcm4gbW9yZSBh
Ym91dCB0aGUgc3RvcnkgYmVoaW5kLiA6KQ0KDQpTbyBpZiB0aGUgc2F5aW5nIGlzIHNvbWUgb2Jq
ZWN0IGlzICJuZXZlciBnb2luZyB0byBiZSBiaWdnZXIgdGhhbiAyRyIsIHRoZXJlIHNob3VsZCBi
ZSBlaXRoZXI6DQoNCjEuIEdFTV9CVUdfT04gbGlrZSBhc3NlcnRpb24gZm9yIGl0IGFuZCBhIHRl
c3QgdGhhdCB0cmllcyB0byBoaXQgaXQsIGJ5IHRyeWluZyB0byBhbGxvY2F0ZSBhIGh1Z2Ugb2Jq
ZWN0IGZvciBleGFtcGxlLCBhbmQgc2hvdWxkIGdldCByZWplY3Rpb24gYXMgLUVJTlZBTA0KDQoy
LiBUZXN0IHRvIHNlZSBpZiB0aGUgb2JqZWN0IGlzIGJpZ2dlciwgYW5kIHByb3BhZ2F0ZSBiYWNr
IHRoZSBlcnJvciBpZiBpdCBpcy4gRWl0aGVyIHJlc3VsdGluZyBpbiB1c2VyIHJlcG9ydGVkIGVy
cm9yIGlmIHRoZSBvcmlnaW4gb2YgdGhlIG9iamVjdCBpcyBvdXRzaWRlIG9mIGtlcm5lbCA8LT4g
aGFyZHdhcmUuIE9yIGEgV0FSTl9PTiBpZiBpdCdzIHN0cmFuZ2UgaGFyZHdhcmUgb3Iga2VybmVs
IGRyaXZlciBiZWhhdmlvci4NCg0KWW91IHNob3VsZCBjaG9vc2UgZGVwZW5kaW5nIG9uIGhvdyBv
ZnRlbiB5b3VyIGZ1bmN0aW9uIGdldHMgY2FsbGVkLCBhbmQgaG93IGNyaXRpY2FsIHRoZSBleGVj
dXRpb24gdGltZSBpcy4NCg0KSG9wZWZ1bGx5IHRoaXMgY2xhcmlmaWVkIHRoaW5ncy4NCg0KUmVn
YXJkcywgSm9vbmFzDQotLQ0KSm9vbmFzIExhaHRpbmVuDQpPcGVuIFNvdXJjZSBUZWNobm9sb2d5
IENlbnRlcg0KSW50ZWwgQ29ycG9yYXRpb24NCg=
^ permalink raw reply [flat|nested] 28+ 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
0 siblings, 0 replies; 28+ 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,
Jani Nikula, 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
^ permalink raw reply [flat|nested] 28+ 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
0 siblings, 0 replies; 28+ 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] 28+ 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
-1 siblings, 0 replies; 28+ 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] 28+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-25 9:32 ` Joonas Lahtinen
0 siblings, 0 replies; 28+ 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] 28+ messages in thread