[v6,03/10] drm/i915/dsb: single register write function for DSB.

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

Details

Message ID 20190911191133.23383-4-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.
DSB support single register write through opcode 0x1. Generic
api created which accumulate all single register write in a batch
buffer and once DSB is triggered, it will program all the registers
at the same time.

v1: Initial version.
v2: Unused macro removed and cosmetic changes done. (Shashank)
v3: set free_pos to zero in dsb-put() instead dsb-get() and
a cosmetic change. (Shashank)

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 | 30 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
 2 files changed, 39 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 7c1b1574788c..e2c383352145 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -9,6 +9,13 @@ 
 
 #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
 
+/* DSB opcodes. */
+#define DSB_OPCODE_SHIFT		24
+#define DSB_OPCODE_MMIO_WRITE		0x1
+#define DSB_OPCODE_INDEXED_WRITE	0x9
+#define DSB_BYTE_EN			0xF
+#define DSB_BYTE_EN_SHIFT		20
+
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
 {
@@ -66,5 +73,28 @@  void intel_dsb_put(struct intel_dsb *dsb)
 		i915_vma_unpin_and_release(&dsb->vma, 0);
 		mutex_unlock(&i915->drm.struct_mutex);
 		dsb->cmd_buf = NULL;
+		dsb->free_pos = 0;
+	}
+}
+
+void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 *buf = dsb->cmd_buf;
+
+	if (!buf) {
+		I915_WRITE(reg, val);
+		return;
+	}
+
+	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
+		DRM_DEBUG_KMS("DSB buffer overflow\n");
+		return;
 	}
+
+	buf[dsb->free_pos++] = val;
+	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  << DSB_OPCODE_SHIFT) |
+			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
+			       i915_mmio_reg_offset(reg);
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
index 27eb68eb5392..31b87dcfe160 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -6,6 +6,8 @@ 
 #ifndef _INTEL_DSB_H
 #define _INTEL_DSB_H
 
+#include "i915_reg.h"
+
 struct intel_crtc;
 struct i915_vma;
 
@@ -21,10 +23,17 @@  struct intel_dsb {
 	enum dsb_id id;
 	u32 *cmd_buf;
 	struct i915_vma *vma;
+
+	/*
+	 * free_pos will point the first free entry position
+	 * and help in calculating tail of command buffer.
+	 */
+	int free_pos;
 };
 
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc);
 void intel_dsb_put(struct intel_dsb *dsb);
+void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
 
 #endif

Comments

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> DSB support single register write through opcode 0x1. Generic
> api created which accumulate all single register write in a batch
> buffer and once DSB is triggered, it will program all the registers
> at the same time.
>
> v1: Initial version.
> v2: Unused macro removed and cosmetic changes done. (Shashank)
> v3: set free_pos to zero in dsb-put() instead dsb-get() and
> a cosmetic change. (Shashank)
>
> 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 | 30 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>   2 files changed, 39 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 7c1b1574788c..e2c383352145 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -9,6 +9,13 @@
>   
>   #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>   
> +/* DSB opcodes. */
> +#define DSB_OPCODE_SHIFT		24
> +#define DSB_OPCODE_MMIO_WRITE		0x1
> +#define DSB_OPCODE_INDEXED_WRITE	0x9
We are not using this macro here, this should go to the Batch 
/INDEXED_WRITE patch.
> +#define DSB_BYTE_EN			0xF
> +#define DSB_BYTE_EN_SHIFT		20
> +
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
>   {
> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>   		i915_vma_unpin_and_release(&dsb->vma, 0);
>   		mutex_unlock(&i915->drm.struct_mutex);
>   		dsb->cmd_buf = NULL;
> +		dsb->free_pos = 0;
> +	}
> +}
> +
I hope this addition of braces are due to diff's adjustment.
> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 *buf = dsb->cmd_buf;
> +
> +	if (!buf) {
> +		I915_WRITE(reg, val);
> +		return;
> +	}
> +
> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
> +		DRM_DEBUG_KMS("DSB buffer overflow\n");
why shouldn't we do a I915_WRITE(reg, val) here too? This is single 
register write, and we can handle this.
> +		return;
>   	}
> +
> +	buf[dsb->free_pos++] = val;
> +	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  << DSB_OPCODE_SHIFT) |
> +			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
> +			       i915_mmio_reg_offset(reg);
>   }
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 27eb68eb5392..31b87dcfe160 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -6,6 +6,8 @@
>   #ifndef _INTEL_DSB_H
>   #define _INTEL_DSB_H
>   
> +#include "i915_reg.h"
> +
>   struct intel_crtc;
>   struct i915_vma;
>   
> @@ -21,10 +23,17 @@ struct intel_dsb {
>   	enum dsb_id id;
>   	u32 *cmd_buf;
>   	struct i915_vma *vma;
> +
> +	/*
> +	 * free_pos will point the first free entry position
> +	 * and help in calculating tail of command buffer.
> +	 */
> +	int free_pos;
>   };
>   
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc);
>   void intel_dsb_put(struct intel_dsb *dsb);
> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>   
>   #endif
On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>> DSB support single register write through opcode 0x1. Generic
>> api created which accumulate all single register write in a batch
>> buffer and once DSB is triggered, it will program all the registers
>> at the same time.
>>
>> v1: Initial version.
>> v2: Unused macro removed and cosmetic changes done. (Shashank)
>> v3: set free_pos to zero in dsb-put() instead dsb-get() and
>> a cosmetic change. (Shashank)
>>
>> 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 | 30 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>>   2 files changed, 39 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index 7c1b1574788c..e2c383352145 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -9,6 +9,13 @@
>>   
>>   #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>   
>> +/* DSB opcodes. */
>> +#define DSB_OPCODE_SHIFT		24
>> +#define DSB_OPCODE_MMIO_WRITE		0x1
>> +#define DSB_OPCODE_INDEXED_WRITE	0x9
> We are not using this macro here, this should go to the Batch 
> /INDEXED_WRITE patch.
>> +#define DSB_BYTE_EN			0xF
>> +#define DSB_BYTE_EN_SHIFT		20
>> +
>>   struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc)
>>   {
>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>   		i915_vma_unpin_and_release(&dsb->vma, 0);
>>   		mutex_unlock(&i915->drm.struct_mutex);
>>   		dsb->cmd_buf = NULL;
>> +		dsb->free_pos = 0;
>> +	}
>> +}
>> +
> I hope this addition of braces are due to diff's adjustment.
>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>> +{
>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	u32 *buf = dsb->cmd_buf;
>> +
>> +	if (!buf) {
>> +		I915_WRITE(reg, val);
>> +		return;
>> +	}
>> +
>> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>> +		DRM_DEBUG_KMS("DSB buffer overflow\n");
> why shouldn't we do a I915_WRITE(reg, val) here too? This is single 
> register write, and we can handle this.

That would assume it's okay to directly mmio write this and the
subsequent values, and write the batch already stored in the buffer
afterwards.

BR,
Jani.

>> +		return;
>>   	}
>> +
>> +	buf[dsb->free_pos++] = val;
>> +	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  << DSB_OPCODE_SHIFT) |
>> +			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>> +			       i915_mmio_reg_offset(reg);
>>   }
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 27eb68eb5392..31b87dcfe160 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>> @@ -6,6 +6,8 @@
>>   #ifndef _INTEL_DSB_H
>>   #define _INTEL_DSB_H
>>   
>> +#include "i915_reg.h"
>> +
>>   struct intel_crtc;
>>   struct i915_vma;
>>   
>> @@ -21,10 +23,17 @@ struct intel_dsb {
>>   	enum dsb_id id;
>>   	u32 *cmd_buf;
>>   	struct i915_vma *vma;
>> +
>> +	/*
>> +	 * free_pos will point the first free entry position
>> +	 * and help in calculating tail of command buffer.
>> +	 */
>> +	int free_pos;
>>   };
>>   
>>   struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc);
>>   void intel_dsb_put(struct intel_dsb *dsb);
>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>   
>>   #endif
On 9/12/2019 6:21 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:
>>> DSB support single register write through opcode 0x1. Generic
>>> api created which accumulate all single register write in a batch
>>> buffer and once DSB is triggered, it will program all the registers
>>> at the same time.
>>>
>>> v1: Initial version.
>>> v2: Unused macro removed and cosmetic changes done. (Shashank)
>>> v3: set free_pos to zero in dsb-put() instead dsb-get() and
>>> a cosmetic change. (Shashank)
>>>
>>> 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 | 30 ++++++++++++++++++++++++
>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>>>    2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> index 7c1b1574788c..e2c383352145 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> @@ -9,6 +9,13 @@
>>>    
>>>    #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>>    
>>> +/* DSB opcodes. */
>>> +#define DSB_OPCODE_SHIFT		24
>>> +#define DSB_OPCODE_MMIO_WRITE		0x1
>>> +#define DSB_OPCODE_INDEXED_WRITE	0x9
>> We are not using this macro here, this should go to the Batch
>> /INDEXED_WRITE patch.
>>> +#define DSB_BYTE_EN			0xF
>>> +#define DSB_BYTE_EN_SHIFT		20
>>> +
>>>    struct intel_dsb *
>>>    intel_dsb_get(struct intel_crtc *crtc)
>>>    {
>>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>>    		i915_vma_unpin_and_release(&dsb->vma, 0);
>>>    		mutex_unlock(&i915->drm.struct_mutex);
>>>    		dsb->cmd_buf = NULL;
>>> +		dsb->free_pos = 0;
>>> +	}
>>> +}
>>> +
>> I hope this addition of braces are due to diff's adjustment.
>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>> +{
>>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +	u32 *buf = dsb->cmd_buf;
>>> +
>>> +	if (!buf) {
>>> +		I915_WRITE(reg, val);
>>> +		return;
>>> +	}
>>> +
>>> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>>> +		DRM_DEBUG_KMS("DSB buffer overflow\n");
>> why shouldn't we do a I915_WRITE(reg, val) here too? This is single
>> register write, and we can handle this.
> That would assume it's okay to directly mmio write this and the
> subsequent values, and write the batch already stored in the buffer
> afterwards.

This is single write API, so I won't expect this to be called in an 
indexed context, also, we have exceeded the buffer size already, so no 
subsequent DSB write would be possible anyways. But I can still see it 
would be some kind of mess only, so doesn't really matter if we do this 
I915_write or not :).

- Shashank

> BR,
> Jani.
>
>>> +		return;
>>>    	}
>>> +
>>> +	buf[dsb->free_pos++] = val;
>>> +	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  << DSB_OPCODE_SHIFT) |
>>> +			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>> +			       i915_mmio_reg_offset(reg);
>>>    }
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> index 27eb68eb5392..31b87dcfe160 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> @@ -6,6 +6,8 @@
>>>    #ifndef _INTEL_DSB_H
>>>    #define _INTEL_DSB_H
>>>    
>>> +#include "i915_reg.h"
>>> +
>>>    struct intel_crtc;
>>>    struct i915_vma;
>>>    
>>> @@ -21,10 +23,17 @@ struct intel_dsb {
>>>    	enum dsb_id id;
>>>    	u32 *cmd_buf;
>>>    	struct i915_vma *vma;
>>> +
>>> +	/*
>>> +	 * free_pos will point the first free entry position
>>> +	 * and help in calculating tail of command buffer.
>>> +	 */
>>> +	int free_pos;
>>>    };
>>>    
>>>    struct intel_dsb *
>>>    intel_dsb_get(struct intel_crtc *crtc);
>>>    void intel_dsb_put(struct intel_dsb *dsb);
>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>>    
>>>    #endif
On 9/12/2019 6:30 PM, Sharma, Shashank wrote:
>
> On 9/12/2019 6:21 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:
>>>> DSB support single register write through opcode 0x1. Generic
>>>> api created which accumulate all single register write in a batch
>>>> buffer and once DSB is triggered, it will program all the registers
>>>> at the same time.
>>>>
>>>> v1: Initial version.
>>>> v2: Unused macro removed and cosmetic changes done. (Shashank)
>>>> v3: set free_pos to zero in dsb-put() instead dsb-get() and
>>>> a cosmetic change. (Shashank)
>>>>
>>>> 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 | 30 
>>>> ++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>>>>    2 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c 
>>>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> index 7c1b1574788c..e2c383352145 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> @@ -9,6 +9,13 @@
>>>>       #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>>>    +/* DSB opcodes. */
>>>> +#define DSB_OPCODE_SHIFT        24
>>>> +#define DSB_OPCODE_MMIO_WRITE        0x1
>>>> +#define DSB_OPCODE_INDEXED_WRITE    0x9
>>> We are not using this macro here, this should go to the Batch
>>> /INDEXED_WRITE patch.
>>>> +#define DSB_BYTE_EN            0xF
>>>> +#define DSB_BYTE_EN_SHIFT        20
>>>> +
>>>>    struct intel_dsb *
>>>>    intel_dsb_get(struct intel_crtc *crtc)
>>>>    {
>>>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>>>            i915_vma_unpin_and_release(&dsb->vma, 0);
>>>>            mutex_unlock(&i915->drm.struct_mutex);
>>>>            dsb->cmd_buf = NULL;
>>>> +        dsb->free_pos = 0;
>>>> +    }
>>>> +}
>>>> +
>>> I hope this addition of braces are due to diff's adjustment.
>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, 
>>>> u32 val)
>>>> +{
>>>> +    struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>>> +    struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>> +    u32 *buf = dsb->cmd_buf;
>>>> +
>>>> +    if (!buf) {
>>>> +        I915_WRITE(reg, val);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>>>> +        DRM_DEBUG_KMS("DSB buffer overflow\n");
>>> why shouldn't we do a I915_WRITE(reg, val) here too? This is single
>>> register write, and we can handle this.
>> That would assume it's okay to directly mmio write this and the
>> subsequent values, and write the batch already stored in the buffer
>> afterwards.
>
> This is single write API, so I won't expect this to be called in an 
> indexed context, also, we have exceeded the buffer size already, so no 
> subsequent DSB write would be possible anyways. But I can still see it 
> would be some kind of mess only, so doesn't really matter if we do 
> this I915_write or not :).

Adding I915_WRITE can be done, but I feel better to break the 
functionality where we have buffer overflow and based on that we can 
fine tune the buffer size.
If a set of register is targetted to write through DSB then some writing 
through MMIO and and rest writing though DSB may not a nice thing.
So added only debug log to capture the issue.

Regards,
Animesh
>
> - Shashank
>
>> BR,
>> Jani.
>>
>>>> +        return;
>>>>        }
>>>> +
>>>> +    buf[dsb->free_pos++] = val;
>>>> +    buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << 
>>>> DSB_OPCODE_SHIFT) |
>>>> +                   (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>>> +                   i915_mmio_reg_offset(reg);
>>>>    }
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h 
>>>> b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> index 27eb68eb5392..31b87dcfe160 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> @@ -6,6 +6,8 @@
>>>>    #ifndef _INTEL_DSB_H
>>>>    #define _INTEL_DSB_H
>>>>    +#include "i915_reg.h"
>>>> +
>>>>    struct intel_crtc;
>>>>    struct i915_vma;
>>>>    @@ -21,10 +23,17 @@ struct intel_dsb {
>>>>        enum dsb_id id;
>>>>        u32 *cmd_buf;
>>>>        struct i915_vma *vma;
>>>> +
>>>> +    /*
>>>> +     * free_pos will point the first free entry position
>>>> +     * and help in calculating tail of command buffer.
>>>> +     */
>>>> +    int free_pos;
>>>>    };
>>>>       struct intel_dsb *
>>>>    intel_dsb_get(struct intel_crtc *crtc);
>>>>    void intel_dsb_put(struct intel_dsb *dsb);
>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, 
>>>> u32 val);
>>>>       #endif
On 9/12/2019 6:37 PM, Animesh Manna wrote:
>
>
> On 9/12/2019 6:30 PM, Sharma, Shashank wrote:
>>
>> On 9/12/2019 6:21 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:
>>>>> DSB support single register write through opcode 0x1. Generic
>>>>> api created which accumulate all single register write in a batch
>>>>> buffer and once DSB is triggered, it will program all the registers
>>>>> at the same time.
>>>>>
>>>>> v1: Initial version.
>>>>> v2: Unused macro removed and cosmetic changes done. (Shashank)
>>>>> v3: set free_pos to zero in dsb-put() instead dsb-get() and
>>>>> a cosmetic change. (Shashank)
>>>>>
>>>>> 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 | 30 
>>>>> ++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>>>>>    2 files changed, 39 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c 
>>>>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>>> index 7c1b1574788c..e2c383352145 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>>> @@ -9,6 +9,13 @@
>>>>>       #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>>>>    +/* DSB opcodes. */
>>>>> +#define DSB_OPCODE_SHIFT        24
>>>>> +#define DSB_OPCODE_MMIO_WRITE        0x1
>>>>> +#define DSB_OPCODE_INDEXED_WRITE    0x9
>>>> We are not using this macro here, this should go to the Batch
>>>> /INDEXED_WRITE patch.
>>>>> +#define DSB_BYTE_EN            0xF
>>>>> +#define DSB_BYTE_EN_SHIFT        20
>>>>> +
>>>>>    struct intel_dsb *
>>>>>    intel_dsb_get(struct intel_crtc *crtc)
>>>>>    {
>>>>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>>>>            i915_vma_unpin_and_release(&dsb->vma, 0);
>>>>>            mutex_unlock(&i915->drm.struct_mutex);
>>>>>            dsb->cmd_buf = NULL;
>>>>> +        dsb->free_pos = 0;
>>>>> +    }
>>>>> +}
>>>>> +
>>>> I hope this addition of braces are due to diff's adjustment.
>>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, 
>>>>> u32 val)
>>>>> +{
>>>>> +    struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>>>> +    struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>> +    u32 *buf = dsb->cmd_buf;
>>>>> +
>>>>> +    if (!buf) {
>>>>> +        I915_WRITE(reg, val);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>>>>> +        DRM_DEBUG_KMS("DSB buffer overflow\n");
>>>> why shouldn't we do a I915_WRITE(reg, val) here too? This is single
>>>> register write, and we can handle this.
>>> That would assume it's okay to directly mmio write this and the
>>> subsequent values, and write the batch already stored in the buffer
>>> afterwards.
>>
>> This is single write API, so I won't expect this to be called in an 
>> indexed context, also, we have exceeded the buffer size already, so 
>> no subsequent DSB write would be possible anyways. But I can still 
>> see it would be some kind of mess only, so doesn't really matter if 
>> we do this I915_write or not :).
>
> Adding I915_WRITE can be done, but I feel better to break the 
> functionality where we have buffer overflow and based on that we can 
> fine tune the buffer size.
> If a set of register is targetted to write through DSB then some 
> writing through MMIO and and rest writing though DSB may not a nice 
> thing.
> So added only debug log to capture the issue.
>
> Regards,
> Animesh

Yeah, broken this way or other, better to warn as soon as possible.

With the above macro comment fixed,

Please feel free to use Reviewed-by: Shashank Sharma 
<shashank.sharma@intel.com>

- Shashank

>
>>
>> - Shashank
>>
>>> BR,
>>> Jani.
>>>
>>>>> +        return;
>>>>>        }
>>>>> +
>>>>> +    buf[dsb->free_pos++] = val;
>>>>> +    buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << 
>>>>> DSB_OPCODE_SHIFT) |
>>>>> +                   (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>>>> +                   i915_mmio_reg_offset(reg);
>>>>>    }
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h 
>>>>> b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>>> index 27eb68eb5392..31b87dcfe160 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>>> @@ -6,6 +6,8 @@
>>>>>    #ifndef _INTEL_DSB_H
>>>>>    #define _INTEL_DSB_H
>>>>>    +#include "i915_reg.h"
>>>>> +
>>>>>    struct intel_crtc;
>>>>>    struct i915_vma;
>>>>>    @@ -21,10 +23,17 @@ struct intel_dsb {
>>>>>        enum dsb_id id;
>>>>>        u32 *cmd_buf;
>>>>>        struct i915_vma *vma;
>>>>> +
>>>>> +    /*
>>>>> +     * free_pos will point the first free entry position
>>>>> +     * and help in calculating tail of command buffer.
>>>>> +     */
>>>>> +    int free_pos;
>>>>>    };
>>>>>       struct intel_dsb *
>>>>>    intel_dsb_get(struct intel_crtc *crtc);
>>>>>    void intel_dsb_put(struct intel_dsb *dsb);
>>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, 
>>>>> u32 val);
>>>>>       #endif
>