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

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 09 May 2025, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> gareth.yu@intel.com
>> Sent: Friday, May 9, 2025 7:43 AM
>> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Cc: Yu, Gareth <gareth.yu@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
>
>> From: Gareth Yu <gareth.yu@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
>
>
>> 
>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14247
>> 
>> Cc : Suraj Kandpal <suraj.kandpal@intel.com> Cc : Jani Nikula
>> <jani.nikula@intel.com>
>> Signed-off-by: Gareth Yu <gareth.yu@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 ?

Fixes first, conversion to REG_BIT() and friends later.

BR,
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
>

-- 
Jani Nikula, Intel