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 <[email protected]>
> ---
> 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
