Re: [PATCH v2 10/23] drm/msm/dcp: Add HFI DSI and panel commands

[Dmitry Baryshkov]

On Mon, Oct 26, 2026 at 11:51:20AM +0530, Jayant Shekhar wrote:
> Translate DSI host and panel operations into HFI commands. Firmware
> owns the DSI hardware, so mode validation, display power, panel setup,
> and DCS transfers must cross the HFI interface instead of accessing
> controller registers from Linux.
> 
> Implement DSI mode validation, display session and power management,
> panel initialization, and DCS transfer helpers while keeping them
> separate from generic display commands.
> 
> Assisted-by: LLM
> Signed-off-by: Jayant Shekhar <jayant.shekhar@xxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/Makefile                |   2 +
>  drivers/gpu/drm/msm/dcp/dcp_dsi_panel.h     |  81 ++++
>  drivers/gpu/drm/msm/dcp/dcp_hfi.h           |  25 ++
>  drivers/gpu/drm/msm/dcp/dcp_hfi_dsi.c       | 252 ++++++++++++
>  drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c | 592 ++++++++++++++++++++++++++++

Please stop calling it a panel. There can be any bridge after the DSI
device. From the Linux point of view, drm_panel is a subclass of
drm_bridge.

>  5 files changed, 952 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index bdc5e3b8efa7..77c268aabca6 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -74,6 +74,8 @@ msm-display-$(CONFIG_DRM_MSM_MDP5) += \
>  msm-display-$(CONFIG_DRM_MSM_DCP) += \
>  	dcp/dcp_hfi_core.o \
>  	dcp/dcp_hfi_crtc.o \
> +	dcp/dcp_hfi_dsi.o \
> +	dcp/dcp_hfi_dsi_panel.o \
>  	dcp/dcp_hfi_encoder.o \
>  	dcp/dcp_hfi_kms.o \
>  	dcp/dcp_hfi_plane.o \
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_dsi_panel.h b/drivers/gpu/drm/msm/dcp/dcp_dsi_panel.h
> new file mode 100644
> index 000000000000..a8898e469b44
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_dsi_panel.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * DCP DSI panel configuration shared by the HFI panel implementation and

What if there is a DSI2DPI or DSI2LVDS bridge and then DPI or LVDS
panel?

