mbox series

[v1,00/12] drm/rockchip: RK356x VOP2 support

Message ID 20211117143347.314294-1-s.hauer@pengutronix.de (mailing list archive)
Headers show
Series drm/rockchip: RK356x VOP2 support | expand

Message

Sascha Hauer Nov. 17, 2021, 2:33 p.m. UTC
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

Sascha

Benjamin Gaignard (2):
  dt-bindings: display: rockchip: Add compatible for rk3568 HDMI
  drm/rockchip: dw_hdmi: add rk3568 support

Sascha Hauer (10):
  drm/rockchip: dw_hdmi: Do not leave clock enabled in error case
  drm/rockchip: dw_hdmi: add regulator support
  of: graph: Allow disabled endpoints
  dt-bindings: of: graph: Allow disabled endpoints
  dt-bindings: display: rockchip: Add binding for VOP2
  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
  drm: rockchip: Add VOP2 driver

 .../display/rockchip/rockchip,dw-hdmi.yaml    |   12 +-
 .../display/rockchip/rockchip-vop2.yaml       |  114 +
 .../bindings/media/video-interfaces.yaml      |    8 +
 arch/arm/configs/multi_v7_defconfig           |    1 +
 .../boot/dts/rockchip/rk3568-evb1-v10.dts     |   24 +
 arch/arm64/boot/dts/rockchip/rk356x.dtsi      |  117 +
 arch/arm64/configs/defconfig                  |    1 +
 drivers/gpu/drm/drm_of.c                      |    6 +-
 drivers/gpu/drm/rockchip/Kconfig              |   13 +
 drivers/gpu/drm/rockchip/Makefile             |    4 +-
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c   |  137 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |    3 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   22 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h   |  774 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  | 3611 +++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c  |  916 +++++
 drivers/of/property.c                         |    3 +
 17 files changed, 5731 insertions(+), 35 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_vop2_reg.c

Comments

Sascha Hauer Nov. 17, 2021, 3:41 p.m. UTC | #1
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
Kever Yang Nov. 18, 2021, 1:27 a.m. UTC | #2
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
Heiko Stübner Nov. 18, 2021, 9:26 a.m. UTC | #3
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
Daniel Stone Nov. 18, 2021, 9:53 a.m. UTC | #4
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
Sascha Hauer Nov. 18, 2021, 10:03 a.m. UTC | #5
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
Kever Yang Nov. 18, 2021, 10:50 a.m. UTC | #6
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
Michael Riesch Nov. 18, 2021, 11:08 a.m. UTC | #7
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
Daniel Stone Nov. 18, 2021, 12:07 p.m. UTC | #8
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
Andy Yan Nov. 18, 2021, 1:14 p.m. UTC | #9
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
>
>
>
Daniel Stone Nov. 18, 2021, 1:24 p.m. UTC | #10
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
Alex Bee Nov. 21, 2021, 11:18 p.m. UTC | #11
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
Sascha Hauer Nov. 22, 2021, 8:10 a.m. UTC | #12
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
Alex Bee Nov. 22, 2021, 5:47 p.m. UTC | #13
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
>
Robin Murphy Nov. 22, 2021, 7:21 p.m. UTC | #14
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.