Hi,
On 8/28/2019 10:16 PM, Sharma, Shashank wrote:
On 8/28/2019 12:40 AM, Animesh Manna wrote:
DSB can program large set of data through indexed register write
(opcode 0x9) in one shot. Will be using for bulk register programming
Reshuffle-> This feature can be used for bulk register programming e.g.
DSB can program large set of data through indexed register write
(opcode 0x9) in one shot. This feature can be used for bulk register
programming e.g. gamma lut programming, HDR meta data programming.
Hope it is fine now?
e.g. gamma lut programming, HDR meta data programming.
v1: Initial version.
v2: simplified code by using ALIGN(). (Chris)
Cc: Shashank Sharma
Cc: Imre Deak
Cc: Jani Nikula
Cc: Rodrigo Vivi
Signed-off-by: Animesh Manna
---
drivers/gpu/drm/i915/display/intel_dsb.c | 48
drivers/gpu/drm/i915/display/intel_dsb.h | 8
2 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
b/drivers/gpu/drm/i915/display/intel_dsb.c
index df288446caeb..520f2bbcc8ae 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -22,6 +22,7 @@
#define DSB_OPCODE_INDEXED_WRITE0x9
#define DSB_OPCODE_POLL0xA
#define DSB_BYTE_EN(0xf << 20)
+#define DSB_REG_VALUE_MASK0xf
struct intel_dsb *
intel_dsb_get(struct intel_crtc *crtc)
@@ -96,6 +97,53 @@ void intel_dsb_put(struct intel_dsb *dsb)
}
}
+void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t
reg,
+ u32 val)
We might need one space here, to get this aligned to start of the line
above (or is this due to my mail client ?).
After applying the patch looks ok maybe some editor issue.
+{
+struct intel_crtc *crtc = dsb->crtc;
+struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+u32 *buf = dsb->cmd_buf;
+u32 reg_val;
+
- Do we need a HAS_DSB check at start or the caller will take care of
it ?
As explained before non-null buf implies HAS_DSB check is passed.
+if (!buf) {
+I915_WRITE(reg, val);
I am under the assumption that indexed reg write is to write multiple
registers, and 'reg' is the base value of the register set. I dont
think it makes sense to write a single register here in case of DSB
failure ?
No, here plan is to replace I915_WRITE with dsb-write and for that
I915_WRITE is needed for platforms which does not support.
+return;
+}
+
+if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
+DRM_DEBUG_KMS("DSB buffer overflow.\n");
+return;
+}
+reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
Why do we have this +1 here ? on the very first run (when
ins_start_offset is 0), this will fetch reg_val = buf[1]; Is this what
we want to do ?
Yes reg offset is stored in buf[x+1] position and buf[x] is for size,
where x is starting index of indexed-write-dsb-instruction.
+if (reg_val != i915_mmio_reg_offset(reg)) {
+/* Every instruction should be 8 byte aligned. */
+dsb->free_pos = ALIGN(dsb->free_pos, 2);
The comment says that you want the position to be 8 bytes align, but
the factor sent to the macro is 2 ?
Yes dsb->free_pos is a index of 4 byte array. The new instruction can
start from 0, 2, 4, 6 ... etc.
+
+/* Update the size. */
+dsb->ins_start_offset = dsb->free_pos;
This comment is irrelevant, you are not updating the size, you are
caching the base memory location.
Yes .. will fix.
+buf[dsb->free_pos++] = 1;
What does this indicate ? What is 1 ?
It would be great if you can add an aski graph/table and explain what
does this DSB command buffer actually contains as per your design.
Yes, as per offline discussion will add in code-comment with ascii
graph/table.
+
+/* Update the opcode and reg. */
+buf[dsb->free_pos++] = (DSB_OPCODE_INDEXED_WRITE <<
+DSB_OPCODE_SHIFT) |
+i915_mmio_reg_offset(reg);
+
+/* Update the value. */
+buf[dsb->free_pos++] = val;
+} else {
+/* Update the new value. */
+buf[dsb->free_pos++] = val;
+
+/* Update the size. */
+buf[dsb->ins_start_offset]++;
+}
The code is looking unnecessarily complicated as ins_start_offset and
free_pos are being used for multiple reasons. I guess you can use some
local variables like:
u32 base = count = ALIGN();
{
buf [count++] = size;
buf [count++] = command;
buf [count++] = value;
}
dsb->start_offset = base;
dsb->free_pos = count;
Here the whole buffer not getting in one shot.
+
+/* if number of data words is odd, then the last dword should be
0.*/
+if (dsb->free_pos & 0x1)
+buf[dsb->free_pos] = 0;
+}
+
void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32
val)
{
struct intel_crtc *crtc = dsb->crtc;
diff --git