| Message ID | 20191111205024.22853-3-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve error handling on DSB | expand |
On Mon, Nov 11, 2019 at 12:50:25PM -0800, Lucas De Marchi wrote: > When we call intel_dsb_get(), the dsb initialization may fail for > various reasons. We already log the error message in that path, making > it unnecessary to trigger a warning that refcount == 0 when calling > intel_dsb_put(). > > So here we simplify the logic and do lazy shutdown: leaving the extra > refcount alive so when we call intel_dsb_put() we end up calling > i915_vma_unpin_and_release(). > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Due to the lack of any actual concurrency, it seems like we could eventually get rid of the whole get/put design and just allocate the buffer once (and pin it during the prepare step). But this seems good enough for now. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dsb.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c > index 4feebbeb0b0c..858af6be9c36 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > @@ -102,6 +102,7 @@ intel_dsb_get(struct intel_crtc *crtc) > struct intel_dsb *dsb = &crtc->dsb; > struct drm_i915_gem_object *obj; > struct i915_vma *vma; > + u32 *buf; > intel_wakeref_t wakeref; > > if (!HAS_DSB(i915)) > @@ -110,7 +111,6 @@ intel_dsb_get(struct intel_crtc *crtc) > if (++dsb->refcount != 1) > return dsb; > > - dsb->id = DSB1; > wakeref = intel_runtime_pm_get(&i915->runtime_pm); > > obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE); > @@ -123,22 +123,29 @@ intel_dsb_get(struct intel_crtc *crtc) > if (IS_ERR(vma)) { > DRM_ERROR("Vma creation failed\n"); > i915_gem_object_put(obj); > - dsb->refcount--; > goto err; > } > > - dsb->cmd_buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC); > - if (IS_ERR(dsb->cmd_buf)) { > + buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC); > + if (IS_ERR(buf)) { > DRM_ERROR("Command buffer creation failed\n"); > - i915_vma_unpin_and_release(&vma, 0); > - dsb->cmd_buf = NULL; > - dsb->refcount--; > goto err; > } > + > + dsb->id = DSB1; > dsb->vma = vma; > + dsb->cmd_buf = buf; > > err: > + /* > + * Set cmd_buf to NULL so the writes pass-through, but leave the > + * dangling refcount to be removed later by the corresponding > + * intel_dsb_put(): the important error message will already be > + * logged above. > + */ > + dsb->cmd_buf = NULL; > intel_runtime_pm_put(&i915->runtime_pm, wakeref); > + > return dsb; > } > > -- > 2.24.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 15 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Fri, Nov 15, 2019 at 12:52:49PM -0800, Matt Roper wrote: >>On Mon, Nov 11, 2019 at 12:50:25PM -0800, Lucas De Marchi wrote: >>> When we call intel_dsb_get(), the dsb initialization may fail for >>> various reasons. We already log the error message in that path, making >>> it unnecessary to trigger a warning that refcount == 0 when calling >>> intel_dsb_put(). >>> >>> So here we simplify the logic and do lazy shutdown: leaving the extra >>> refcount alive so when we call intel_dsb_put() we end up calling >>> i915_vma_unpin_and_release(). >>> >>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> >>Due to the lack of any actual concurrency, it seems like we could >>eventually get rid of the whole get/put design and just allocate the >>buffer once (and pin it during the prepare step). But this seems good > > I assumed this was designed to accept the pattern > > intel_dsb_get(); > intel_dsb_get(); > intel_dsb_put(); > intel_dsb_put(); Yeah it wasn't necessarily for concurrency. More to ensure the buffer doesn't vanish under the engine. BR, Jani. > >>enough for now. >> >>Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > thanks > Lucas De Marchi > >> >> >>> --- >>> drivers/gpu/drm/i915/display/intel_dsb.c | 21 ++++++++++++++------- >>> 1 file changed, 14 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c >>> index 4feebbeb0b0c..858af6be9c36 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c >>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c >>> @@ -102,6 +102,7 @@ intel_dsb_get(struct intel_crtc *crtc) >>> struct intel_dsb *dsb = &crtc->dsb; >>> struct drm_i915_gem_object *obj; >>> struct i915_vma *vma; >>> + u32 *buf; >>> intel_wakeref_t wakeref; >>> >>> if (!HAS_DSB(i915)) >>> @@ -110,7 +111,6 @@ intel_dsb_get(struct intel_crtc *crtc) >>> if (++dsb->refcount != 1) >>> return dsb; >>> >>> - dsb->id = DSB1; >>> wakeref = intel_runtime_pm_get(&i915->runtime_pm); >>> >>> obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE); >>> @@ -123,22 +123,29 @@ intel_dsb_get(struct intel_crtc *crtc) >>> if (IS_ERR(vma)) { >>> DRM_ERROR("Vma creation failed\n"); >>> i915_gem_object_put(obj); >>> - dsb->refcount--; >>> goto err; >>> } >>> >>> - dsb->cmd_buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC); >>> - if (IS_ERR(dsb->cmd_buf)) { >>> + buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC); >>> + if (IS_ERR(buf)) { >>> DRM_ERROR("Command buffer creation failed\n"); >>> - i915_vma_unpin_and_release(&vma, 0); >>> - dsb->cmd_buf = NULL; >>> - dsb->refcount--; >>> goto err; >>> } >>> + >>> + dsb->id = DSB1; >>> dsb->vma = vma; >>> + dsb->cmd_buf = buf; >>> >>> err: >>> + /* >>> + * Set cmd_buf to NULL so the writes pass-through, but leave the >>> + * dangling refcount to be removed later by the corresponding >>> + * intel_dsb_put(): the important error message will already be >>> + * logged above. >>> + */ >>> + dsb->cmd_buf = NULL; >>> intel_runtime_pm_put(&i915->runtime_pm, wakeref); >>> + >>> return dsb; >>> } >>> >>> -- >>> 2.24.0 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >>-- >>Matt Roper >>Graphics Software Engineer >>VTT-OSGC Platform Enablement >>Intel Corporation >>(916) 356-2795 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c index 4feebbeb0b0c..858af6be9c36 100644 --- a/drivers/gpu/drm/i915/display/intel_dsb.c +++ b/drivers/gpu/drm/i915/display/intel_dsb.c @@ -102,6 +102,7 @@ intel_dsb_get(struct intel_crtc *crtc) struct intel_dsb *dsb = &crtc->dsb; struct drm_i915_gem_object *obj; struct i915_vma *vma; + u32 *buf; intel_wakeref_t wakeref; if (!HAS_DSB(i915)) @@ -110,7 +111,6 @@ intel_dsb_get(struct intel_crtc *crtc) if (++dsb->refcount != 1) return dsb; - dsb->id = DSB1; wakeref = intel_runtime_pm_get(&i915->runtime_pm); obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE); @@ -123,22 +123,29 @@ intel_dsb_get(struct intel_crtc *crtc) if (IS_ERR(vma)) { DRM_ERROR("Vma creation failed\n"); i915_gem_object_put(obj); - dsb->refcount--; goto err; } - dsb->cmd_buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC); - if (IS_ERR(dsb->cmd_buf)) { + buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC); + if (IS_ERR(buf)) { DRM_ERROR("Command buffer creation failed\n"); - i915_vma_unpin_and_release(&vma, 0); - dsb->cmd_buf = NULL; - dsb->refcount--; goto err; } + + dsb->id = DSB1; dsb->vma = vma; + dsb->cmd_buf = buf; err: + /* + * Set cmd_buf to NULL so the writes pass-through, but leave the + * dangling refcount to be removed later by the corresponding + * intel_dsb_put(): the important error message will already be + * logged above. + */ + dsb->cmd_buf = NULL; intel_runtime_pm_put(&i915->runtime_pm, wakeref); + return dsb; }
When we call intel_dsb_get(), the dsb initialization may fail for various reasons. We already log the error message in that path, making it unnecessary to trigger a warning that refcount == 0 when calling intel_dsb_put(). So here we simplify the logic and do lazy shutdown: leaving the extra refcount alive so when we call intel_dsb_put() we end up calling i915_vma_unpin_and_release(). Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/display/intel_dsb.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)