> + * the DRM DSI bridge.
> + */
> +
> +#ifndef __DCP_DSI_PANEL_H__
> +#define __DCP_DSI_PANEL_H__
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/sizes.h>
> +#include <linux/types.h>
> +
> +#define DCP_DSI_MAX_PHY_TIMINGS	32
> +
> +/**
> + * struct dcp_dsi_hfi_panel_config - Complete HFI PANEL_INIT configuration.
> + *
> + * This contains the bridge-owned panel configuration plus the
> + * mode-dependent values assembled before the HFI PANEL_INIT command.
> + */
> +struct dcp_dsi_hfi_panel_config {
> +	u32 display_type;
> +	u32 num_ctrls;
> +	u32 ctrl_indices[4];
> +	u32 num_phys;
> +	u32 phy_indices[4];
> +	u32 vsync_source;
> +	u32 te_dcs_command;
> +	u32 dma_trigger;
> +	u32 stream_trigger;
> +	u32 wr_mem_start;
> +	u32 wr_mem_continue;
> +	u32 dma_sched_line;
> +	u32 dma_sched_window;
> +	u32 phy_type;
> +	bool cphy_mode;
> +	bool lp11_init;
> +	bool ulps_supported;
> +	bool bllp_eof_power_mode;
> +	bool bllp_power_mode;
> +	u32 lane_map;
> +	u32 color_order;
> +	u32 esd_mode;
> +	u32 jitter_numer;
> +	u32 jitter_denom;
> +	dma_addr_t dcs_tx_dva;
> +	dma_addr_t dcs_tx_iova;
> +	u32 dcs_tx_buf_size;
> +	u32 phy_timings[DCP_DSI_MAX_PHY_TIMINGS];
> +	u32 num_phy_timings;
> +	bool dsc_enabled;
> +	u32 dsc_version_major;
> +	u32 dsc_version_minor;
> +	u32 dsc_scr_version;
> +	u32 dsc_slice_height;
> +	u32 dsc_slice_width;
> +	u32 dsc_slices_per_pkt;
> +	u32 dsc_bits_per_component;
> +	u32 dsc_pps_delay_ms;
> +	u32 dsc_bits_per_pixel;
> +	u32 dsc_chroma_format;
> +	u32 dsc_color_space;
> +	u32 dsc_block_prediction_enable;

Why do you need to copy those? Can't you use struct drm_dsc directly?
Having it as a non-NULL pointer would have saved you from having
the dsc_enabled flag.

> +	/* Optional DSC rate-control override panel configuration. */
> +	bool dsc_rc_override;
> +	u8 dsc_rc_min_qp[15];
> +	u8 dsc_rc_max_qp[15];
> +	u8 dsc_rc_bpg_offsets[15];
> +	u32 topology_mixer_count;
> +	u32 topology_enc_count;
> +	u32 topology_intf_count;
> +};
> +
> +/* DCS buffers use IOVAs directly in the firmware command descriptor. */
> +#define DCP_DSI_DCS_TX_BUF_SIZE	SZ_4K
> +#define DCP_DSI_DCS_RX_BUF_SIZE	SZ_4K
> +
> +#endif /* __DCP_DSI_PANEL_H__ */
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_hfi.h b/drivers/gpu/drm/msm/dcp/dcp_hfi.h
> index 6d3eadb5e811..88d73de69383 100644
> --- a/drivers/gpu/drm/msm/dcp/dcp_hfi.h
> +++ b/drivers/gpu/drm/msm/dcp/dcp_hfi.h
> @@ -242,6 +242,31 @@ int dcp_hfi_send_resource_register(struct dcp_hfi *hfi);
>  int dcp_hfi_set_display_property(struct dcp_hfi *hfi, u32 session_id,
>  				 u32 prop_id, u32 value);
>  
> +struct dcp_dsi_hfi_panel_config;
> +int dcp_hfi_panel_init(struct dcp_hfi *hfi, u32 intf_idx,
> +		       const struct drm_display_mode *mode,
> +		       const struct mipi_dsi_device *dsi,
> +		       const struct dcp_dsi_hfi_panel_config *cfg);
> +int dcp_hfi_display_enable(struct dcp_hfi *hfi, u32 intf_idx,
> +			   u32 display_id, u32 *session_id);
> +int dcp_hfi_display_close(struct dcp_hfi *hfi, u32 session_id);
> +int dcp_hfi_display_mode_validate(struct dcp_hfi *hfi, u32 session_id,
> +				  const struct drm_display_mode *mode,
> +				  u64 clk_rate_hz);
> +int dcp_hfi_display_set_mode(struct dcp_hfi *hfi, u32 session_id,
> +			     const struct drm_display_mode *mode,
> +			     u64 clk_rate_hz);
> +int dcp_hfi_display_power_register(struct dcp_hfi *hfi, u32 session_id);
> +int dcp_hfi_display_power_control_ack(struct dcp_hfi *hfi, u32 session_id,
> +				      u32 packet_id);
> +int dcp_hfi_display_post_enable(struct dcp_hfi *hfi, u32 session_id);
> +int dcp_hfi_display_disable(struct dcp_hfi *hfi, u32 session_id);
> +int dcp_hfi_display_post_disable(struct dcp_hfi *hfi, u32 session_id);
> +ssize_t dcp_hfi_dcs_transfer(struct dcp_hfi *hfi, u32 session_id,
> +			     const struct mipi_dsi_msg *msg,
> +			     void *tx_buf, dma_addr_t tx_iova,
> +			     void *rx_buf, dma_addr_t rx_iova,
> +			     u32 post_wait_ms, u32 ctrl_idx);
>  #endif /* __DCP_HFI_H__ */
>  void dcp_hfi_set_power_control_cb(struct dcp_hfi *hfi, void *priv,
>  				  void (*cb)(void *priv, u32 display_id,
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi.c b/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi.c
> new file mode 100644
> index 000000000000..fbeec03cf90d
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * HFI commands for the DSI display-session lifecycle.
> + * Panel initialization and DCS transfers are implemented in dcp_hfi_dsi_panel.c.
> + */
> +
> +#include <linux/device.h>
> +#include <drm/drm_modes.h>
> +
> +#include "dcp_hfi.h"
> +#include "dcp_hfi_priv.h"
> +#include "hfi/hfi_commands_display.h"
> +#include "hfi/hfi_defs_common.h"
> +#include "hfi/hfi_defs_display.h"
> +#include "hfi/hfi_packet.h"
> +#include "hfi/hfi_packer.h"
> +#include "hfi/hfi_properties_display.h"
> +
> +static void dcp_hfi_fill_mode_info(struct hfi_display_mode_info *info,
> +				   const struct drm_display_mode *mode)
> +{
> +	memset(info, 0, sizeof(*info));
> +	info->size            = sizeof(*info);
> +	info->h_active        = mode->hdisplay;
> +	info->h_back_porch    = mode->htotal - mode->hsync_end;
> +	info->h_sync_width    = mode->hsync_end - mode->hsync_start;
> +	info->h_front_porch   = mode->hsync_start - mode->hdisplay;
> +	info->h_skew          = mode->hskew;
> +	info->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
> +	info->v_active        = mode->vdisplay;
> +	info->v_back_porch    = mode->vtotal - mode->vsync_end;
> +	info->v_sync_width    = mode->vsync_end - mode->vsync_start;
> +	info->v_front_porch   = mode->vsync_start - mode->vdisplay;
> +	info->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
> +	info->refresh_rate    = drm_mode_vrefresh(mode);
> +	info->clk_rate_hz_lo = lower_32_bits((u64)mode->clock * 1000);
> +	info->clk_rate_hz_hi = upper_32_bits((u64)mode->clock * 1000);
> +}
> +
> +static void dcp_hfi_dump_mode_info(struct dcp_hfi *hfi, const char *cmd,
> +				   u32 session_id,
> +				   const struct hfi_display_mode_info *info)
> +{
> +	dev_dbg(hfi->dev,
> +		"HFI %s s=%u h=%u/%u/%u/%u v=%u/%u/%u/%u p=%u/%u r=%u clk=%llu sz=%u\n",
> +		cmd, session_id,
> +		info->h_active, info->h_back_porch, info->h_sync_width,
> +		info->h_front_porch,
> +		info->v_active, info->v_back_porch, info->v_sync_width,
> +		info->v_front_porch,
> +		info->h_sync_polarity, info->v_sync_polarity,
> +		info->refresh_rate,
> +		(u64)info->clk_rate_hz_lo | ((u64)info->clk_rate_hz_hi << 32),
> +		info->size);
> +}
> +
> +int dcp_hfi_display_mode_validate(struct dcp_hfi *hfi, u32 session_id,
> +				  const struct drm_display_mode *mode, u64 clk_rate_hz)
> +{
> +	struct hfi_display_mode_info info;
> +	int ret;
> +
> +	dcp_hfi_fill_mode_info(&info, mode);
> +	info.clk_rate_hz_lo = lower_32_bits(clk_rate_hz);
> +	info.clk_rate_hz_hi = upper_32_bits(clk_rate_hz);
> +	dcp_hfi_dump_mode_info(hfi, "HFI MODE_VALIDATE", session_id, &info);
> +
> +	ret = dcp_hfi_send_sync(hfi,
> +				HFI_COMMAND_DISPLAY_MODE_VALIDATE,
> +				HFI_CMD_BUFF_DISPLAY, session_id,
> +				HFI_PAYLOAD_U32_ARRAY,
> +				&info, sizeof(info),
> +				HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				HFI_TX_FLAGS_NON_DISCARDABLE,
> +				NULL, 0,
> +				DCP_HFI_TIMEOUT_MODE_MS);
> +
> +	return ret;
> +}
> +
> +int dcp_hfi_display_set_mode(struct dcp_hfi *hfi, u32 session_id,
> +			     const struct drm_display_mode *mode, u64 clk_rate_hz)
> +{
> +	struct hfi_display_mode_info info;
> +	int ret;
> +
> +	dcp_hfi_fill_mode_info(&info, mode);
> +	info.clk_rate_hz_lo = lower_32_bits(clk_rate_hz);
> +	info.clk_rate_hz_hi = upper_32_bits(clk_rate_hz);
> +	dcp_hfi_dump_mode_info(hfi, "HFI SET_MODE", session_id, &info);
> +
> +	ret = dcp_hfi_send_sync(hfi,
> +				HFI_COMMAND_DISPLAY_SET_MODE,
> +				HFI_CMD_BUFF_DISPLAY, session_id,
> +				HFI_PAYLOAD_U32_ARRAY,
> +				&info, sizeof(info),
> +				HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				HFI_TX_FLAGS_NON_DISCARDABLE,
> +				NULL, 0,
> +				DCP_HFI_TIMEOUT_MODE_MS);
> +
> +	return ret;
> +}
> +
> +int dcp_hfi_display_power_register(struct dcp_hfi *hfi, u32 session_id)
> +{
> +	return dcp_hfi_send_sync(hfi,
> +				 HFI_COMMAND_DISPLAY_POWER_REGISTER,
> +				 HFI_CMD_BUFF_DISPLAY, session_id,
> +				 HFI_PAYLOAD_NONE, NULL, 0,
> +				 HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				 HFI_TX_FLAGS_NON_DISCARDABLE,
> +				 NULL, 0,
> +				 DCP_HFI_TIMEOUT_DISPLAY_MS);
> +}
> +
> +int dcp_hfi_display_power_control_ack(struct dcp_hfi *hfi, u32 session_id,
> +				      u32 packet_id)
> +{
> +	struct dcp_hfi_buf *buf;
> +	struct hfi_cmd_buff_hdl hdl;
> +	struct hfi_header_info hdr_info;
> +	struct hfi_packet_info pkt_info;
> +	int ret;
> +
> +	buf = dcp_hfi_buf_alloc(hfi);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	hdl.cmd_buffer = buf->data;
> +	hdl.size       = DCP_HFI_BUF_SIZE;
> +
> +	hdr_info.num_packets   = 0;
> +	hdr_info.cmd_buff_type = HFI_CMD_BUFF_DISPLAY;
> +	hdr_info.object_id     = session_id;
> +	hdr_info.header_id     = dcp_hfi_next_packet_id(hfi);
> +
> +	ret = hfi_create_header(&hdl, &hdr_info);
> +	if (ret)
> +		goto err_free_buf;
> +
> +	pkt_info.cmd          = HFI_COMMAND_DISPLAY_POWER_CONTROL;
> +	pkt_info.id           = session_id & 0xFFFF;
> +	pkt_info.flags        = HFI_RX_FLAGS_SUCCESS;
> +	pkt_info.packet_id    = packet_id;
> +	pkt_info.payload_type = HFI_PAYLOAD_NONE;
> +	pkt_info.payload_size = 0;
> +	pkt_info.payload_ptr  = NULL;
> +
> +	ret = hfi_create_packet_header(&hdl, &pkt_info);
> +	if (ret)
> +		goto err_free_buf;
> +
> +	return dcp_hfi_send_cmd_async(hfi, buf, hfi_get_header_size(&hdl), NULL);
> +
> +err_free_buf:
> +	dcp_hfi_buf_free(hfi, buf);
> +	return ret;
> +}
> +
> +int dcp_hfi_display_enable(struct dcp_hfi *hfi, u32 intf_idx, u32 display_id,
> +			   u32 *session_id)
> +{
> +	int ret;
> +
> +	ret = dcp_hfi_send_sync(hfi,
> +				HFI_COMMAND_DISPLAY_ENABLE,
> +				HFI_CMD_BUFF_DISPLAY, display_id,
> +				HFI_PAYLOAD_NONE, NULL, 0,
> +				HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				HFI_TX_FLAGS_NON_DISCARDABLE,
> +				NULL, 0,
> +				DCP_HFI_TIMEOUT_DISPLAY_MS);
> +	if (ret) {
> +		dev_err(hfi->dev,
> +			"HFI DISPLAY_ENABLE intf=%u display_id=%u failed: %d\n",
> +			intf_idx, display_id, ret);
> +		return ret;
> +	}
> +
> +	*session_id = display_id;
> +
> +	return 0;
> +}
> +
> +int dcp_hfi_display_post_enable(struct dcp_hfi *hfi, u32 session_id)
> +{
> +	return dcp_hfi_send_sync(hfi,
> +				 HFI_COMMAND_DISPLAY_POST_ENABLE,
> +				 HFI_CMD_BUFF_DISPLAY, session_id,
> +				 HFI_PAYLOAD_NONE, NULL, 0,
> +				 HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				 HFI_TX_FLAGS_NON_DISCARDABLE,
> +				 NULL, 0,
> +				 DCP_HFI_TIMEOUT_DISPLAY_MS);
> +}
> +
> +int dcp_hfi_display_disable(struct dcp_hfi *hfi, u32 session_id)
> +{
> +	return dcp_hfi_send_sync(hfi,
> +				 HFI_COMMAND_DISPLAY_DISABLE,
> +				 HFI_CMD_BUFF_DISPLAY, session_id,
> +				 HFI_PAYLOAD_NONE, NULL, 0,
> +				 HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				 HFI_TX_FLAGS_NON_DISCARDABLE,
> +				 NULL, 0,
> +				 DCP_HFI_TIMEOUT_DISPLAY_MS);
> +}
> +
> +int dcp_hfi_display_post_disable(struct dcp_hfi *hfi, u32 session_id)
> +{
> +	return dcp_hfi_send_sync(hfi,
> +				 HFI_COMMAND_DISPLAY_POST_DISABLE,
> +				 HFI_CMD_BUFF_DISPLAY, session_id,
> +				 HFI_PAYLOAD_NONE, NULL, 0,
> +				 HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				 HFI_TX_FLAGS_NON_DISCARDABLE,
> +				 NULL, 0,
> +				 DCP_HFI_TIMEOUT_DISPLAY_MS);
> +}
> +
> +int dcp_hfi_display_close(struct dcp_hfi *hfi, u32 session_id)
> +{
> +	int ret;
> +
> +	ret = dcp_hfi_display_disable(hfi, session_id);
> +	if (ret) {
> +		dev_warn(hfi->dev, "HFI DISPLAY_DISABLE failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = dcp_hfi_display_post_disable(hfi, session_id);
> +	if (ret) {
> +		dev_warn(hfi->dev, "HFI DISPLAY_POST_DISABLE failed: %d\n", ret);
> +		return ret;
> +	}

Why are there two types of calls, pre and post? Should they go to the
corresponding separate DRM callbacks?

> +
> +	ret = dcp_hfi_send_sync(hfi,
> +				HFI_COMMAND_DISPLAY_CLOSE,
> +				HFI_CMD_BUFF_DISPLAY, session_id,
> +				HFI_PAYLOAD_NONE, NULL, 0,
> +				HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				HFI_TX_FLAGS_NON_DISCARDABLE,
> +				NULL, 0,
> +				DCP_HFI_TIMEOUT_DISPLAY_MS);
> +	if (ret)
> +		dev_warn(hfi->dev, "HFI DISPLAY_CLOSE failed: %d\n", ret);
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c b/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c
> new file mode 100644
> index 000000000000..b27fa8fa3160
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c
> @@ -0,0 +1,592 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * HFI commands for panel initialization and DCS transfers.
> + * Firmware owns the DSI controller and PHY.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/math64.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_mipi_dsi.h>
> +
> +#include "dcp_dsi_panel.h"
> +#include "dcp_hfi.h"
> +#include "dcp_hfi_priv.h"
> +#include "hfi/hfi_commands_display.h"
> +#include "hfi/hfi_commands_panel.h"
> +#include "hfi/hfi_defs_common.h"
> +#include "hfi/hfi_defs_panel.h"
> +#include "hfi/hfi_packet.h"
> +#include "hfi/hfi_packer.h"
> +#include "hfi/hfi_properties_panel.h"
> +
> +#define DCP_DSI_CTRL_CMD_READ              BIT(0)
> +#define DCP_DSI_CTRL_CMD_FETCH_MEMORY      BIT(5)
> +#define DCP_DSI_CTRL_CMD_LAST_COMMAND      BIT(6)
> +
> +static u32 dcp_bpp_to_hfi(u32 bpp)
> +{
> +	switch (bpp) {
> +	case 16: return HFI_PANEL_BPP_16;
> +	case 18: return HFI_PANEL_BPP_18;
> +	case 30: return HFI_PANEL_BPP_30;
> +	case 24:
> +	default: return HFI_PANEL_BPP_24;
> +	}
> +}
> +
> +static u32 dcp_dsi_traffic_mode(const struct mipi_dsi_device *dsi)
> +{
> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO))
> +		return HFI_PANEL_TRAFFIC_NON_BURST_SYNC_EVENT_MODE;
> +	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> +		return HFI_PANEL_TRAFFIC_BURST_MODE;
> +	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> +		return HFI_PANEL_TRAFFIC_NON_BURST_SYNC_PULSE_MODE;
> +	return HFI_PANEL_TRAFFIC_NON_BURST_SYNC_EVENT_MODE;
> +}
> +
> +static u32 dcp_lanes_to_hfi(u32 num_lanes)
> +{
> +	u32 mask = 0;
> +
> +	if (num_lanes >= 1)
> +		mask |= HFI_PANEL_LANE_0;

There is always at least one lane. Set the mask to HFI_PANEL_LANE_0.

> +	if (num_lanes >= 2)
> +		mask |= HFI_PANEL_LANE_1;
> +	if (num_lanes >= 3)
> +		mask |= HFI_PANEL_LANE_2;
> +	if (num_lanes >= 4)
> +		mask |= HFI_PANEL_LANE_3;
> +
> +	return mask ?: HFI_PANEL_LANE_0;
> +}
> +
> +static bool dcp_hfi_panel_wait(struct dcp_hfi *hfi,
> +			       struct dcp_hfi_completion *comp)
> +{
> +	unsigned long flags;
> +
> +	if (wait_for_completion_timeout(&comp->done,
> +					msecs_to_jiffies(DCP_HFI_TIMEOUT_DISPLAY_MS)))
> +		return false;
> +
> +	/* Serialize removal with response dispatch before the stack object dies. */
> +	spin_lock_irqsave(&hfi->resp_drain_lock, flags);
> +	dcp_hfi_pending_find(hfi, comp->packet_id);
> +	spin_unlock_irqrestore(&hfi->resp_drain_lock, flags);
> +	return true;
> +}
> +
> +int dcp_hfi_panel_init(struct dcp_hfi *hfi, u32 intf_idx,
> +		       const struct drm_display_mode *mode,
> +		       const struct mipi_dsi_device *dsi,
> +		       const struct dcp_dsi_hfi_panel_config *cfg)
> +{
> +	u32 num_lanes;
> +	u32 bpp;
> +	bool is_cmd_mode;
> +	u32 display_type;
> +	u32 vsync_source;
> +	u32 traffic_mode;
> +	u32 phy_type;
> +
> +	struct dcp_hfi_buf *buf1, *buf2;
> +	struct hfi_cmd_buff_hdl hdl1, hdl2;
> +	struct hfi_header_info hdr_info1, hdr_info2;
> +	struct hfi_packet_info pkt_info;
> +	struct dcp_hfi_completion comp1, comp2, comp3;
> +	u32 payload[128];
> +	u32 *p;
> +	u32 packet_id, packet_id2, packet_id3;
> +	u64 clk_hz;
> +	int ret;
> +	bool timeout1, timeout2, timeout3;
> +	u32 link_bpp_x16;

Reverse christmas tree, apply universally to all your code.

> +
> +	if (!hfi || !mode || !dsi || !dsi->lanes || !cfg)
> +		return -EINVAL;

And which of those can actually be passed as NULL?

> +	if (cfg->num_ctrls > ARRAY_SIZE(cfg->ctrl_indices) ||
> +	    cfg->num_phys > ARRAY_SIZE(cfg->phy_indices))
> +		return -EINVAL;
> +
> +	display_type = cfg->display_type;
> +	vsync_source = cfg->vsync_source;
> +	phy_type = cfg->phy_type;
> +	num_lanes = dsi->lanes;
> +	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +	if (bpp < 0)
> +		return -EINVAL;
> +	is_cmd_mode = !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
> +	traffic_mode = dcp_dsi_traffic_mode(dsi);
> +	link_bpp_x16 = bpp << 4;
> +
> +	buf1 = dcp_hfi_buf_alloc(hfi);
> +	if (!buf1) {
> +		dev_err(hfi->dev, "HFI PANEL_INIT: TX buffer exhausted\n");
> +		return -ENOMEM;
> +	}
> +
> +	hdl1.cmd_buffer = buf1->data;
> +	hdl1.size       = DCP_HFI_BUF_SIZE;
> +
> +	hdr_info1.num_packets   = 0;
> +	hdr_info1.cmd_buff_type = HFI_CMD_BUFF_DISPLAY;
> +	hdr_info1.object_id     = intf_idx;
> +	hdr_info1.header_id     = dcp_hfi_next_packet_id(hfi);
> +
> +	ret = hfi_create_header(&hdl1, &hdr_info1);
> +	if (ret) {
> +		dev_err(hfi->dev, "PANEL_INIT: hfi_create_header (A) failed: %d\n",
> +			ret);
> +		goto err_free_buf1;
> +	}
> +
> +	{

Jayant, you really like to ignore comments. Please stop doing that. And
also please stop having unconditional nested blocks. If you need one,
extract it to a function.

> +		u32 num_props = 0;
> +
> +		p = payload + 1;
> +		num_props++;
> +		p += HFI_PACK(p, HFI_PROPERTY_PANEL_TIMING_MODE_COUNT, 1);
> +
> +		/* DCS TX buffer is described by both DVA and IOVA properties. */
> +		if (cfg->dcs_tx_iova && cfg->dcs_tx_dva &&
> +		    !(cfg->dcs_tx_iova & 0x3ff) && !(cfg->dcs_tx_dva & 0x3ff)) {
> +			num_props++;
> +			p += HFI_PACK(p, HFI_PROPERTY_PANEL_DCS_CMD_TX_BUF_DVA,
> +				       lower_32_bits(cfg->dcs_tx_dva),
> +				       upper_32_bits(cfg->dcs_tx_dva),
> +				       cfg->dcs_tx_buf_size, 0, 0);
> +			num_props++;
> +			p += HFI_PACK(p, HFI_PROPERTY_PANEL_DCS_CMD_TX_BUF_IOVA,
> +				       lower_32_bits(cfg->dcs_tx_iova),
> +				       upper_32_bits(cfg->dcs_tx_iova),
> +				       cfg->dcs_tx_buf_size, 0, 0);
> +		}
> +		payload[0] = num_props;
> +	}
> +

-- 
With best wishes
Dmitry