| Message ID | 20220328151116.2034635-21-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/rockchip: RK356x VOP2 support | expand |
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. > + > 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 . > help > This selects support for Rockchip SoC specific extensions > for the Analogix Core DP driver. If you want to enable DP > diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile > index 1a56f696558ca..dfc5512fdb9f1 100644 > --- a/drivers/gpu/drm/rockchip/Makefile > +++ b/drivers/gpu/drm/rockchip/Makefile > @@ -4,8 +4,9 @@ > # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. > > rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \ > - rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o > + rockchip_drm_gem.o > > +rockchipdrm-$(CONFIG_ROCKCHIP_VOP) += rockchip_drm_vop.o rockchip_vop_reg.o > rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o > rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o > rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > index 214214190fef1..7cf8b9665e5cf 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > @@ -494,7 +494,7 @@ static int __init rockchip_drm_init(void) > return -ENODEV; > > num_rockchip_sub_drivers = 0; > - ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_DRM_ROCKCHIP); > + ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_ROCKCHIP_VOP); > ADD_ROCKCHIP_SUB_DRIVER(rockchip_lvds_driver, > CONFIG_ROCKCHIP_LVDS); > ADD_ROCKCHIP_SUB_DRIVER(rockchip_dp_driver,
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 . 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. Sascha
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 . > 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 int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout) { struct rockchip_drm_private *priv; int pipe, ret = 0; if (!crtc) return -ENODEV; if (mstimeout <= 0) return -EINVAL; priv = crtc->dev->dev_private; pipe = drm_crtc_index(crtc); if (priv->crtc_funcs[pipe] && priv->crtc_funcs[pipe]->wait_vact_end) ret = priv->crtc_funcs[pipe]->wait_vact_end(crtc, mstimeout); return ret; } EXPORT_SYMBOL(rockchip_drm_wait_vact_end); > Sascha > >
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. Sascha
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. I think the current solution is just a workaround to make your patch pass the kernel compile but not reflect the real situation. > Sascha >
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. Well at least I have patches here which make DW_MIPI work with VOP2 ;) What about the others, like LVDS and 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? Sascha
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. > 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. > > Sascha >
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. > > 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. Ok, this means we can keep my current approach with just letting ROCKCHIP_ANALOGIX_DP depend on ROCKCHIP_VOP to avoid having a non buildable kernel. Sascha
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. >>> 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. > 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 ? I said before, vop and vop2 based platforms both have ROCKCHIP_ANALOGIX_DP. If this patch will cause the compile error, please do a real fix, not a workaround that may deliver misleading information. > buildable kernel. > > Sascha >
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. > > 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. > > 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. Moving rockchip_drm_wait_vact_end() to rockchip_drm_drv.c doesn't work with mainline currently, we first would have to add a struct crtc_funcs to struct rockchip_drm_private. Yes, that could be done. > > workaround that may deliver misleading information. The Kconfig dependency quite clearly says that the Analogix DP currently doesn't work with the VOP2. Anyone who wants to change that can use that information as a starting point and implement whatever is necessary and likely has the hardware to verify the work. I don't want to solve problems that *might* arise in the future, and in this case it's not a direction decision that we might regret in the future. Sascha
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. 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. > > Moving rockchip_drm_wait_vact_end() to rockchip_drm_drv.c doesn't work > with mainline currently, we first would have to add a struct crtc_funcs > to struct rockchip_drm_private. Yes, that could be done. > >> workaround that may deliver misleading information. > The Kconfig dependency quite clearly says that the Analogix DP currently > doesn't work with the VOP2. Anyone who wants to change that can use that > information as a starting point and implement whatever is necessary and > likely has the hardware to verify the work. I don't want to solve > problems that *might* arise in the future, and in this case it's not a > direction decision that we might regret in the future. > > Sascha > > >
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. > > > 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
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 >
On Wed, Apr 06, 2022 at 03:47:18PM +0800, Andy Yan wrote: > 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. I guessed that. Well we can do the same once we are there, but currently we are not. My patch doesn't break any existing features and we can add Analogix DP support later. I know the VOP2 driver is not feature complete and it doesn't aim to be. Please let's stop arguing about this topic. Once somebody wants to add Analogix DP support with VOP2 we can discuss the right way at length and until then my patch won't do any harm. Sascha
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. + config ROCKCHIP_ANALOGIX_DP bool "Rockchip specific extensions for Analogix DP driver" + depends on ROCKCHIP_VOP help This selects support for Rockchip SoC specific extensions for the Analogix Core DP driver. If you want to enable DP diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile index 1a56f696558ca..dfc5512fdb9f1 100644 --- a/drivers/gpu/drm/rockchip/Makefile +++ b/drivers/gpu/drm/rockchip/Makefile @@ -4,8 +4,9 @@ # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \ - rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o + rockchip_drm_gem.o +rockchipdrm-$(CONFIG_ROCKCHIP_VOP) += rockchip_drm_vop.o rockchip_vop_reg.o rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index 214214190fef1..7cf8b9665e5cf 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -494,7 +494,7 @@ static int __init rockchip_drm_init(void) return -ENODEV; num_rockchip_sub_drivers = 0; - ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_DRM_ROCKCHIP); + ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_ROCKCHIP_VOP); ADD_ROCKCHIP_SUB_DRIVER(rockchip_lvds_driver, CONFIG_ROCKCHIP_LVDS); ADD_ROCKCHIP_SUB_DRIVER(rockchip_dp_driver,
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(-)