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 <shashank.sharma@xxxxxxxxx>
Cc: Imre Deak <imre.deak@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
---
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_WRITE 0x9
#define DSB_OPCODE_POLL 0xA
#define DSB_BYTE_EN (0xf << 20)
+#define DSB_REG_VALUE_MASK 0xfffff
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 a/drivers/gpu/drm/i915/display/intel_dsb.h
b/drivers/gpu/drm/i915/display/intel_dsb.h
index 1b33ab118640..c848747f52d9 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -30,11 +30,19 @@ struct intel_dsb {
* and help in calculating cmd_buf_tail.
*/
int free_pos;
+
+ /*
+ * ins_start_offset will help to store start address
+ * of the dsb instuction of auto-increment register.
+ */
+ u32 ins_start_offset;
How about the variable to be named as base_offset ? Also, do we need
to keep this saved, even when the writing is done ?
Yes it is needed to compare the register value of the next write. As
agreed offline we can have ins_start_offset as variable-name.
};
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);
+void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
+ u32 val);
Cross check the alignment of second line.
Yes looks fine after applying the patch.
Regards,
Animesh
- Shashank
#endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
