* [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb @ 2015-06-23 14:26 Imre Deak 2015-06-23 14:31 ` Chris Wilson 0 siblings, 1 reply; 10+ messages in thread From: Imre Deak @ 2015-06-23 14:26 UTC (permalink / raw) To: intel-gfx On the GEN!=8 error path we call kmap_atomic() which returns in atomic context and then lrc_destroy_wa_ctx_obj() which can be called only in process context. Fix this by preserving the correct cleanup order on this error path. Also convert the WARN to DRM_ERROR the stack trace isn't really useful. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 1b50dd7..8bff1a2 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) if (ret) goto out; } else { - WARN(INTEL_INFO(ring->dev)->gen >= 8, - "WA batch buffer is not initialized for Gen%d\n", - INTEL_INFO(ring->dev)->gen); + if (INTEL_INFO(ring->dev)->gen >= 8) + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", + INTEL_INFO(ring->dev)->gen); + + kunmap_atomic(batch); lrc_destroy_wa_ctx_obj(ring); + + return 0; } out: -- 2.1.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 14:26 [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb Imre Deak @ 2015-06-23 14:31 ` Chris Wilson 2015-06-23 14:36 ` Imre Deak 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2015-06-23 14:31 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: > On the GEN!=8 error path we call kmap_atomic() which returns in atomic > context and then lrc_destroy_wa_ctx_obj() which can be called only in > process context. Fix this by preserving the correct cleanup order on > this error path. > > Also convert the WARN to DRM_ERROR the stack trace isn't really useful. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 1b50dd7..8bff1a2 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) > if (ret) > goto out; > } else { > - WARN(INTEL_INFO(ring->dev)->gen >= 8, > - "WA batch buffer is not initialized for Gen%d\n", > - INTEL_INFO(ring->dev)->gen); > + if (INTEL_INFO(ring->dev)->gen >= 8) > + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", > + INTEL_INFO(ring->dev)->gen); > + Do this test upfront, then we don't have multiple error paths. http://paste.debian.net/255769 -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 14:31 ` Chris Wilson @ 2015-06-23 14:36 ` Imre Deak 2015-06-23 15:13 ` Siluvery, Arun 0 siblings, 1 reply; 10+ messages in thread From: Imre Deak @ 2015-06-23 14:36 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On ti, 2015-06-23 at 15:31 +0100, Chris Wilson wrote: > On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: > > On the GEN!=8 error path we call kmap_atomic() which returns in atomic > > context and then lrc_destroy_wa_ctx_obj() which can be called only in > > process context. Fix this by preserving the correct cleanup order on > > this error path. > > > > Also convert the WARN to DRM_ERROR the stack trace isn't really useful. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index 1b50dd7..8bff1a2 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) > > if (ret) > > goto out; > > } else { > > - WARN(INTEL_INFO(ring->dev)->gen >= 8, > > - "WA batch buffer is not initialized for Gen%d\n", > > - INTEL_INFO(ring->dev)->gen); > > + if (INTEL_INFO(ring->dev)->gen >= 8) > > + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", > > + INTEL_INFO(ring->dev)->gen); > > + > > Do this test upfront, then we don't have multiple error paths. > http://paste.debian.net/255769 I didn't bother moving it, I suppose GEN9 support will be added soon anyway and we get a bit more test coverage on GEN9 meanwhile. But if you insist I can move it. > -Chris > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 14:36 ` Imre Deak @ 2015-06-23 15:13 ` Siluvery, Arun 2015-06-23 15:18 ` Imre Deak 0 siblings, 1 reply; 10+ messages in thread From: Siluvery, Arun @ 2015-06-23 15:13 UTC (permalink / raw) To: intel-gfx On 23/06/2015 15:36, Imre Deak wrote: > On ti, 2015-06-23 at 15:31 +0100, Chris Wilson wrote: >> On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: >>> On the GEN!=8 error path we call kmap_atomic() which returns in atomic >>> context and then lrc_destroy_wa_ctx_obj() which can be called only in >>> process context. Fix this by preserving the correct cleanup order on >>> this error path. >>> >>> Also convert the WARN to DRM_ERROR the stack trace isn't really useful. >>> >>> Signed-off-by: Imre Deak <imre.deak@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>> index 1b50dd7..8bff1a2 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) >>> if (ret) >>> goto out; >>> } else { >>> - WARN(INTEL_INFO(ring->dev)->gen >= 8, >>> - "WA batch buffer is not initialized for Gen%d\n", >>> - INTEL_INFO(ring->dev)->gen); >>> + if (INTEL_INFO(ring->dev)->gen >= 8) >>> + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", >>> + INTEL_INFO(ring->dev)->gen); >>> + >> >> Do this test upfront, then we don't have multiple error paths. >> http://paste.debian.net/255769 > > I didn't bother moving it, I suppose GEN9 support will be added soon > anyway and we get a bit more test coverage on GEN9 meanwhile. But if you > insist I can move it. Hi Imre, I sent the following patch with the changes suggested by Chris. https://patchwork.kernel.org/patch/6661891/ Since you sent it first, my patch can be ignored if your patch is updated. regards Arun > >> -Chris >> > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 15:13 ` Siluvery, Arun @ 2015-06-23 15:18 ` Imre Deak 2015-06-23 15:44 ` Chris Wilson 0 siblings, 1 reply; 10+ messages in thread From: Imre Deak @ 2015-06-23 15:18 UTC (permalink / raw) To: Siluvery, Arun; +Cc: intel-gfx On ti, 2015-06-23 at 16:13 +0100, Siluvery, Arun wrote: > On 23/06/2015 15:36, Imre Deak wrote: > > On ti, 2015-06-23 at 15:31 +0100, Chris Wilson wrote: > >> On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: > >>> On the GEN!=8 error path we call kmap_atomic() which returns in atomic > >>> context and then lrc_destroy_wa_ctx_obj() which can be called only in > >>> process context. Fix this by preserving the correct cleanup order on > >>> this error path. > >>> > >>> Also convert the WARN to DRM_ERROR the stack trace isn't really useful. > >>> > >>> Signed-off-by: Imre Deak <imre.deak@intel.com> > >>> --- > >>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- > >>> 1 file changed, 7 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >>> index 1b50dd7..8bff1a2 100644 > >>> --- a/drivers/gpu/drm/i915/intel_lrc.c > >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c > >>> @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) > >>> if (ret) > >>> goto out; > >>> } else { > >>> - WARN(INTEL_INFO(ring->dev)->gen >= 8, > >>> - "WA batch buffer is not initialized for Gen%d\n", > >>> - INTEL_INFO(ring->dev)->gen); > >>> + if (INTEL_INFO(ring->dev)->gen >= 8) > >>> + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", > >>> + INTEL_INFO(ring->dev)->gen); > >>> + > >> > >> Do this test upfront, then we don't have multiple error paths. > >> http://paste.debian.net/255769 > > > > I didn't bother moving it, I suppose GEN9 support will be added soon > > anyway and we get a bit more test coverage on GEN9 meanwhile. But if you > > insist I can move it. > > Hi Imre, > > I sent the following patch with the changes suggested by Chris. > https://patchwork.kernel.org/patch/6661891/ > Since you sent it first, my patch can be ignored if your patch is updated. I'm fine applying your patch, but I would ask to convert the WARN to DRM_ERROR. The stack trace doesn't add much to the error message and the WARN is needlessly verbose now on BXT,SKL.. --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 15:18 ` Imre Deak @ 2015-06-23 15:44 ` Chris Wilson 2015-06-23 15:58 ` Imre Deak 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2015-06-23 15:44 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Tue, Jun 23, 2015 at 06:18:21PM +0300, Imre Deak wrote: > On ti, 2015-06-23 at 16:13 +0100, Siluvery, Arun wrote: > > On 23/06/2015 15:36, Imre Deak wrote: > > > On ti, 2015-06-23 at 15:31 +0100, Chris Wilson wrote: > > >> On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: > > >>> On the GEN!=8 error path we call kmap_atomic() which returns in atomic > > >>> context and then lrc_destroy_wa_ctx_obj() which can be called only in > > >>> process context. Fix this by preserving the correct cleanup order on > > >>> this error path. > > >>> > > >>> Also convert the WARN to DRM_ERROR the stack trace isn't really useful. > > >>> > > >>> Signed-off-by: Imre Deak <imre.deak@intel.com> > > >>> --- > > >>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- > > >>> 1 file changed, 7 insertions(+), 3 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > >>> index 1b50dd7..8bff1a2 100644 > > >>> --- a/drivers/gpu/drm/i915/intel_lrc.c > > >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c > > >>> @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) > > >>> if (ret) > > >>> goto out; > > >>> } else { > > >>> - WARN(INTEL_INFO(ring->dev)->gen >= 8, > > >>> - "WA batch buffer is not initialized for Gen%d\n", > > >>> - INTEL_INFO(ring->dev)->gen); > > >>> + if (INTEL_INFO(ring->dev)->gen >= 8) > > >>> + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", > > >>> + INTEL_INFO(ring->dev)->gen); > > >>> + > > >> > > >> Do this test upfront, then we don't have multiple error paths. > > >> http://paste.debian.net/255769 > > > > > > I didn't bother moving it, I suppose GEN9 support will be added soon > > > anyway and we get a bit more test coverage on GEN9 meanwhile. But if you > > > insist I can move it. > > > > Hi Imre, > > > > I sent the following patch with the changes suggested by Chris. > > https://patchwork.kernel.org/patch/6661891/ > > Since you sent it first, my patch can be ignored if your patch is updated. > > I'm fine applying your patch, but I would ask to convert the WARN to > DRM_ERROR. The stack trace doesn't add much to the error message and the > WARN is needlessly verbose now on BXT,SKL.. I presumed Arun choose WARN because we are missing w/a and wanted someone to step forward and prove the fixes? -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 15:44 ` Chris Wilson @ 2015-06-23 15:58 ` Imre Deak 2015-06-23 16:01 ` Chris Wilson 0 siblings, 1 reply; 10+ messages in thread From: Imre Deak @ 2015-06-23 15:58 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On ti, 2015-06-23 at 16:44 +0100, Chris Wilson wrote: > On Tue, Jun 23, 2015 at 06:18:21PM +0300, Imre Deak wrote: > > On ti, 2015-06-23 at 16:13 +0100, Siluvery, Arun wrote: > > > On 23/06/2015 15:36, Imre Deak wrote: > > > > On ti, 2015-06-23 at 15:31 +0100, Chris Wilson wrote: > > > >> On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: > > > >>> On the GEN!=8 error path we call kmap_atomic() which returns in atomic > > > >>> context and then lrc_destroy_wa_ctx_obj() which can be called only in > > > >>> process context. Fix this by preserving the correct cleanup order on > > > >>> this error path. > > > >>> > > > >>> Also convert the WARN to DRM_ERROR the stack trace isn't really useful. > > > >>> > > > >>> Signed-off-by: Imre Deak <imre.deak@intel.com> > > > >>> --- > > > >>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- > > > >>> 1 file changed, 7 insertions(+), 3 deletions(-) > > > >>> > > > >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > > >>> index 1b50dd7..8bff1a2 100644 > > > >>> --- a/drivers/gpu/drm/i915/intel_lrc.c > > > >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c > > > >>> @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) > > > >>> if (ret) > > > >>> goto out; > > > >>> } else { > > > >>> - WARN(INTEL_INFO(ring->dev)->gen >= 8, > > > >>> - "WA batch buffer is not initialized for Gen%d\n", > > > >>> - INTEL_INFO(ring->dev)->gen); > > > >>> + if (INTEL_INFO(ring->dev)->gen >= 8) > > > >>> + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", > > > >>> + INTEL_INFO(ring->dev)->gen); > > > >>> + > > > >> > > > >> Do this test upfront, then we don't have multiple error paths. > > > >> http://paste.debian.net/255769 > > > > > > > > I didn't bother moving it, I suppose GEN9 support will be added soon > > > > anyway and we get a bit more test coverage on GEN9 meanwhile. But if you > > > > insist I can move it. > > > > > > Hi Imre, > > > > > > I sent the following patch with the changes suggested by Chris. > > > https://patchwork.kernel.org/patch/6661891/ > > > Since you sent it first, my patch can be ignored if your patch is updated. > > > > I'm fine applying your patch, but I would ask to convert the WARN to > > DRM_ERROR. The stack trace doesn't add much to the error message and the > > WARN is needlessly verbose now on BXT,SKL.. > > I presumed Arun choose WARN because we are missing w/a and wanted > someone to step forward and prove the fixes? Imo it's unnecessarily verbose, during development when loading the driver I know that things are mostly ok if I can't see any such backtraces. But no strong opinion, I can also change this locally. --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 15:58 ` Imre Deak @ 2015-06-23 16:01 ` Chris Wilson 2015-06-23 16:21 ` Siluvery, Arun 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2015-06-23 16:01 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Tue, Jun 23, 2015 at 06:58:42PM +0300, Imre Deak wrote: > On ti, 2015-06-23 at 16:44 +0100, Chris Wilson wrote: > > On Tue, Jun 23, 2015 at 06:18:21PM +0300, Imre Deak wrote: > > > On ti, 2015-06-23 at 16:13 +0100, Siluvery, Arun wrote: > > > > On 23/06/2015 15:36, Imre Deak wrote: > > > > > On ti, 2015-06-23 at 15:31 +0100, Chris Wilson wrote: > > > > >> On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: > > > > >>> On the GEN!=8 error path we call kmap_atomic() which returns in atomic > > > > >>> context and then lrc_destroy_wa_ctx_obj() which can be called only in > > > > >>> process context. Fix this by preserving the correct cleanup order on > > > > >>> this error path. > > > > >>> > > > > >>> Also convert the WARN to DRM_ERROR the stack trace isn't really useful. > > > > >>> > > > > >>> Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > >>> --- > > > > >>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- > > > > >>> 1 file changed, 7 insertions(+), 3 deletions(-) > > > > >>> > > > > >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > > > >>> index 1b50dd7..8bff1a2 100644 > > > > >>> --- a/drivers/gpu/drm/i915/intel_lrc.c > > > > >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c > > > > >>> @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) > > > > >>> if (ret) > > > > >>> goto out; > > > > >>> } else { > > > > >>> - WARN(INTEL_INFO(ring->dev)->gen >= 8, > > > > >>> - "WA batch buffer is not initialized for Gen%d\n", > > > > >>> - INTEL_INFO(ring->dev)->gen); > > > > >>> + if (INTEL_INFO(ring->dev)->gen >= 8) > > > > >>> + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", > > > > >>> + INTEL_INFO(ring->dev)->gen); > > > > >>> + > > > > >> > > > > >> Do this test upfront, then we don't have multiple error paths. > > > > >> http://paste.debian.net/255769 > > > > > > > > > > I didn't bother moving it, I suppose GEN9 support will be added soon > > > > > anyway and we get a bit more test coverage on GEN9 meanwhile. But if you > > > > > insist I can move it. > > > > > > > > Hi Imre, > > > > > > > > I sent the following patch with the changes suggested by Chris. > > > > https://patchwork.kernel.org/patch/6661891/ > > > > Since you sent it first, my patch can be ignored if your patch is updated. > > > > > > I'm fine applying your patch, but I would ask to convert the WARN to > > > DRM_ERROR. The stack trace doesn't add much to the error message and the > > > WARN is needlessly verbose now on BXT,SKL.. > > > > I presumed Arun choose WARN because we are missing w/a and wanted > > someone to step forward and prove the fixes? > > Imo it's unnecessarily verbose, during development when loading the > driver I know that things are mostly ok if I can't see any such > backtraces. But no strong opinion, I can also change this locally. An alternative would be to provide the stub wa_bb emission functions so that future wa need only start with a plain copy'n'paste. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 16:01 ` Chris Wilson @ 2015-06-23 16:21 ` Siluvery, Arun 2015-06-23 21:13 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Siluvery, Arun @ 2015-06-23 16:21 UTC (permalink / raw) To: Chris Wilson, Imre Deak, intel-gfx On 23/06/2015 17:01, Chris Wilson wrote: > On Tue, Jun 23, 2015 at 06:58:42PM +0300, Imre Deak wrote: >> On ti, 2015-06-23 at 16:44 +0100, Chris Wilson wrote: >>> On Tue, Jun 23, 2015 at 06:18:21PM +0300, Imre Deak wrote: >>>> On ti, 2015-06-23 at 16:13 +0100, Siluvery, Arun wrote: >>>>> On 23/06/2015 15:36, Imre Deak wrote: >>>>>> On ti, 2015-06-23 at 15:31 +0100, Chris Wilson wrote: >>>>>>> On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: >>>>>>>> On the GEN!=8 error path we call kmap_atomic() which returns in atomic >>>>>>>> context and then lrc_destroy_wa_ctx_obj() which can be called only in >>>>>>>> process context. Fix this by preserving the correct cleanup order on >>>>>>>> this error path. >>>>>>>> >>>>>>>> Also convert the WARN to DRM_ERROR the stack trace isn't really useful. >>>>>>>> >>>>>>>> Signed-off-by: Imre Deak <imre.deak@intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- >>>>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>>>>>>> index 1b50dd7..8bff1a2 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>>>>>>> @@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) >>>>>>>> if (ret) >>>>>>>> goto out; >>>>>>>> } else { >>>>>>>> - WARN(INTEL_INFO(ring->dev)->gen >= 8, >>>>>>>> - "WA batch buffer is not initialized for Gen%d\n", >>>>>>>> - INTEL_INFO(ring->dev)->gen); >>>>>>>> + if (INTEL_INFO(ring->dev)->gen >= 8) >>>>>>>> + DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", >>>>>>>> + INTEL_INFO(ring->dev)->gen); >>>>>>>> + >>>>>>> >>>>>>> Do this test upfront, then we don't have multiple error paths. >>>>>>> http://paste.debian.net/255769 >>>>>> >>>>>> I didn't bother moving it, I suppose GEN9 support will be added soon >>>>>> anyway and we get a bit more test coverage on GEN9 meanwhile. But if you >>>>>> insist I can move it. >>>>> >>>>> Hi Imre, >>>>> >>>>> I sent the following patch with the changes suggested by Chris. >>>>> https://patchwork.kernel.org/patch/6661891/ >>>>> Since you sent it first, my patch can be ignored if your patch is updated. >>>> >>>> I'm fine applying your patch, but I would ask to convert the WARN to >>>> DRM_ERROR. The stack trace doesn't add much to the error message and the >>>> WARN is needlessly verbose now on BXT,SKL.. >>> >>> I presumed Arun choose WARN because we are missing w/a and wanted >>> someone to step forward and prove the fixes? >> >> Imo it's unnecessarily verbose, during development when loading the >> driver I know that things are mostly ok if I can't see any such >> backtraces. But no strong opinion, I can also change this locally. Error message can easily get lost and also it is not an error to not apply these WA which is why we also continue. I thought WARN will probably get more attention and help in adding missing WA quickly. regards Arun > > An alternative would be to provide the stub wa_bb emission functions so > that future wa need only start with a plain copy'n'paste. > -Chris > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb 2015-06-23 16:21 ` Siluvery, Arun @ 2015-06-23 21:13 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2015-06-23 21:13 UTC (permalink / raw) To: Siluvery, Arun; +Cc: intel-gfx On Tue, Jun 23, 2015 at 05:21:16PM +0100, Siluvery, Arun wrote: > On 23/06/2015 17:01, Chris Wilson wrote: > >On Tue, Jun 23, 2015 at 06:58:42PM +0300, Imre Deak wrote: > >>On ti, 2015-06-23 at 16:44 +0100, Chris Wilson wrote: > >>>On Tue, Jun 23, 2015 at 06:18:21PM +0300, Imre Deak wrote: > >>>>On ti, 2015-06-23 at 16:13 +0100, Siluvery, Arun wrote: > >>>>>On 23/06/2015 15:36, Imre Deak wrote: > >>>>>>On ti, 2015-06-23 at 15:31 +0100, Chris Wilson wrote: > >>>>>>>On Tue, Jun 23, 2015 at 05:26:13PM +0300, Imre Deak wrote: > >>>>>>>>On the GEN!=8 error path we call kmap_atomic() which returns in atomic > >>>>>>>>context and then lrc_destroy_wa_ctx_obj() which can be called only in > >>>>>>>>process context. Fix this by preserving the correct cleanup order on > >>>>>>>>this error path. > >>>>>>>> > >>>>>>>>Also convert the WARN to DRM_ERROR the stack trace isn't really useful. > >>>>>>>> > >>>>>>>>Signed-off-by: Imre Deak <imre.deak@intel.com> > >>>>>>>>--- > >>>>>>>> drivers/gpu/drm/i915/intel_lrc.c | 10 +++++++--- > >>>>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) > >>>>>>>> > >>>>>>>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >>>>>>>>index 1b50dd7..8bff1a2 100644 > >>>>>>>>--- a/drivers/gpu/drm/i915/intel_lrc.c > >>>>>>>>+++ b/drivers/gpu/drm/i915/intel_lrc.c > >>>>>>>>@@ -1289,10 +1289,14 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring) > >>>>>>>> if (ret) > >>>>>>>> goto out; > >>>>>>>> } else { > >>>>>>>>- WARN(INTEL_INFO(ring->dev)->gen >= 8, > >>>>>>>>- "WA batch buffer is not initialized for Gen%d\n", > >>>>>>>>- INTEL_INFO(ring->dev)->gen); > >>>>>>>>+ if (INTEL_INFO(ring->dev)->gen >= 8) > >>>>>>>>+ DRM_ERROR("WA batch buffer is not initialized for Gen%d\n", > >>>>>>>>+ INTEL_INFO(ring->dev)->gen); > >>>>>>>>+ > >>>>>>> > >>>>>>>Do this test upfront, then we don't have multiple error paths. > >>>>>>>http://paste.debian.net/255769 > >>>>>> > >>>>>>I didn't bother moving it, I suppose GEN9 support will be added soon > >>>>>>anyway and we get a bit more test coverage on GEN9 meanwhile. But if you > >>>>>>insist I can move it. > >>>>> > >>>>>Hi Imre, > >>>>> > >>>>>I sent the following patch with the changes suggested by Chris. > >>>>>https://patchwork.kernel.org/patch/6661891/ > >>>>>Since you sent it first, my patch can be ignored if your patch is updated. > >>>> > >>>>I'm fine applying your patch, but I would ask to convert the WARN to > >>>>DRM_ERROR. The stack trace doesn't add much to the error message and the > >>>>WARN is needlessly verbose now on BXT,SKL.. > >>> > >>>I presumed Arun choose WARN because we are missing w/a and wanted > >>>someone to step forward and prove the fixes? > >> > >>Imo it's unnecessarily verbose, during development when loading the > >>driver I know that things are mostly ok if I can't see any such > >>backtraces. But no strong opinion, I can also change this locally. > > Error message can easily get lost and also it is not an error to not apply > these WA which is why we also continue. I thought WARN will probably get > more attention and help in adding missing WA quickly. Friendly inquiry from your maintainer: Can we perhaps not bikeshed the benefits of DRM_ERROR vs. WARN_ON and just get this reviewed&merged? From an igt/piglit pov both cause a test failure and so are equivalent. And I'm hopeful that the new QA team actually catches dmesg fallout reliable rsn. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-06-23 21:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-06-23 14:26 [PATCH] drm/i915/gen9: fix error path in intel_init_workaround_bb Imre Deak 2015-06-23 14:31 ` Chris Wilson 2015-06-23 14:36 ` Imre Deak 2015-06-23 15:13 ` Siluvery, Arun 2015-06-23 15:18 ` Imre Deak 2015-06-23 15:44 ` Chris Wilson 2015-06-23 15:58 ` Imre Deak 2015-06-23 16:01 ` Chris Wilson 2015-06-23 16:21 ` Siluvery, Arun 2015-06-23 21:13 ` Daniel Vetter
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.