From: Jani Nikula <jani.nikula@linux.intel.com>
To: Animesh Manna <animesh.manna@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915/dsb: DSB code refactoring
Date: Mon, 25 Sep 2023 15:30:04 +0300 [thread overview]
Message-ID: <87lecumo4z.fsf@intel.com> (raw)
In-Reply-To: <20230922193518.723664-1-animesh.manna@intel.com>
On Sat, 23 Sep 2023, Animesh Manna <animesh.manna@intel.com> wrote:
> Refactor DSB implementation to be compatible with Xe driver.
Sad trombone.
struct intel_dsb should remain an opaque type. I put effort into hiding
its definition, so its guts wouldn't be accessed nilly-willy all over
the place. If it's not hidden, it just will get accessed.
BR,
Jani.
>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/display/intel_dsb.c | 115 ++++---------------
> drivers/gpu/drm/i915/display/intel_dsb.h | 41 ++++++-
> drivers/gpu/drm/i915/display/intel_dsb_ops.c | 67 +++++++++++
> 4 files changed, 130 insertions(+), 94 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/display/intel_dsb_ops.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 1b2e02e9d92c..7fbb5055b85b 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -256,6 +256,7 @@ i915-y += \
> display/intel_dpt.o \
> display/intel_drrs.o \
> display/intel_dsb.o \
> + display/intel_dsb_ops.o \
> display/intel_fb.o \
> display/intel_fb_pin.o \
> display/intel_fbc.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 9a507b9ad82c..f7c6b9aa130f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -4,8 +4,6 @@
> *
> */
>
> -#include "gem/i915_gem_internal.h"
> -
> #include "i915_drv.h"
> #include "i915_reg.h"
> #include "intel_de.h"
> @@ -13,41 +11,7 @@
> #include "intel_dsb.h"
> #include "intel_dsb_regs.h"
>
> -struct i915_vma;
> -
> -enum dsb_id {
> - INVALID_DSB = -1,
> - DSB1,
> - DSB2,
> - DSB3,
> - MAX_DSB_PER_PIPE
> -};
> -
> -struct intel_dsb {
> - enum dsb_id id;
> -
> - u32 *cmd_buf;
> - struct i915_vma *vma;
> - struct intel_crtc *crtc;
> -
> - /*
> - * maximum number of dwords the buffer will hold.
> - */
> - unsigned int size;
> -
> - /*
> - * free_pos will point the first free dword and
> - * help in calculating tail of command buffer.
> - */
> - unsigned int free_pos;
> -
> - /*
> - * ins_start_offset will help to store start dword of the dsb
> - * instuction and help in identifying the batch of auto-increment
> - * register.
> - */
> - unsigned int ins_start_offset;
> -};
> +#define CACHELINE_BYTES 64
>
> /**
> * DOC: DSB
> @@ -117,8 +81,6 @@ static bool is_dsb_busy(struct drm_i915_private *i915, enum pipe pipe,
>
> static void intel_dsb_emit(struct intel_dsb *dsb, u32 ldw, u32 udw)
> {
> - u32 *buf = dsb->cmd_buf;
> -
> if (!assert_dsb_has_room(dsb))
> return;
>
> @@ -127,14 +89,13 @@ static void intel_dsb_emit(struct intel_dsb *dsb, u32 ldw, u32 udw)
>
> dsb->ins_start_offset = dsb->free_pos;
>
> - buf[dsb->free_pos++] = ldw;
> - buf[dsb->free_pos++] = udw;
> + intel_dsb_write(dsb, dsb->free_pos++, ldw);
> + intel_dsb_write(dsb, dsb->free_pos++, udw);
> }
>
> static bool intel_dsb_prev_ins_is_write(struct intel_dsb *dsb,
> u32 opcode, i915_reg_t reg)
> {
> - const u32 *buf = dsb->cmd_buf;
> u32 prev_opcode, prev_reg;
>
> /*
> @@ -145,8 +106,8 @@ static bool intel_dsb_prev_ins_is_write(struct intel_dsb *dsb,
> if (dsb->free_pos == 0)
> return false;
>
> - prev_opcode = buf[dsb->ins_start_offset + 1] & ~DSB_REG_VALUE_MASK;
> - prev_reg = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
> + prev_opcode = intel_dsb_read(dsb, dsb->ins_start_offset + 1) >> DSB_OPCODE_SHIFT;
> + prev_reg = intel_dsb_read(dsb, dsb->ins_start_offset + 1) & DSB_REG_VALUE_MASK;
>
> return prev_opcode == opcode && prev_reg == i915_mmio_reg_offset(reg);
> }
> @@ -179,6 +140,8 @@ static bool intel_dsb_prev_ins_is_indexed_write(struct intel_dsb *dsb, i915_reg_
> void intel_dsb_reg_write(struct intel_dsb *dsb,
> i915_reg_t reg, u32 val)
> {
> + u32 old_val;
> +
> /*
> * For example the buffer will look like below for 3 dwords for auto
> * increment register:
> @@ -202,31 +165,30 @@ void intel_dsb_reg_write(struct intel_dsb *dsb,
> (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
> i915_mmio_reg_offset(reg));
> } else {
> - u32 *buf = dsb->cmd_buf;
> -
> if (!assert_dsb_has_room(dsb))
> return;
>
> /* convert to indexed write? */
> if (intel_dsb_prev_ins_is_mmio_write(dsb, reg)) {
> - u32 prev_val = buf[dsb->ins_start_offset + 0];
> + u32 prev_val = intel_dsb_read(dsb, dsb->ins_start_offset + 0);
>
> - buf[dsb->ins_start_offset + 0] = 1; /* count */
> - buf[dsb->ins_start_offset + 1] =
> - (DSB_OPCODE_INDEXED_WRITE << DSB_OPCODE_SHIFT) |
> - i915_mmio_reg_offset(reg);
> - buf[dsb->ins_start_offset + 2] = prev_val;
> + intel_dsb_write(dsb, dsb->ins_start_offset + 0, 1); /* count */
> + intel_dsb_write(dsb, dsb->ins_start_offset + 1,
> + (DSB_OPCODE_INDEXED_WRITE << DSB_OPCODE_SHIFT) |
> + i915_mmio_reg_offset(reg));
> + intel_dsb_write(dsb, dsb->ins_start_offset + 2, prev_val);
>
> dsb->free_pos++;
> }
>
> - buf[dsb->free_pos++] = val;
> + intel_dsb_write(dsb, dsb->free_pos++, val);
> /* Update the count */
> - buf[dsb->ins_start_offset]++;
> + old_val = intel_dsb_read(dsb, dsb->ins_start_offset);
> + intel_dsb_write(dsb, dsb->ins_start_offset, old_val + 1);
>
> /* if number of data words is odd, then the last dword should be 0.*/
> if (dsb->free_pos & 0x1)
> - buf[dsb->free_pos] = 0;
> + intel_dsb_write(dsb, dsb->free_pos, 0);
> }
> }
>
> @@ -238,8 +200,8 @@ static void intel_dsb_align_tail(struct intel_dsb *dsb)
> aligned_tail = ALIGN(tail, CACHELINE_BYTES);
>
> if (aligned_tail > tail)
> - memset(&dsb->cmd_buf[dsb->free_pos], 0,
> - aligned_tail - tail);
> + intel_dsb_memset(dsb, dsb->free_pos, 0,
> + aligned_tail - tail);
>
> dsb->free_pos = aligned_tail / 4;
> }
> @@ -277,9 +239,9 @@ void intel_dsb_commit(struct intel_dsb *dsb, bool wait_for_vblank)
> (wait_for_vblank ? DSB_WAIT_FOR_VBLANK : 0) |
> DSB_ENABLE);
> intel_de_write(dev_priv, DSB_HEAD(pipe, dsb->id),
> - i915_ggtt_offset(dsb->vma));
> + intel_dsb_ggtt_offset(dsb));
> intel_de_write(dev_priv, DSB_TAIL(pipe, dsb->id),
> - i915_ggtt_offset(dsb->vma) + tail);
> + intel_dsb_ggtt_offset(dsb) + tail);
> }
>
> void intel_dsb_wait(struct intel_dsb *dsb)
> @@ -325,12 +287,9 @@ struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc,
> unsigned int max_cmds)
> {
> struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> - struct drm_i915_gem_object *obj;
> intel_wakeref_t wakeref;
> struct intel_dsb *dsb;
> - struct i915_vma *vma;
> unsigned int size;
> - u32 *buf;
>
> if (!HAS_DSB(i915))
> return NULL;
> @@ -344,28 +303,11 @@ struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc,
> /* ~1 qword per instruction, full cachelines */
> size = ALIGN(max_cmds * 8, CACHELINE_BYTES);
>
> - obj = i915_gem_object_create_internal(i915, PAGE_ALIGN(size));
> - if (IS_ERR(obj))
> - goto out_put_rpm;
> -
> - vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
> - if (IS_ERR(vma)) {
> - i915_gem_object_put(obj);
> + if (!intel_dsb_buffer_create(crtc, dsb, size))
> goto out_put_rpm;
> - }
> -
> - buf = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WC);
> - if (IS_ERR(buf)) {
> - i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
> - goto out_put_rpm;
> - }
>
> intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>
> - dsb->id = DSB1;
> - dsb->vma = vma;
> - dsb->crtc = crtc;
> - dsb->cmd_buf = buf;
> dsb->size = size / 4; /* in dwords */
> dsb->free_pos = 0;
> dsb->ins_start_offset = 0;
> @@ -382,16 +324,3 @@ struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc,
>
> return NULL;
> }
> -
> -/**
> - * intel_dsb_cleanup() - To cleanup DSB context.
> - * @dsb: DSB context
> - *
> - * This function cleanup the DSB context by unpinning and releasing
> - * the VMA object associated with it.
> - */
> -void intel_dsb_cleanup(struct intel_dsb *dsb)
> -{
> - i915_vma_unpin_and_release(&dsb->vma, I915_VMA_RELEASE_MAP);
> - kfree(dsb);
> -}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index b8148b47022d..7feeb37e00a5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -11,7 +11,41 @@
> #include "i915_reg_defs.h"
>
> struct intel_crtc;
> -struct intel_dsb;
> +struct i915_vma;
> +
> +enum dsb_id {
> + INVALID_DSB = -1,
> + DSB1,
> + DSB2,
> + DSB3,
> + MAX_DSB_PER_PIPE
> +};
> +
> +struct intel_dsb {
> + enum dsb_id id;
> +
> + u32 *cmd_buf;
> + struct i915_vma *vma;
> + struct intel_crtc *crtc;
> +
> + /*
> + * maximum number of dwords the buffer will hold.
> + */
> + unsigned int size;
> +
> + /*
> + * free_pos will point the first free dword and
> + * help in calculating tail of command buffer.
> + */
> + unsigned int free_pos;
> +
> + /*
> + * ins_start_offset will help to store start dword of the dsb
> + * instuction and help in identifying the batch of auto-increment
> + * register.
> + */
> + unsigned int ins_start_offset;
> +};
>
> struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc,
> unsigned int max_cmds);
> @@ -22,5 +56,10 @@ void intel_dsb_reg_write(struct intel_dsb *dsb,
> void intel_dsb_commit(struct intel_dsb *dsb,
> bool wait_for_vblank);
> void intel_dsb_wait(struct intel_dsb *dsb);
> +u32 intel_dsb_ggtt_offset(struct intel_dsb *dsb);
> +void intel_dsb_write(struct intel_dsb *dsb, u32 idx, u32 val);
> +u32 intel_dsb_read(struct intel_dsb *dsb, u32 idx);
> +void intel_dsb_memset(struct intel_dsb *dsb, u32 idx, u32 val, u32 sz);
> +bool intel_dsb_buffer_create(struct intel_crtc *crtc, struct intel_dsb *dsb, u32 size);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb_ops.c b/drivers/gpu/drm/i915/display/intel_dsb_ops.c
> new file mode 100644
> index 000000000000..9fe125abb890
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dsb_ops.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2023, Intel Corporation.
> + */
> +
> +#include "gem/i915_gem_internal.h"
> +#include "i915_drv.h"
> +#include "i915_vma.h"
> +#include "intel_display_types.h"
> +#include "intel_dsb.h"
> +
> +u32 intel_dsb_ggtt_offset(struct intel_dsb *dsb)
> +{
> + return i915_ggtt_offset(dsb->vma);
> +}
> +
> +void intel_dsb_write(struct intel_dsb *dsb, u32 idx, u32 val)
> +{
> + dsb->cmd_buf[idx] = val;
> +}
> +
> +u32 intel_dsb_read(struct intel_dsb *dsb, u32 idx)
> +{
> + return dsb->cmd_buf[idx];
> +}
> +
> +void intel_dsb_memset(struct intel_dsb *dsb, u32 idx, u32 val, u32 sz)
> +{
> + memset(&dsb->cmd_buf[idx], val, sz);
> +}
> +
> +bool intel_dsb_buffer_create(struct intel_crtc *crtc, struct intel_dsb *dsb, u32 size)
> +{
> + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> + struct drm_i915_gem_object *obj;
> + struct i915_vma *vma;
> + u32 *buf;
> +
> + obj = i915_gem_object_create_internal(i915, PAGE_ALIGN(size));
> + if (IS_ERR(obj))
> + return false;
> +
> + vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
> + if (IS_ERR(vma)) {
> + i915_gem_object_put(obj);
> + return false;
> + }
> +
> + buf = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WC);
> + if (IS_ERR(buf)) {
> + i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
> + return false;
> + }
> +
> + dsb->id = DSB1;
> + dsb->vma = vma;
> + dsb->crtc = crtc;
> + dsb->cmd_buf = buf;
> +
> + return true;
> +}
> +
> +void intel_dsb_cleanup(struct intel_dsb *dsb)
> +{
> + i915_vma_unpin_and_release(&dsb->vma, I915_VMA_RELEASE_MAP);
> + kfree(dsb);
> +}
--
Jani Nikula, Intel
next prev parent reply other threads:[~2023-09-25 12:30 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-22 19:35 [Intel-gfx] [PATCH] drm/i915/dsb: DSB code refactoring Animesh Manna
2023-09-23 3:51 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2023-09-23 3:51 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-09-23 4:08 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2023-09-25 12:30 ` Jani Nikula [this message]
2023-09-26 7:13 ` [Intel-gfx] [PATCH] " Manna, Animesh
2023-09-26 9:34 ` Jani Nikula
2023-09-27 13:50 ` Ville Syrjälä
2023-09-27 14:50 ` Jani Nikula
2023-09-27 15:07 ` Ville Syrjälä
2023-09-27 16:04 ` Jani Nikula
2023-09-27 16:17 ` Ville Syrjälä
-- strict thread matches above, loose matches on Subject: below --
2023-09-27 9:11 Animesh Manna
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87lecumo4z.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=animesh.manna@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.