Re: [PATCH -next v2 2/2] drm: verisilicon: add support for cursor planes

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 05.05.26 um 10:25 schrieb Icenowy Zheng:
[...]
>>> +	/*
>>> +	 * Only certain PoT square sizes is supported, the minimum
>>> supported
>>> +	 * size is 32x32 and the maximum known size is 256x256.
>>> +	 */
>>> +	drm_WARN_ON_ONCE(plane->dev, dc->identity.max_cursor_size
>>> < 32 ||
>>> +				     dc->identity.max_cursor_size
>>>> 256);
>> Rather make this drm_dbg().  User space might just run an
>> atomic_check
>> on an atomic_state to test if it is supported. That should not leave
>> warnings in the logs.
> The thing being checked is the driver's HWDB (instead of user input),
> if this check fails the driver is surely buggy and misoperating.

Got it. It's a hardware limit.

>
> Or maybe this check should be moved to the creating code of cursor
> planes?

Yeah, makes sense. Tested it once when you create the cursor plane.

Best regards
Thomas

>
>>> +
>>> +	if (!is_power_of_2(new_plane_state->crtc_w) ||
>>> +	    new_plane_state->crtc_w < 32 ||
>>> +	    new_plane_state->crtc_w > dc-
>>>> identity.max_cursor_size)
>>> +		return -EINVAL;
>>> +
>>> +	if (new_plane_state->crtc_w != new_plane_state->crtc_h)
>>> +		return -EINVAL;
>>> +
>>> +	/* Check if the cursor is inside the register fields'
>>> range */
>>> +	if (!vs_cursor_plane_check_coord(new_plane_state->crtc_x)
>>> ||
>>> +	    !vs_cursor_plane_check_coord(new_plane_state->crtc_y))
>>> +		return -EINVAL;
>>> +
>>> +	if (fb) {
>> Not needed. The case of !fb would have been detected by
>> drm_atomic_helper_check_plane_state(). Best regards Thomas
> Yes, I checked this function and it will set visible to false (thus
> here early return is triggered) when no FB is bound.
>
> Thanks,
> Icenowy
>
>>> +		/* Extra line padding isn't supported */
>>> +		if (fb->pitches[0] !=
>>> +		    drm_format_info_min_pitch(fb->format, 0,
>>> +					      new_plane_state-
>>>> crtc_w))
>>> +			return -EINVAL;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void vs_cursor_plane_commit(struct vs_dc *dc, unsigned int
>>> output)
>>> +{
>>> +	regmap_set_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
>>> +			VSDC_CURSOR_CONFIG_COMMIT |
>>> +			VSDC_CURSOR_CONFIG_IMG_UPDATE);
>>> +}
>>> +
>>> +static void vs_cursor_plane_atomic_enable(struct drm_plane *plane,
>>> +					   struct drm_atomic_state
>>> *atomic_state)
>>> +{
>>> +	struct drm_plane_state *state =
>>> drm_atomic_get_new_plane_state(atomic_state,
>>> +								
>>>       plane);
>>> +	struct drm_crtc *crtc = state->crtc;
>>> +	struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
>>> +	unsigned int output = vcrtc->id;
>>> +	struct vs_dc *dc = vcrtc->dc;
>>> +
>>> +	regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
>>> +			   VSDC_CURSOR_CONFIG_FMT_MASK,
>>> +			   VSDC_CURSOR_CONFIG_FMT_ARGB8888);
>>> +
>>> +	vs_cursor_plane_commit(dc, output);
>>> +}
>>> +
>>> +static void vs_cursor_plane_atomic_disable(struct drm_plane
>>> *plane,
>>> +					    struct
>>> drm_atomic_state *atomic_state)
>>> +{
>>> +	struct drm_plane_state *state =
>>> drm_atomic_get_old_plane_state(atomic_state,
>>> +								
>>>       plane);
>>> +	struct drm_crtc *crtc = state->crtc;
>>> +	struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
>>> +	unsigned int output = vcrtc->id;
>>> +	struct vs_dc *dc = vcrtc->dc;
>>> +
>>> +	regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
>>> +			   VSDC_CURSOR_CONFIG_FMT_MASK,
>>> +			   VSDC_CURSOR_CONFIG_FMT_OFF);
>>> +
>>> +	vs_cursor_plane_commit(dc, output);
>>> +}
>>> +
>>> +static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
>>> +					   struct drm_atomic_state
>>> *atomic_state)
>>> +{
>>> +	struct drm_plane_state *state =
>>> drm_atomic_get_new_plane_state(atomic_state,
>>> +								
>>>       plane);
>>> +	struct drm_framebuffer *fb = state->fb;
>>> +	struct drm_crtc *crtc = state->crtc;
>>> +	struct vs_dc *dc;
>>> +	struct vs_crtc *vcrtc;
>>> +	unsigned int output;
>>> +	dma_addr_t dma_addr;
>>> +
>>> +	if (!state->visible) {
>>> +		vs_cursor_plane_atomic_disable(plane,
>>> atomic_state);
>>> +		return;
>>> +	}
>>> +
>>> +	vcrtc = drm_crtc_to_vs_crtc(crtc);
>>> +	output = vcrtc->id;
>>> +	dc = vcrtc->dc;
>>> +
>>> +	/* Other sizes should be rejected by atomic_check */
>>> +	switch (state->crtc_w) {
>>> +	case 32:
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_CONFIG(output),
>>> +				   VSDC_CURSOR_CONFIG_SIZE_MASK,
>>> +				   VSDC_CURSOR_CONFIG_SIZE_32);
>>> +		break;
>>> +	case 64:
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_CONFIG(output),
>>> +				   VSDC_CURSOR_CONFIG_SIZE_MASK,
>>> +				   VSDC_CURSOR_CONFIG_SIZE_64);
>>> +		break;
>>> +	case 128:
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_CONFIG(output),
>>> +				   VSDC_CURSOR_CONFIG_SIZE_MASK,
>>> +				   VSDC_CURSOR_CONFIG_SIZE_128);
>>> +		break;
>>> +	case 256:
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_CONFIG(output),
>>> +				   VSDC_CURSOR_CONFIG_SIZE_MASK,
>>> +				   VSDC_CURSOR_CONFIG_SIZE_256);
>>> +		break;
>>> +	}
>>> +
>>> +	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
>>> +
>>> +	regmap_write(dc->regs, VSDC_CURSOR_ADDRESS(output),
>>> +		     lower_32_bits(dma_addr));
>>> +
>>> +	/*
>>> +	 * The X_OFF and Y_OFF fields define which point does the
>>> LOCATION
>>> +	 * register represent in the cursor image, and LOCATION
>>> register
>>> +	 * values are unsigned. To for positive left-top
>>> coordinates the
>>> +	 * offset is set to 0 and the location is set to the
>>> coordinate, for
>>> +	 * negative coordinates the location is set to 0 and the
>>> offset
>>> +	 * is set to the opposite number of the coordinate to
>>> offset the
>>> +	 * cursor image partly off-screen.
>>> +	 */
>>> +	if (state->crtc_x >= 0) {
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_CONFIG(output),
>>> +				   VSDC_CURSOR_CONFIG_X_OFF_MASK,
>>> 0);
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_LOCATION(output),
>>> +				   VSDC_CURSOR_LOCATION_X_MASK,
>>> +				   VSDC_CURSOR_LOCATION_X(state-
>>>> crtc_x));
>>> +	} else {
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_CONFIG(output),
>>> +				   VSDC_CURSOR_CONFIG_X_OFF_MASK,
>>> +				   -state->crtc_x);
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_LOCATION(output),
>>> +				   VSDC_CURSOR_LOCATION_X_MASK,
>>> 0);
>>> +	}
>>> +
>>> +	if (state->crtc_y >= 0) {
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_CONFIG(output),
>>> +				   VSDC_CURSOR_CONFIG_Y_OFF_MASK,
>>> 0);
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_LOCATION(output),
>>> +				   VSDC_CURSOR_LOCATION_Y_MASK,
>>> +				   VSDC_CURSOR_LOCATION_Y(state-
>>>> crtc_y));
>>> +	} else {
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_CONFIG(output),
>>> +				   VSDC_CURSOR_CONFIG_Y_OFF_MASK,
>>> +				   -state->crtc_y);
>>> +		regmap_update_bits(dc->regs,
>>> VSDC_CURSOR_LOCATION(output),
>>> +				   VSDC_CURSOR_LOCATION_Y_MASK,
>>> 0);
>>> +	}
>>> +
>>> +	vs_cursor_plane_commit(dc, output);
>>> +}
>>> +
>>> +static const struct drm_plane_helper_funcs
>>> vs_cursor_plane_helper_funcs = {
>>> +	.atomic_check	= vs_cursor_plane_atomic_check,
>>> +	.atomic_update	= vs_cursor_plane_atomic_update,
>>> +	.atomic_enable	= vs_cursor_plane_atomic_enable,
>>> +	.atomic_disable	= vs_cursor_plane_atomic_disable,
>>> +};
>>> +
>>> +static const struct drm_plane_funcs vs_cursor_plane_funcs = {
>>> +	.atomic_destroy_state	=
>>> drm_atomic_helper_plane_destroy_state,
>>> +	.atomic_duplicate_state	=
>>> drm_atomic_helper_plane_duplicate_state,
>>> +	.disable_plane		= drm_atomic_helper_disable_plane,
>>> +	.reset			= drm_atomic_helper_plane_reset,
>>> +	.update_plane		= drm_atomic_helper_update_plane,
>>> +};
>>> +
>>> +static const u32 vs_cursor_plane_formats[] = {
>>> +	DRM_FORMAT_ARGB8888,
>>> +};
>>> +
>>> +static const u64 vs_cursor_plane_modifiers[] = {
>>> +	DRM_FORMAT_MOD_LINEAR,
>>> +	DRM_FORMAT_MOD_INVALID, /* sentinel */
>>> +};
>>> +
>>> +struct drm_plane *vs_cursor_plane_init(struct drm_device *drm_dev,
>>> +				       struct vs_dc *dc)
>>> +{
>>> +	struct drm_plane *plane;
>>> +
>>> +	plane = drmm_universal_plane_alloc(drm_dev, struct
>>> drm_plane, dev, 0,
>>> +					   &vs_cursor_plane_funcs,
>>> +					
>>> vs_cursor_plane_formats,
>>> +					
>>> ARRAY_SIZE(vs_cursor_plane_formats),
>>> +					
>>> vs_cursor_plane_modifiers,
>>> +					   DRM_PLANE_TYPE_CURSOR,
>>> +					   NULL);
>>> +
>>> +	if (IS_ERR(plane))
>>> +		return plane;
>>> +
>>> +	drm_plane_helper_add(plane,
>>> &vs_cursor_plane_helper_funcs);
>>> +
>>> +	return plane;
>>> +}
>>> diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane_regs.h
>>> b/drivers/gpu/drm/verisilicon/vs_cursor_plane_regs.h
>>> new file mode 100644
>>> index 0000000000000..99693f2c95b94
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane_regs.h
>>> @@ -0,0 +1,44 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright (C) 2025 Icenowy Zheng <uwu@icenowy.me>
>>> + *
>>> + * Based on vs_dc_hw.h, which is:
>>> + *   Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
>>> + */
>>> +
>>> +#ifndef _VS_CURSOR_PLANE_REGS_H_
>>> +#define _VS_CURSOR_PLANE_REGS_H_
>>> +
>>> +#include <linux/bits.h>
>>> +
>>> +#define VSDC_CURSOR_CONFIG(n)		(0x1468 + 0x1080 * (n))
>>> +#define VSDC_CURSOR_CONFIG_FMT_MASK	GENMASK(1, 0)
>>> +#define VSDC_CURSOR_CONFIG_FMT_ARGB8888	(0x2 << 0)
>>> +#define VSDC_CURSOR_CONFIG_FMT_OFF	(0x0 << 0)
>>> +#define VSDC_CURSOR_CONFIG_IMG_UPDATE	BIT(2)
>>> +#define VSDC_CURSOR_CONFIG_COMMIT	BIT(3)
>>> +#define VSDC_CURSOR_CONFIG_SIZE_MASK	GENMASK(7, 5)
>>> +#define VSDC_CURSOR_CONFIG_SIZE_32	(0x0 << 5)
>>> +#define VSDC_CURSOR_CONFIG_SIZE_64	(0x1 << 5)
>>> +#define VSDC_CURSOR_CONFIG_SIZE_128	(0x2 << 5)
>>> +#define VSDC_CURSOR_CONFIG_SIZE_256	(0x3 << 5)
>>> +#define VSDC_CURSOR_CONFIG_Y_OFF_MASK	GENMASK(12, 8)
>>> +#define VSDC_CURSOR_CONFIG_Y_OFF(v)	((v) << 8)
>>> +#define VSDC_CURSOR_CONFIG_X_OFF_MASK	GENMASK(20, 16)
>>> +#define VSDC_CURSOR_CONFIG_X_OFF(v)	((v) << 16)
>>> +
>>> +#define VSDC_CURSOR_ADDRESS(n)		(0x146C + 0x1080 * (n))
>>> +
>>> +#define VSDC_CURSOR_LOCATION(n)		(0x1470 + 0x1080 *
>>> (n))
>>> +#define VSDC_CURSOR_LOCATION_X_MASK	GENMASK(14, 0)
>>> +#define VSDC_CURSOR_LOCATION_X(v)	((v) << 0)
>>> +#define VSDC_CURSOR_LOCATION_Y_MASK	GENMASK(30, 16)
>>> +#define VSDC_CURSOR_LOCATION_Y(v)	((v) << 16)
>>> +
>>> +#define VSDC_CURSOR_BACKGROUND(n)	(0x1474 + 0x1080 * (n))
>>> +#define VSDC_CURSOR_BACKGRUOND_DEFAULT	0x00FFFFFF
>>> +
>>> +#define VSDC_CURSOR_FOREGROUND(n)	(0x1478 + 0x1080 * (n))
>>> +#define VSDC_CURSOR_FOREGRUOND_DEFAULT	0x00AAAAAA
>>> +
>>> +#endif /* _VS_CURSOR_PLANE_REGS_H_ */
>>> diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h
>>> b/drivers/gpu/drm/verisilicon/vs_plane.h
>>> index 41875ea3d66a5..60b5b3a1bc22a 100644
>>> --- a/drivers/gpu/drm/verisilicon/vs_plane.h
>>> +++ b/drivers/gpu/drm/verisilicon/vs_plane.h
>>> @@ -68,5 +68,6 @@ dma_addr_t vs_fb_get_dma_addr(struct
>>> drm_framebuffer *fb,
>>>    			      const struct drm_rect *src_rect);
>>>    
>>>    struct drm_plane *vs_primary_plane_init(struct drm_device *dev,
>>> struct vs_dc *dc);
>>> +struct drm_plane *vs_cursor_plane_init(struct drm_device *dev,
>>> struct vs_dc *dc);
>>>    
>>>    #endif /* _VS_PLANE_H_ */

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)