| Message ID | 20220217082954.2967889-1-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/rockchip: RK356x VOP2 support | expand |
17.02.2022 11:29, Sascha Hauer пишет: > @@ -28,6 +28,12 @@ config ROCKCHIP_VOP > This selects support for the VOP driver. You should enable it > on all older SoCs up to RK3399. > > +config ROCKCHIP_VOP2 > + bool "Rockchip VOP2 driver" > + help > + This selects support for the VOP2 driver. You should enable it > + on all newer SoCs beginning form RK3568. s/form/from/ The ROCKCHIP_VOP option is "default y". Do you really want "default n" for the VOP2?
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. > > 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. Sascha
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. > > > > > 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. agreed :-) . I.e. adding ACPI support for Rockchip drivers separately later on makes things way easier. Having a separate discussion about ACPI changes at that point also makes the whole process easier, as adding the whole thing here will delay everything even more. Also if a later series really only is about adding ACPI support, this makes for easier discussion but also easier review of changes. The new VOP2 driver is big enough as it is. Heiko
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. 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. > agreed :-) . > > I.e. adding ACPI support for Rockchip drivers separately later on > makes things way easier. > > Having a separate discussion about ACPI changes at that point > also makes the whole process easier, as adding the whole thing > here will delay everything even more. Heiko: I am not ask to add new code for future ACPI support, I just hope the original downstream method can keep to make future work easier. > Also if a later series really only is about adding ACPI support, this > makes for easier discussion but also easier review of changes. > The new VOP2 driver is big enough as it is. > > > Heiko > >
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. Sascha
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 > > Sascha >
On Thu, Feb 17, 2022 at 04:24:29PM +0300, Dmitry Osipenko wrote: > 17.02.2022 11:29, Sascha Hauer пишет: > > @@ -28,6 +28,12 @@ config ROCKCHIP_VOP > > This selects support for the VOP driver. You should enable it > > on all older SoCs up to RK3399. > > > > +config ROCKCHIP_VOP2 > > + bool "Rockchip VOP2 driver" > > + help > > + This selects support for the VOP2 driver. You should enable it > > + on all newer SoCs beginning form RK3568. > > s/form/from/ > > The ROCKCHIP_VOP option is "default y". Do you really want "default n" > for the VOP2? ROCKCHIP_VOP is only default y to keep the VOP driver enabled for existing defconfig that were generated before the introduction of that symbol. We don't have this problem for VOP2, so no need to make it default y. Sascha
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(). Sascha
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 >
On Thu, Feb 24, 2022 at 06:54:35PM +0800, Andy Yan wrote: > 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, I can make the defines RK3568 agnostic and use ROCKCHIP_ as prefix. The actual numbers don't matter much, so we can add new interfaces or instances thereof at the end with the next free number. Sascha
On 2/24/22 10:47, Sascha Hauer wrote: > On Thu, Feb 17, 2022 at 04:24:29PM +0300, Dmitry Osipenko wrote: >> 17.02.2022 11:29, Sascha Hauer пишет: >>> @@ -28,6 +28,12 @@ config ROCKCHIP_VOP >>> This selects support for the VOP driver. You should enable it >>> on all older SoCs up to RK3399. >>> >>> +config ROCKCHIP_VOP2 >>> + bool "Rockchip VOP2 driver" >>> + help >>> + This selects support for the VOP2 driver. You should enable it >>> + on all newer SoCs beginning form RK3568. >> >> s/form/from/ >> >> The ROCKCHIP_VOP option is "default y". Do you really want "default n" >> for the VOP2? > > ROCKCHIP_VOP is only default y to keep the VOP driver enabled for > existing defconfig that were generated before the introduction of > that symbol. > We don't have this problem for VOP2, so no need to make it default y. To me it will be more consistent of you'll have both defaulting to y, since both options are behind DRM_ROCKCHIP.
On Thu, Feb 24, 2022 at 05:36:29PM +0300, Dmitry Osipenko wrote: > On 2/24/22 10:47, Sascha Hauer wrote: > > On Thu, Feb 17, 2022 at 04:24:29PM +0300, Dmitry Osipenko wrote: > >> 17.02.2022 11:29, Sascha Hauer пишет: > >>> @@ -28,6 +28,12 @@ config ROCKCHIP_VOP > >>> This selects support for the VOP driver. You should enable it > >>> on all older SoCs up to RK3399. > >>> > >>> +config ROCKCHIP_VOP2 > >>> + bool "Rockchip VOP2 driver" > >>> + help > >>> + This selects support for the VOP2 driver. You should enable it > >>> + on all newer SoCs beginning form RK3568. > >> > >> s/form/from/ > >> > >> The ROCKCHIP_VOP option is "default y". Do you really want "default n" > >> for the VOP2? > > > > ROCKCHIP_VOP is only default y to keep the VOP driver enabled for > > existing defconfig that were generated before the introduction of > > that symbol. > > We don't have this problem for VOP2, so no need to make it default y. > > To me it will be more consistent of you'll have both defaulting to y, > since both options are behind DRM_ROCKCHIP. New drivers should not be enabled by default, at least that's what I have been told before. The VOP driver is enabled by default for the reasons explained. But yes, you are right, it's more consistent to have the same default on both drivers. Personally I don't care much, for now I just follow what Heiko suggests as he is the one who hopefully merges these patches ;) Sascha
This is v6 of adding RK356x VOP2 support. Biggest change this time is that I no longer modify struct drm_encoder, instead the rockchip drivers now embed struct drm_encoder in a rockchip specific struct. Sascha Changes since v5: - Add new patch to fix dw-hdmi of_graph binding - Drop "drm/encoder: Add of_graph port to struct drm_encoder" and solve issue internally in the driver - make checkpatch cleaner Changes since v4: - Reorder patches in a way that binding/dts/driver patches are closer together - Drop clk patches already applied by Heiko Changes since v3: - added changelog to each patch - Add 4k support to hdmi driver - rebase on v5.17-rc1 Changes since v2: - Add pin names to HDMI supply pin description - Add hclk support to HDMI driver - Dual license rockchip-vop2 binding, update binding - Add HDMI connector to board dts files - drop unnecessary gamma_lut registers from vop2 - Update dclk_vop[012] clock handling, no longer hacks needed - Complete regmap conversion Changes since v1: - drop all unnecessary waiting for frames within atomic modeset and plane update - Cluster subwin support removed - gamma support removed - unnecessary irq_lock removed - interrupt handling simplified - simplified zpos handling - drop is_alpha_support(), use fb->format->has_alpha instead - use devm_regulator_get() rather than devm_regulator_get_optional() for hdmi regulators - Use fixed number of planes per video port - Drop homegrown regmap code from vop2 driver (not complete yet) - Add separate include file for vop2 driver to not pollute the vop include Andy Yan (1): drm: rockchip: Add VOP2 driver Benjamin Gaignard (1): dt-bindings: display: rockchip: dw-hdmi: Add compatible for rk3568 HDMI Douglas Anderson (2): drm/rockchip: dw_hdmi: Use auto-generated tables drm/rockchip: dw_hdmi: Set cur_ctr to 0 always Michael Riesch (1): arm64: dts: rockchip: enable vop2 and hdmi tx on quartz64a Nickey Yang (1): drm/rockchip: dw_hdmi: add default 594Mhz clk for 4K@60hz Sascha Hauer (17): drm/rockchip: Embed drm_encoder into rockchip_decoder drm/rockchip: dw_hdmi: rename vpll clock to reference clock dt-bindings: display: rockchip: dw-hdmi: use "ref" as clock name arm64: dts: rockchip: rk3399: rename HDMI ref clock to 'ref' drm/rockchip: dw_hdmi: add rk3568 support drm/rockchip: dw_hdmi: add regulator support dt-bindings: display: rockchip: dw-hdmi: Add regulator support drm/rockchip: dw_hdmi: Add support for hclk dt-bindings: display: rockchip: dw-hdmi: Add additional clock drm/rockchip: dw_hdmi: drop mode_valid hook dt-bindings: display: rockchip: dw-hdmi: Make unwedge pinctrl optional arm64: dts: rockchip: rk356x: Add VOP2 nodes arm64: dts: rockchip: rk356x: Add HDMI nodes arm64: dts: rockchip: rk3568-evb: Enable VOP2 and hdmi drm/rockchip: Make VOP driver optional dt-bindings: display: rockchip: Add binding for VOP2 dt-bindings: display: rockchip: dw-hdmi: fix ports description .../display/rockchip/rockchip,dw-hdmi.yaml | 53 +- .../display/rockchip/rockchip-vop2.yaml | 140 + arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +- .../boot/dts/rockchip/rk3566-quartz64-a.dts | 47 + arch/arm64/boot/dts/rockchip/rk3566.dtsi | 4 + .../boot/dts/rockchip/rk3568-evb1-v10.dts | 47 + arch/arm64/boot/dts/rockchip/rk3568.dtsi | 4 + arch/arm64/boot/dts/rockchip/rk356x.dtsi | 83 + drivers/gpu/drm/rockchip/Kconfig | 14 + drivers/gpu/drm/rockchip/Makefile | 4 +- .../gpu/drm/rockchip/analogix_dp-rockchip.c | 32 +- drivers/gpu/drm/rockchip/cdn-dp-core.c | 18 +- drivers/gpu/drm/rockchip/cdn-dp-core.h | 2 +- .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 17 +- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 294 +- drivers/gpu/drm/rockchip/inno_hdmi.c | 32 +- drivers/gpu/drm/rockchip/rk3066_hdmi.c | 34 +- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 3 +- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 17 +- drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 2 + drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 15 + drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2708 +++++++++++++++++ drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 477 +++ drivers/gpu/drm/rockchip/rockchip_lvds.c | 26 +- drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 281 ++ include/dt-bindings/soc/rockchip,vop2.h | 14 + 26 files changed, 4175 insertions(+), 195 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h create mode 100644 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c create mode 100644 include/dt-bindings/soc/rockchip,vop2.h