| Message ID | 20211117143347.314294-1-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/rockchip: RK356x VOP2 support | expand |
On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > This series adds initial graphics support for the Rockchip RK356[68] > SoCs. Graphics support is based around the VOP2 controller which > replaces the VOP controller found on earlier Rockchip SoCs. The driver > has been tested with HDMI support included in this series and MIPI-DSI > which is not included because it needs some more work. The driver is > taken from the downstream Rockchip kernel and heavily polished, most non > standard features have been removed for now. I tested the driver with > the libdrm modetest utility and also with weston with both pixman and > panfrost driver support. Michael Riesch reported the driver to work on > the RK3566 as well, but device tree support for this SoC is not yet > included in this series. Can you outline what exactly you want to disable? I don't think 'status' is the right way. I think between the parent device being disabled, an incomplete graph and user configuration choice that should be enough to disable parts. Rob
On Wed, Nov 17, 2021 at 08:54:37AM -0600, Rob Herring wrote: > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > This series adds initial graphics support for the Rockchip RK356[68] > > SoCs. Graphics support is based around the VOP2 controller which > > replaces the VOP controller found on earlier Rockchip SoCs. The driver > > has been tested with HDMI support included in this series and MIPI-DSI > > which is not included because it needs some more work. The driver is > > taken from the downstream Rockchip kernel and heavily polished, most non > > standard features have been removed for now. I tested the driver with > > the libdrm modetest utility and also with weston with both pixman and > > panfrost driver support. Michael Riesch reported the driver to work on > > the RK3566 as well, but device tree support for this SoC is not yet > > included in this series. > > Can you outline what exactly you want to disable? I don't think > 'status' is the right way. I think between the parent device being > disabled, an incomplete graph and user configuration choice that > should be enough to disable parts. The VOP2 on the RK3568 has three CRTCS, or video ports (VP) in Rockchip nomenclature. Each of them can be connected to the different outputs, like HDMI, MIPI-DSI and so on. In the device tree the CRTCs are described as of-graph ports with links to the HDMI, MIPI-DSI nodes. An example limited to HDMI looks like this: vop: vop@fe040000 { compatible = "rockchip,rk3568-vop"; vop_out: ports { vp0: port@0 { vp0_out_hdmi: endpoint@0 { reg = <0>; remote-endpoint = <&hdmi_in_vp0>; status = "disabled"; }; ... MIPI, dP, ... }; vp1: port@1 { vp1_out_hdmi: endpoint@0 { reg = <0>; remote-endpoint = <&hdmi_in_vp1>; status = "disabled"; }; ... MIPI, dP, ... }; vp2: port@2 { ... }; }; }; hdmi: hdmi@fe0a0000 { compatible = "rockchip,rk3568-dw-hdmi"; ports { hdmi_in: port@0 { hdmi_in_vp0: endpoint@0 { reg = <0>; remote-endpoint = <&vp0_out_hdmi>; status = "disabled"; }; hdmi_in_vp1: endpoint@1 { reg = <1>; remote-endpoint = <&vp1_out_hdmi>; status = "disabled"; }; ... }; }; }; Theoretically every VP can be routed to every output, but depending on the board there are some constraints. For example for the three vps there are only two PLLs for the pixel clock, and the HDMI port is hardwired to one single PLL. To avoid different VPs setting conflicting rates on a PLL we can only allow a subset of the possible routes. Sascha
Hi Sascha Hauer, On 2021/11/17 下午10:33, Sascha Hauer wrote: > This series adds initial graphics support for the Rockchip RK356[68] > SoCs. Graphics support is based around the VOP2 controller which > replaces the VOP controller found on earlier Rockchip SoCs. The driver > has been tested with HDMI support included in this series and MIPI-DSI > which is not included because it needs some more work. The driver is > taken from the downstream Rockchip kernel Yes, you do know this is from Rockchip kernel. Could you point me out where is the information about original author in your commit? > and heavily polished, most non > standard features have been removed for now. I don't agree with this, we do believe you have do some clean up to meet the requirement of upstream, but all the framework and feature implement are from Rockchip engineer, we have made a great effort to make everything work which block us to upstream this driver for now. NAK for this series. - Kever
Hi Kever, Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: > Hi Sascha Hauer, > > On 2021/11/17 下午10:33, Sascha Hauer wrote: > > This series adds initial graphics support for the Rockchip RK356[68] > > SoCs. Graphics support is based around the VOP2 controller which > > replaces the VOP controller found on earlier Rockchip SoCs. The driver > > has been tested with HDMI support included in this series and MIPI-DSI > > which is not included because it needs some more work. The driver is > > taken from the downstream Rockchip kernel > > Yes, you do know this is from Rockchip kernel. > > Could you point me out where is the information about original author > in your commit? The copyrights for added files seem to have stayed intact. For example the added rockchip_drm_vop2.c file in patch12 does contain the copyright as Copyright (c) 2020 Rockchip Electronics Co., Ltd. Author: Andy Yan <andy.yan@rock-chips.com> We can of course debate if the commit-author should also be set to Andy or another Rockchip engineer, with Sascha adding a "Co-developed-by" with his credentials. That's probably a nice compromise, I guess. > > and heavily polished, most non > > standard features have been removed for now. > > I don't agree with this, we do believe you have do some clean up to meet > the requirement > > of upstream, but all the framework and feature implement are from > Rockchip engineer, > > we have made a great effort to make everything work which block us to > upstream this driver for now. I don't fully understand what you mean here (language barrier probably), but dropping non-essential functionality in a first round is pretty common to at least get basic functionality working for everyone. With the special features getting added again in later patches over time. This happenened on the old vop as well. And of course, having a kernel that can "just" do normal graphics without the additional features is still preferable over having _NO_ graphics support at all ;-) > NAK for this series. As you might've seen from previous graphics related patches, there is a big number of people _and companies_ that seems to want/need to work with the rk3566/rk3568 with a kernel based on mainline. --> Most likely even in real products! While Rockchip did say that they want to upstream VOP2 support, there has been _NO_ movement or even information at all on this over at least the last year(!), so it's pretty understandable that developers will do this themself at some point, because they don't want to wait anymore for something that might never happen. So a simple "NAK" without additional information is not really helpful here. If you don't like Sascha's series, I really want to know _WHEN_ Rockchip plans on upstreaming at least basic graphis support themself. The kernel is often called a do-ocracy - the one who does the work, gets to decide. So if you really don't like Sascha's series at all, I do expect Rockchip to step up and provide a solution themself - and in a usable timeframe. Heiko
Hi, On Thu, 18 Nov 2021 at 09:26, Heiko Stübner <heiko@sntech.de> wrote: > Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: > > I don't agree with this, we do believe you have do some clean up to meet > > the requirement > > > > of upstream, but all the framework and feature implement are from > > Rockchip engineer, > > > > we have made a great effort to make everything work which block us to > > upstream this driver for now. > > I don't fully understand what you mean here (language barrier probably), > but dropping non-essential functionality in a first round is pretty common > to at least get basic functionality working for everyone. With the special > features getting added again in later patches over time. This happenened > on the old vop as well. > > And of course, having a kernel that can "just" do normal graphics without > the additional features is still preferable over having _NO_ graphics support > at all ;-) > > > NAK for this series. > > As you might've seen from previous graphics related patches, there > is a big number of people _and companies_ that seems to want/need > to work with the rk3566/rk3568 with a kernel based on mainline. > > --> Most likely even in real products! Yes, we've been trying to ship a real product based on RK356x. We started by using the vendor VOP2 driver, but it is broken beyond belief. The driver needs a fundamental ground-up rework, and all the additional features get in the way of doing this core rework to make it actually function correctly. So, NAK to the NAK. I would like to see the VOP2 support start simple, with more features being added one by one. > While Rockchip did say that they want to upstream VOP2 support, there > has been _NO_ movement or even information at all on this over at least > the last year(!), so it's pretty understandable that developers will do this > themself at some point, because they don't want to wait anymore for > something that might never happen. > > So a simple "NAK" without additional information is not really helpful here. > > If you don't like Sascha's series, I really want to know _WHEN_ Rockchip > plans on upstreaming at least basic graphis support themself. > > The kernel is often called a do-ocracy - the one who does the work, gets > to decide. So if you really don't like Sascha's series at all, I do expect > Rockchip to step up and provide a solution themself - and in a usable > timeframe. Exactly what Heiko said. If you would like to upstream the driver then that would be fantastic to see, but I'm afraid you do not get to prevent someone else from doing the work themselves. Cheers, Daniel
On Thu, Nov 18, 2021 at 10:26:29AM +0100, Heiko Stübner wrote: > Hi Kever, > > Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: > > Hi Sascha Hauer, > > > > On 2021/11/17 下午10:33, Sascha Hauer wrote: > > > This series adds initial graphics support for the Rockchip RK356[68] > > > SoCs. Graphics support is based around the VOP2 controller which > > > replaces the VOP controller found on earlier Rockchip SoCs. The driver > > > has been tested with HDMI support included in this series and MIPI-DSI > > > which is not included because it needs some more work. The driver is > > > taken from the downstream Rockchip kernel > > > > Yes, you do know this is from Rockchip kernel. > > > > Could you point me out where is the information about original author > > in your commit? > > The copyrights for added files seem to have stayed intact. > For example the added rockchip_drm_vop2.c file in patch12 > does contain the copyright as > > Copyright (c) 2020 Rockchip Electronics Co., Ltd. > Author: Andy Yan <andy.yan@rock-chips.com> > > > We can of course debate if the commit-author should also be set to > Andy or another Rockchip engineer, with Sascha adding a > "Co-developed-by" with his credentials. > > That's probably a nice compromise, I guess. The commit author was merely lost while copying the driver file into my git. Changing it back to Andy is fine with me, I didn't mean to steal anyones credits. Sascha
On 2021/11/18 下午5:53, Daniel Stone wrote: > Hi, > > On Thu, 18 Nov 2021 at 09:26, Heiko Stübner <heiko@sntech.de> wrote: >> Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: >>> I don't agree with this, we do believe you have do some clean up to meet >>> the requirement >>> >>> of upstream, but all the framework and feature implement are from >>> Rockchip engineer, >>> >>> we have made a great effort to make everything work which block us to >>> upstream this driver for now. >> I don't fully understand what you mean here (language barrier probably), >> but dropping non-essential functionality in a first round is pretty common >> to at least get basic functionality working for everyone. With the special >> features getting added again in later patches over time. This happenened >> on the old vop as well. >> >> And of course, having a kernel that can "just" do normal graphics without >> the additional features is still preferable over having _NO_ graphics support >> at all ;-) >> >>> NAK for this series. >> As you might've seen from previous graphics related patches, there >> is a big number of people _and companies_ that seems to want/need >> to work with the rk3566/rk3568 with a kernel based on mainline. >> >> --> Most likely even in real products! > Yes, we've been trying to ship a real product based on RK356x. We > started by using the vendor VOP2 driver, but it is broken beyond > belief. The driver needs a fundamental ground-up rework, and all the > additional features get in the way of doing this core rework to make > it actually function correctly. > > So, NAK to the NAK. I would like to see the VOP2 support start simple, > with more features being added one by one. > >> While Rockchip did say that they want to upstream VOP2 support, there >> has been _NO_ movement or even information at all on this over at least >> the last year(!), so it's pretty understandable that developers will do this >> themself at some point, because they don't want to wait anymore for >> something that might never happen. >> >> So a simple "NAK" without additional information is not really helpful here. >> >> If you don't like Sascha's series, I really want to know _WHEN_ Rockchip >> plans on upstreaming at least basic graphis support themself. >> >> The kernel is often called a do-ocracy - the one who does the work, gets >> to decide. So if you really don't like Sascha's series at all, I do expect >> Rockchip to step up and provide a solution themself - and in a usable >> timeframe. > Exactly what Heiko said. If you would like to upstream the driver then > that would be fantastic to see, but I'm afraid you do not get to > prevent someone else from doing the work themselves. First of all, we never stop any one to doing there work on upstream if the source code is write totally by themselves. Second, there are also many modules are upstream by developers based on Rockchip source code, please note that all of them have basic respect to our work, they do communicate with us first. But this committer do not take any respect to our engineers and their hard working: - He didn't contact with us; - There isn't any information about original author in the commit message; As I have known, if I use source code from another developer, I need to at least add a "Signed-off-by" with original author; - This commit and mail does not even have a 'CC' to original author. I NAK because I think this is not part of the open source spirit, and this kind of behavior should not be encouraged. Thanks, - Kever
Hello Kever, On 11/18/21 11:50 AM, Kever Yang wrote: > > On 2021/11/18 下午5:53, Daniel Stone wrote: >> Hi, >> >> On Thu, 18 Nov 2021 at 09:26, Heiko Stübner <heiko@sntech.de> wrote: >>> Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: >>>> I don't agree with this, we do believe you have do some clean up to >>>> meet >>>> the requirement >>>> >>>> of upstream, but all the framework and feature implement are from >>>> Rockchip engineer, >>>> >>>> we have made a great effort to make everything work which block us to >>>> upstream this driver for now. >>> I don't fully understand what you mean here (language barrier probably), >>> but dropping non-essential functionality in a first round is pretty >>> common >>> to at least get basic functionality working for everyone. With the >>> special >>> features getting added again in later patches over time. This happenened >>> on the old vop as well. >>> >>> And of course, having a kernel that can "just" do normal graphics >>> without >>> the additional features is still preferable over having _NO_ graphics >>> support >>> at all ;-) >>> >>>> NAK for this series. >>> As you might've seen from previous graphics related patches, there >>> is a big number of people _and companies_ that seems to want/need >>> to work with the rk3566/rk3568 with a kernel based on mainline. >>> >>> --> Most likely even in real products! >> Yes, we've been trying to ship a real product based on RK356x. We >> started by using the vendor VOP2 driver, but it is broken beyond >> belief. The driver needs a fundamental ground-up rework, and all the >> additional features get in the way of doing this core rework to make >> it actually function correctly. >> >> So, NAK to the NAK. I would like to see the VOP2 support start simple, >> with more features being added one by one. >> >>> While Rockchip did say that they want to upstream VOP2 support, there >>> has been _NO_ movement or even information at all on this over at least >>> the last year(!), so it's pretty understandable that developers will >>> do this >>> themself at some point, because they don't want to wait anymore for >>> something that might never happen. >>> >>> So a simple "NAK" without additional information is not really >>> helpful here. >>> >>> If you don't like Sascha's series, I really want to know _WHEN_ Rockchip >>> plans on upstreaming at least basic graphis support themself. >>> >>> The kernel is often called a do-ocracy - the one who does the work, gets >>> to decide. So if you really don't like Sascha's series at all, I do >>> expect >>> Rockchip to step up and provide a solution themself - and in a usable >>> timeframe. >> Exactly what Heiko said. If you would like to upstream the driver then >> that would be fantastic to see, but I'm afraid you do not get to >> prevent someone else from doing the work themselves. > > First of all, we never stop any one to doing there work on upstream if > the source code is write totally by themselves. > > Second, there are also many modules are upstream by developers based on > Rockchip source code, please note that > all of them have basic respect to our work, they do communicate with us > first. > > > But this committer do not take any respect to our engineers and their > hard working: > - He didn't contact with us; I approached Andy Yan and you off-list on October 20, 2021 in this regard, as Andy mentioned on linux-rockchip in July 2021 some plans to bring the driver mainline. Since there was no response, we asked Sascha to make this happen. > - There isn't any information about original author in the commit message; > As I have known, if I use source code from another developer, I > need to at least add a "Signed-off-by" with original author; As has been discussed before, this will be fixed in v2. Simple mistake, no harm intended. > - This commit and mail does not even have a 'CC' to original author. > > I NAK because I think this is not part of the open source spirit, and > this kind of behavior should not be encouraged. It is great to hear that you care about the open source spirit. IMHO communication is a big part thereof. If Rockchip would communicate better their plans to bring things mainline including a time schedule, it would be a lot easier for all of us. Best regards, Michael
Hi Kever, On Thu, 18 Nov 2021 at 10:50, Kever Yang <kever.yang@rock-chips.com> wrote: > On 2021/11/18 下午5:53, Daniel Stone wrote: > > Exactly what Heiko said. If you would like to upstream the driver then > > that would be fantastic to see, but I'm afraid you do not get to > > prevent someone else from doing the work themselves. > > First of all, we never stop any one to doing there work on upstream if > the source code is write totally by themselves. > > Second, there are also many modules are upstream by developers based on > Rockchip source code, please note that > all of them have basic respect to our work, they do communicate with us > first. > > But this committer do not take any respect to our engineers and their > hard working: > - He didn't contact with us; > - There isn't any information about original author in the commit message; > As I have known, if I use source code from another developer, I > need to at least add a "Signed-off-by" with original author; > - This commit and mail does not even have a 'CC' to original author. > > I NAK because I think this is not part of the open source spirit, and > this kind of behavior should not be encouraged. OK, I see where you're coming from, and I agree that the attribution should have been handled more carefully. On the other hand, please consider this from the other perspective. Sascha has been free to take the downstream Rockchip BSP code and attempt to upstream it to the Linux kernel, which you are unhappy about. But then the Rockchip driver was developed totally downstream, with no attempt to ever communicate with the upstream Linux or DRM/KMS developers. Rockchip advertises that it is shipped as a Linux kernel with a KMS driver. But we were never informed, or CCed, or anything. If you would like the community to more actively work with you - then please yourself work more actively with the community. The first commit of the VOP2 driver was in July 2020, and that was of the full driver so presumably it started quite some time before then. So that is a minimum of 17 months that you have had to engage with upstream ... Technically, the driver cannot be upstreamed as-is. It looks as if it were a pre-atomic driver, that was half-converted to atomic, and then has been half-converted to atomic helpers as well. Things like reference counting and global state are not handled correctly at all. You can see this if you try to run Weston on top of the VOP2 driver: the framerate is decimated because the event handling massively over-synchronises, and the event timestamps which arrive are incorrect. This would be fixed by correctly using the event helpers that we have had in the tree for years (which would also eliminate the unnecessary framebuffer reference handling). It also does not work with the GPU drivers in the tree because it lacks the one-liner to correctly handle dma_resv synchronisation, which makes it both too fast as it displays content which is not ready, and too slow because it can't do it at full frame rate. Similarly, on the RK3566 EVB, the DSI does not work unless HDMI is also active, but when HDMI is active at the same time as DSI, it just shows a blank screen. I believe the root cause of this is that the VOP2 driver does not use any of the atomic state correctly, and instead stores its own state in driver-global structures, using a lot of unnecessary mutexes to try to synchronise this state. Not only does this synchronisation not actually work, but it causes a severe performance degradation due to mutex contention. I believe the best path forward to an upstream VOP2 driver is a patch series consisting of: - start from a blank slate, using the atomic framework and helpers as they were intended to be, with basic support for the VOP2 and one or two connector types, doing linear XRGB only - any cleanups which would enable this to share more code with - add YUV support, including planar buffers - add AFBC support, with the AFBC enable/disable correctly synchronised through atomic state (this is necessary since the AFBC decoder is not directly on the planes per se but shared) - add more connector types - add writeback support - add other Rockchip-specific codepaths such as HDR10 Cheers, Daniel
Hi Daniel: On 11/18/21 8:07 PM, Daniel Stone wrote: > Hi Kever, > > On Thu, 18 Nov 2021 at 10:50, Kever Yang <kever.yang@rock-chips.com> wrote: >> On 2021/11/18 下午5:53, Daniel Stone wrote: >>> Exactly what Heiko said. If you would like to upstream the driver then >>> that would be fantastic to see, but I'm afraid you do not get to >>> prevent someone else from doing the work themselves. >> First of all, we never stop any one to doing there work on upstream if >> the source code is write totally by themselves. >> >> Second, there are also many modules are upstream by developers based on >> Rockchip source code, please note that >> all of them have basic respect to our work, they do communicate with us >> first. >> >> But this committer do not take any respect to our engineers and their >> hard working: >> - He didn't contact with us; >> - There isn't any information about original author in the commit message; >> As I have known, if I use source code from another developer, I >> need to at least add a "Signed-off-by" with original author; >> - This commit and mail does not even have a 'CC' to original author. >> >> I NAK because I think this is not part of the open source spirit, and >> this kind of behavior should not be encouraged. > OK, I see where you're coming from, and I agree that the attribution > should have been handled more carefully. > > On the other hand, please consider this from the other perspective. > Sascha has been free to take the downstream Rockchip BSP code and > attempt to upstream it to the Linux kernel, which you are unhappy > about. But then the Rockchip driver was developed totally downstream, > with no attempt to ever communicate with the upstream Linux or DRM/KMS > developers. Rockchip advertises that it is shipped as a Linux kernel > with a KMS driver. But we were never informed, or CCed, or anything. > > If you would like the community to more actively work with you - then > please yourself work more actively with the community. The first > commit of the VOP2 driver was in July 2020, and that was of the full > driver so presumably it started quite some time before then. So that > is a minimum of 17 months that you have had to engage with upstream > ... > > Technically, the driver cannot be upstreamed as-is. It looks as if it > were a pre-atomic driver, that was half-converted to atomic, and then > has been half-converted to atomic helpers as well. Things like > reference counting and global state are not handled correctly at all. > You can see this if you try to run Weston on top of the VOP2 driver: > the framerate is decimated because the event handling massively > over-synchronises, and the event timestamps which arrive are > incorrect. This would be fixed by correctly using the event helpers > that we have had in the tree for years (which would also eliminate the > unnecessary framebuffer reference handling). It also does not work > with the GPU drivers in the tree because it lacks the one-liner to > correctly handle dma_resv synchronisation, which makes it both too > fast as it displays content which is not ready, and too slow because > it can't do it at full frame rate. We have different team run Android , X11, Weston on rk356x, especially for android, we can run at 60 fps. Our vop2 driver is developed on Linux 4.19, am not sure which version of kernel you put our drivers on. > > Similarly, on the RK3566 EVB, the DSI does not work unless HDMI is > also active, but when HDMI is active at the same time as DSI, it just I am very sure rk3566 evb DSI can work without HDMI. But take care that the vop on rk3566 has a special limitation: there are three windows(Cluster1/Esmart1/Smart1) that have a mirror lock, that means they can't be programed framebuffer address independently , they can only share framebuffer address with Cluster0/Esmart0/Smart0. We use these feature on Android. I have comment these limitation in our driver. Compared to old vop, vop is strong but a bit complicated, we try very had to make it work on as much display framework as possible. We have upstream plane, but I am really in a rush this year. So sorry for the late of upstream, but we glad to work with community. So Sascha, please feel free to go on with your work. > shows a blank screen. I believe the root cause of this is that the > VOP2 driver does not use any of the atomic state correctly, and > instead stores its own state in driver-global structures, using a lot > of unnecessary mutexes to try to synchronise this state. Not only does > this synchronisation not actually work, but it causes a severe > performance degradation due to mutex contention. > > I believe the best path forward to an upstream VOP2 driver is a patch > series consisting of: > - start from a blank slate, using the atomic framework and helpers > as they were intended to be, with basic support for the VOP2 and one > or two connector types, doing linear XRGB only > - any cleanups which would enable this to share more code with > - add YUV support, including planar buffers > - add AFBC support, with the AFBC enable/disable correctly > synchronised through atomic state (this is necessary since the AFBC > decoder is not directly on the planes per se but shared) > - add more connector types > - add writeback support > - add other Rockchip-specific codepaths such as HDR10 > > Cheers, > Daniel > > >
Hi Andy, On Thu, 18 Nov 2021 at 13:14, Andy Yan <andy.yan@rock-chips.com> wrote: > On 11/18/21 8:07 PM, Daniel Stone wrote: > > Technically, the driver cannot be upstreamed as-is. It looks as if it > > were a pre-atomic driver, that was half-converted to atomic, and then > > has been half-converted to atomic helpers as well. Things like > > reference counting and global state are not handled correctly at all. > > You can see this if you try to run Weston on top of the VOP2 driver: > > the framerate is decimated because the event handling massively > > over-synchronises, and the event timestamps which arrive are > > incorrect. This would be fixed by correctly using the event helpers > > that we have had in the tree for years (which would also eliminate the > > unnecessary framebuffer reference handling). It also does not work > > with the GPU drivers in the tree because it lacks the one-liner to > > correctly handle dma_resv synchronisation, which makes it both too > > fast as it displays content which is not ready, and too slow because > > it can't do it at full frame rate. > > We have different team run Android , X11, Weston on rk356x, especially > for android, we can run at 60 fps. > > Our vop2 driver is developed on Linux 4.19, am not sure which version of > kernel you put our drivers on. We forward-ported it to a current mainline kernel and started to work on fixing some of the issues. When we did this, we went back to the BSP tree posted on GitHub to test using a pure-BSP environment, and observed the same breakage there. > > Similarly, on the RK3566 EVB, the DSI does not work unless HDMI is > > also active, but when HDMI is active at the same time as DSI, it just > > I am very sure rk3566 evb DSI can work without HDMI. I'd love to know how. :) Using the meta-rockchip layer as published on GitHub, we cannot get working DSI without HDMI, using upstream Weston. When the HDMI connector is disabled, DSI comes up blank. When the HDMI connector is enabled, DSI works fine but HDMI is blank. > But take care that the vop on rk3566 has a special limitation: there are > three windows(Cluster1/Esmart1/Smart1) that have a mirror lock, that means they > can't be programed framebuffer address independently , they can only > share framebuffer address with Cluster0/Esmart0/Smart0. We use these feature > on Android. > > I have comment these limitation in our driver. Yeah, we noticed this. > Compared to old vop, vop is strong but a bit complicated, we try very had to > make it work on as much display framework as possible. > > We have upstream plane, but I am really in a rush this year. So sorry > for the late of upstream, but we glad to work with community. > > So Sascha, please feel free to go on with your work. Great, thanks. If you agree with the plan I posted, then we can all go forward with that upstream, doing the development work with ourselves and Sascha. Hopefully Rockchip will be able to support this effort. Cheers, Daniel
Hi Sascha, Am 17.11.21 um 15:33 schrieb Sascha Hauer: > This series adds initial graphics support for the Rockchip RK356[68] > SoCs. Graphics support is based around the VOP2 controller which > replaces the VOP controller found on earlier Rockchip SoCs. The driver > has been tested with HDMI support included in this series and MIPI-DSI > which is not included because it needs some more work. The driver is > taken from the downstream Rockchip kernel and heavily polished, most non > standard features have been removed for now. I tested the driver with > the libdrm modetest utility and also with weston with both pixman and > panfrost driver support. Michael Riesch reported the driver to work on > the RK3566 as well, but device tree support for this SoC is not yet > included in this series. > > The HDMI changes are based on patches from Benjamin Gaignard, but > modified a bit as I found out that the HDMI port on the RK3568 only > needs one additional clock, not two. Also I added regulator support > which is needed to get the HDMI up on the rk3568-EVB board. > > All review and testing feedback welcome thanks for working on that - it's very (very,very) much appreciated. It took me some time to figure it out: It seems rk3568-iommu driver s broken - I did only get "white noise" when using it alongside vop (similar like it was reported here before). However: removing the iommu-property from vop makes it working for me with HDMI output on quartz64 as well. Could you check if you have the iommu driver in kernel enabled if it works for you, if the property is present in DT? (I used 5.16-rc1 + this series + [0]). Also vop mmu seems to have the power-domain missing in your series (same as downstream) - however adding that doesn't help much currently. As a sidenote: I verfied this with using Ezequiel's vpu addtion for RK356x: It did only work when removing the iommu there as well (getting tons of page faults otherwise) - so iommu driver really seems to broken, at least for RK3566. (Or I'm a missing a option in kernel config, which wasn't required for the older iommu version?) But as reported before: For HDMI this does currently only work for pixel clock rates, which are integer-divisable with hpll clock rate (which is the hardcoded parent of vop0's dclk) As discussed in Benjamin's initial submission of the addition of RK3568's hdmi controller [1] same as with RK3288's and RK3399's hdmi phy needs a reference clock (it's called vpll there) which needs to get switched before the vop switches the mode (since phy rate switching is done before) - it's HPLL in case of RK356x. For whatever reason it's called "ref" for RK356x only downstream [2] - so you should add another clock "vpll" (renaming it to "ref" for _ALL_ SoCs which have it would be a _GREAT_ idea) which is <&pmucru PLL_HPLL>. What brings us to the "real" clock problem and the reason, why non-integer divisable pixel clock rates are not possible ATM: This is a long standing issue for RK3288 and RK3399 as well (and one of the main reasons why 4k modes are not possible for those older SoCs currently): Upstream all PLL rates are controlled with those PLL rate tables in the clock driver and they have to be _exactly_ defined as they are used (HDMI sinks are very picky). You will not see any additional rates downstream for RK3568: they have a mechanism there to automatically calculate the PLL settings if the rate doesn't exist in these tables (IIRC this was submitted upstream also: but it was rejected/ignored by maintainers). As a quick hackarround (for testing): You could use this table [3] we are using in LibreElec for RK3399 to get 4k modes working and assign it to HPLL in RK3568's clock driver (I tested it and it works great). It might be possible to just add those rates (some also without frac dividers) to the common PLL table for RK3568. I'm sorry I didn't reply inline as I'm supposed to do: It's late and I wanted to offload my findings now :) (You probably should also remove the printks in V2) Best, Alex [0] https://patchwork.kernel.org/project/linux-rockchip/patch/20211117154429.2274443-1-michael.riesch@wolfvision.net/ [1] https://patchwork.kernel.org/comment/24295683/ [2] https://github.com/rockchip-linux/kernel/blob/develop-4.19/arch/arm64/boot/dts/rockchip/rk3568.dtsi#L1715-L1720 [3] https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-1000-drm-rockchip.patch#L3155-L3182
Hi Alex, On Mon, Nov 22, 2021 at 12:18:47AM +0100, Alex Bee wrote: > Hi Sascha, > > Am 17.11.21 um 15:33 schrieb Sascha Hauer: > > This series adds initial graphics support for the Rockchip RK356[68] > > SoCs. Graphics support is based around the VOP2 controller which > > replaces the VOP controller found on earlier Rockchip SoCs. The driver > > has been tested with HDMI support included in this series and MIPI-DSI > > which is not included because it needs some more work. The driver is > > taken from the downstream Rockchip kernel and heavily polished, most non > > standard features have been removed for now. I tested the driver with > > the libdrm modetest utility and also with weston with both pixman and > > panfrost driver support. Michael Riesch reported the driver to work on > > the RK3566 as well, but device tree support for this SoC is not yet > > included in this series. > > > > The HDMI changes are based on patches from Benjamin Gaignard, but > > modified a bit as I found out that the HDMI port on the RK3568 only > > needs one additional clock, not two. Also I added regulator support > > which is needed to get the HDMI up on the rk3568-EVB board. > > > > All review and testing feedback welcome > > > thanks for working on that - it's very (very,very) much appreciated. > > It took me some time to figure it out: It seems rk3568-iommu driver s > broken - I did only get "white noise" when using it alongside vop > (similar like it was reported here before). However: removing the > iommu-property from vop makes it working for me with HDMI output on > quartz64 as well. Could you check if you have the iommu driver in kernel > enabled if it works for you, if the property is present in DT? (I used > 5.16-rc1 + this series + [0]). I have the iommu driver enabled and it works for me. I get this during boot: [0.263287] rockchip-vop2 fe040000.vop: Adding to iommu group 0 So I expect it is indeed used. > Also vop mmu seems to have the > power-domain missing in your series (same as downstream) - however > adding that doesn't help much currently. Probably the power domain gets enabled anyway when the VOP is activated, so adding it to the iommu won't help anything. Nevertheless it seems correct to add the property, I'll do so in the next round. > As a sidenote: I verfied this with using Ezequiel's vpu addtion for > RK356x: It did only work when removing the iommu there as well (getting > tons of page faults otherwise) - so iommu driver really seems to broken, > at least for RK3566. (Or I'm a missing a option in kernel config, which > wasn't required for the older iommu version?) I don't think so. I started from defconfig and disabled other architectures and unneeded drivers, but I did not enable anything specific to iommu. > > But as reported before: For HDMI this does currently only work for pixel > clock rates, which are integer-divisable with hpll clock rate (which is > the hardcoded parent of vop0's dclk) > As discussed in Benjamin's initial submission of the addition of > RK3568's hdmi controller [1] same as with RK3288's and RK3399's hdmi phy > needs a reference clock (it's called vpll there) which needs to get > switched before the vop switches the mode (since phy rate switching is > done before) - it's HPLL in case of RK356x. For whatever reason it's > called "ref" for RK356x only downstream [2] - so you should add another > clock "vpll" (renaming it to "ref" for _ALL_ SoCs which have it would be > a _GREAT_ idea) which is <&pmucru PLL_HPLL>. Yeah, a consumer clock should be named after the usage in the consumer, not after the provider name. I also stumbled over this and naming it "ref" makes much more sense. We'll likely have to keep supporting "vpll" as well for compatibility to old device trees. > What brings us to the "real" clock problem and the reason, why > non-integer divisable pixel clock rates are not possible ATM: This is a > long standing issue for RK3288 and RK3399 as well (and one of the main > reasons why 4k modes are not possible for those older SoCs currently): > Upstream all PLL rates are controlled with those PLL rate tables in the > clock driver and they have to be _exactly_ defined as they are used > (HDMI sinks are very picky). > You will not see any additional rates downstream for RK3568: they have a > mechanism there to automatically calculate the PLL settings if the rate > doesn't exist in these tables (IIRC this was submitted upstream also: > but it was rejected/ignored by maintainers). Looks like we have to try harder to get it upstream. Do you have a pointer to this patch? > As a quick hackarround (for > testing): You could use this table [3] we are using in LibreElec for > RK3399 to get 4k modes working and assign it to HPLL in RK3568's clock > driver (I tested it and it works great). It might be possible to just > add those rates (some also without frac dividers) to the common PLL > table for RK3568. Thanks for noting. This could also explain why currently only 1080p is working. Sascha
Am 22.11.21 um 09:10 schrieb Sascha Hauer: > Hi Alex, > > On Mon, Nov 22, 2021 at 12:18:47AM +0100, Alex Bee wrote: >> Hi Sascha, >> >> Am 17.11.21 um 15:33 schrieb Sascha Hauer: >>> This series adds initial graphics support for the Rockchip RK356[68] >>> SoCs. Graphics support is based around the VOP2 controller which >>> replaces the VOP controller found on earlier Rockchip SoCs. The driver >>> has been tested with HDMI support included in this series and MIPI-DSI >>> which is not included because it needs some more work. The driver is >>> taken from the downstream Rockchip kernel and heavily polished, most non >>> standard features have been removed for now. I tested the driver with >>> the libdrm modetest utility and also with weston with both pixman and >>> panfrost driver support. Michael Riesch reported the driver to work on >>> the RK3566 as well, but device tree support for this SoC is not yet >>> included in this series. >>> >>> The HDMI changes are based on patches from Benjamin Gaignard, but >>> modified a bit as I found out that the HDMI port on the RK3568 only >>> needs one additional clock, not two. Also I added regulator support >>> which is needed to get the HDMI up on the rk3568-EVB board. >>> >>> All review and testing feedback welcome >> >> thanks for working on that - it's very (very,very) much appreciated. >> >> It took me some time to figure it out: It seems rk3568-iommu driver s >> broken - I did only get "white noise" when using it alongside vop >> (similar like it was reported here before). However: removing the >> iommu-property from vop makes it working for me with HDMI output on >> quartz64 as well. Could you check if you have the iommu driver in kernel >> enabled if it works for you, if the property is present in DT? (I used >> 5.16-rc1 + this series + [0]). > I have the iommu driver enabled and it works for me. I get this during > boot: > > [0.263287] rockchip-vop2 fe040000.vop: Adding to iommu group 0 > > So I expect it is indeed used. > >> Also vop mmu seems to have the >> power-domain missing in your series (same as downstream) - however >> adding that doesn't help much currently. > Probably the power domain gets enabled anyway when the VOP is activated, > so adding it to the iommu won't help anything. Nevertheless it seems > correct to add the property, I'll do so in the next round. > >> As a sidenote: I verfied this with using Ezequiel's vpu addtion for >> RK356x: It did only work when removing the iommu there as well (getting >> tons of page faults otherwise) - so iommu driver really seems to broken, >> at least for RK3566. (Or I'm a missing a option in kernel config, which >> wasn't required for the older iommu version?) > I don't think so. I started from defconfig and disabled other > architectures and unneeded drivers, but I did not enable anything > specific to iommu. I've found out now that I can make it work with iommu, by limiting the available memory to something below 4G (I have a 8G board). So there is something wrong in the driver or somewhere in memory mapping, iommu api (since it works when using CMA), ... however: it does clearly not relate to your patch. >> >> But as reported before: For HDMI this does currently only work for pixel >> clock rates, which are integer-divisable with hpll clock rate (which is >> the hardcoded parent of vop0's dclk) >> As discussed in Benjamin's initial submission of the addition of >> RK3568's hdmi controller [1] same as with RK3288's and RK3399's hdmi phy >> needs a reference clock (it's called vpll there) which needs to get >> switched before the vop switches the mode (since phy rate switching is >> done before) - it's HPLL in case of RK356x. For whatever reason it's >> called "ref" for RK356x only downstream [2] - so you should add another >> clock "vpll" (renaming it to "ref" for _ALL_ SoCs which have it would be >> a _GREAT_ idea) which is <&pmucru PLL_HPLL>. > Yeah, a consumer clock should be named after the usage in the consumer, > not after the provider name. I also stumbled over this and naming it > "ref" makes much more sense. We'll likely have to keep supporting "vpll" > as well for compatibility to old device trees. > >> What brings us to the "real" clock problem and the reason, why >> non-integer divisable pixel clock rates are not possible ATM: This is a >> long standing issue for RK3288 and RK3399 as well (and one of the main >> reasons why 4k modes are not possible for those older SoCs currently): >> Upstream all PLL rates are controlled with those PLL rate tables in the >> clock driver and they have to be _exactly_ defined as they are used >> (HDMI sinks are very picky). >> You will not see any additional rates downstream for RK3568: they have a >> mechanism there to automatically calculate the PLL settings if the rate >> doesn't exist in these tables (IIRC this was submitted upstream also: >> but it was rejected/ignored by maintainers). > Looks like we have to try harder to get it upstream. Do you have a > pointer to this patch? Sure: https://patchwork.kernel.org/project/linux-clk/patch/20191204082527.19957-1-zhangqing@rock-chips.com/ I don't know if that is the was last submitted version. Best regards, Alex >> As a quick hackarround (for >> testing): You could use this table [3] we are using in LibreElec for >> RK3399 to get 4k modes working and assign it to HPLL in RK3568's clock >> driver (I tested it and it works great). It might be possible to just >> add those rates (some also without frac dividers) to the common PLL >> table for RK3568. > Thanks for noting. This could also explain why currently only 1080p is > working. > > Sascha >
On 2021-11-22 17:47, Alex Bee wrote: > Am 22.11.21 um 09:10 schrieb Sascha Hauer: >> Hi Alex, >> >> On Mon, Nov 22, 2021 at 12:18:47AM +0100, Alex Bee wrote: >>> Hi Sascha, >>> >>> Am 17.11.21 um 15:33 schrieb Sascha Hauer: >>>> This series adds initial graphics support for the Rockchip RK356[68] >>>> SoCs. Graphics support is based around the VOP2 controller which >>>> replaces the VOP controller found on earlier Rockchip SoCs. The driver >>>> has been tested with HDMI support included in this series and MIPI-DSI >>>> which is not included because it needs some more work. The driver is >>>> taken from the downstream Rockchip kernel and heavily polished, most non >>>> standard features have been removed for now. I tested the driver with >>>> the libdrm modetest utility and also with weston with both pixman and >>>> panfrost driver support. Michael Riesch reported the driver to work on >>>> the RK3566 as well, but device tree support for this SoC is not yet >>>> included in this series. >>>> >>>> The HDMI changes are based on patches from Benjamin Gaignard, but >>>> modified a bit as I found out that the HDMI port on the RK3568 only >>>> needs one additional clock, not two. Also I added regulator support >>>> which is needed to get the HDMI up on the rk3568-EVB board. >>>> >>>> All review and testing feedback welcome >>> >>> thanks for working on that - it's very (very,very) much appreciated. >>> >>> It took me some time to figure it out: It seems rk3568-iommu driver s >>> broken - I did only get "white noise" when using it alongside vop >>> (similar like it was reported here before). However: removing the >>> iommu-property from vop makes it working for me with HDMI output on >>> quartz64 as well. Could you check if you have the iommu driver in kernel >>> enabled if it works for you, if the property is present in DT? (I used >>> 5.16-rc1 + this series + [0]). >> I have the iommu driver enabled and it works for me. I get this during >> boot: >> >> [0.263287] rockchip-vop2 fe040000.vop: Adding to iommu group 0 >> >> So I expect it is indeed used. >> >>> Also vop mmu seems to have the >>> power-domain missing in your series (same as downstream) - however >>> adding that doesn't help much currently. >> Probably the power domain gets enabled anyway when the VOP is activated, >> so adding it to the iommu won't help anything. Nevertheless it seems >> correct to add the property, I'll do so in the next round. >> >>> As a sidenote: I verfied this with using Ezequiel's vpu addtion for >>> RK356x: It did only work when removing the iommu there as well (getting >>> tons of page faults otherwise) - so iommu driver really seems to broken, >>> at least for RK3566. (Or I'm a missing a option in kernel config, which >>> wasn't required for the older iommu version?) >> I don't think so. I started from defconfig and disabled other >> architectures and unneeded drivers, but I did not enable anything >> specific to iommu. > > I've found out now that I can make it work with iommu, by limiting the > available memory to something below 4G (I have a 8G board). So there is > something wrong in the driver or somewhere in memory mapping, iommu api > (since it works when using CMA), ... however: it does clearly not relate > to your patch. FWIW it doesn't surprise me that there might still be bugs lurking in the IOMMU driver's relatively recent changes for packing 40-bit physical addresses into 32-bit pagetable entries and registers - that sort of thing is always tricky to get right. You're correct that that's something that wants debugging in its own right, though. Robin.