Hi Animesh,
> -----Original Message-----
> From: Manna, Animesh <animesh.ma...@intel.com>
> Sent: Wednesday, May 21, 2025 10:17 AM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.bo...@intel.com>; intel-
> x...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: ville.syrj...@linux.intel.com; Shankar, Uma <uma.shan...@intel.com>
> Subject: RE: [v7 04/11] drm/i915/dsb: Implement intel_dsb_gosub()
>
>
>
> > -----Original Message-----
> > From: Borah, Chaitanya Kumar <chaitanya.kumar.bo...@intel.com>
> > Sent: Tuesday, May 20, 2025 1:26 PM
> > To: intel...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> > Cc: ville.syrj...@linux.intel.com; Shankar, Uma
> > <uma.shan...@intel.com>; Manna, Animesh
> <animesh.ma...@intel.com>;
> > Borah, Chaitanya Kumar <chaitanya.kumar.bo...@intel.com>
> > Subject: [v7 04/11] drm/i915/dsb: Implement intel_dsb_gosub()
> >
> > From: Ville Syrjälä <ville.syrj...@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.syrj...@linux.intel.com>
> > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.bo...@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.
*/
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
