[PATCH v9 00/23] drm/rockchip: RK356x VOP2 support

[Andy Yan]

Hi:

On 4/5/22 17:37, Sascha Hauer wrote:
> On Sat, Apr 02, 2022 at 09:37:17AM +0800, Andy Yan wrote:
>> Hi Sacha:
>>
>> On 4/1/22 20:52, Sascha Hauer wrote:
>>> -- 
>>> >From cbc03073623a7180243331ac24c3afaf9dec7522 Mon Sep 17 00:00:00 2001
>>> From: Sascha Hauer<s.hauer at pengutronix.de>
>>> Date: Fri, 1 Apr 2022 14:48:49 +0200
>>> Subject: [PATCH] fixup! drm: rockchip: Add VOP2 driver
>>>
>>> ---
>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> index 7dba7b9b63dc6..1421bf2f133f1 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> @@ -2287,6 +2287,20 @@ static int vop2_create_crtc(struct vop2 *vop2)
>>>    			}
>>>    		}
>>> +		if (vop2->data->soc_id == 3566) {
>>> +			/*
>>> +			 * On RK3566 these windows don't have an independent
>>> +			 * framebuffer. They share the framebuffer with smart0,
>>> +			 * esmart0 and cluster0 respectively.
>>> +			 */
>>> +			switch (win->data->phys_id) {
>>> +			case ROCKCHIP_VOP2_SMART1:
>>> +			case ROCKCHIP_VOP2_ESMART1:
>>> +			case ROCKCHIP_VOP2_CLUSTER1:
>>> +				continue;
>>> +			}
>>
>> Think about this , there maybe other upcoming vop2 base soc, they may only
>> have
>>
>> mirror window Smart1 Esmart1, or Smart1, Esmart1, Esmart2, Cluster1.
>>
>> I think this should add WIN_FEATURE at the platform description file
>> rockchip_vop2_reg.c, then
>>
>> check the FEATURE to decide whether the driver should give this window a
>> special treatment.
>>
>> this can make one code run for different soc with different platform
>> description. or we should add
>>
>> the same code logic for different soc again and again.
> You mean like done in the downstream Kernel? Here indeed we have a
> WIN_FEATURE_MIRROR flag added to the platform description. This is then
> evaluated with:
>
> static bool vop2_is_mirror_win(struct vop2_win *win)
> {
>          return soc_is_rk3566() && (win->feature & WIN_FEATURE_MIRROR);
> }
>
> So a flag is added and afterwards its evaluation is SoC specific. That
> doesn't help at all and only obfuscates things.
>
> Besides, experience shows that you can't predict a good abstraction for

This is not a  predict,  this is an IP feature, so it will appeared on 
upcoming SOC.

We have rk3588 with 8 windows(4 Cluster + 4 Esmart, no Smart window), and

also have a entry level soc which only have 4 windows, they both have 
this feature.

> future hardware revisions, the hardware guys are just too creative in
> creating hardware that breaks existing abstractions.
>
> Sascha
>