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

[Andy Yan]

Hi:

On 4/6/22 16:13, Sascha Hauer wrote:
> On Wed, Apr 06, 2022 at 10:02:59AM +0800, Andy Yan wrote:
>> 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.
> Same as with the other discussion: Please let's solve this once we are
> there.


I am not sure if this is the suitable way for upstream, this sound like

just solve the issue appeared at the front of eyes and not think any

thing about make this driver easy to support new hardware in the future.

> For now my addition is the easiest way out. Once other SoCs shall be
> supported we can re-evaluate that and find better suitable ways for SoC
> abstractions. This may result in just your suggestion (in which case you
> can say told-you-so) or completely different.
>
> Sascha
>