[1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check
diff mbox series

Message ID 20200129182034.26138-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check
Related show

Commit Message

Ville Syrjälä Jan. 29, 2020, 6:20 p.m. UTC
From: Ville Syrjälä <ville.syrjala@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.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

José Roberto de Souza Jan. 30, 2020, 6:13 p.m. UTC | #1
On Wed, 2020-01-29 at 20:20 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@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)
>  		return;
>  
>  	if (WARN_ON(dsb->refcount == 0))
Manna, Animesh Jan. 31, 2020, 9:34 a.m. UTC | #2
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@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@intel.com>
>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@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.
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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Jan. 31, 2020, 11:42 a.m. UTC | #3
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@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@intel.com>
> >
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@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.

> 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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Manna, Animesh Jan. 31, 2020, 12:06 p.m. UTC | #4
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@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@intel.com>
>>>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Jan. 31, 2020, 12:16 p.m. UTC | #5
On Fri, Jan 31, 2020 at 05:36:15PM +0530, Manna, Animesh wrote:
> 
> 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@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@intel.com>
> >>>
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@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.

None of this stuff should really exist in the guts of the dsb code
anyway. It's all just a hack to get the dsb code in without actually
taking advantage of the dsb. The real solution would involve doing the
dsb vs. mmio decision upfront at the start of the atomic commit, and
then using totally different codeepaths for those two cases. No real
need for refcounts in that case. But first we'd need to finish the
vblank workers so we'd have the mmio path sorted out.
Sharma, Swati2 March 3, 2020, 10:43 a.m. UTC | #6
Hi Ville,

Can you please rebase on current drm-tip and submit new revision so that 
new run can be executed? BAT failure was observed with the last revision
submitted.

On 29-Jan-20 11:50 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@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)
>   		return;
>   
>   	if (WARN_ON(dsb->refcount == 0))
>

Patch
diff mbox series

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)
 		return;
 
 	if (WARN_ON(dsb->refcount == 0))