[Intel-gfx] [PATCH v6 03/10] drm/i915/dsb: single register write function for DSB.

[Sharma, Shashank]

On 9/12/2019 6:21 PM, Jani Nikula wrote:
> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma at 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 at intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna at 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