[v6,07/10] drm/i915/dsb: function to trigger workload execution of DSB.

Submitted by Animesh Manna on Sept. 11, 2019, 7:11 p.m.

Details

Message ID 20190911191133.23383-8-animesh.manna@intel.com
State New
Headers show
Series "DSB enablement." ( rev: 6 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Animesh Manna Sept. 11, 2019, 7:11 p.m.
Batch buffer will be created through dsb-reg-write function which can have
single/multiple request based on usecase and once the buffer is ready
commit function will trigger the execution of the batch buffer. All
the registers will be updated simultaneously.

v1: Initial version.
v2: Optimized code few places. (Chris)
v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)

Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
 drivers/gpu/drm/i915/i915_reg.h          |  2 ++
 3 files changed, 45 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 2b0ffc0afb74..eea86afb0583 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -212,3 +212,45 @@  void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
 			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
 			       i915_mmio_reg_offset(reg);
 }
+
+void intel_dsb_commit(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	enum pipe pipe = crtc->pipe;
+	u32 tail;
+
+	if (!dsb->free_pos)
+		return;
+
+	if (!intel_dsb_enable_engine(dsb))
+		goto reset;
+
+	if (is_dsb_busy(dsb)) {
+		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
+		goto reset;
+	}
+	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
+
+	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
+	if (tail > dsb->free_pos * 4)
+		memset(&dsb->cmd_buf[dsb->free_pos], 0,
+		       (tail - dsb->free_pos * 4));
+
+	if (is_dsb_busy(dsb)) {
+		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
+		goto reset;
+	}
+	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
+		      i915_ggtt_offset(dsb->vma), tail);
+	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
+	if (wait_for(!is_dsb_busy(dsb), 1)) {
+		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
+		goto reset;
+	}
+
+reset:
+	dsb->free_pos = 0;
+	intel_dsb_disable_engine(dsb);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
index 9b2522f20bfb..7389c8c5b665 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -43,5 +43,6 @@  void intel_dsb_put(struct intel_dsb *dsb);
 void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
 void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
 				 u32 val);
+void intel_dsb_commit(struct intel_dsb *dsb);
 
 #endif
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2dbaa49f5c74..c77b5066d8dd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -11687,6 +11687,8 @@  enum skl_power_gate {
 #define _DSBSL_INSTANCE_BASE		0x70B00
 #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
 					 (pipe) * 0x1000 + (id) * 100)
+#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
+#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
 #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
 #define   DSB_ENABLE			(1 << 31)
 #define   DSB_STATUS			(1 << 0)

Comments

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> Batch buffer will be created through dsb-reg-write function which can have
> single/multiple request based on usecase and once the buffer is ready
> commit function will trigger the execution of the batch buffer. All
> the registers will be updated simultaneously.
>
> v1: Initial version.
> v2: Optimized code few places. (Chris)
> v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)
>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>   drivers/gpu/drm/i915/i915_reg.h          |  2 ++
>   3 files changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 2b0ffc0afb74..eea86afb0583 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>   			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>   			       i915_mmio_reg_offset(reg);
>   }
> +
> +void intel_dsb_commit(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 tail;
> +
> +	if (!dsb->free_pos)
> +		return;
> +
> +	if (!intel_dsb_enable_engine(dsb))
> +		goto reset;
> +
> +	if (is_dsb_busy(dsb)) {
> +		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
> +		goto reset;
> +	}
> +	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
> +
> +	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
> +	if (tail > dsb->free_pos * 4)
> +		memset(&dsb->cmd_buf[dsb->free_pos], 0,
> +		       (tail - dsb->free_pos * 4));
> +
> +	if (is_dsb_busy(dsb)) {
> +		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
> +		goto reset;
> +	}
> +	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
> +		      i915_ggtt_offset(dsb->vma), tail);
> +	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
> +	if (wait_for(!is_dsb_busy(dsb), 1)) {
> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
> +		goto reset;
> +	}
> +
> +reset:
> +	dsb->free_pos = 0;
> +	intel_dsb_disable_engine(dsb);

I am still not very convince if a commit function should be void, I 
would still want it to return success or failure, so that we would know 
if the last operation was successful or not.

I would wait Jani N to comment here, on what he feels about this.

- Shashank

> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 9b2522f20bfb..7389c8c5b665 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -43,5 +43,6 @@ void intel_dsb_put(struct intel_dsb *dsb);
>   void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>   void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>   				 u32 val);
> +void intel_dsb_commit(struct intel_dsb *dsb);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2dbaa49f5c74..c77b5066d8dd 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11687,6 +11687,8 @@ enum skl_power_gate {
>   #define _DSBSL_INSTANCE_BASE		0x70B00
>   #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>   					 (pipe) * 0x1000 + (id) * 100)
> +#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
> +#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
>   #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
>   #define   DSB_ENABLE			(1 << 31)
>   #define   DSB_STATUS			(1 << 0)
On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>> Batch buffer will be created through dsb-reg-write function which can have
>> single/multiple request based on usecase and once the buffer is ready
>> commit function will trigger the execution of the batch buffer. All
>> the registers will be updated simultaneously.
>>
>> v1: Initial version.
>> v2: Optimized code few places. (Chris)
>> v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)
>>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>>   drivers/gpu/drm/i915/i915_reg.h          |  2 ++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index 2b0ffc0afb74..eea86afb0583 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>   			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>   			       i915_mmio_reg_offset(reg);
>>   }
>> +
>> +void intel_dsb_commit(struct intel_dsb *dsb)
>> +{
>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	enum pipe pipe = crtc->pipe;
>> +	u32 tail;
>> +
>> +	if (!dsb->free_pos)
>> +		return;
>> +
>> +	if (!intel_dsb_enable_engine(dsb))
>> +		goto reset;
>> +
>> +	if (is_dsb_busy(dsb)) {
>> +		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
>> +		goto reset;
>> +	}
>> +	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
>> +
>> +	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
>> +	if (tail > dsb->free_pos * 4)
>> +		memset(&dsb->cmd_buf[dsb->free_pos], 0,
>> +		       (tail - dsb->free_pos * 4));
>> +
>> +	if (is_dsb_busy(dsb)) {
>> +		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
>> +		goto reset;
>> +	}
>> +	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
>> +		      i915_ggtt_offset(dsb->vma), tail);
>> +	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
>> +	if (wait_for(!is_dsb_busy(dsb), 1)) {
>> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
>> +		goto reset;
>> +	}
>> +
>> +reset:
>> +	dsb->free_pos = 0;
>> +	intel_dsb_disable_engine(dsb);
>
> I am still not very convince if a commit function should be void, I 
> would still want it to return success or failure, so that we would know 
> if the last operation was successful or not.
>
> I would wait Jani N to comment here, on what he feels about this.

The question becomes, what do you *do* with the return value? If none of
the callers are going to use it, don't return it.

BR,
Jani.

>
> - Shashank
>
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 9b2522f20bfb..7389c8c5b665 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>> @@ -43,5 +43,6 @@ void intel_dsb_put(struct intel_dsb *dsb);
>>   void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>   void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>>   				 u32 val);
>> +void intel_dsb_commit(struct intel_dsb *dsb);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 2dbaa49f5c74..c77b5066d8dd 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -11687,6 +11687,8 @@ enum skl_power_gate {
>>   #define _DSBSL_INSTANCE_BASE		0x70B00
>>   #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>>   					 (pipe) * 0x1000 + (id) * 100)
>> +#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
>> +#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
>>   #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
>>   #define   DSB_ENABLE			(1 << 31)
>>   #define   DSB_STATUS			(1 << 0)
On 9/12/2019 7:09 PM, Jani Nikula wrote:
> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
>> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>>> Batch buffer will be created through dsb-reg-write function which can have
>>> single/multiple request based on usecase and once the buffer is ready
>>> commit function will trigger the execution of the batch buffer. All
>>> the registers will be updated simultaneously.
>>>
>>> v1: Initial version.
>>> v2: Optimized code few places. (Chris)
>>> v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)
>>>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>>>    drivers/gpu/drm/i915/i915_reg.h          |  2 ++
>>>    3 files changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> index 2b0ffc0afb74..eea86afb0583 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> @@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>>    			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>>    			       i915_mmio_reg_offset(reg);
>>>    }
>>> +
>>> +void intel_dsb_commit(struct intel_dsb *dsb)
>>> +{
>>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>> +	struct drm_device *dev = crtc->base.dev;
>>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>>> +	enum pipe pipe = crtc->pipe;
>>> +	u32 tail;
>>> +
>>> +	if (!dsb->free_pos)
>>> +		return;
>>> +
>>> +	if (!intel_dsb_enable_engine(dsb))
>>> +		goto reset;
>>> +
>>> +	if (is_dsb_busy(dsb)) {
>>> +		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
>>> +		goto reset;
>>> +	}
>>> +	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
>>> +
>>> +	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
>>> +	if (tail > dsb->free_pos * 4)
>>> +		memset(&dsb->cmd_buf[dsb->free_pos], 0,
>>> +		       (tail - dsb->free_pos * 4));
>>> +
>>> +	if (is_dsb_busy(dsb)) {
>>> +		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
>>> +		goto reset;
>>> +	}
>>> +	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
>>> +		      i915_ggtt_offset(dsb->vma), tail);
>>> +	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
>>> +	if (wait_for(!is_dsb_busy(dsb), 1)) {
>>> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
>>> +		goto reset;
>>> +	}
>>> +
>>> +reset:
>>> +	dsb->free_pos = 0;
>>> +	intel_dsb_disable_engine(dsb);
>> I am still not very convince if a commit function should be void, I
>> would still want it to return success or failure, so that we would know
>> if the last operation was successful or not.
>>
>> I would wait Jani N to comment here, on what he feels about this.
> The question becomes, what do you *do* with the return value? If none of
> the callers are going to use it, don't return it.

I was thinking we should check the return value of the DSB commit (if 
not writes), so that we would be aware that the register programming 
failed, and later even can think about a fallback method. Too ambitious ?

- Shashank

> BR,
> Jani.
>
>> - Shashank
>>
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> index 9b2522f20bfb..7389c8c5b665 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> @@ -43,5 +43,6 @@ void intel_dsb_put(struct intel_dsb *dsb);
>>>    void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>>    void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>>>    				 u32 val);
>>> +void intel_dsb_commit(struct intel_dsb *dsb);
>>>    
>>>    #endif
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 2dbaa49f5c74..c77b5066d8dd 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -11687,6 +11687,8 @@ enum skl_power_gate {
>>>    #define _DSBSL_INSTANCE_BASE		0x70B00
>>>    #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>>>    					 (pipe) * 0x1000 + (id) * 100)
>>> +#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
>>> +#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
>>>    #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
>>>    #define   DSB_ENABLE			(1 << 31)
>>>    #define   DSB_STATUS			(1 << 0)