Re: [PATCH v6 5/6] drm/verisilicon: add DCUltraLite chip identity to HWDB

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Negative cursor coordinates are programmed incorrectly because the offset values are not shifted before being passed to regmap_update_bits().
--

commit 71dc0bfa33a2021c1ba93da0095ed32939fb4009
Author: Joey Lu <a0987203069@gmail.com>

drm/verisilicon: add DCUltraLite chip identity to HWDB
    
The Nuvoton MA35D1 chip contains a DCUltraLite display controller with
model number 0x0 (sic, the model name contains no number either),
revision 0x5560 and customer ID 0x305.  It has a similar register map
with DC8000, only one display output and only 32x32 cursor supported.

> diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c b/drivers/gpu/drm/verisilicon/vs_hwdb.c
> index 91524d16f7785..7d630a667a3f0 100644
> --- a/drivers/gpu/drm/verisilicon/vs_hwdb.c
> +++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c
[ ... ]
> @@ -129,6 +129,16 @@ static struct vs_chip_identity vs_chip_identities[] = {
>  		.max_cursor_size = 64,
>  		.formats = &vs_formats_no_yuv444,
>  	},
> +	{
> +		.model = 0x0,		/* DCUltraLite */
> +		.revision = 0x5560,
> +		.customer_id = 0x305,
> +
> +		.generation = VSDC_GEN_DC8000,
> +		.display_count = 1,
> +		.max_cursor_size = 32,
> +		.formats = &vs_formats_no_yuv444,
> +	},
>  };

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the cursor
handling code related to this new controller, I noticed an issue with how
negative cursor coordinates are programmed.

Looking at vs_cursor_plane_atomic_update() in
drivers/gpu/drm/verisilicon/vs_cursor_plane.c, when handling negative
X coordinates:

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

Is the unshifted X offset (-state->crtc_x) completely masked out to 0 by
regmap_update_bits() because it lacks the necessary shift macro (like
VSDC_CURSOR_CONFIG_X_OFF)?

Similarly, for the Y coordinate:

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

Does this also cause the unshifted Y offset (-state->crtc_y) to be
masked out to 0?

When the cursor is moved off the left or top edge of the screen, the hardware
offset is intended to clip the cursor.

However, if the unshifted offset value is masked out by regmap_update_bits(),
it results in an offset of 0.

This would cause the cursor to stick at the edge of the screen fully visible,
rather than sliding smoothly off-screen.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908092840.225220-1-a0987203069@gmail.com?part=5