Re: [PATCH 2/2] drm/i915/dsb: fix extra warning on error path handling

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx