Re: [Intel-gfx] [PATCH] drm/i915/dsb: modified to drm_info in dsb_prepare()

["Das, Nirmoy" <nirmoy.das@linux.intel.com>]

On 3/25/2022 2:22 PM, Manna, Animesh wrote:
>
>> -----Original Message-----
>> From: Das, Nirmoy <nirmoy.das@linux.intel.com>
>> Sent: Thursday, March 24, 2022 2:39 PM
>> To: Manna, Animesh <animesh.manna@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dsb: modified to drm_info in
>> dsb_prepare()
>>
>>
>> On 3/24/2022 8:43 AM, Animesh Manna wrote:
>>> The request to aqquire gem resources is failing for DSB in rare
>>> scenario where it is busy and the register programming will be done
>>> through mmio fallback path.
>>>
>>> DSB has extra advantage of faster register programming which may go
>>> away through mmio path. Adding wait for gem resource also may not be
>>> right as anyways losing time.
>>>
>>> To make the CI execution happy replaced drm_dbg_kms() to drm_info()
>>> for printing debuf info during dsb buffer preparation.
>>>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
>>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> index b34a67309976..b68dd7bd5271 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> @@ -275,7 +275,7 @@ void intel_dsb_prepare(struct intel_crtc_state
>>> *crtc_state)
>>>
>>>    	dsb = kmalloc(sizeof(*dsb), GFP_KERNEL);
>>>    	if (!dsb) {
>>> -		drm_err(&i915->drm, "DSB object creation failed\n");
>>> +		drm_info(&i915->drm, "DSB object creation failed\n");
>>>    		return;
>>>    	}
>>>
>>> @@ -283,14 +283,14 @@ void intel_dsb_prepare(struct intel_crtc_state
>>> *crtc_state)
>>>
>>>    	obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
>>>    	if (IS_ERR(obj)) {
>>> -		drm_err(&i915->drm, "Gem object creation failed\n");
>>> +		drm_info(&i915->drm, "Gem object creation failed\n");
>> If CI is happy with drm_warn then it makes sense to use drm_warn.
> Checked with CI team, seems drm_warn also considered as bug, is it ok to use drm_info?


In that case: don't print anything on each error and at out label,  you 
can print an info about the fallback option.


Nirmoy

>
>>
>>>    		kfree(dsb);
>>>    		goto out;
>>>    	}
>>>
>>>    	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
>>>    	if (IS_ERR(vma)) {
>>> -		drm_err(&i915->drm, "Vma creation failed\n");
>>> +		drm_info(&i915->drm, "Vma creation failed\n");
>>
>> These messages are bit vague, add "DSB VMA creation failed" or something
>> similar.
>>
>> With that Acked-by: Nirmoy Das <nirmoy.das@intel.com>
> Thanks for review, will modify in next version.
>
> Regards,
> Animesh
>   
>>
>> Nirmoy
>>
>>
>>>    		i915_gem_object_put(obj);
>>>    		kfree(dsb);
>>>    		goto out;
>>> @@ -298,7 +298,7 @@ void intel_dsb_prepare(struct intel_crtc_state
>>> *crtc_state)
>>>
>>>    	buf = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WC);
>>>    	if (IS_ERR(buf)) {
>>> -		drm_err(&i915->drm, "Command buffer creation failed\n");
>>> +		drm_info(&i915->drm, "Command buffer creation failed\n");
>>>    		i915_vma_unpin_and_release(&vma,
>> I915_VMA_RELEASE_MAP);
>>>    		kfree(dsb);
>>>    		goto out;