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.
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 ?).
+{
+ 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 ?
+ 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 ?
+ 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 ?
+ 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 ?
+
+ /* 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.
+ 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.
+
+ /* 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;
+
+ /* 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 ?
};
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.
- Shashank
#endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
