Re: [PATCH v9 20/23] drm/rockchip: Make VOP driver optional

[Andy Yan <andy.yan@rock-chips.com>]

Hi:

On 4/6/22 15:04, Sascha Hauer wrote:
> On Wed, Apr 06, 2022 at 09:43:49AM +0800, Andy Yan wrote:
>> Hi Sacha:
>>
>> On 4/5/22 17:05, Sascha Hauer wrote:
>>> On Sat, Apr 02, 2022 at 09:25:33AM +0800, Andy Yan wrote:
>>>> Hi Sascha:
>>>>
>>>> On 4/1/22 20:55, Sascha Hauer wrote:
>>>>> On Thu, Mar 31, 2022 at 07:00:34PM +0800, Andy Yan wrote:
>>>>>> Hi:
>>>>>>
>>>>>> On 3/31/22 16:18, Sascha Hauer wrote:
>>>>>>> On Thu, Mar 31, 2022 at 03:20:37PM +0800, Andy Yan wrote:
>>>>>>>> Hi Sascha:
>>>>>>>>
>>>>>>>> On 3/31/22 15:06, Sascha Hauer wrote:
>>>>>>>>> On Wed, Mar 30, 2022 at 08:50:09PM +0800, Andy Yan wrote:
>>>>>>>>>> Hi Sascha:
>>>>>>>>>>
>>>>>>>>>> On 3/30/22 14:39, Sascha Hauer wrote:
>>>>>>>>>>> Hi Andy,
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Mar 29, 2022 at 07:56:27PM +0800, Andy Yan wrote:
>>>>>>>>>>>> Hi Sascha:
>>>>>>>>>>>>
>>>>>>>>>>>> On 3/28/22 23:11, Sascha Hauer wrote:
>>>>>>>>>>>>> With upcoming VOP2 support VOP won't be the only choice anymore, so make
>>>>>>>>>>>>> the VOP driver optional.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>         drivers/gpu/drm/rockchip/Kconfig            | 8 ++++++++
>>>>>>>>>>>>>         drivers/gpu/drm/rockchip/Makefile           | 3 ++-
>>>>>>>>>>>>>         drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +-
>>>>>>>>>>>>>         3 files changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
>>>>>>>>>>>>> index fa5cfda4e90e3..7d22e2997a571 100644
>>>>>>>>>>>>> --- a/drivers/gpu/drm/rockchip/Kconfig
>>>>>>>>>>>>> +++ b/drivers/gpu/drm/rockchip/Kconfig
>>>>>>>>>>>>> @@ -23,8 +23,16 @@ config DRM_ROCKCHIP
>>>>>>>>>>>>>         if DRM_ROCKCHIP
>>>>>>>>>>>>> +config ROCKCHIP_VOP
>>>>>>>>>>>>> +	bool "Rockchip VOP driver"
>>>>>>>>>>>>> +	default y
>>>>>>>>>>>>> +	help
>>>>>>>>>>>>> +	  This selects support for the VOP driver. You should enable it
>>>>>>>>>>>>> +	  on all older SoCs up to RK3399.
>>>>>>>>>>> That reminds me that I wanted to rephrase this. Will change in next
>>>>>>>>>>> round.
>>>>>>>>>>>
>>>>>>>>>>>>> +
>>>>>>>>>>>>>         config ROCKCHIP_ANALOGIX_DP
>>>>>>>>>>>>>         	bool "Rockchip specific extensions for Analogix DP driver"
>>>>>>>>>>>>> +	depends on ROCKCHIP_VOP
>>>>>>>>>>>> Aanlogix dp is also on vop2 base soc such as  rk356x and rk3588.
>>>>>>>>> BTW I just looked at the downstream driver. Here we have the same
>>>>>>>>> situation that the analogix dp driver calls rockchip_drm_wait_vact_end()
>>>>>>>>> which is implemented in the VOP driver, so when the analogix dp driver
>>>>>>>>> is actually used on a VOP2 SoC then it is either used in a way that
>>>>>>>>> rockchip_drm_wait_vact_end() will never be called or it explodes in all
>>>>>>>>> colours.
>>>>>>>>>
>>>>>>>>>>> I added the dependency because analogix_dp-rockchip.c calls
>>>>>>>>>>> rockchip_drm_wait_vact_end() which is implemented in the VOP driver,
>>>>>>>>>>> so this driver currenty can't work with the VOP2 driver and can't
>>>>>>>>>>> be linked without the VOP driver being present.
>>>>>>>>>>> I'll add a few words to the commit message.
>>>>>>>>>> Maybe a better direction is move rockchip_drm_wait_vact_end from the VOP
>>>>>>>>>> driver to rockchip_drm_drv.c
>>>>>>>>> I am not sure if that's really worth it. Yes, the direction might be the
>>>>>>>>> right one, but I would really prefer when somebody does the change who
>>>>>>>>> can test and confirm that the analogix dp really works with VOP2 in the
>>>>>>>>> end.
>>>>>>>> If follow this point, the current DW_MIPI also has not been tested for
>>>>>>>> confirm that it
>>>>>>>>
>>>>>>>> can really work with VOP2, so you should also make it depends on
>>>>>>>> ROCKCHIP_VOP.
>>> Here you are suggesting to add even more Kconfig dependencies.
>>>
>>>>>>> Well at least I have patches here which make DW_MIPI work with VOP2 ;)
>>>>>> But you DW_MIPI patches for rk356x didn't come. So this is not keep
>>>>>> consistency with this point.
>>>>>>
>>>>>>> What about the others, like LVDS and RGB?
>>>>>> Yes, we also have other interface , RK356X has LVDS/RGB/BT1120/BT656, RK3588
>>>>>> has BT1120/BT656, no LVDS or RGB.
>>>>>>
>>>>>>>> I think the current solution is just a workaround to make your patch pass
>>>>>>>> the kernel compile
>>>>>>> Indeed.
>>>>>>>
>>>>>>> I agree that it would be good to add a note somewhere which outputs
>>>>>>> work with the VOP2 driver (currently only HDMI), but I wonder if Kconfig
>>>>>>> dependencies is the right place for it, because only people who deliberately
>>>>>>> disable VOP support will see this information.
>>>>>>> Maybe we should rather add it to the Kconfig help text?
>>>>>> If a device is supported for this soc, we will add dt node at the dtsi file.
>>>>>>
>>>>>> A Kconfig dependencies don't seems a good idea.
>>> Here you say Kconfig dependencies are no good idea.
>>
>> Yes. It's not a good idea. So I don't want to see you use a Kcofig
>> dependence
>>
>> to disable a module to avoid compile which introduced by your patch.
>>
>>>>> Ok, this means we can keep my current approach with just letting
>>>>> ROCKCHIP_ANALOGIX_DP depend on ROCKCHIP_VOP to avoid having a non
>>>> Excuse me? How do you get this conclusion ?
>>> Given that you say that you want to have both more and less Kconfig
>>> dependencies I came to the conclusion that I only add one where it's
>>> necessary to compile the driver.
>>>
>>>> I said before,  vop and vop2 based platforms both have ROCKCHIP_ANALOGIX_DP.
>>> Maybe, but vop2 with ROCKCHIP_ANALOGIX_DP doesn't even work in the
>>> Rockchip downstream kernel, so I wonder how relevant this usecase really
>>> is.
>>
>> No, this is not the truth. Rockchip_ANALOGIX_DP of course work with the
>> vendor kernel. We have many rk356x based products shipped with edp.
>> Even the VGA output interface on RK3568_EVB1 is drived by
>> ROCKCHIP_ANALOGIX_DP with a RTD2166 eDP to VGA convert
>> chip.
>>
>>
>> So how do you get conclusion that ROCKCHIP_ANALOGIX_DP can't work with
>> the Rockchip downstream kernel? Is it because you can't make the DP work on
>> your board? If it is, please contact the supplier who gave you the board.
> In the downstream kernel I have available (which is a 5.10.66)
> analogix_dp-rockchip.c calls rockchip_drm_wait_vact_end() which is
> implemented in rockchip_drm_vop.c and assumes that the passed struct
> drm_crtc * can be converted to a struct vop *. Basically it's the same
> situation we have right now with the mainline kernel, just that the
> linker issues won't show up because the VOP driver can't be disabled
> in the downstream kernel.


