[PATCH v6 21/23] drm: rockchip: Add VOP2 driver

[Andy Yan]

Hi Sascha:

On 2/24/22 16:19, Sascha Hauer wrote:
> On Sat, Feb 19, 2022 at 03:35:12PM +0800, Andy Yan wrote:
>> Hi Sascha:
>>
>> On 2/18/22 16:00, Sascha Hauer wrote:
>>> On Fri, Feb 18, 2022 at 11:50:32AM +0800, Andy Yan wrote:
>>>> Hi Sascha:
>>>>
>>>> On 2/17/22 22:06, Heiko Stübner wrote:
>>>>> Am Donnerstag, 17. Februar 2022, 14:58:23 CET schrieb Sascha Hauer:
>>>>>> Hi Andy,
>>>>>>
>>>>>> Please trim the context in your answers to the relevant parts, it makes
>>>>>> it easier to find the things you said.
>>>>>>
>>>>>> On Thu, Feb 17, 2022 at 08:00:11PM +0800, Andy Yan wrote:
>>>>>>> Hi Sascha:
>>>>>>>
>>>>>>>> +
>>>>>>>> +	drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>>>>>>>> +		struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>>>>>>>> +		struct device_node *node, *parent;
>>>>>>>> +
>>>>>>>> +		parent = of_get_parent(rkencoder->port);
>>>>>>>> +
>>>>>>>> +		for_each_endpoint_of_node(parent, node) {
>>>>>>> Is there any hurt directly use our downstream vendor kernel method here: use
>>>>>>> vcstate->output_if set by encoder driver to get which interface we should
>>>>>>> enable here?
>>>>>> There is no vcstate->output_if in mainline currently. Ok, we could add
>>>>>> that. The other thing is that there are multiple HDMI interfaces and
>>>>>> the id of the HDMI encoder is encoded into output_if. Downstream kernel
>>>>>> adds OF aliases to the HDMI ports. I didn't want to go that route
>>>>>> because it doesn't seem to be very elegant to me.
>>>> aliases is a very comm strategy in device tree world.
>>> Yes, but not for retrieving bit offsets into registers. Normally aliases
>>> can be changed at board level without confusing drivers.
>>>
>>>> And your method also
>>>> add need additional dt binds to define RK3568_VOP2_EP_xxx
>>>>>>> You method is ok with device tree,  but it tied up this driver to device
>>>>>>> tree, we are now tring to extend vop2 driver work with ACPI, so we hope this
>>>>>>> driver can be much more flexible.
>>>>>> The current rockchip drm driver seems to be pretty much tied to device
>>>>>> tree. There are probably many other places that need parallel paths for
>>>>>> ACPI support, I think we can delay this particular part until we see the
>>>>>> whole picture. In the end we can still retrieve the output_if
>>>>>> information differently with ACPI while still retrieving the information
>>>>>> from the device tree the way we are doing currently.
>>>> The current driver only reference device thee at driver initial, we not wrap
>>>>
>>>> device tree related things in other parts, so if we extend it to support
>>>> ACPI,
>>>>
>>>> we just need modify the initial code, this make things easier.
>>> The device tree parsing could be moved out of vop2_crtc_atomic_enable()
>>> into some initialisation path. In the end it's static information,
>>> there's no need to do it repeatedly in atomic_enable.
>> This could be one solution, the repeatedly parsing device tree in
>> atomic_enable is also my concern.
>>
>> In addition, there are 2 HDMI, 2 eDP, 2 MIPI on the coming rk3588, so it's
>> better to consider give position
>>
>> for HDMI1, EDP1, in  include/dt-bindings/soc/rockchip,vop2.h
> The defines are rk3568 specific. rk3588 would use a set of rk3588
> specific defines along with a rk3588_set_intf_mux().


Why not try to share these RK3568_VOP2_EP_XXX across all vop2 even vop 
based rockchip socs?

If make these definition RK3568 specific, we need copy all of it and 
change 3568 to 3588 than add HDMI1, HDMI0, EDP1,EDP0

when rk3588 coming, if there is another rk35xx, we need to the same 
thing again.... but they share same code logic and number,

the only difference is the definition name.


Please take a look at the current upstream vop driver,  it support 13 
socs, when we add support for a new vop , most of

the work is just add registers definition in rockchip_vop_reg.c, we 
don't need to duplicate soc specific code in rockchip_drm_vop.c,

these make the upstream process much easier, and keep the vop driver 
tiny and clean.

> Sascha
>