On Mon, Oct 26, 2026 at 11:51:21AM +0530, Jayant Shekhar wrote:
> Create the DRM CRTC objects and shared state needed to expose the
> firmware-owned DCP pipeline through MSM DRM. Without CRTC state,
> DRM cannot track firmware sessions, aggregate bandwidth, or validate
> the display configuration.
>
> Implement CRTC objects, display lookup and session state, CRTC
> bandwidth aggregation, and the shared plane state used by validation
> and plane programming.
>
> Assisted-by: LLM
> Signed-off-by: Jayant Shekhar <[email protected]>
> ---
> drivers/gpu/drm/msm/Makefile | 2 +
> drivers/gpu/drm/msm/dcp/dcp_crtc.c | 725
> ++++++++++++++++++++++++++++++
> drivers/gpu/drm/msm/dcp/dcp_crtc.h | 104 +++++
> drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c | 21 +
> drivers/gpu/drm/msm/dcp/dcp_dpu_kms.h | 54 +++
> drivers/gpu/drm/msm/dcp/dcp_plane_state.h | 37 ++
If you need plane state for the CRTCs, then put planes support before
adding CRTCs.
> 6 files changed, 943 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 77c268aabca6..80341ce07d53 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -72,6 +72,8 @@ msm-display-$(CONFIG_DRM_MSM_MDP5) += \
> disp/mdp5/mdp5_smp.o \
>
> msm-display-$(CONFIG_DRM_MSM_DCP) += \
> + dcp/dcp_crtc.o \
> + dcp/dcp_dpu_kms.o \
> dcp/dcp_hfi_core.o \
> dcp/dcp_hfi_crtc.o \
> dcp/dcp_hfi_dsi.o \
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_crtc.c
> b/drivers/gpu/drm/msm/dcp/dcp_crtc.c
> new file mode 100644
> index 000000000000..ec4f74c2920f
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_crtc.c
> @@ -0,0 +1,725 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + * DCP CRTC implementation.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/wait.h>
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_blend.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_rect.h>
> +#include <drm/drm_vblank.h>
> +
> +#include <drm/drm_managed.h>
> +
> +#include "msm_drv.h"
> +
> +#include "dcp_crtc.h"
> +#include "dcp_drv.h"
> +#include "dcp_hfi.h"
> +#include "dcp_dpu_kms.h"
> +#include "dcp_plane_state.h"
> +
> +#include "hfi/hfi_defs_display.h"
> +#include "hfi/hfi_properties_display.h"
> +
> +#define DCP_FRAME_DONE_TIMEOUT_MS 500
> +
> +static struct dcp_dpu_kms *dcp_crtc_get_kms(struct drm_crtc *crtc)
> +{
> + struct msm_drm_private *priv = crtc->dev->dev_private;
> +
> + return to_dcp_dpu_kms(priv->kms);
> +}
> +
> +static struct drm_crtc_state *
> +dcp_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
> +{
> + struct dcp_crtc_state *cstate;
> +
> + if (WARN_ON(!crtc->state))
> + return NULL;
> +
> + cstate = kzalloc_obj(*cstate, GFP_KERNEL);
> + if (!cstate)
> + return NULL;
> +
> + __drm_atomic_helper_crtc_duplicate_state(crtc, &cstate->base);
> +
> + cstate->topology_mixer_count =
> + to_dcp_crtc_state(crtc->state)->topology_mixer_count;
> + cstate->topology_bits_per_component =
> + to_dcp_crtc_state(crtc->state)->topology_bits_per_component;
> + memcpy(cstate->mixer_roi, to_dcp_crtc_state(crtc->state)->mixer_roi,
> + sizeof(cstate->mixer_roi));
> + cstate->update_roi = to_dcp_crtc_state(crtc->state)->update_roi;
> + return &cstate->base;
> +}
> +
> +static void dcp_crtc_atomic_destroy_state(struct drm_crtc *crtc,
> + struct drm_crtc_state *state)
> +{
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(state);
> +
> + __drm_atomic_helper_crtc_destroy_state(state);
> + kfree(cstate);
> +}
> +
> +static struct drm_crtc_state *
> +dcp_crtc_atomic_create_state(struct drm_crtc *crtc)
> +{
> + struct dcp_crtc_state *cstate;
> +
> + cstate = kzalloc_obj(*cstate, GFP_KERNEL);
> + if (!cstate)
> + return ERR_PTR(-ENOMEM);
> +
> + __drm_atomic_helper_crtc_state_init(&cstate->base, crtc);
> + cstate->topology_mixer_count = 1;
> + cstate->topology_bits_per_component = 8;
> + return &cstate->base;
> +}
> +
> +static void dcp_crtc_aggregate_bw(struct drm_crtc_state *crtc_state,
> + bool enable)
> +{
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(crtc_state);
> + struct drm_plane *plane;
> + const struct drm_plane_state *pstate;
> +
> + cstate->dram_ib = 0;
> + cstate->dram_ab = 0;
> + cstate->llcc_ib = 0;
> + cstate->llcc_ab = 0;
> + cstate->core_clk = 0;
> +
> + if (!enable)
> + return;
> +
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
> + if (!pstate->visible || !pstate->fb)
Can it be visible if there is no fb?
> + continue;
> +
> + const struct dcp_plane_state *dcp_pstate =
> to_dcp_plane_state(pstate);
> +
> + cstate->dram_ib = max(cstate->dram_ib, dcp_pstate->fetch_bw);
> + cstate->dram_ab += dcp_pstate->fetch_bw;
> + cstate->llcc_ib = max(cstate->llcc_ib, dcp_pstate->fetch_bw);
> + cstate->llcc_ab += dcp_pstate->fetch_bw;
> + cstate->core_clk = max(cstate->core_clk, dcp_pstate->clk);
> + }
> +}
> +
> +static enum drm_mode_status
> +dcp_crtc_mode_valid(struct drm_crtc *crtc,
> + const struct drm_display_mode *mode)
> +{
> + struct dcp_dpu_kms *kms = dcp_crtc_get_kms(crtc);
> + u32 max_width, max_height;
> +
> + dcp_mode_limits(mode, kms->caps->max_width, kms->caps->max_height,
> + &max_width, &max_height);
> +
> + return drm_mode_validate_size(mode, max_width, max_height);
> +}
> +
> +static void dcp_crtc_atomic_begin(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(crtc->state);
> + struct dcp_dpu_kms *dcp_kms = dcp_crtc_get_kms(crtc);
> + u32 mixer_count;
> + u32 session_id;
> + u32 i;
> + int ret;
> +
> + if (!crtc->state->active)
> + return;
Can't you just use DRM_PLANE_COMMIT_ACTIVE_ONLY?
> + if (!dcp_kms->commit_pm_active) {
> + cstate->hfi_error = -EIO;
> + return;
> + }
> +
> + if (!dcp_crtc_get_session(crtc, &session_id)) {
> + cstate->hfi_error = -ENODEV;
> + return;
> + }
> +
> + if (cstate->core_clk > 0 || cstate->dram_ib > 0) {
> + ret = dcp_hfi_crtc_set_bw(dcp_kms->hfi, session_id,
> + cstate->dram_ib, cstate->dram_ab,
> + cstate->llcc_ib, cstate->llcc_ab,
> + cstate->core_clk);
> + if (ret) {
> + cstate->hfi_error = ret;
> + return;
> + }
> + }
> +
> + mixer_count = cstate->topology_mixer_count;
> + if (mixer_count < 1 || mixer_count > DCP_MAX_MIXERS) {
> + cstate->hfi_error = -EINVAL;
> + return;
> + }
> + for (i = 0; i < mixer_count; i++) {
> + struct dcp_mixer_roi *roi = &cstate->mixer_roi[i];
> +
> + if (!roi->w || !roi->h) {
> + drm_err(crtc->dev, "CRTC%u: invalid zero-sized LM%u
> ROI\n",
> + dcp_crtc->display_id, i);
> + cstate->hfi_error = -EINVAL;
> + return;
> + }
> +
> + ret = dcp_hfi_crtc_set_resource(dcp_kms->hfi, session_id,
> + i, roi->w, roi->h, roi->x,
> roi->y);
> + if (ret) {
> + cstate->hfi_error = ret;
> + return;
> + }
> + }
> +
> + ret = dcp_hfi_crtc_set_dest_roi(dcp_kms->hfi, session_id,
> + cstate->update_roi.x,
> cstate->update_roi.y,
> + cstate->update_roi.w,
> cstate->update_roi.h);
> + if (ret) {
> + cstate->hfi_error = ret;
> + return;
> + }
> +}
> +
> +static void dcp_crtc_atomic_flush(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(crtc->state);
> + struct dcp_dpu_kms *dcp_kms = dcp_crtc_get_kms(crtc);
> + unsigned long flags;
> + u32 session_id;
> + int ret;
> +
> + if (!crtc->state->enable)
> + return;
So, atomic_being happens only for active CRTCs, but atomic_flush also
happens for enabled, but not active CRTCs. Why?
> + drm_dbg_driver(crtc->dev, "DCP_FLUSH CRTC%u mask=0x%x err=%d\n",
> + dcp_crtc->display_id, crtc->state->plane_mask,
> + cstate->hfi_error);
> +
> + if (!dcp_crtc_get_session(crtc, &session_id)) {
> + drm_dbg_driver(crtc->dev,
> + "CRTC%u: atomic flush without HFI session\n",
> + dcp_crtc->display_id);
> + goto flush_done;
> + }
> +
> + if (cstate->hfi_error)
> + goto flush_done;
> +
> + ret = dcp_hfi_crtc_set_scan_sequence_id(dcp_kms->hfi, session_id,
> + ++dcp_crtc->commit_seq_id);
> + if (ret)
> + goto flush_error;
> +
> + ret = dcp_hfi_crtc_set_autorefresh(dcp_kms->hfi,
> + session_id, true, 1);
> + if (ret)
> + goto flush_error;
> +
> + goto flush_done;
> +
> +flush_error:
> + cstate->hfi_error = ret;
> +
> +flush_done:
> + spin_lock_irqsave(&crtc->dev->event_lock, flags);
> + dcp_crtc->event = crtc->state->event;
> + crtc->state->event = NULL;
> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> +}
> +
> +void dcp_crtc_set_session(struct drm_crtc *crtc, u32 session_id)
> +{
> + struct dcp_crtc *dcp_crtc;
> + unsigned long flags;
> +
> + if (!crtc)
> + return;
Why?
> +
> + dcp_crtc = to_dcp_crtc(crtc);
> + spin_lock_irqsave(&dcp_crtc->spin_lock, flags);
> + dcp_crtc->session_id = session_id;
> + dcp_crtc->session_valid = true;
> + spin_unlock_irqrestore(&dcp_crtc->spin_lock, flags);
> +}
> +
> +void dcp_crtc_clear_session(struct drm_crtc *crtc)
> +{
> + struct dcp_crtc *dcp_crtc;
> + unsigned long flags;
> +
> + if (!crtc)
> + return;
Why?
> +
> + dcp_crtc = to_dcp_crtc(crtc);
> + spin_lock_irqsave(&dcp_crtc->spin_lock, flags);
> + dcp_crtc->session_valid = false;
> + dcp_crtc->session_id = 0;
> + spin_unlock_irqrestore(&dcp_crtc->spin_lock, flags);
> +}
> +
> +bool dcp_crtc_get_session(struct drm_crtc *crtc, u32 *session_id)
> +{
> + struct dcp_crtc *dcp_crtc;
> + unsigned long flags;
> + bool valid;
> +
> + if (!crtc)
> + return false;
Why?
> +
> + dcp_crtc = to_dcp_crtc(crtc);
> + spin_lock_irqsave(&dcp_crtc->spin_lock, flags);
> + valid = dcp_crtc->session_valid;
> + if (valid && session_id)
> + *session_id = dcp_crtc->session_id;
> + spin_unlock_irqrestore(&dcp_crtc->spin_lock, flags);
> +
> + return valid;
> +}
> +
> +void dcp_crtc_set_topology_state(struct drm_crtc_state *state,
> + u32 mixer_count,
> + u32 bits_per_component)
> +{
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(state);
> +
> + cstate->topology_mixer_count = mixer_count;
> + cstate->topology_bits_per_component = bits_per_component;
> +}
> +
> +void dcp_crtc_commit_kickoff(struct drm_crtc *crtc)
> +{
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> + struct dcp_dpu_kms *dcp_kms = dcp_crtc_get_kms(crtc);
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(crtc->state);
> + u32 session_id;
> + int ret;
> +
> + if (cstate->hfi_error) {
> + drm_err(crtc->dev,
> + "DCP_TRIGGER CRTC%u suppressed after HFI error %d\n",
> + dcp_crtc->display_id, cstate->hfi_error);
> + return;
> + }
> +
> + if (!dcp_crtc_get_session(crtc, &session_id)) {
> + drm_warn(crtc->dev,
> + "CRTC%u: no active session, skipping FRAME_TRIGGER\n",
> + dcp_crtc->display_id);
> + return;
> + }
> +
> + /* MSM serializes commits, so only one frame completion may be pending.
> */
> + if (atomic_cmpxchg(&dcp_crtc->pending_kickoff_cnt, 0, 1)) {
> + drm_err(crtc->dev,
> + "CRTC%u: frame completion already pending\n",
> + dcp_crtc->display_id);
> + cstate->hfi_error = -EBUSY;
> + return;
> + }
> + dcp_crtc->pending_seq_id = dcp_crtc->commit_seq_id;
> +
> + ret = dcp_hfi_display_commit(dcp_kms->hfi, session_id);
> + if (ret) {
> + drm_err(crtc->dev,
> + "HFI commit failed for CRTC%u (session=%u): %d\n",
> + dcp_crtc->display_id, session_id, ret);
> + atomic_dec(&dcp_crtc->pending_kickoff_cnt);
> + wake_up(&dcp_crtc->frame_done_wq);
> + }
> +}
> +
> +void dcp_crtc_wait_for_commit_done(struct drm_crtc *crtc)
> +{
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> + unsigned long timeout = msecs_to_jiffies(DCP_FRAME_DONE_TIMEOUT_MS);
> +
> + if (!atomic_read(&dcp_crtc->pending_kickoff_cnt))
> + return;
> +
> + if (!wait_event_timeout(dcp_crtc->frame_done_wq,
> + atomic_read(&dcp_crtc->pending_kickoff_cnt) ==
> 0,
> + timeout)) {
> + drm_err(crtc->dev,
> + "frame done timeout for CRTC%u\n",
> + dcp_crtc->display_id);
> + atomic_set(&dcp_crtc->pending_kickoff_cnt, 0);
> + }
> +}
> +
> +void dcp_crtc_complete_commit(struct drm_crtc *crtc)
> +{
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> + unsigned long flags;
> +
> + if (!crtc->state->active)
> + dcp_crtc_aggregate_bw(crtc->state, false);
> +
> + spin_lock_irqsave(&crtc->dev->event_lock, flags);
> + if (dcp_crtc->event) {
> + drm_crtc_send_vblank_event(crtc, dcp_crtc->event);
> + dcp_crtc->event = NULL;
> + }
> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> +}
> +
> +int dcp_crtc_vblank(struct drm_crtc *crtc, bool enable)
> +{
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> + struct dcp_dpu_kms *dcp_kms = dcp_crtc_get_kms(crtc);
> + u32 session_id;
> + int ret;
> +
> + if (!dcp_kms->hfi || !dcp_crtc_get_session(crtc, &session_id))
Can HFI be NULL here?
> + return -ENODEV;
> +
> + if (enable)
> + ret = dcp_hfi_display_event_register_async(dcp_kms->hfi,
> + session_id,
> HFI_EVENT_VSYNC);
> + else
> + ret = dcp_hfi_display_event_deregister_async(dcp_kms->hfi,
> + session_id,
> HFI_EVENT_VSYNC);
> + if (ret)
> + return ret;
> +
> + WRITE_ONCE(dcp_crtc->vblank_enabled, enable);
Why do you need vblank_enabled in addition to the
drm_vblank_crtc::enabled?
> + return 0;
> +}
> +
> +void dcp_crtc_vblank_callback(struct drm_crtc *crtc)
> +{
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> +
> + if (!READ_ONCE(dcp_crtc->vblank_enabled))
> + return;
> +
> + if (ktime_compare(dcp_crtc->vblank_cb_time, ktime_set(0, 0)) == 0)
> + dcp_crtc->vblank_cb_time = ktime_get();
> + else
> + dcp_crtc->vblank_cb_count++;
> + drm_crtc_handle_vblank(crtc);
> +}
> +
> +void dcp_crtc_frame_done_callback(struct drm_crtc *crtc)
> +{
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> +
> + atomic_add_unless(&dcp_crtc->pending_kickoff_cnt, -1, 0);
> + wake_up(&dcp_crtc->frame_done_wq);
> +}
> +
> +static struct drm_crtc *dcp_crtc_find_by_session_id(struct dcp_drv *drv,
> + u32 session_id)
> +{
> + struct dcp_dpu_kms *dcp_kms;
> + struct drm_crtc *crtc;
> + u32 active_session_id;
> +
> + if (!drv || !drv->kms)
> + return NULL;
Why?
> +
> + dcp_kms = drv->kms;
> +
> + crtc = dcp_dpu_kms_get_crtc(dcp_kms, session_id);
Hmm, why are you passing session_id here even though the function
accepts display_id?
> + if (!crtc || !dcp_crtc_get_session(crtc, &active_session_id) ||
> + active_session_id != session_id)
> + return NULL;
> +
> + return crtc;
> +}
> +
> +void dcp_crtc_hfi_vblank_cb(struct dcp_drv *drv, u32 session_id)
> +{
> + struct drm_device *dev = drv && drv->kms ? drv->kms->dev : NULL;
> + struct drm_crtc *crtc = dcp_crtc_find_by_session_id(drv, session_id);
> +
> + if (crtc)
> + dcp_crtc_vblank_callback(crtc);
> + else if (dev)
Can dev be NULL here?
> + drm_err_ratelimited(dev,
> + "unknown CRTC for HFI VSYNC session %u\n",
> + session_id);
> +}
> +
> +void dcp_crtc_hfi_frame_done_cb(struct dcp_drv *drv, u32 session_id, u32
> sequence_id)
> +{
> + struct drm_device *dev = drv && drv->kms ? drv->kms->dev : NULL;
> + struct drm_crtc *crtc = dcp_crtc_find_by_session_id(drv, session_id);
> +
> + if (crtc) {
Why? Also, if the !crtc can actually happen, handle it as something
non-standard:
if (!crtc) {
drm_err_ratelimited(...);
return;
}
struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
...
> + struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc);
> +
> + /* A single pending commit makes equality valid across sequence
> wrap. */
> + if (sequence_id != dcp_crtc->pending_seq_id) {
> + drm_warn(crtc->dev,
> + "CRTC%u: ignoring stale FRAME_SCAN_COMPLETE
> seq=%u expected=%u\n",
> + dcp_crtc->display_id, sequence_id,
> + dcp_crtc->pending_seq_id);
> + return;
> + }
> + dcp_crtc_frame_done_callback(crtc);
Can't you inline it here?
> + } else if (dev)
> + drm_err_ratelimited(dev,
> + "unknown CRTC for HFI frame-done session %u
> seq=%u\n",
> + session_id, sequence_id);
> +}
> +
> +static int dcp_crtc_setup_mixer_rois(struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_state,
> + struct dcp_crtc_state *cstate)
> +{
> + const struct drm_plane_state *pstate;
> + struct drm_plane *plane;
> + const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> + u32 mixer_count = cstate->topology_mixer_count;
> + u32 mixer_width = mode->hdisplay / mixer_count;
> + u32 i;
> +
> + if (!mixer_width)
> + return -EINVAL;
> +
> + cstate->update_roi.x = 0;
> + cstate->update_roi.y = 0;
> + cstate->update_roi.w = mode->hdisplay;
> + cstate->update_roi.h = mode->vdisplay;
> + for (i = 0; i < mixer_count; i++) {
> + cstate->mixer_roi[i].x = (mode->hdisplay * i) / mixer_count;
> + cstate->mixer_roi[i].y = 0;
> + cstate->mixer_roi[i].w = (mode->hdisplay * (i + 1)) /
> mixer_count -
> + cstate->mixer_roi[i].x;
> + cstate->mixer_roi[i].h = mode->vdisplay;
> + drm_dbg_driver(crtc->dev,
> + "DCP_CHECK CRTC%u LM%u roi=(%u,%u %ux%u)\n",
> + to_dcp_crtc(crtc)->display_id, i,
> + cstate->mixer_roi[i].x, cstate->mixer_roi[i].y,
> + cstate->mixer_roi[i].w, cstate->mixer_roi[i].h);
> + }
> +
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
> + bool fullscreen;
> + bool assigned = false;
> + u32 mixer_idx;
> +
> + if (!pstate || !pstate->visible || !pstate->fb)
> + continue;
> +
> + for (mixer_idx = 0; mixer_idx < mixer_count; mixer_idx++) {
> + struct dcp_mixer_roi *mixer =
> &cstate->mixer_roi[mixer_idx];
> +
> + if (pstate->crtc_x >= mixer->x &&
> + pstate->crtc_x + pstate->crtc_w <= mixer->x +
> mixer->w) {
> + assigned = true;
> + break;
> + }
> + }
> +
> + /* Any plane may cover the complete split display. Plane type is
> + * not a resource-selection mechanism in the universal-plane
> API.
> + */
> + fullscreen = pstate->crtc_x == 0 && pstate->crtc_y == 0 &&
> + pstate->crtc_w == mode->hdisplay &&
> + pstate->crtc_h == mode->vdisplay;
Why do we need to special-handle the fullscreen planes?
> +
> + if (!assigned && !fullscreen && mixer_count > 1) {
> + /*
> + * Split-pipe/source-split and multirect allocation are
> + * deferred until the atomic pipe resource manager is
> added.
> + * Reject planes needing multiple mixer rectangles for
> now.
> + */
> + return -EOPNOTSUPP;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int dcp_crtc_atomic_check(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct dcp_dpu_kms *dcp_kms = dcp_crtc_get_kms(crtc);
> + struct drm_crtc_state *crtc_state =
> + drm_atomic_get_new_crtc_state(state, crtc);
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(crtc_state);
> + struct drm_plane *plane;
> + const struct drm_plane_state *pstate;
> + u32 max_width, max_height;
> + u32 mixer_count;
> + int num_planes = 0;
> + int ret;
> +
> + drm_dbg_atomic(crtc->dev, "CRTC%u: atomic_check enable=%d active=%d
> mode=%s\n",
> + crtc->base.id, crtc_state->enable, crtc_state->active,
> + crtc_state->mode.name[0] ? crtc_state->mode.name :
> "none");
> +
> + if (!crtc_state->enable || !crtc_state->active)
> + return 0;
> +
> + mixer_count = cstate->topology_mixer_count;
> + if (mixer_count < 1 || mixer_count > DCP_MAX_MIXERS) {
> + drm_err(crtc->dev,
> + "CRTC%u: invalid mixer count %u (limit 1..%u)\n",
> + crtc->base.id, mixer_count, DCP_MAX_MIXERS);
No, don't let userspace spam the kernel log.
> + return -EINVAL;
> + }
> +
> + ret = dcp_crtc_setup_mixer_rois(crtc, crtc_state, cstate);
> + if (ret)
> + return ret;
> +
> + dcp_mode_limits(&crtc_state->adjusted_mode,
> + dcp_kms->caps->max_width, dcp_kms->caps->max_height,
> + &max_width, &max_height);
> +
> + if (crtc_state->adjusted_mode.hdisplay > max_width ||
> + crtc_state->adjusted_mode.vdisplay > max_height) {
> + drm_err(crtc->dev, "CRTC%u: mode %dx%d exceeds DCP caps
> %dx%d\n",
> + crtc->base.id,
> + crtc_state->adjusted_mode.hdisplay,
> + crtc_state->adjusted_mode.vdisplay,
> + dcp_kms->caps->max_width,
> + dcp_kms->caps->max_height);
> + return -E2BIG;
> + }
> +
> + /* Limit visible planes to the firmware blend-stage count. */
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
> + if (!pstate || !pstate->visible)
> + continue;
> + num_planes++;
I think I commented here. Can't you use normalized zpos in plane's
atomic_check instead?
> + }
> +
> + if (num_planes > DCP_MAX_BLEND_STAGES) {
> + drm_err(crtc->dev, "CRTC%u: %d planes exceeds max blend stages
> %d\n",
> + crtc->base.id, num_planes,
> + DCP_MAX_BLEND_STAGES);
> + return -EINVAL;
> + }
> +
> + dcp_crtc_aggregate_bw(crtc_state, true);
> +
> + return 0;
> +}
> +
> +static u32 dcp_crtc_get_vblank_counter(struct drm_crtc *crtc)
> +{
> + return drm_crtc_vblank_count(crtc);
> +}
> +
> +static void dcp_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct dcp_dpu_kms *dcp_kms = dcp_crtc_get_kms(crtc);
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(crtc->state);
> + u32 session_id;
> + int ret;
> +
> + if (!dcp_crtc_get_session(crtc, &session_id))
> + return;
> +
> + ret = dcp_hfi_crtc_set_power_mode(dcp_kms->hfi, session_id,
> + HFI_MODE_DPMS_ON);
> + if (ret) {
> + drm_err(crtc->dev, "CRTC%u: DPMS ON failed: %d\n",
> + to_dcp_crtc(crtc)->display_id, ret);
> + cstate->hfi_error = ret;
> + }
> +
> + drm_crtc_vblank_on(crtc);
> +}
> +
> +static void dcp_crtc_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct dcp_dpu_kms *dcp_kms = dcp_crtc_get_kms(crtc);
> + struct dcp_crtc_state *cstate = to_dcp_crtc_state(crtc->state);
> + u32 session_id;
> + int ret;
> +
> + if (dcp_crtc_get_session(crtc, &session_id)) {
> + ret = dcp_hfi_crtc_set_bw(dcp_kms->hfi, session_id, 0, 0, 0, 0,
> 0);
> + if (ret) {
> + drm_err(crtc->dev,
> + "CRTC%u: clearing bandwidth vote failed: %d\n",
> + to_dcp_crtc(crtc)->display_id, ret);
> + cstate->hfi_error = ret;
> + }
> +
> + ret = dcp_hfi_crtc_set_power_mode(dcp_kms->hfi, session_id,
> + HFI_MODE_DPMS_OFF);
> + if (ret) {
> + drm_err(crtc->dev, "CRTC%u: DPMS OFF failed: %d\n",
> + to_dcp_crtc(crtc)->display_id, ret);
> + cstate->hfi_error = ret;
> + }
> + }
> +
> + drm_crtc_vblank_off(crtc);
> +}
> +
> +static const struct drm_crtc_funcs dcp_crtc_funcs = {
> + .set_config = drm_atomic_helper_set_config,
> + .page_flip = drm_atomic_helper_page_flip,
> + .atomic_create_state = dcp_crtc_atomic_create_state,
> + .atomic_duplicate_state = dcp_crtc_atomic_duplicate_state,
> + .atomic_destroy_state = dcp_crtc_atomic_destroy_state,
> + .enable_vblank = msm_crtc_enable_vblank,
> + .disable_vblank = msm_crtc_disable_vblank,
> + .get_vblank_counter = dcp_crtc_get_vblank_counter,
> +};
> +
> +static const struct drm_crtc_helper_funcs dcp_crtc_helper_funcs = {
> + .atomic_check = dcp_crtc_atomic_check,
> + .atomic_enable = dcp_crtc_atomic_enable,
> + .atomic_disable = dcp_crtc_atomic_disable,
> + .atomic_begin = dcp_crtc_atomic_begin,
> + .atomic_flush = dcp_crtc_atomic_flush,
> + .mode_valid = dcp_crtc_mode_valid,
> +};
> +
> +struct drm_crtc *dcp_crtc_init(struct dcp_dpu_kms *kms,
> + struct drm_plane *primary_plane,
> + u32 display_id)
> +{
> + struct drm_device *dev = kms->dev;
> + struct dcp_crtc *dcp_crtc;
> + int ret;
> +
> + dcp_crtc = drmm_kzalloc(dev, sizeof(*dcp_crtc), GFP_KERNEL);
> + if (!dcp_crtc)
> + return ERR_PTR(-ENOMEM);
> +
> + dcp_crtc->display_id = display_id;
> +
> + spin_lock_init(&dcp_crtc->spin_lock);
> + init_waitqueue_head(&dcp_crtc->frame_done_wq);
> + atomic_set(&dcp_crtc->pending_kickoff_cnt, 0);
> +
> + ret = drmm_crtc_init_with_planes(dev, &dcp_crtc->base,
> + primary_plane, NULL,
> + &dcp_crtc_funcs,
> + NULL);
> + if (ret) {
> + drm_err(dev, "CRTC%u init failed: %d\n", display_id, ret);
> + return ERR_PTR(ret);
> + }
> +
> + drm_crtc_helper_add(&dcp_crtc->base, &dcp_crtc_helper_funcs);
> +
> + drm_dbg_driver(dev, "CRTC%u initialized\n", display_id);
> + return &dcp_crtc->base;
> +}
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_crtc.h
> b/drivers/gpu/drm/msm/dcp/dcp_crtc.h
> new file mode 100644
> index 000000000000..ae7046e8d97a
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_crtc.h
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * DCP CRTC implementation.
> + */
> +
> +#ifndef __DCP_CRTC_H__
> +#define __DCP_CRTC_H__
> +
> +#include <linux/completion.h>
> +#include <linux/spinlock.h>
> +
> +#include <drm/drm_crtc.h>
> +
> +struct dcp_dpu_kms;
> +struct drm_plane;
> +
> +#define DCP_MAX_MIXERS 6
> +#define DCP_MAX_BLEND_STAGES 12
> +
> +static inline void dcp_mode_limits(const struct drm_display_mode *mode,
> + u32 cap_width, u32 cap_height,
> + u32 *max_width, u32 *max_height)
> +{
> + *max_width = cap_width;
> + *max_height = cap_height;
> +
> + if (mode->hdisplay < mode->vdisplay && cap_width > cap_height) {
> + *max_width = cap_height;
> + *max_height = cap_width;
Why? I guess, the caps interface is just incorrect. We have two
limitations: the maximum width and the maximum clock / bandwdith.
maximum height doesn't seem to make any sense here.
> + }
> +}
> +
> +struct dcp_mixer_roi {
> + u32 x;
> + u32 y;
> + u32 w;
> + u32 h;
> +};
Is this an HFI or an internal value? If it's the latter, please use
drm_rect.
> +
> +struct dcp_crtc {
> + struct drm_crtc base;
> + u32 display_id;
> + u32 session_id;
> + bool session_valid;
Why do we need a separate flag here? When is the session being set and
when is it removed?
> + struct drm_pending_vblank_event *event;
> + wait_queue_head_t frame_done_wq;
> + atomic_t pending_kickoff_cnt;
> + /* Protects CRTC-local vblank and commit state. */
> + spinlock_t spin_lock;
> + u32 vblank_cb_count;
> + bool vblank_enabled;
> + ktime_t vblank_cb_time;
> + bool enabled;
> + u32 commit_seq_id;
> + u32 pending_seq_id;
> +};
> +
> +struct dcp_crtc_state {
> + struct drm_crtc_state base;
> +
> + u32 topology_mixer_count;
> + u32 topology_bits_per_component;
> + struct dcp_mixer_roi mixer_roi[DCP_MAX_MIXERS];
> +
> + struct dcp_mixer_roi update_roi;
It's set exactly to the display mode. Can we drop it?
> + int hfi_error;
Why is it a part of the state? What does it mean if we got an HFI error?
What is a recovery model? Is it fine to restart the session? Or is the
whole device wedged?
> + u64 bw_ctl;
> +
> + u64 dram_ib;
> + u64 dram_ab;
> + u64 llcc_ib;
> + u64 llcc_ab;
> + u64 core_clk;
> +};
> +
> +#define to_dcp_crtc(x) container_of(x, struct dcp_crtc, base)
> +#define to_dcp_crtc_state(x) container_of(x, struct dcp_crtc_state, base)
> +
> +struct drm_crtc *dcp_crtc_init(struct dcp_dpu_kms *kms,
> + struct drm_plane *primary_plane,
> + u32 display_id);
> +
> +void dcp_crtc_commit_kickoff(struct drm_crtc *crtc);
> +void dcp_crtc_wait_for_commit_done(struct drm_crtc *crtc);
> +void dcp_crtc_complete_commit(struct drm_crtc *crtc);
> +int dcp_crtc_vblank(struct drm_crtc *crtc, bool enable);
> +void dcp_crtc_vblank_callback(struct drm_crtc *crtc);
> +void dcp_crtc_frame_done_callback(struct drm_crtc *crtc);
Are those used outside of dcp_crtc.c? If not, drop all the prototypes
and definitions which are local to the module. Reorder the functions as
required to remove the need for the forward declarations, where
possible.
> +struct dcp_drv;
> +void dcp_crtc_hfi_vblank_cb(struct dcp_drv *drv, u32 session_id);
> +void dcp_crtc_hfi_frame_done_cb(struct dcp_drv *drv, u32 session_id, u32
> sequence_id);
> +
> +void dcp_crtc_set_session(struct drm_crtc *crtc, u32 session_id);
> +void dcp_crtc_clear_session(struct drm_crtc *crtc);
> +bool dcp_crtc_get_session(struct drm_crtc *crtc, u32 *session_id);
> +void dcp_crtc_set_topology_state(struct drm_crtc_state *state,
> + u32 mixer_count,
> + u32 bits_per_component);
> +
> +#endif /* __DCP_CRTC_H__ */
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c
> b/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c
> new file mode 100644
> index 000000000000..44b1c2200aee
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + * DCP KMS driver.
> + */
> +
> +#include <drm/drm_crtc.h>
> +
> +#include "dcp_dpu_kms.h"
> +
> +struct drm_crtc *dcp_dpu_kms_get_crtc(struct dcp_dpu_kms *dcp_kms,
> + u32 display_id)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < dcp_kms->num_crtcs; i++)
> + if (dcp_kms->crtcs[i].display_id == display_id)
> + return dcp_kms->crtcs[i].crtc;
> +
> + return NULL;
> +}
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.h
> b/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.h
> new file mode 100644
> index 000000000000..f8010fc5d32f
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_dpu_kms.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + * DCP KMS state.
> + */
> +
> +#ifndef __DCP_DPU_KMS_H__
> +#define __DCP_DPU_KMS_H__
> +
> +#include <linux/spinlock.h>
> +
> +#include "msm_drv.h"
> +#include "msm_kms.h"
> +
> +struct dcp_hfi;
> +struct dcp_caps;
> +struct dcp_dsi_bridge;
> +struct dcp_drv;
> +struct drm_crtc;
> +
> +#define DCP_DPU_AUTOSUSPEND_DELAY_MS 5000
> +#define DCP_DPU_MAX_CRTCS 4
> +#define DCP_DPU_MAX_PLANES 12
> +
> +struct dcp_dpu_kms {
> + struct msm_kms base;
> + struct drm_device *dev;
> + struct device *pm_dev;
> + struct dcp_drv *drv;
> + struct dcp_hfi *hfi;
> + struct dcp_caps *caps;
> + struct device_node *dpus_np;
> + struct device *iommu_dev;
> + bool rpm_enabled;
> + bool commit_pm_active;
> + struct dcp_dsi_bridge *dsi_bridge[2];
> + struct {
> + struct drm_crtc *crtc;
> + u32 display_id;
> + } crtcs[DCP_DPU_MAX_CRTCS];
> + unsigned int num_crtcs;
> +};
> +
> +#define to_dcp_dpu_kms(x) container_of(x, struct dcp_dpu_kms, base)
> +struct drm_crtc *dcp_dpu_kms_get_crtc(struct dcp_dpu_kms *dcp_kms,
> + u32 display_id);
> +int dcp_dpu_kms_prepare_crtc_session(struct dcp_dpu_kms *dcp_kms,
> + struct drm_crtc *crtc);
> +
> +int dcp_dpu_drm_obj_init(struct dcp_dpu_kms *dcp_kms);
> +extern const struct msm_kms_funcs dcp_dpu_kms_funcs;
> +int dcp_kms_init(struct drm_device *ddev);
> +
> +#endif /* __DCP_DPU_KMS_H__ */
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_plane_state.h
> b/drivers/gpu/drm/msm/dcp/dcp_plane_state.h
> new file mode 100644
> index 000000000000..aafa93cb69b9
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_plane_state.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * DCP plane state definitions.
> + */
> +
> +#ifndef __DCP_PLANE_STATE_H__
> +#define __DCP_PLANE_STATE_H__
> +
> +#include <drm/drm_plane.h>
> +
> +#define DCP_PLANE_PROP_PKT_MAX_SIZE 512
> +
> +struct dcp_plane_state {
> + struct drm_plane_state base;
> +
> + u64 fetch_bw;
> + u64 clk;
> +
> + u8 prop_pkt[DCP_PLANE_PROP_PKT_MAX_SIZE];
> + size_t prop_pkt_size;
> + u32 prop_pkt_id;
> +
> + bool needs_dirtyfb;
> +
> + u32 src_img_w;
> + u32 src_img_h;
> +
> + u32 hfi_pipe_id;
> + u32 hfi_format;
> +};
> +
> +#define to_dcp_plane_state(x) \
> + container_of(x, struct dcp_plane_state, base)
> +
> +#endif /* __DCP_PLANE_STATE_H__ */
>
> --
> 2.43.0
>
--
With best wishes
Dmitry
