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

[Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>]

On 27-10-2026 16:36, Dmitry Baryshkov wrote:
> 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@oss.qualcomm.com>
>> ---
>>   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.
>

Will remove in v3.


>> +	/* 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?
>

This display close function would be removed, its redundant.


>> +
>> +	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.
>

Sorry about that, probably missed at one place, had addressed all the 
other places.

All other comments including this to be addressed in v3.


>> +		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;
>> +	}
>> +