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

From: Lucas De Marchi <lucas.demarchi@intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/i915/dsb: fix extra warning on error path handling
Date: Fri, 15 Nov 2019 15:04:07 -0800	[thread overview]
Message-ID: <20191115230407.wcywmy2coeks7gta@ldmartin-desk1> (raw)
In-Reply-To: <20191115205249.GA21445@mdroper-desk1.amr.corp.intel.com>

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();

>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