Received-SPF: pass (google.com: domain of linux-kernel+bounces-167661-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <062d86ab-1518-4160-870c-3cd36228d055@cherry.de>
Date: Fri, 3 May 2024 14:57:03 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] drm/rockchip: vop2: configure layers for vp3 on
 rk3588
To: Heiko Stuebner <heiko@sntech.de>
Cc: hjc@rock-chips.com, andy.yan@rock-chips.com,
 dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org,
 linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
 Heiko Stuebner <heiko.stuebner@cherry.de>
References: <20240425195506.2935955-1-heiko@sntech.de>
 <20240425195506.2935955-3-heiko@sntech.de>
Content-Language: en-US
From: Quentin Schulz <quentin.schulz@cherry.de>
In-Reply-To: <20240425195506.2935955-3-heiko@sntech.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?aVNjVEp4d08rS0hnRS9CSU1JTEZobStEbm5veVRtYTI0UDFwdzBxVThTSTVJ?=
 =?utf-8?B?WHBvMWROMUpyUWowMzg4M3M3S1Mzd0RNaHZmOWVKdm9YalNFbHZkaGtGdVdk?=
 =?utf-8?B?ckhxT1dxcmxpdEZjWnRZTGgvTEo0ZlYySzIwQThOamdEbGZUUHE1YlFoMkk0?=
 =?utf-8?B?a3o2M1FpTU9vbHNpbmNHdzNwU25tRXY3TlZOMTB1STFack44TkxFV2xPSDFq?=
 =?utf-8?B?d01BZHVTeDdoelBSZ25lTHNSSnZPZmhLbmZ3RkJ3OWh0bFBoU21FREZoVUVt?=
 =?utf-8?B?bEZXcVIvZzltTWtncVgyTHM4U01HbVlZdG5RUjJDSTlrZWJPcXc3RUs5MEt6?=
 =?utf-8?B?VkhuemRYVkxzQVg2TEFFeitGbHlUeEpZTGhrZ3doRzFjWGRVMWVkL2VtMTNa?=
 =?utf-8?B?amZsOGhlamhYR25ibldqK2ZJWkJZVHJPMklKNEVKbWU0eEg0cTdnNEFEUFFr?=
 =?utf-8?B?NDBDYnpRYzFNVFMrT2JacjJacnRaRkI2aXFDNHlHVmp2V3ZVckZUUDRUY0Jy?=
 =?utf-8?B?d1p0cFJCb1Q0bUVMaHRzV3htM1pkUWVDWWtaOEJCbmJEOU5vL2Vxbkw5OEJz?=
 =?utf-8?B?Q1FtWVBHZlVBZFhTcmZEanhtem95aGNjL0hQRUZNZnc0S1F5aHNhUDkyb1RE?=
 =?utf-8?B?ZmZnS2Evb0UwSEJOaGtPZTIvU0xGS3lSV3NHQ0h6a016THJkYTlmM1RMd1c0?=
 =?utf-8?B?cm91elVvRUV3VC9ydllQRU1YUDBObWJpRmI2NXE0N2NjY1Y0dDllNjJqQkQ5?=
 =?utf-8?B?bmxsM2hVLzBXSTBpckgrN3VFVFZDUmZUUWVPK1RKRXdRaGtZL1IyYmN4WHZO?=
 =?utf-8?B?VEU4cTk2NGlSazcreWYycHRMQmNJNWk3K3JPTlg3YWUxWFoxalNaeE1yMjll?=
 =?utf-8?B?T0VObTJKSkFoRTY0WlBLVVNQUVU4aWpGcjAwbnBIOVVDNHE5eUllTml0R0E1?=
 =?utf-8?B?S2pTS05LNEdaRldxaU1CSzVtR1lYV2lDeVB1cCtiMUU0aWs0V2lneEtQem1a?=
 =?utf-8?B?aUV3Z1ltc0p4WWowbDhtNEJ6cWJ6cXdELzlIL1RxZGY2Ry9mTnB2VWUyNEEz?=
 =?utf-8?B?UkVLeTJGNTFVVjhHZXhuSEpuWTBweFVsUnZuS3dCaHJSUDM4bWhhLy9BUDhs?=
 =?utf-8?B?TkozZE00ZTZPejFLYWN4ZXRnMExtSEtBVDZpNVZ0ZU5PNjVGRDdqNDZhTU4y?=
 =?utf-8?B?U2JyR09jOU50bHlld3hKeXlhNjZuckJ0V2tPRForY2sxemRqOTFKMi90NHlQ?=
 =?utf-8?B?UVF5aXFMTU1mSHB2elh6cHdlWVNUemVSMW5Ud2pJQm5VbjN3T055SUtnaHli?=
 =?utf-8?B?c1Vlc3dtT3VqNnh3NUE4SVdMQTdIOGN4VFVwV1FNeTlHUDM0UmU5aEFSMm1Y?=
 =?utf-8?B?eS9JcmFHd2tZbU4reUNRNExEYjI3Z3BUUldxWk1aMUlhWHprVTkwcDZKMDRa?=
 =?utf-8?B?TS9DM1FSSTZoWVpXUitDaGRuU240cFNycGIySEQ4OE9PY255eEtLUFFLcnU5?=
 =?utf-8?B?VGhZNEd1dnVaaHl6dGg4RjVBNWYrNWhJL1JtY3hFajd6MXptVFc3R3BTS3hS?=
 =?utf-8?B?Z0laT20vTklyVE5GLzg1a2ZPRFllMldHK09JWjlkejBWTnY5RGRZaitnMTZU?=
 =?utf-8?B?cHJJNHdJMzR2RHErSGFtRWcxZUxublZHelgvSHdRaDJJQlVNdjVVa3QwYW0z?=
 =?utf-8?B?RERMS0JjQnhZQXZzK0lSOXhXZGxCSktja2crdGhDcDZsY0FVTlhDQVd3b01y?=
 =?utf-8?B?U00zK2xYZ1dTeEx4N3JpMTd1UDE1QWhYSWRyY1VtRFpmNVhBM1lnVUpkd1VV?=
 =?utf-8?B?MmNEaWpFU2UrWkd1V1g4U2E1WjB6S2RyeFEwb09kaFNoQ1JCZkpIWlQ5NnNo?=
 =?utf-8?B?YXFhRkVBelpuUk9yRENMNGhIbGxvTFJqZGlFUW0yS29PVXFDTkNTVlBSWDZ1?=
 =?utf-8?B?Z09mKzVvRkNoTmFoSEgyV3ZIcHhWanJvMU1CY3Z0OE1tbGtYM2pWbk85L1Ru?=
 =?utf-8?B?VWpCRFdoSGlrQXNBeDVGcnFBbUVpYTRhN2tCbnFMcW0xYW1jbEN6RHNEeVVo?=
 =?utf-8?B?QlI3ZTh0UTAyNEsxWnpxenFFditEVmxISFBpWkRTVEoyczlsYXNDVDVEdksx?=
 =?utf-8?B?TFBDYVpwNWR1OSswOW9LZDk2b3kvVGJHRlJJSDlEblZUYzJsNHUrT091dlpN?=
 =?utf-8?B?S1E9PQ==?=
X-OriginatorOrg: cherry.de
X-MS-Exchange-CrossTenant-Network-Message-Id: 79bf0adb-8e86-43b5-a4ef-08dc6b7088af
X-MS-Exchange-CrossTenant-AuthSource: DB7PR04MB4842.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 12:57:05.4937
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5e0e1b52-21b5-4e7b-83bb-514ec460677e
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TcmikpbHMykAYstIDM9k8MDten0EaXXKdvqdFG28XEobvxLMp7aA/gnZdxk1EEpdk/jQ81kccHgpxtgHTHsoA7kPTS+auksb3PFQa9e6bPA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR04MB8352

Hi Heiko,

On 4/25/24 9:55 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> The rk3588 VOP2 has 4 video-ports, yet the driver currently only
> configures the first 3, as used on the rk3568.
> 

I'm wondering whether we should update the drawing at the top of the 
driver then?

> Add another block to configure the vp3 as well, if applicable.
> 
> Fixes: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 12 ++++++++++++
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  1 +
>   2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 523880a4e8e74..1a327a9ed7ee4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2303,6 +2303,7 @@ static void vop2_setup_alpha(struct vop2_video_port *vp)
>   static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>   {
>   	struct vop2 *vop2 = vp->vop2;
> +	const struct vop2_data *vop2_data = vop2->data;
>   	struct drm_plane *plane;
>   	u32 layer_sel = 0;
>   	u32 port_sel;
> @@ -2344,6 +2345,17 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>   	else
>   		port_sel |= FIELD_PREP(RK3568_OVL_PORT_SET__PORT2_MUX, 8);
>   
> +	/* configure vp3 */
> +	if (vop2_data->soc_id == 3588) {

I think it'd be smarter to check against vop2->data->nr_vps >= 4; so 
that we don't need to maintain a list of SoCs that support a specific 
number of video ports.

> +		struct vop2_video_port *vp3 = &vop2->vps[3];

This is always possible because vps is statically allocated for 4 items, 
c.f. struct vop2_video_port vps[ROCKCHIP_MAX_CRTC]; so we don't 
necessarily need it in this specific location and can group it with the 
others. Cosmetic suggestion though.

Otherwise, the change itself makes sense to me, so:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin