[Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Manna, Animesh]

On 31-01-2020 17:12, Ville Syrjälä wrote:
> On Fri, Jan 31, 2020 at 03:04:17PM +0530, Manna, Animesh wrote:
>> On 30-01-2020 23:43, Souza, Jose wrote:
>>> On Wed, 2020-01-29 at 20:20 +0200, Ville Syrjala wrote:
>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>
>>>> We may want to not use the DSB even if the platform has one.
>>>> So replace the HAS_DSB check in the _put() with a cmd_buf check
>>>> that will work in either case.
>>> Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
>>>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> index 9dd18144a664..12776f09f227 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> @@ -160,9 +160,8 @@ intel_dsb_get(struct intel_crtc *crtc)
>>>>    void intel_dsb_put(struct intel_dsb *dsb)
>>>>    {
>>>>    	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc),
>>>> dsb);
>>>> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>>>>    
>>>> -	if (!HAS_DSB(i915))
>>>> +	if (!dsb->cmd_buf)
>> Ville and Jose,
>>
>> Have a concern here. In intel_dsb_get() if get failure during i915_gem_object_create_internal, i915_gem_object_ggtt_pin, i915_gem_object_pin_map then we may not have dsb->cmd_buf.
>> Then ref-count mechanism will break.
> Hmm. Yeah. The refcount WARN could easily be fixed by either
> decrementung refcount on get() fail or doing the "let's never use
> DSB" patch after the refcount inc.

Hmm, from design point get/put/ref-count mechanism introduced to check dsp-api are used properly or not.
For erroneous case managing ref-count in get() itself void the purpose of put() call.
For example,

intel_dsb_get()
got error from i915_gem_object_create_internal, i915_gem_object_ggtt_pin, i915_gem_object_pin_map
intel_dsb_put
intel_dsb_put
...

Should throw warning but can not if we manage in get() itself.

Regards,
Animesh

>
>> I feel HAS_DSB(i915) check is better than dsb->cmd_buf otherwise need to do some cleanup is intel_dsb_get() as well.
>>
>> Regards,
>> Animesh
>>
>>>>    		return;
>>>>    
>>>>    	if (WARN_ON(dsb->refcount == 0))
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx