Re: [PATCH v5 4/4] drm/rockchip: vop2: Support setting custom background color

From: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
To: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@gmail.com>,
	"Simona Vetter" <simona@ffwll.ch>,
	"Sandy Huang" <hjc@rock-chips.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Andy Yan" <andy.yan@rock-chips.com>,
	"Louis Chauvet" <louis.chauvet@bootlin.com>,
	"Haneen Mohammed" <hamohammed.sa@gmail.com>,
	"Melissa Wen" <melissa.srw@gmail.com>,
	"Robert Mader" <robert.mader@collabora.com>,
	kernel@collabora.com, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org,
	"Diederik de Haas" <diederik@cknow-tech.com>
Subject: Re: [PATCH v5 4/4] drm/rockchip: vop2: Support setting custom background color
Date: Thu, 29 Jan 2026 03:29:53 +0200	[thread overview]
Message-ID: <64633be5-4be9-469c-9164-c1d584ea4c7b@collabora.com> (raw)
In-Reply-To: <aXjPjNWl3oVOVYxV@black.igk.intel.com>

On 1/27/26 4:45 PM, Andy Shevchenko wrote:
> On Tue, Jan 27, 2026 at 10:45:36AM +0200, Cristian Ciocaltea wrote:
>> The Rockchip VOP2 display controller allows configuring the background
>> color of each video output port.
>>
>> Since a previous patch introduced the BACKGROUND_COLOR CRTC property,
>> which defaults to solid black, make use of it when programming the
>> hardware.
>>
>> Note the maximum precision allowed by the display controller is 10bpc,
>> while the alpha component is not supported, hence ignored.
> 
> ...
> 
>> +	/*
>> +	 * Background color is programmed with 10 bits of precision.
>> +	 * Since performance is more important than accuracy here,
>> +	 * do *not* make use of the DRM_ARGB64_GET*_BPC() helpers.
>> +	 */
>> +	val = FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_RED, DRM_ARGB64_GETR(bgcolor) >> 6);
>> +	val |= FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_GREEN, DRM_ARGB64_GETG(bgcolor) >> 6);
>> +	val |= FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_BLUE, DRM_ARGB64_GETB(bgcolor) >> 6);
> 
> There is FIELD_MODIFY() for a few cycles already.

I've been aware, but found it more appropriate when modifying an existing value
(e.g. previously read from a register) rather than when computing one from
scratch. 

> But here it probably makes no much difference.

Thinking again about this, it is actually handy when keeping the first
FIELD_PREP (for initialization), and using it just for the subsequent
operations.  Hence considered this approach in v6.

> ...
> 
>> +	seq_printf(s, "\tbackground color (10bpc): r=0x%x g=0x%x b=0x%x\n",
>> +		   DRM_ARGB64_GETR(cstate->background_color) >> 6,
>> +		   DRM_ARGB64_GETG(cstate->background_color) >> 6,
>> +		   DRM_ARGB64_GETB(cstate->background_color) >> 6);
> 
> Probably you want to have the alternative to the DRM_ARGB64_GETx() macros which
> incorporates a right-shift. But it all in regard to DRM style and preferences.

I tried to keep the API as simple as possible, but you're right, it's better to
have those macros rather than open-coding the bit-shift all over.  Also handled
in v6.

Thanks again,
Cristian

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