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

[Jani Nikula <jani.nikula@intel.com>]

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

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx