All of lore.kernel.org
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Zhentao Guo" <zhentao.guo@amlogic.com>
Cc: linux-amlogic@lists.infradead.org, devicetree@vger.kernel.org,
	robh@kernel.org, conor+dt@kernel.org, neil.armstrong@linaro.org,
	media-ci@linuxtv.org
Subject: Re: [PATCH v12 6/6] arm64: defconfig: Enable CONFIG_VIDEO_AMLOGIC_VDEC
Date: Thu, 22 Oct 2026 05:53:53 +0000	[thread overview]
Message-ID: <sashiko-outbox-181043@kernel.org> (raw)
In-Reply-To: <20261022-b4-s4-vdec-upstream-v12-6-616240978792@amlogic.com>

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] media: amlogic: vdec: unchecked ref_list bounds and canvas lifecycle in h264
- [High] media: amlogic: vdec: aml_vdec_init_dec_inst() returns -1 on output restart
- [High] media: amlogic: vdec: check DPB entry flags in poc_is_in_dpb() and get_dpb_by_poc()
- [High] media: amlogic: vdec: out-of-bounds SPS workspace write in h264_config_params()
- [High] media: amlogic: vdec: ignored pm-domain error, unenabled clk, early vdev reg
- [High] media: amlogic: vdec: wrong IS_ERR() check on meson_sm_get() return
- [Medium] media: amlogic: vdec: uninitialized pic_info, prematuresizeimage calc, and split locks
- [Medium] media: amlogic: vdec: missing error unwind and propagation in vdec_poweron()
- [Low] media: amlogic: vdec: missing select REGMAP_MMIO in Kconfig
--

commit 05491c1c72b85040b071b152b79a908d403d95f0
Author: Zhentao Guo <zhentao.guo@amlogic.com>

arm64: defconfig: Enable CONFIG_VIDEO_AMLOGIC_VDEC

This commit enables the Amlogic V4L2 stateless video decoder driver
(CONFIG_VIDEO_AMLOGIC_VDEC=m) as a module in the arm64 defconfig for
Amlogic SoCs such as the S4 (S805X2) platform.

> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 7b4985b29b21c..f00c8a418a464 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -929,6 +929,7 @@ CONFIG_USB_VIDEO_CLASS=m
>  CONFIG_V4L_PLATFORM_DRIVERS=y
>  CONFIG_SDR_PLATFORM_DRIVERS=y
>  CONFIG_V4L_MEM2MEM_DRIVERS=y
> +CONFIG_VIDEO_AMLOGIC_VDEC=m

[Severity: Critical]
Can h264_config_ref_lists() and h264_reorder_reflists() in
drivers/media/platform/amlogic/vdec/h264.c overflow ref_list0 and ref_list1
and leak meson_canvas indices?

First, in struct aml_h264_ctx, ref_list0 and ref_list1 are sized to
V4L2_H264_NUM_DPB_ENTRIES + 1 (17 elements). However, in
h264_config_ref_lists(), v4l2_h264_init_reflist_builder() sets
builder.num_valid up to V4L2_H264_REF_LIST_LEN (32) when
V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC is set in decode->flags, and passes
builder.num_valid as list_size to config_p_reflist() and config_b_reflist():

drivers/media/platform/amlogic/vdec/h264.c:config_p_reflist() {
	for (i = 0; i < list_size; i++) {
		index = v4l2_p0_reflist[i].index;
		h264_ctx->ref_list0[i].used = 1;
		...
	}
}

Won't list_size > 17 write past the end of ref_list0 and ref_list1 in
config_p_reflist() and config_b_reflist(), as well as in the loops using
h264_ctx->list_size[] in h264_config_decode_spec(), get_canvas_index(), and
h264_release_decode_spec()?

Second, when all 16 dpb entries are active and num_ref_idx_lx_active is 16,
reorder_short_term() and reorder_long_term() shift ref_list_reordered[15] into
the sentinel slot ref_list_reordered[16] without clearing the sentinel
afterward:

drivers/media/platform/amlogic/vdec/h264.c:reorder_short_term() {
	for (c_idx = num_ref_idx_lx_active; c_idx > *ref_idx_lx; c_idx--)
		memcpy(&ref_list_reordered[c_idx], &ref_list_reordered[c_idx - 1],
		       sizeof(struct h264_decode_buf_spec));
	...
	h264_ctx->list_size[cur_list] = num_ref_idx_lx_active;
}

Since get_refidx_by_picnum(), get_st_refpic_by_num(), and
get_lt_refpic_by_num() iterate until ref_list[i].dpb is NULL with no upper
bound on i:

drivers/media/platform/amlogic/vdec/h264.c:get_refidx_by_picnum() {
	for (i = 0; ref_list[i].dpb; i++) {
		if (pic_num == ref_list[i].dpb->pic_num)
			return i;
	}
}

can overwriting ref_list[16].dpb cause these helpers to read past the end of
ref_list0 and ref_list1 on subsequent reordering commands?

Third, reorder_short_term() and reorder_long_term() overwrite
h264_ctx->list_size[cur_list] with num_ref_idx_lx_active. In
get_canvas_index(), config_decode_canvas() is called for every entry below
list_size without checking buf->used or buf->valid, while
h264_release_decode_spec() only calls release_canvas_index() when buf->used is
non-zero:

drivers/media/platform/amlogic/vdec/h264.c:get_canvas_index() {
	if (h264_ctx->list_size[0] > 0) {
		for (i = 0; i < h264_ctx->list_size[0]; i++) {
			buf = &h264_ctx->ref_list0[i];
			ret = config_decode_canvas(hw, buf, h264_ctx->mb_width,
						   h264_ctx->mb_height);
			...
		}
	}
}

drivers/media/platform/amlogic/vdec/h264.c:h264_release_decode_spec() {
	if (h264_ctx->list_size[0] > 0) {
		for (i = 0; i < h264_ctx->list_size[0]; i++) {
			buf = &h264_ctx->ref_list0[i];
			buf->valid = 0;
			if (buf->used) {
				buf->dpb = NULL;
				release_canvas_index(hw, buf);
				buf->used = 0;
			}
		}
		h264_ctx->list_size[0] = 0;
	}
}

If num_ref_idx_lx_active is larger than the initial list_size, won't
get_canvas_index() allocate meson canvases for entries with buf->used == 0
that h264_release_decode_spec() never frees, and if num_ref_idx_lx_active is
smaller, won't entries above the new list_size retain stale state?

[Severity: High]
What happens in drivers/media/platform/amlogic/vdec/aml_vdec.c when the output
queue is stopped and restarted while the capture queue remains streaming?