So you judge ROCKCHIP_ANALOGIX_DP can't work by this(as vop2 doesn't has 
rockchip_drm_wait_vact_end interface)?

No, you may not know clearly about this function, this function is used 
for eDP PSR, which is a

optional function, that means not every eDP panel has PSR function(this 
function usually parsed from edid).

This really not means than ROCKCHIP_ANALOGIX_DP can't work.


And your 5.10 downstream kernel seems out of dated. We have already move 
this function out from rockchip_drm_vop to rockchip_drm_drv.


>>
>> Do you have a RK3568_EVB1 that has a VGA output interface on board?
>>
>> If you have it, I can offer you image to verify the DP.
>>
>>
>>>> If this patch will cause the compile error, please do a real fix, not a
>>> I can't, because I don't have any hardware to test the Analogix DP on a
>>> VOP hardware, and given that Analogix DP in conjunction with VOP2 hardware is
>>> not even supported in the downstream Kernel I am not sure if it's really
>>> worth doing that.
>>
>> Again, this is not the truth, see above.
>> I am not ask you support the ROCKCHIP_ANALOGIX_DP on upstream, I just
>> want you can give a better solution when you patch cause the compile error.
>> Disable a module when it conflict with your patch is too rough.
> It does not conflict with my patch. The dependency only means that you can't
> enable the Analogix DP driver when the VOP driver is disabled. That
> doesn't hurt, because currently you can't do anything with the Analogix
> DP driver without the VOP driver. With the added dependency the Analogix DP
> driver can still be used with the VOP driver, even when the VOP2 driver
> is enabled. I really can't see a problem with that.
>
> Sascha
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel