[v7 04/11] drm/i915/dsb: Implement intel_dsb_gosub()
Manna, Animesh
animesh.manna at intel.com
Thu May 22 04:18:54 UTC 2025
> -----Original Message-----
> From: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>
> Sent: Wednesday, May 21, 2025 2:03 PM
> To: Manna, Animesh <animesh.manna at intel.com>; intel-
> xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> Cc: ville.syrjala at linux.intel.com; Shankar, Uma <uma.shankar at intel.com>
> Subject: RE: [v7 04/11] drm/i915/dsb: Implement intel_dsb_gosub()
>
> Hi Animesh,
>
> > -----Original Message-----
> > From: Manna, Animesh <animesh.manna at intel.com>
> > Sent: Wednesday, May 21, 2025 10:17 AM
> > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>; intel-
> > xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> > Cc: ville.syrjala at linux.intel.com; Shankar, Uma
> > <uma.shankar at intel.com>
> > Subject: RE: [v7 04/11] drm/i915/dsb: Implement intel_dsb_gosub()
> >
> >
> >
> > > -----Original Message-----
> > > From: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>
> > > Sent: Tuesday, May 20, 2025 1:26 PM
> > > To: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> > > Cc: ville.syrjala at linux.intel.com; Shankar, Uma
> > > <uma.shankar at intel.com>; Manna, Animesh
> > <animesh.manna at intel.com>;
> > > Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>
> > > Subject: [v7 04/11] drm/i915/dsb: Implement intel_dsb_gosub()
> > >
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > Add support for the new GOSUB DSB instruction (available on ptl+),
> > > which instructs the DSB to jump to a different buffer, executie the
> > > commands there, and then return execution to the next instruction in
> > > the
> > original buffer.
> > >
> > > There are a few alignment related workarounds that need to be dealt
> > > with when emitting GOSUB instruction.
> > >
> > > v2: Right shift head and tail pointer passed to gosub command
> > > (chaitanya)
> > > v3: Add macro for right shifting head/tail pointers (Animesh)
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Signed-off-by: Chaitanya Kumar Borah
> > > <chaitanya.kumar.borah at intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dsb.c | 57
> > > ++++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_dsb.h |
> > > ++++++++++++++++++++++++ 2
> > > +
> > > 2 files changed, 59 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > index 9b2de4e7e681..dad483d4b1cf 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > @@ -93,6 +93,10 @@ struct intel_dsb {
> > > /* see DSB_REG_VALUE_MASK */
> > > #define DSB_OPCODE_POLL 0xA
> > > /* see DSB_REG_VALUE_MASK */
> > > +#define DSB_OPCODE_GOSUB 0xC /* ptl+ */
> > > +#define DSB_GOSUB_HEAD_SHIFT 26
> > > +#define DSB_GOSUB_TAIL_SHIFT 0
> > > +#define DSB_GOSUB_CONVERT_ADDR(x) ((x) >> 6)
> >
> > Please add a code comment what is special about GOSUB and why the
> > above conversion is needed which was pointed out in previous review.
> > Otherwise the other changes LGTM.
> >
>
> Thank you for the review.
> Apologies for missing the comment.
>
> Does the following text within the intel_dsb_gosub() work?
>
> /*
> * The GOSUB instruction has the following memory layout.
> *
> * +---------------------------------------------------------------------+
> * | Opcode | Rsvd | Head Ptr | Tail Ptr |
> * | 0x0c | | | |
> * +----------------------------------------------------------------------+
> * |<- 8bits->|<- 4bits ->|<-- 26bits -->|<-- 26bits -->|
> *
> * We have only 26 bits each to represent the head and tail
> * pointers even though the addresses itself are of 32 bit. However, this
> * is not a problem because the addresses are 64 bit aligned and
> therefore
> * the last 6 bits are always Zero's. Therefore, we right shift the address
> * by 6 before embedding it into the GOSUB instruction.
> */
LGTM. With the above change
Reviewed-by: Animesh Manna <animesh.manna at intel.com>
>
> Regards
>
> Chaitanya
>
> > Regards,
> > Animesh
> > >
> > > static bool pre_commit_is_vrr_active(struct intel_atomic_state *state,
> > > struct intel_crtc *crtc)
> > > @@ -533,6 +537,59 @@ static void intel_dsb_align_tail(struct
> > > intel_dsb
> > > *dsb)
> > > dsb->free_pos = aligned_tail / 4;
> > > }
> > >
> > > +static void intel_dsb_gosub_align(struct intel_dsb *dsb) {
> > > + u32 aligned_tail, tail;
> > > +
> > > + intel_dsb_ins_align(dsb);
> > > +
> > > + tail = dsb->free_pos * 4;
> > > + aligned_tail = ALIGN(tail, CACHELINE_BYTES);
> > > +
> > > + /*
> > > + * "The GOSUB instruction cannot be placed in
> > > + * cacheline QW slot 6 or 7 (numbered 0-7)"
> > > + */
> > > + if (aligned_tail - tail <= 2 * 8)
> > > + intel_dsb_buffer_memset(&dsb->dsb_buf, dsb->free_pos, 0,
> > > + aligned_tail - tail);
> > > +
> > > + dsb->free_pos = aligned_tail / 4;
> > > +}
> > > +
> > > +void intel_dsb_gosub(struct intel_dsb *dsb,
> > > + struct intel_dsb *sub_dsb)
> > > +{
> > > + struct intel_crtc *crtc = dsb->crtc;
> > > + struct intel_display *display = to_intel_display(crtc->base.dev);
> > > + unsigned int head, tail;
> > > + u64 head_tail;
> > > +
> > > + if (drm_WARN_ON(display->drm, dsb->id != sub_dsb->id))
> > > + return;
> > > +
> > > + if (!assert_dsb_tail_is_aligned(sub_dsb))
> > > + return;
> > > +
> > > + intel_dsb_gosub_align(dsb);
> > > +
> > > + head = intel_dsb_head(sub_dsb);
> > > + tail = intel_dsb_tail(sub_dsb);
> > > +
> > > + head_tail = ((u64)(DSB_GOSUB_CONVERT_ADDR(head)) <<
> > > DSB_GOSUB_HEAD_SHIFT) |
> > > + ((u64)(DSB_GOSUB_CONVERT_ADDR(tail)) <<
> > > DSB_GOSUB_TAIL_SHIFT);
> > > +
> > > + intel_dsb_emit(dsb, lower_32_bits(head_tail),
> > > + (DSB_OPCODE_GOSUB << DSB_OPCODE_SHIFT) |
> > > + upper_32_bits(head_tail));
> > > +
> > > + /*
> > > + * "NOTE: the instructions within the cacheline
> > > + * FOLLOWING the GOSUB instruction must be NOPs."
> > > + */
> > > + intel_dsb_align_tail(dsb);
> > > +}
> > > +
> > > void intel_dsb_finish(struct intel_dsb *dsb) {
> > > 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 e843c52bf97c..8b2cf0a7b7e6 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> > > @@ -57,6 +57,8 @@ void intel_dsb_vblank_evade(struct
> > > intel_atomic_state *state, void intel_dsb_poll(struct intel_dsb *dsb,
> > > i915_reg_t reg, u32 mask, u32 val,
> > > int wait_us, int count);
> > > +void intel_dsb_gosub(struct intel_dsb *dsb,
> > > + struct intel_dsb *sub_dsb);
> > > void intel_dsb_chain(struct intel_atomic_state *state,
> > > struct intel_dsb *dsb,
> > > struct intel_dsb *chained_dsb,
> > > --
> > > 2.25.1
More information about the Intel-gfx
mailing list