When a client stops the output queue via vb2ops_vdec_stop_streaming() while
ctx->is_cap_streamon is still set, aml_vdec_release_instance() is not called
and ctx->codec_priv remains non-NULL:

drivers/media/platform/amlogic/vdec/aml_vdec.c:vb2ops_vdec_stop_streaming() {
	if (!ctx->is_output_streamon && !ctx->is_cap_streamon)
		aml_vdec_release_instance(ctx);
}

When the output queue is subsequently restarted, vb2ops_vdec_start_streaming()
calls aml_vdec_init_dec_inst(), which initializes ret to -1 and skips
ctx->codec_ops->init(ctx) because ctx->codec_priv is already allocated:

drivers/media/platform/amlogic/vdec/aml_vdec.c:aml_vdec_init_dec_inst() {
	int ret = -1;
	...
	if (ctx->codec_ops->init && !ctx->codec_priv) {
		ret = ctx->codec_ops->init(ctx);
		if (ret < 0)
			return ret;
	}
	ctx->curr_dec_type = fmt_out->codec_type;
	...
	return ret;
}

Because ret stays -1, vb2ops_vdec_start_streaming() treats the call as a
failure, returns all queued source buffers via v4l2_m2m_src_buf_remove() and
v4l2_m2m_buf_done(), and fails VIDIOC_STREAMON with -EINVAL:

drivers/media/platform/amlogic/vdec/aml_vdec.c:vb2ops_vdec_start_streaming() {
		if (aml_vdec_init_dec_inst(ctx) < 0) {
			while ((src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx)))
				v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_QUEUED);
			return -EINVAL;
		}
}

Should ret be initialized to 0 in aml_vdec_init_dec_inst() so restarting the
output queue succeeds when ctx->codec_priv is already present?

[Severity: High]
Should poc_is_in_dpb() and get_dpb_by_poc() in
drivers/media/platform/amlogic/vdec/h264.c check whether a dpb entry is active
before matching top_field_order_cnt?

During H264_SLICE_HEADER_DONE handling in
h264_threaded_isr_func()->h264_config_decode_spec()->clear_unused_col_buf(),
poc_is_in_dpb() is called to decide whether to release colocated_buf and
canvas_pos slots:

drivers/media/platform/amlogic/vdec/h264.c:poc_is_in_dpb() {
	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
		if (poc == dpb[i].top_field_order_cnt) {
			ret = 1;
			break;
		}
	}
}

Because decode->dpb has 16 fixed entries and poc_is_in_dpb() does not check
dpb[i].flags for V4L2_H264_DPB_ENTRY_FLAG_ACTIVE (or
V4L2_H264_DPB_ENTRY_FLAG_VALID), any unused zeroed dpb entry has
top_field_order_cnt == 0, causing poc_is_in_dpb(0, decode->dpb) to always
return 1 and leak the colocated_buf and canvas_pos slots for poc 0 in
clear_unused_col_buf():

drivers/media/platform/amlogic/vdec/h264.c:clear_unused_col_buf() {
	for (i = 0; i < h264_ctx->colocated_buf_num; i++) {
		if (!((h264_ctx->colocated_buf_map >> i) & 0x1))
			continue;
		col_poc = h264_ctx->colocated_buf_poc[i];
		if (poc_is_in_dpb(col_poc, decode->dpb) != 1)
			release_colocate_buf(h264_ctx, i);
	}

	for (i = 0; i < (V4L2_H264_NUM_DPB_ENTRIES + 1); i++) {
		if (!((h264_ctx->canvas_pos_map >> i) & 0x1))
			continue;
		col_poc = h264_ctx->ref_canvas[i].poc;
		if (poc_is_in_dpb(col_poc, decode->dpb) != 1)
			release_canvas_pos(h264_ctx, i);
	}
}

Similarly, if userspace clears flags on retired dpb entries without zeroing
top_field_order_cnt, their colocated_buf and canvas_pos slots are never freed
until all slots are exhausted.

In addition, h264_config_decode_buf() looks up reference entries with
get_dpb_by_poc():

drivers/media/platform/amlogic/vdec/h264.c:get_dpb_by_poc() {
	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
		if (decode->dpb[i].top_field_order_cnt == poc)
			return &decode->dpb[i];
	}
	return NULL;
}

drivers/media/platform/amlogic/vdec/h264.c:h264_config_decode_buf() {
			dpb = get_dpb_by_poc(decode, poc);
			if (dpb && (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)) {
}

If an earlier inactive dpb slot has the same top_field_order_cnt as a later
active dpb slot (for instance, an unused zeroed slot before an active poc 0
entry), won't get_dpb_by_poc() return the inactive slot and cause
h264_config_decode_buf() to skip writing the active reference frame's info
into H264_BUFFER_INFO_DATA?

[Severity: High]
Can a userspace V4L2_CID_STATELESS_H264_SPS control with
sps->seq_parameter_set_id >= 32 cause an out-of-bounds write in
h264_config_params() in drivers/media/platform/amlogic/vdec/h264.c?

In drivers/media/v4l2-core/v4l2-ctrls-core.c, std_validate_compound() does not
validate that p_h264_sps->seq_parameter_set_id is within 0..31, and
aml_vdec_ctrls_setup() in drivers/media/platform/amlogic/vdec/aml_vdec.c does
not provide custom validation. When aml_h264_dec_run() calls
h264_config_params()->config_sps_params(), p_sps_base is calculated directly
from sps->seq_parameter_set_id (a u8 up to 255):

drivers/media/platform/amlogic/vdec/h264.c:h264_config_params() {
	p_sps_base = (unsigned short *)(h264_ctx->workspace_vaddr +
		MEM_SPS_BASE + sps->seq_parameter_set_id * 0x400);
	p_pps_base = (unsigned short *)(h264_ctx->workspace_vaddr +
		MEM_PPS_BASE + pps->pic_parameter_set_id * 0x200);
}

drivers/media/platform/amlogic/vdec/h264.c:config_sps_params() {
	memset(sps_base, 0, 0x100);
	memset(data_tmp, 0, sizeof(data_tmp));
}

Since MEM_SPS_BASE (0x01c3c00) to MEM_PPS_BASE (0x01cbc00) only has room for
32 entries of 0x400 bytes, won't seq_parameter_set_id >= 32 cause
config_sps_params() to overwrite the MEM_PPS_BASE table and adjacent regions
of h264_ctx->workspace_vaddr?

[Severity: High]
Could drivers/media/platform/amlogic/vdec/aml_vdec_hw.c and
drivers/media/platform/amlogic/vdec/aml_vdec_drv.c access unclocked or
unpowered DOS_BUS registers if power domain attachment fails or before
vdec_enable() runs?

In pm_vdec_power_domain_init(), errors from dev_pm_domain_attach_by_name()
(including -EPROBE_DEFER) are ignored and pd[i].dev is set to NULL:

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:pm_vdec_power_domain_init() {
		pd[i].dev = dev_pm_domain_attach_by_name(hw->dev, pd[i].name);
		if (IS_ERR_OR_NULL(pd[i].dev)) {
			err = PTR_ERR(pd[i].dev);
			dev_dbg(hw->dev, "Get %s failed, pm-domain: %d\n",
				pd[i].name, err);
			pd[i].dev = NULL;
			continue;
		}
}

When pd_data[core].dev is NULL, vdec_poweron() skips hw->pm->power_on(hw, core)
and leaves both the vdec power domain and vdec clock disabled, so subsequent
DOS_BUS register accesses in m2mops_vdec_device_run()->dos_enable() can
trigger a synchronous external abort:

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:vdec_poweron() {
	mutex_lock(&hw->pm_mutex);
	if (!hw->pm->pd_data[core].dev)
		goto out;
	...
	hw->pm->power_on(hw, core);
}

In addition, vdec_clock_gate_init() acquires the dos clock in
hw->gates[CLK_DOS]:

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:vdec_clock_gate_init() {
	hw->gates[CLK_DOS].id = "dos";
	hw->gates[CLK_VDEC].id = "vdec";
	hw->gates[CLK_HVDEC].id = "hvdec";

	return devm_clk_bulk_get(hw->dev, CLK_MAX, hw->gates);
}

Yet pm_vdec_power_domain_power_on() only enables vdec and hvdec and never
enables hw->gates[CLK_DOS].clk.

Furthermore, in aml_vdec_drv_probe() in
drivers/media/platform/amlogic/vdec/aml_vdec_drv.c, video_register_device() is
called before dev->v4l2_dev.mdev is initialized and before
vdec_enable(dev->dec_hw) is called:

drivers/media/platform/amlogic/vdec/aml_vdec_drv.c:aml_vdec_drv_probe() {
	ret = video_register_device(vfd_dec, VFL_TYPE_VIDEO, -1);
	if (ret) {
		v4l2_err(&dev->v4l2_dev, "Failed to register video device");
		goto err_vid_dev_register;
	}

	dev->mdev.dev = &pdev->dev;
	strscpy(dev->mdev.model, AML_VDEC_DRV_NAME, sizeof(dev->mdev.model));
	media_device_init(&dev->mdev);
	dev->mdev.ops = &aml_m2m_media_ops;
	dev->v4l2_dev.mdev = &dev->mdev;
	...
	vdec_enable(dev->dec_hw);
}

Should pm_vdec_power_domain_init() propagate errors from
dev_pm_domain_attach_by_name(), should CLK_DOS be enabled before accessing
DOS_BUS registers, and should vdec_enable() and media_device_init() run before
video_register_device() publishes the device node?

[Severity: High]
Why does dev_request_hw_resources() in
drivers/media/platform/amlogic/vdec/aml_vdec_hw.c check meson_sm_get() with
IS_ERR() instead of checking for NULL?

In drivers/firmware/meson/meson_sm.c, meson_sm_get() returns NULL when the
secure-monitor device is missing or has not finished probing:

drivers/firmware/meson/meson_sm.c:meson_sm_get() {
	if (!pdev)
		return NULL;

	fw = platform_get_drvdata(pdev);

	put_device(&pdev->dev);

	return fw;
}

However, dev_request_hw_resources() only checks IS_ERR(hw->sec_fw):

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:dev_request_hw_resources() {
	hw->sec_fw = meson_sm_get(sm_np);
	of_node_put(sm_np);
	if (IS_ERR(hw->sec_fw))
		return PTR_ERR(hw->sec_fw);
}

If meson_sm_get() returns NULL, probe succeeds with hw->sec_fw == NULL, and
when a decode job runs m2mops_vdec_device_run()->load_firmware() in
drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c:

drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c:load_firmware() {
	meson_sm_call(hw->sec_fw, SM_LOAD_VIDEO_FW, &ret,
		      video_fw->fw_format, video_fw->core,
		      video_fw->is_swap, 0, 0);
}

won't meson_sm_call() dereference a NULL hw->sec_fw pointer instead of probe
returning -EPROBE_DEFER?

[Severity: Medium]
Are ctx->pic_info, plane_fmt[0].sizeimage, and the format/queue locks in
drivers/media/platform/amlogic/vdec/aml_vdec.c initialized in the right order?

First, when a file handle is opened, fops_vcodec_open()->aml_vdec_reset_fmts()
calls reset_output_fmts(), which sets ctx->pix_fmt[AML_FMT_SRC] without
calling set_pic_info():

drivers/media/platform/amlogic/vdec/aml_vdec.c:reset_output_fmts() {
	vdec_try_fmt_mp(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, &fmt,
			out_fmt);

	ctx->pix_fmt[AML_FMT_SRC] = fmt;
}

If userspace starts streaming with the default format without calling
VIDIOC_S_FMT on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, ctx->pic_info.fb_size[0]
remains 0, so h264_config_decode_spec() and h264_config_ref_spec() in
drivers/media/platform/amlogic/vdec/h264.c set c_dma_addr equal to y_dma_addr
for single-planar capture formats:

drivers/media/platform/amlogic/vdec/h264.c:h264_config_decode_spec() {
	h264_ctx->curr_spec.y_dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
	if (ctx->pix_fmt[AML_FMT_DST].num_planes > 1)
		h264_ctx->curr_spec.c_dma_addr =
		    vb2_dma_contig_plane_dma_addr(vb, 1);
	else
		h264_ctx->curr_spec.c_dma_addr =
		    h264_ctx->curr_spec.y_dma_addr + ctx->pic_info.fb_size[0];
}

Second, in vdec_try_fmt_mp(), plane_fmt[0].sizeimage for the output queue is
computed from pix_mp->height and pix_mp->width before ALIGN() and
v4l2_apply_frmsize_constraints() clamp and align them:

drivers/media/platform/amlogic/vdec/aml_vdec.c:vdec_try_fmt_mp() {
	if (V4L2_TYPE_IS_OUTPUT(type)) {
		pix_mp->num_planes = dec_fmt->num_planes;
		pix_mp->pixelformat = dec_fmt->fourcc;
		if (!pix_mp->plane_fmt[0].sizeimage)
			pix_mp->plane_fmt[0].sizeimage =
			    (pix_mp->height * pix_mp->width * 3) / 2;
		pix_mp->height = ALIGN(pix_mp->height, align);
		pix_mp->width = ALIGN(pix_mp->width, align);
	} else {
		...
	}

	v4l2_apply_frmsize_constraints(&pix_mp->width, &pix_mp->height,
				       &dec_fmt->stepwise);
}

If userspace passes width == 0 or height == 0 with sizeimage == 0, sizeimage
remains 0 even after width and height are clamped to the minimum frame size.

Third, aml_vdec_reset_fmts() and aml_vdec_queue_init() set
ctx->m2m_ctx->q_lock, src_vq->lock, and dst_vq->lock to &ctx->v4l2_intf_lock,
whereas aml_vdec_drv_probe() sets vfd_dec->lock to &dev->dev_mutex:

drivers/media/platform/amlogic/vdec/aml_vdec.c:aml_vdec_reset_fmts() {
	ctx->m2m_ctx->q_lock = &ctx->v4l2_intf_lock;
	reset_output_fmts(ctx);
	reset_capture_fmts(ctx);
}

Doesn't using two different mutexes allow VIDIOC_S_FMT and VIDIOC_G_FMT to
race with queue ioctls such as VIDIOC_REQBUFS, VIDIOC_QBUF, and
VIDIOC_STREAMON?

[Severity: Medium]
What happens in drivers/media/platform/amlogic/vdec/aml_vdec_hw.c when
pm_runtime_resume_and_get() or clk_prepare_enable() fails during
vdec_poweron()?

In pm_vdec_power_domain_power_on(), if clk_prepare_enable(gate_node->clk)
succeeds and pm_runtime_resume_and_get(dev) fails, gate_node->clk is not
disabled and unprepared, and the error is not returned to the caller:

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:pm_vdec_power_domain_power_on() {
	if (gate_node) {
		ret = clk_prepare_enable(gate_node->clk);
	...
	if (!ret && dev) {
		ret = pm_runtime_resume_and_get(dev);
		if (ret < 0)
			dev_err(hw->dev, "failed to power on %s domain: %d\n",
				pm->pd_data[id].name, ret);
	...
	if (!ret)
		dos_local_config(hw, 1, id);
}

Because vdec_poweron() increments hw->pm->pd_data[core].ref_count before
calling hw->pm->power_on(hw, core) and vdec_enable() always returns 0 to
aml_vdec_drv_probe():

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:vdec_poweron() {
	hw->pm->pd_data[core].ref_count++;
	if (hw->pm->pd_data[core].ref_count > 1)
		goto out;

	hw->pm->power_on(hw, core);
}

when aml_vdec_drv_remove() later calls
vdec_disable()->vdec_poweroff()->pm_vdec_power_domain_power_off():

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:pm_vdec_power_domain_power_off() {
	dos_local_config(hw, 0, id);

	if (dev) {
		pm_runtime_put_sync(dev);
}

won't pm_vdec_power_domain_power_off() access unpowered DOS_BUS registers in
dos_local_config(hw, 0, id) and cause unbalanced pm_runtime_put_sync() and
clk_disable_unprepare() calls?

[Severity: Low]
Should VIDEO_AMLOGIC_VDEC in drivers/media/platform/amlogic/vdec/Kconfig
select REGMAP_MMIO?

In drivers/media/platform/amlogic/vdec/aml_vdec_hw.c,
dev_request_hw_resources() initializes regmaps via devm_regmap_init_mmio():

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:dev_request_hw_resources() {
		if (i == DOS_BUS) {
			hw->map[i] = devm_regmap_init_mmio(&pdev->dev, reg_base[i],
							   &dos_regmap_conf);
		} else if (i == DMC_BUS) {
			hw->map[i] = devm_regmap_init_mmio(&pdev->dev, reg_base[i],
							   &dmc_regmap_conf);
		}
}

However, VIDEO_AMLOGIC_VDEC in drivers/media/platform/amlogic/vdec/Kconfig
does not select REGMAP_MMIO:

drivers/media/platform/amlogic/vdec/Kconfig: {
config VIDEO_AMLOGIC_VDEC
    tristate "Amlogic Video Decoder Driver"
    ...
    select VIDEOBUF2_DMA_CONTIG
    select V4L2_H264
    select V4L2_MEM2MEM_DEV
    select MESON_CANVAS
}

Could configurations enabling CONFIG_VIDEO_AMLOGIC_VDEC without another driver
selecting CONFIG_REGMAP_MMIO fail to build or link?

>  CONFIG_VIDEO_AMPHION_VPU=m
>  CONFIG_VIDEO_CADENCE_CSI2RX=m
>  CONFIG_VIDEO_WAVE_VPU=m

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-b4-s4-vdec-upstream-v12-0-616240978792@amlogic.com?part=6

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

WARNING: multiple messages have this Message-ID (diff)
From: sashiko-bot@kernel.org
To: "Zhentao Guo" <zhentao.guo@amlogic.com>
Cc: linux-amlogic@lists.infradead.org, devicetree@vger.kernel.org,
	robh@kernel.org, conor+dt@kernel.org, neil.armstrong@linaro.org,
	media-ci@linuxtv.org
Subject: Re: [PATCH v12 6/6] arm64: defconfig: Enable CONFIG_VIDEO_AMLOGIC_VDEC
Date: Thu, 22 Oct 2026 05:53:53 +0000	[thread overview]
Message-ID: <sashiko-outbox-181043@kernel.org> (raw)
In-Reply-To: <20261022-b4-s4-vdec-upstream-v12-6-616240978792@amlogic.com>

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] media: amlogic: vdec: unchecked ref_list bounds and canvas lifecycle in h264
- [High] media: amlogic: vdec: aml_vdec_init_dec_inst() returns -1 on output restart
- [High] media: amlogic: vdec: check DPB entry flags in poc_is_in_dpb() and get_dpb_by_poc()
- [High] media: amlogic: vdec: out-of-bounds SPS workspace write in h264_config_params()
- [High] media: amlogic: vdec: ignored pm-domain error, unenabled clk, early vdev reg
- [High] media: amlogic: vdec: wrong IS_ERR() check on meson_sm_get() return
- [Medium] media: amlogic: vdec: uninitialized pic_info, prematuresizeimage calc, and split locks
- [Medium] media: amlogic: vdec: missing error unwind and propagation in vdec_poweron()
- [Low] media: amlogic: vdec: missing select REGMAP_MMIO in Kconfig
--

commit 05491c1c72b85040b071b152b79a908d403d95f0
Author: Zhentao Guo <zhentao.guo@amlogic.com>

arm64: defconfig: Enable CONFIG_VIDEO_AMLOGIC_VDEC

This commit enables the Amlogic V4L2 stateless video decoder driver
(CONFIG_VIDEO_AMLOGIC_VDEC=m) as a module in the arm64 defconfig for
Amlogic SoCs such as the S4 (S805X2) platform.

> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 7b4985b29b21c..f00c8a418a464 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -929,6 +929,7 @@ CONFIG_USB_VIDEO_CLASS=m
>  CONFIG_V4L_PLATFORM_DRIVERS=y
>  CONFIG_SDR_PLATFORM_DRIVERS=y
>  CONFIG_V4L_MEM2MEM_DRIVERS=y
> +CONFIG_VIDEO_AMLOGIC_VDEC=m

[Severity: Critical]
Can h264_config_ref_lists() and h264_reorder_reflists() in
drivers/media/platform/amlogic/vdec/h264.c overflow ref_list0 and ref_list1
and leak meson_canvas indices?

First, in struct aml_h264_ctx, ref_list0 and ref_list1 are sized to
V4L2_H264_NUM_DPB_ENTRIES + 1 (17 elements). However, in
h264_config_ref_lists(), v4l2_h264_init_reflist_builder() sets
builder.num_valid up to V4L2_H264_REF_LIST_LEN (32) when
V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC is set in decode->flags, and passes
builder.num_valid as list_size to config_p_reflist() and config_b_reflist():

drivers/media/platform/amlogic/vdec/h264.c:config_p_reflist() {
	for (i = 0; i < list_size; i++) {
		index = v4l2_p0_reflist[i].index;
		h264_ctx->ref_list0[i].used = 1;
		...
	}
}

Won't list_size > 17 write past the end of ref_list0 and ref_list1 in
config_p_reflist() and config_b_reflist(), as well as in the loops using
h264_ctx->list_size[] in h264_config_decode_spec(), get_canvas_index(), and
h264_release_decode_spec()?

Second, when all 16 dpb entries are active and num_ref_idx_lx_active is 16,
reorder_short_term() and reorder_long_term() shift ref_list_reordered[15] into
the sentinel slot ref_list_reordered[16] without clearing the sentinel
afterward:

drivers/media/platform/amlogic/vdec/h264.c:reorder_short_term() {
	for (c_idx = num_ref_idx_lx_active; c_idx > *ref_idx_lx; c_idx--)
		memcpy(&ref_list_reordered[c_idx], &ref_list_reordered[c_idx - 1],
		       sizeof(struct h264_decode_buf_spec));
	...
	h264_ctx->list_size[cur_list] = num_ref_idx_lx_active;
}

Since get_refidx_by_picnum(), get_st_refpic_by_num(), and
get_lt_refpic_by_num() iterate until ref_list[i].dpb is NULL with no upper
bound on i:

drivers/media/platform/amlogic/vdec/h264.c:get_refidx_by_picnum() {
	for (i = 0; ref_list[i].dpb; i++) {
		if (pic_num == ref_list[i].dpb->pic_num)
			return i;
	}
}

can overwriting ref_list[16].dpb cause these helpers to read past the end of
ref_list0 and ref_list1 on subsequent reordering commands?

Third, reorder_short_term() and reorder_long_term() overwrite
h264_ctx->list_size[cur_list] with num_ref_idx_lx_active. In
get_canvas_index(), config_decode_canvas() is called for every entry below
list_size without checking buf->used or buf->valid, while
h264_release_decode_spec() only calls release_canvas_index() when buf->used is
non-zero:

drivers/media/platform/amlogic/vdec/h264.c:get_canvas_index() {
	if (h264_ctx->list_size[0] > 0) {
		for (i = 0; i < h264_ctx->list_size[0]; i++) {
			buf = &h264_ctx->ref_list0[i];
			ret = config_decode_canvas(hw, buf, h264_ctx->mb_width,
						   h264_ctx->mb_height);
			...
		}
	}
}

drivers/media/platform/amlogic/vdec/h264.c:h264_release_decode_spec() {
	if (h264_ctx->list_size[0] > 0) {
		for (i = 0; i < h264_ctx->list_size[0]; i++) {
			buf = &h264_ctx->ref_list0[i];
			buf->valid = 0;
			if (buf->used) {
				buf->dpb = NULL;
				release_canvas_index(hw, buf);
				buf->used = 0;
			}
		}
		h264_ctx->list_size[0] = 0;
	}
}

If num_ref_idx_lx_active is larger than the initial list_size, won't
get_canvas_index() allocate meson canvases for entries with buf->used == 0
that h264_release_decode_spec() never frees, and if num_ref_idx_lx_active is
smaller, won't entries above the new list_size retain stale state?

[Severity: High]
What happens in drivers/media/platform/amlogic/vdec/aml_vdec.c when the output
queue is stopped and restarted while the capture queue remains streaming?

When a client stops the output queue via vb2ops_vdec_stop_streaming() while
ctx->is_cap_streamon is still set, aml_vdec_release_instance() is not called
and ctx->codec_priv remains non-NULL:

drivers/media/platform/amlogic/vdec/aml_vdec.c:vb2ops_vdec_stop_streaming() {
	if (!ctx->is_output_streamon && !ctx->is_cap_streamon)
		aml_vdec_release_instance(ctx);
}

When the output queue is subsequently restarted, vb2ops_vdec_start_streaming()
calls aml_vdec_init_dec_inst(), which initializes ret to -1 and skips
ctx->codec_ops->init(ctx) because ctx->codec_priv is already allocated:

drivers/media/platform/amlogic/vdec/aml_vdec.c:aml_vdec_init_dec_inst() {
	int ret = -1;
	...
	if (ctx->codec_ops->init && !ctx->codec_priv) {
		ret = ctx->codec_ops->init(ctx);
		if (ret < 0)
			return ret;
	}
	ctx->curr_dec_type = fmt_out->codec_type;
	...
	return ret;
}

Because ret stays -1, vb2ops_vdec_start_streaming() treats the call as a
failure, returns all queued source buffers via v4l2_m2m_src_buf_remove() and
v4l2_m2m_buf_done(), and fails VIDIOC_STREAMON with -EINVAL:

drivers/media/platform/amlogic/vdec/aml_vdec.c:vb2ops_vdec_start_streaming() {
		if (aml_vdec_init_dec_inst(ctx) < 0) {
			while ((src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx)))
				v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_QUEUED);
			return -EINVAL;
		}
}

Should ret be initialized to 0 in aml_vdec_init_dec_inst() so restarting the
output queue succeeds when ctx->codec_priv is already present?

[Severity: High]
Should poc_is_in_dpb() and get_dpb_by_poc() in
drivers/media/platform/amlogic/vdec/h264.c check whether a dpb entry is active
before matching top_field_order_cnt?

During H264_SLICE_HEADER_DONE handling in
h264_threaded_isr_func()->h264_config_decode_spec()->clear_unused_col_buf(),
poc_is_in_dpb() is called to decide whether to release colocated_buf and
canvas_pos slots:

drivers/media/platform/amlogic/vdec/h264.c:poc_is_in_dpb() {
	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
		if (poc == dpb[i].top_field_order_cnt) {
			ret = 1;
			break;
		}
	}
}

Because decode->dpb has 16 fixed entries and poc_is_in_dpb() does not check
dpb[i].flags for V4L2_H264_DPB_ENTRY_FLAG_ACTIVE (or
V4L2_H264_DPB_ENTRY_FLAG_VALID), any unused zeroed dpb entry has
top_field_order_cnt == 0, causing poc_is_in_dpb(0, decode->dpb) to always
return 1 and leak the colocated_buf and canvas_pos slots for poc 0 in
clear_unused_col_buf():

drivers/media/platform/amlogic/vdec/h264.c:clear_unused_col_buf() {
	for (i = 0; i < h264_ctx->colocated_buf_num; i++) {
		if (!((h264_ctx->colocated_buf_map >> i) & 0x1))
			continue;
		col_poc = h264_ctx->colocated_buf_poc[i];
		if (poc_is_in_dpb(col_poc, decode->dpb) != 1)
			release_colocate_buf(h264_ctx, i);
	}

	for (i = 0; i < (V4L2_H264_NUM_DPB_ENTRIES + 1); i++) {
		if (!((h264_ctx->canvas_pos_map >> i) & 0x1))
			continue;
		col_poc = h264_ctx->ref_canvas[i].poc;
		if (poc_is_in_dpb(col_poc, decode->dpb) != 1)
			release_canvas_pos(h264_ctx, i);
	}
}

Similarly, if userspace clears flags on retired dpb entries without zeroing
top_field_order_cnt, their colocated_buf and canvas_pos slots are never freed
until all slots are exhausted.

In addition, h264_config_decode_buf() looks up reference entries with
get_dpb_by_poc():

drivers/media/platform/amlogic/vdec/h264.c:get_dpb_by_poc() {
	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
		if (decode->dpb[i].top_field_order_cnt == poc)
			return &decode->dpb[i];
	}
	return NULL;
}

drivers/media/platform/amlogic/vdec/h264.c:h264_config_decode_buf() {
			dpb = get_dpb_by_poc(decode, poc);
			if (dpb && (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)) {
}

If an earlier inactive dpb slot has the same top_field_order_cnt as a later
active dpb slot (for instance, an unused zeroed slot before an active poc 0
entry), won't get_dpb_by_poc() return the inactive slot and cause
h264_config_decode_buf() to skip writing the active reference frame's info
into H264_BUFFER_INFO_DATA?

[Severity: High]
Can a userspace V4L2_CID_STATELESS_H264_SPS control with
sps->seq_parameter_set_id >= 32 cause an out-of-bounds write in
h264_config_params() in drivers/media/platform/amlogic/vdec/h264.c?

In drivers/media/v4l2-core/v4l2-ctrls-core.c, std_validate_compound() does not
validate that p_h264_sps->seq_parameter_set_id is within 0..31, and
aml_vdec_ctrls_setup() in drivers/media/platform/amlogic/vdec/aml_vdec.c does
not provide custom validation. When aml_h264_dec_run() calls
h264_config_params()->config_sps_params(), p_sps_base is calculated directly
from sps->seq_parameter_set_id (a u8 up to 255):

drivers/media/platform/amlogic/vdec/h264.c:h264_config_params() {
	p_sps_base = (unsigned short *)(h264_ctx->workspace_vaddr +
		MEM_SPS_BASE + sps->seq_parameter_set_id * 0x400);
	p_pps_base = (unsigned short *)(h264_ctx->workspace_vaddr +
		MEM_PPS_BASE + pps->pic_parameter_set_id * 0x200);
}

drivers/media/platform/amlogic/vdec/h264.c:config_sps_params() {
	memset(sps_base, 0, 0x100);
	memset(data_tmp, 0, sizeof(data_tmp));
}

Since MEM_SPS_BASE (0x01c3c00) to MEM_PPS_BASE (0x01cbc00) only has room for
32 entries of 0x400 bytes, won't seq_parameter_set_id >= 32 cause
config_sps_params() to overwrite the MEM_PPS_BASE table and adjacent regions
of h264_ctx->workspace_vaddr?

[Severity: High]
Could drivers/media/platform/amlogic/vdec/aml_vdec_hw.c and
drivers/media/platform/amlogic/vdec/aml_vdec_drv.c access unclocked or
unpowered DOS_BUS registers if power domain attachment fails or before
vdec_enable() runs?

In pm_vdec_power_domain_init(), errors from dev_pm_domain_attach_by_name()
(including -EPROBE_DEFER) are ignored and pd[i].dev is set to NULL:

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:pm_vdec_power_domain_init() {
		pd[i].dev = dev_pm_domain_attach_by_name(hw->dev, pd[i].name);
		if (IS_ERR_OR_NULL(pd[i].dev)) {
			err = PTR_ERR(pd[i].dev);
			dev_dbg(hw->dev, "Get %s failed, pm-domain: %d\n",
				pd[i].name, err);
			pd[i].dev = NULL;
			continue;
		}
}

When pd_data[core].dev is NULL, vdec_poweron() skips hw->pm->power_on(hw, core)
and leaves both the vdec power domain and vdec clock disabled, so subsequent
DOS_BUS register accesses in m2mops_vdec_device_run()->dos_enable() can
trigger a synchronous external abort:

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:vdec_poweron() {
	mutex_lock(&hw->pm_mutex);
	if (!hw->pm->pd_data[core].dev)
		goto out;
	...
	hw->pm->power_on(hw, core);
}

In addition, vdec_clock_gate_init() acquires the dos clock in
hw->gates[CLK_DOS]:

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:vdec_clock_gate_init() {
	hw->gates[CLK_DOS].id = "dos";
	hw->gates[CLK_VDEC].id = "vdec";
	hw->gates[CLK_HVDEC].id = "hvdec";

	return devm_clk_bulk_get(hw->dev, CLK_MAX, hw->gates);
}

Yet pm_vdec_power_domain_power_on() only enables vdec and hvdec and never
enables hw->gates[CLK_DOS].clk.

Furthermore, in aml_vdec_drv_probe() in
drivers/media/platform/amlogic/vdec/aml_vdec_drv.c, video_register_device() is
called before dev->v4l2_dev.mdev is initialized and before
vdec_enable(dev->dec_hw) is called:

drivers/media/platform/amlogic/vdec/aml_vdec_drv.c:aml_vdec_drv_probe() {
	ret = video_register_device(vfd_dec, VFL_TYPE_VIDEO, -1);
	if (ret) {
		v4l2_err(&dev->v4l2_dev, "Failed to register video device");
		goto err_vid_dev_register;
	}

	dev->mdev.dev = &pdev->dev;
	strscpy(dev->mdev.model, AML_VDEC_DRV_NAME, sizeof(dev->mdev.model));
	media_device_init(&dev->mdev);
	dev->mdev.ops = &aml_m2m_media_ops;
	dev->v4l2_dev.mdev = &dev->mdev;
	...
	vdec_enable(dev->dec_hw);
}

Should pm_vdec_power_domain_init() propagate errors from
dev_pm_domain_attach_by_name(), should CLK_DOS be enabled before accessing
DOS_BUS registers, and should vdec_enable() and media_device_init() run before
video_register_device() publishes the device node?

[Severity: High]
Why does dev_request_hw_resources() in
drivers/media/platform/amlogic/vdec/aml_vdec_hw.c check meson_sm_get() with
IS_ERR() instead of checking for NULL?

In drivers/firmware/meson/meson_sm.c, meson_sm_get() returns NULL when the
secure-monitor device is missing or has not finished probing:

drivers/firmware/meson/meson_sm.c:meson_sm_get() {
	if (!pdev)
		return NULL;

	fw = platform_get_drvdata(pdev);

	put_device(&pdev->dev);

	return fw;
}

However, dev_request_hw_resources() only checks IS_ERR(hw->sec_fw):

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:dev_request_hw_resources() {
	hw->sec_fw = meson_sm_get(sm_np);
	of_node_put(sm_np);
	if (IS_ERR(hw->sec_fw))
		return PTR_ERR(hw->sec_fw);
}

If meson_sm_get() returns NULL, probe succeeds with hw->sec_fw == NULL, and
when a decode job runs m2mops_vdec_device_run()->load_firmware() in
drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c:

drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c:load_firmware() {
	meson_sm_call(hw->sec_fw, SM_LOAD_VIDEO_FW, &ret,
		      video_fw->fw_format, video_fw->core,
		      video_fw->is_swap, 0, 0);
}

won't meson_sm_call() dereference a NULL hw->sec_fw pointer instead of probe
returning -EPROBE_DEFER?

[Severity: Medium]
Are ctx->pic_info, plane_fmt[0].sizeimage, and the format/queue locks in
drivers/media/platform/amlogic/vdec/aml_vdec.c initialized in the right order?

First, when a file handle is opened, fops_vcodec_open()->aml_vdec_reset_fmts()
calls reset_output_fmts(), which sets ctx->pix_fmt[AML_FMT_SRC] without
calling set_pic_info():

drivers/media/platform/amlogic/vdec/aml_vdec.c:reset_output_fmts() {
	vdec_try_fmt_mp(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, &fmt,
			out_fmt);

	ctx->pix_fmt[AML_FMT_SRC] = fmt;
}

If userspace starts streaming with the default format without calling
VIDIOC_S_FMT on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, ctx->pic_info.fb_size[0]
remains 0, so h264_config_decode_spec() and h264_config_ref_spec() in
drivers/media/platform/amlogic/vdec/h264.c set c_dma_addr equal to y_dma_addr
for single-planar capture formats:

drivers/media/platform/amlogic/vdec/h264.c:h264_config_decode_spec() {
	h264_ctx->curr_spec.y_dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
	if (ctx->pix_fmt[AML_FMT_DST].num_planes > 1)
		h264_ctx->curr_spec.c_dma_addr =
		    vb2_dma_contig_plane_dma_addr(vb, 1);
	else
		h264_ctx->curr_spec.c_dma_addr =
		    h264_ctx->curr_spec.y_dma_addr + ctx->pic_info.fb_size[0];
}

Second, in vdec_try_fmt_mp(), plane_fmt[0].sizeimage for the output queue is
computed from pix_mp->height and pix_mp->width before ALIGN() and
v4l2_apply_frmsize_constraints() clamp and align them:

drivers/media/platform/amlogic/vdec/aml_vdec.c:vdec_try_fmt_mp() {
	if (V4L2_TYPE_IS_OUTPUT(type)) {
		pix_mp->num_planes = dec_fmt->num_planes;
		pix_mp->pixelformat = dec_fmt->fourcc;
		if (!pix_mp->plane_fmt[0].sizeimage)
			pix_mp->plane_fmt[0].sizeimage =
			    (pix_mp->height * pix_mp->width * 3) / 2;
		pix_mp->height = ALIGN(pix_mp->height, align);
		pix_mp->width = ALIGN(pix_mp->width, align);
	} else {
		...
	}

	v4l2_apply_frmsize_constraints(&pix_mp->width, &pix_mp->height,
				       &dec_fmt->stepwise);
}

If userspace passes width == 0 or height == 0 with sizeimage == 0, sizeimage
remains 0 even after width and height are clamped to the minimum frame size.

Third, aml_vdec_reset_fmts() and aml_vdec_queue_init() set
ctx->m2m_ctx->q_lock, src_vq->lock, and dst_vq->lock to &ctx->v4l2_intf_lock,
whereas aml_vdec_drv_probe() sets vfd_dec->lock to &dev->dev_mutex:

drivers/media/platform/amlogic/vdec/aml_vdec.c:aml_vdec_reset_fmts() {
	ctx->m2m_ctx->q_lock = &ctx->v4l2_intf_lock;
	reset_output_fmts(ctx);
	reset_capture_fmts(ctx);
}

Doesn't using two different mutexes allow VIDIOC_S_FMT and VIDIOC_G_FMT to
race with queue ioctls such as VIDIOC_REQBUFS, VIDIOC_QBUF, and
VIDIOC_STREAMON?

[Severity: Medium]
What happens in drivers/media/platform/amlogic/vdec/aml_vdec_hw.c when
pm_runtime_resume_and_get() or clk_prepare_enable() fails during
vdec_poweron()?

In pm_vdec_power_domain_power_on(), if clk_prepare_enable(gate_node->clk)
succeeds and pm_runtime_resume_and_get(dev) fails, gate_node->clk is not
disabled and unprepared, and the error is not returned to the caller:

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:pm_vdec_power_domain_power_on() {
	if (gate_node) {
		ret = clk_prepare_enable(gate_node->clk);
	...
	if (!ret && dev) {
		ret = pm_runtime_resume_and_get(dev);
		if (ret < 0)
			dev_err(hw->dev, "failed to power on %s domain: %d\n",
				pm->pd_data[id].name, ret);
	...
	if (!ret)
		dos_local_config(hw, 1, id);
}

Because vdec_poweron() increments hw->pm->pd_data[core].ref_count before
calling hw->pm->power_on(hw, core) and vdec_enable() always returns 0 to
aml_vdec_drv_probe():

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:vdec_poweron() {
	hw->pm->pd_data[core].ref_count++;
	if (hw->pm->pd_data[core].ref_count > 1)
		goto out;

	hw->pm->power_on(hw, core);
}

when aml_vdec_drv_remove() later calls
vdec_disable()->vdec_poweroff()->pm_vdec_power_domain_power_off():

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:pm_vdec_power_domain_power_off() {
	dos_local_config(hw, 0, id);

	if (dev) {
		pm_runtime_put_sync(dev);
}

won't pm_vdec_power_domain_power_off() access unpowered DOS_BUS registers in
dos_local_config(hw, 0, id) and cause unbalanced pm_runtime_put_sync() and
clk_disable_unprepare() calls?

[Severity: Low]
Should VIDEO_AMLOGIC_VDEC in drivers/media/platform/amlogic/vdec/Kconfig
select REGMAP_MMIO?

In drivers/media/platform/amlogic/vdec/aml_vdec_hw.c,
dev_request_hw_resources() initializes regmaps via devm_regmap_init_mmio():

drivers/media/platform/amlogic/vdec/aml_vdec_hw.c:dev_request_hw_resources() {
		if (i == DOS_BUS) {
			hw->map[i] = devm_regmap_init_mmio(&pdev->dev, reg_base[i],
							   &dos_regmap_conf);
		} else if (i == DMC_BUS) {
			hw->map[i] = devm_regmap_init_mmio(&pdev->dev, reg_base[i],
							   &dmc_regmap_conf);
		}
}

However, VIDEO_AMLOGIC_VDEC in drivers/media/platform/amlogic/vdec/Kconfig
does not select REGMAP_MMIO:

drivers/media/platform/amlogic/vdec/Kconfig: {
config VIDEO_AMLOGIC_VDEC
    tristate "Amlogic Video Decoder Driver"
    ...
    select VIDEOBUF2_DMA_CONTIG
    select V4L2_H264
    select V4L2_MEM2MEM_DEV
    select MESON_CANVAS
}

Could configurations enabling CONFIG_VIDEO_AMLOGIC_VDEC without another driver
selecting CONFIG_REGMAP_MMIO fail to build or link?

>  CONFIG_VIDEO_AMPHION_VPU=m
>  CONFIG_VIDEO_CADENCE_CSI2RX=m
>  CONFIG_VIDEO_WAVE_VPU=m

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-b4-s4-vdec-upstream-v12-0-616240978792@amlogic.com?part=6

  reply	other threads:[~2026-10-22  5:53 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-22  5:26 [PATCH v12 0/6] Add Amlogic stateless H.264 video decoder for S4 Zhentao Guo via B4 Relay
2026-10-22  5:26 ` Zhentao Guo via B4 Relay
2026-10-22  5:26 ` Zhentao Guo
2026-10-22  5:26 ` [PATCH v12 1/6] firmware: meson: sm: video firmware loading via secure monitor Zhentao Guo via B4 Relay
2026-10-22  5:26   ` Zhentao Guo
2026-10-22  5:26   ` Zhentao Guo via B4 Relay
2026-10-22  5:27 ` [PATCH v12 2/6] firmware: meson: sm: Add video firmware loading SMC call Zhentao Guo via B4 Relay
2026-10-22  5:27   ` Zhentao Guo via B4 Relay
2026-10-22  5:27   ` Zhentao Guo
2026-10-22  5:27 ` [PATCH v12 3/6] media: dt-bindings: Add Amlogic V4L2 video decoder Zhentao Guo via B4 Relay
2026-10-22  5:27   ` Zhentao Guo
2026-10-22  5:27   ` Zhentao Guo via B4 Relay
2026-10-30 11:27   ` Krzysztof Kozlowski
2026-10-30 11:27     ` Krzysztof Kozlowski
2026-10-30 11:57     ` Zhentao Guo
2026-10-30 11:57       ` Zhentao Guo
2026-10-22  5:27 ` [PATCH v12 4/6] decoder: Add V4L2 stateless H.264 decoder driver Zhentao Guo via B4 Relay
2026-10-22  5:27   ` Zhentao Guo via B4 Relay
2026-10-22  5:27   ` Zhentao Guo
2026-10-30 11:30   ` Krzysztof Kozlowski
2026-10-30 11:30     ` Krzysztof Kozlowski
2026-10-30 11:51     ` Zhentao Guo
2026-10-30 11:51       ` Zhentao Guo
2026-10-22  5:27 ` [PATCH v12 5/6] arm64: dts: amlogic: Add video decoder driver support for S4 SOCs Zhentao Guo via B4 Relay
2026-10-22  5:27   ` Zhentao Guo
2026-10-22  5:27   ` Zhentao Guo via B4 Relay
2026-10-22  6:01   ` sashiko-bot
2026-10-22  6:01     ` sashiko-bot
2026-10-23 11:25     ` Zhentao Guo
2026-10-23 11:25       ` Zhentao Guo
2026-10-22  5:27 ` [PATCH v12 6/6] arm64: defconfig: Enable CONFIG_VIDEO_AMLOGIC_VDEC Zhentao Guo via B4 Relay
2026-10-22  5:27   ` Zhentao Guo
2026-10-22  5:27   ` Zhentao Guo via B4 Relay
2026-10-22  5:53   ` sashiko-bot [this message]
2026-10-22  5:53     ` sashiko-bot
2026-10-23  9:46     ` Zhentao Guo
2026-10-23  9:46       ` Zhentao Guo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=sashiko-outbox-181043@kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=media-ci@linuxtv.org \
    --cc=neil.armstrong@linaro.org \
    --cc=robh@kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=zhentao.guo@amlogic.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.