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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 9/12/2019 6:30 PM, Sharma, Shashank wrote:

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

Adding I915_WRITE can be done, but I feel better to break the functionality where we have buffer overflow and based on that we can fine tune the buffer size. If a set of register is targetted to write through DSB then some writing through MMIO and and rest writing though DSB may not a nice thing.
So added only debug log to capture the issue.

Regards,
Animesh

- 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux