[PATCH] drm/i915/display: Send DSI DCS commands with pipeline flush in High Speed

[Yu, Gareth]

> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal at intel.com>
> Sent: Friday, May 9, 2025 12:17 PM
> To: Yu, Gareth <gareth.yu at intel.com>; intel-gfx at lists.freedesktop.org; intel-
> xe at lists.freedesktop.org
> Cc: Yu, Gareth <gareth.yu at intel.com>; Nikula, Jani <jani.nikula at intel.com>
> Subject: RE: [PATCH] drm/i915/display: Send DSI DCS commands with pipeline
> flush in High Speed
> 
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
> > gareth.yu at intel.com
> > Sent: Friday, May 9, 2025 7:43 AM
> > To: intel-gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> > Cc: Yu, Gareth <gareth.yu at intel.com>
> > Subject: [PATCH] drm/i915/display: Send DSI DCS commands with pipeline
> > flush in High Speed
> >
> 
> Clip the subject make it a little smaller

How about "Always send high speed commands with pipeline flush"?

> 
> > From: Gareth Yu <gareth.yu at intel.com>
> >
> > With all of the boundary conditions when streaming the commands B2B in
> > our validation (part of the reason we added the flush),  the Flush
> > effectively serializes the transmission of each command enqueued
> > within the command dispatcher to one per V. Blank line which
> > simplifies the behavior of the High Speed Arbitration.
> >
> > So, unless we absolutely have to burst these to the Sink, we should be
> > using the Pipeline Flush bit to serialize the commands.
> 
> Do we have a bspec reference and/or dsi spec reference through which we can
> verify the data that needs to be filled out for the dsi packet header.
> Add it here will help make the review smoother and easier

BSPEC: 50193

> 
> 
> >
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14247
> >
> > Cc : Suraj Kandpal <suraj.kandpal at intel.com> Cc : Jani Nikula
> > <jani.nikula at intel.com>
> > Signed-off-by: Gareth Yu <gareth.yu at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/icl_dsi.c      | 4 ++--
> >  drivers/gpu/drm/i915/display/icl_dsi_regs.h | 1 +
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> > b/drivers/gpu/drm/i915/display/icl_dsi.c
> > index 402b7b2e1829..f91f27067768 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > @@ -191,12 +191,12 @@ static int dsi_send_pkt_hdr(struct
> > intel_dsi_host *host,
> >  	else
> >  		tmp &= ~PAYLOAD_PRESENT;
> >
> > -	tmp &= ~VBLANK_FENCE;
> > +	tmp &= ~(VBLANK_FENCE | LP_DATA_TRANSFER | PIPELINE_FLUSH);
> >
> >  	if (enable_lpdt)
> >  		tmp |= LP_DATA_TRANSFER;
> >  	else
> > -		tmp &= ~LP_DATA_TRANSFER;
> > +		tmp |= PIPELINE_FLUSH;
> 
> Seems a little fishy but cannot be sure till I have the spec and I can verify it
> 
> >
> >  	tmp &= ~(PARAM_WC_MASK | VC_MASK | DT_MASK);
> >  	tmp |= ((packet->header[0] & VC_MASK) << VC_SHIFT); diff --git
> > a/drivers/gpu/drm/i915/display/icl_dsi_regs.h
> > b/drivers/gpu/drm/i915/display/icl_dsi_regs.h
> > index d4845ac65acc..b601b7632339 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi_regs.h
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi_regs.h
> > @@ -272,6 +272,7 @@
> >  #define  PAYLOAD_PRESENT		(1 << 31)
> >  #define  LP_DATA_TRANSFER		(1 << 30)
> >  #define  VBLANK_FENCE			(1 << 29)
> > +#define  PIPELINE_FLUSH			(1 << 28)
> 
> We now follow the REG_BIT(28) macro instead of using  (1 << 28) convention
> Would like to see that change too, ill send over a fix to get this to file use
> REG_BIT instead.
> Or do we leave it as is ? What do you think Jani ?
> 
> Regards,
> Suraj Kandpal
> 
> >  #define  PARAM_WC_MASK			(0xffff << 8)
> >  #define  PARAM_WC_LOWER_SHIFT		8
> >  #define  PARAM_WC_UPPER_SHIFT		16
> > --
> > 2.34.1