| Message ID | 20230630144006.1513270-1-pan@semihalf.com |
|---|---|
| Headers | show |
| Series | Google Chameleon v3 video driver | expand |
On Fri, Jun 30, 2023 at 8:26 PM Conor Dooley <conor@kernel.org> wrote: > > On Fri, Jun 30, 2023 at 04:40:06PM +0200, Paweł Anikiel wrote: > > Add node for the video system device. > > > > Signed-off-by: Paweł Anikiel <pan@semihalf.com> > > --- > > .../socfpga/socfpga_arria10_chameleonv3.dts | 54 +++++++++++++++++++ > > 1 file changed, 54 insertions(+) > > > > diff --git a/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts b/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts > > index 422d00cd4c74..5e66363d4ab5 100644 > > --- a/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts > > +++ b/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts > > @@ -10,6 +10,60 @@ / { > > compatible = "google,chameleon-v3", "enclustra,mercury-aa1", > > "altr,socfpga-arria10", "altr,socfpga"; > > > > + soc { > > + video@c0060500 { > > + compatible = "google,chv3-video"; > > This compatible does not seem to be documented & I did not see a comment > about the lack of a binding in the cover letter. What am I missing? Yes, the compatible is not documented for now (I'll do that in a later patchset), sorry for not mentioning that in the cover letter.
On 03/07/2023 13:44, Paweł Anikiel wrote: >>> diff --git a/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts b/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts >>> index 422d00cd4c74..5e66363d4ab5 100644 >>> --- a/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts >>> +++ b/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts >>> @@ -10,6 +10,60 @@ / { >>> compatible = "google,chameleon-v3", "enclustra,mercury-aa1", >>> "altr,socfpga-arria10", "altr,socfpga"; >>> >>> + soc { >>> + video@c0060500 { >>> + compatible = "google,chv3-video"; >> >> This compatible does not seem to be documented & I did not see a comment >> about the lack of a binding in the cover letter. What am I missing? > > Yes, the compatible is not documented for now (I'll do that in a later > patchset), sorry for not mentioning that in the cover letter. You cannot add undocumented compatible. This cannot be fixed in "a later patchset". Best regards, Krzysztof
On Mon, Jul 3, 2023 at 2:33 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 03/07/2023 13:44, Paweł Anikiel wrote: > >>> diff --git a/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts b/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts > >>> index 422d00cd4c74..5e66363d4ab5 100644 > >>> --- a/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts > >>> +++ b/arch/arm/boot/dts/intel/socfpga/socfpga_arria10_chameleonv3.dts > >>> @@ -10,6 +10,60 @@ / { > >>> compatible = "google,chameleon-v3", "enclustra,mercury-aa1", > >>> "altr,socfpga-arria10", "altr,socfpga"; > >>> > >>> + soc { > >>> + video@c0060500 { > >>> + compatible = "google,chv3-video"; > >> > >> This compatible does not seem to be documented & I did not see a comment > >> about the lack of a binding in the cover letter. What am I missing? > > > > Yes, the compatible is not documented for now (I'll do that in a later > > patchset), sorry for not mentioning that in the cover letter. > > You cannot add undocumented compatible. This cannot be fixed in "a later > patchset". I meant later revision, I'm certainly not expecting this one to land (I sent is as an RFC). Is it really necessary to document the compatible to get any form of feedback on the overall structure of the driver?
On 04/07/2023 18:16, Paweł Anikiel wrote: >>>>> + soc { >>>>> + video@c0060500 { >>>>> + compatible = "google,chv3-video"; >>>> >>>> This compatible does not seem to be documented & I did not see a comment >>>> about the lack of a binding in the cover letter. What am I missing? >>> >>> Yes, the compatible is not documented for now (I'll do that in a later >>> patchset), sorry for not mentioning that in the cover letter. >> >> You cannot add undocumented compatible. This cannot be fixed in "a later >> patchset". > > I meant later revision, I'm certainly not expecting this one to land > (I sent is as an RFC). That's not clear. RFC is interpreted differently by different people. Some just ignore it entirely, some still review. > Is it really necessary to document the > compatible to get any form of feedback on the overall structure of the > driver? Depends on the person. Anyway no problem for me - I will just ignore the patchset. Best regards, Krzysztof
On Tue, Jul 04, 2023 at 06:23:10PM +0200, Krzysztof Kozlowski wrote: > On 04/07/2023 18:16, Paweł Anikiel wrote: > >>>>> + soc { > >>>>> + video@c0060500 { > >>>>> + compatible = "google,chv3-video"; > >>>> > >>>> This compatible does not seem to be documented & I did not see a comment > >>>> about the lack of a binding in the cover letter. What am I missing? > >>> > >>> Yes, the compatible is not documented for now (I'll do that in a later > >>> patchset), sorry for not mentioning that in the cover letter. > >> > >> You cannot add undocumented compatible. This cannot be fixed in "a later > >> patchset". > > > > I meant later revision, I'm certainly not expecting this one to land > > (I sent is as an RFC). > > That's not clear. RFC is interpreted differently by different people. > Some just ignore it entirely, some still review. > > > Is it really necessary to document the > > compatible to get any form of feedback on the overall structure of the > > driver? > > Depends on the person. Anyway no problem for me - I will just ignore the > patchset. FWIW, I was asking about it in case you weren't aware Paweł that you would need to document the properties, since it wasn't mentioned anywhere.
On Fri, Jun 30, 2023 at 10:40 AM Paweł Anikiel <pan@semihalf.com> wrote: > > The Google Chameleon v3 is a testing device for external displays. It > is based on an Arria 10 SoCFPGA. This patch adds a V4L2 driver for the > video system. The video system consists of: > * Six video interfaces (DMA ping pong buffers) in the FPGA, called > "framebuffers". > * Two Intel DisplayPort DPRX IP cores in the FPGA, one MST x4, one SST > * IT68051 chip, handled by EC firmware > > The driver is implemented as a single device driver, because the video > interface devices need to talk to the DisplayPort IP core devices > (e.g. to configure the EDID). This has the effect of the DPRX driver > being in the chameleonv3 directory even though it's an Intel IP. > > The DPRX code handles all the AUX communication (DPCD, sideband messages, > message transfers). There is similarity to what's already present in > the DRM subsystem, but I found it hard to reuse that code effectively. > > My main concern is with the overall structure of the driver - how it's > divided into parts, the interfaces and APIs used, etc. Any feedback is > greately appreciated. > > Paweł Anikiel (3): > media: Add 10, 12, and 16 bit RGB formats > media: Add Google Chameleon v3 video driver > ARM: dts: Add Chameleon v3 video node > > .../socfpga/socfpga_arria10_chameleonv3.dts | 54 ++ > drivers/media/platform/Kconfig | 1 + > drivers/media/platform/Makefile | 1 + > drivers/media/platform/google/Kconfig | 4 + > drivers/media/platform/google/Makefile | 2 + > .../media/platform/google/chameleonv3/Kconfig | 9 + > .../platform/google/chameleonv3/Makefile | 15 + > .../platform/google/chameleonv3/chv3-core.c | 292 ++++++++++ > .../platform/google/chameleonv3/chv3-core.h | 17 + > .../platform/google/chameleonv3/chv3-fb.c | 539 ++++++++++++++++++ > .../platform/google/chameleonv3/chv3-fb.h | 34 ++ > .../platform/google/chameleonv3/dprx-aux.c | 77 +++ > .../platform/google/chameleonv3/dprx-dp.c | 82 +++ > .../platform/google/chameleonv3/dprx-dpcd.c | 424 ++++++++++++++ > .../platform/google/chameleonv3/dprx-dprx.c | 262 +++++++++ > .../platform/google/chameleonv3/dprx-edid.c | 39 ++ > .../platform/google/chameleonv3/dprx-i2c.c | 41 ++ > .../platform/google/chameleonv3/dprx-mt.c | 184 ++++++ > .../platform/google/chameleonv3/dprx-sbmsg.c | 162 ++++++ > .../media/platform/google/chameleonv3/dprx.h | 128 +++++ > drivers/media/v4l2-core/v4l2-ioctl.c | 5 + > include/uapi/linux/videodev2.h | 5 + > 22 files changed, 2377 insertions(+) > create mode 100644 drivers/media/platform/google/Kconfig > create mode 100644 drivers/media/platform/google/Makefile > create mode 100644 drivers/media/platform/google/chameleonv3/Kconfig > create mode 100644 drivers/media/platform/google/chameleonv3/Makefile > create mode 100644 drivers/media/platform/google/chameleonv3/chv3-core.c > create mode 100644 drivers/media/platform/google/chameleonv3/chv3-core.h > create mode 100644 drivers/media/platform/google/chameleonv3/chv3-fb.c > create mode 100644 drivers/media/platform/google/chameleonv3/chv3-fb.h > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-aux.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-dp.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-dpcd.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-dprx.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-edid.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-i2c.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-mt.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-sbmsg.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx.h > > -- > 2.41.0.255.g8b1d071c50-goog > Thank you Pawel for sending this. To generate more interest in this device/driver I wanted to share that the FPGA implementation (the registers/hw this driver talks to) is also open source [1]. This is to show that this is not some throw away driver for some proprietary thing. The previous Chameleon device has been useful for testing drivers using the Intel Graphics Test [2] which can exercise a lot of the DRM video output kernel drivers on many devices, v3 is also compatible with the same RPC protocol. [1] https://chromium.googlesource.com/chromiumos/platform/chameleon/+/refs/heads/main/v3/fpga/ [2] https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-Chamelium.html Alexandru Stan (amstan)
Hi Pawel, Apologies for the much-too-long delay, but I finally had time to look at this. On 30/06/2023 16:40, Paweł Anikiel wrote: > The Google Chameleon v3 is a testing device for external displays. It > is based on an Arria 10 SoCFPGA. This patch adds a V4L2 driver for the > video system. The video system consists of: > * Six video interfaces (DMA ping pong buffers) in the FPGA, called > "framebuffers". > * Two Intel DisplayPort DPRX IP cores in the FPGA, one MST x4, one SST > * IT68051 chip, handled by EC firmware > > The driver is implemented as a single device driver, because the video > interface devices need to talk to the DisplayPort IP core devices > (e.g. to configure the EDID). This has the effect of the DPRX driver > being in the chameleonv3 directory even though it's an Intel IP. > > The DPRX code handles all the AUX communication (DPCD, sideband messages, > message transfers). There is similarity to what's already present in > the DRM subsystem, but I found it hard to reuse that code effectively. > > My main concern is with the overall structure of the driver - how it's > divided into parts, the interfaces and APIs used, etc. Any feedback is > greately appreciated. I need to see the v4l2-compliance output for this new driver. Typically 'v4l2-compliance -s -d /dev/videoX' should be enough. You have to compile v4l2-compliance from the git repo (git://linuxtv.org/v4l-utils.git) to ensure you use the latest and greatest version. Obviously any failures should be fixed. Just copy-and-paste the v4l2-compliance output to the cover letter. Regards, Hans > > Paweł Anikiel (3): > media: Add 10, 12, and 16 bit RGB formats > media: Add Google Chameleon v3 video driver > ARM: dts: Add Chameleon v3 video node > > .../socfpga/socfpga_arria10_chameleonv3.dts | 54 ++ > drivers/media/platform/Kconfig | 1 + > drivers/media/platform/Makefile | 1 + > drivers/media/platform/google/Kconfig | 4 + > drivers/media/platform/google/Makefile | 2 + > .../media/platform/google/chameleonv3/Kconfig | 9 + > .../platform/google/chameleonv3/Makefile | 15 + > .../platform/google/chameleonv3/chv3-core.c | 292 ++++++++++ > .../platform/google/chameleonv3/chv3-core.h | 17 + > .../platform/google/chameleonv3/chv3-fb.c | 539 ++++++++++++++++++ > .../platform/google/chameleonv3/chv3-fb.h | 34 ++ > .../platform/google/chameleonv3/dprx-aux.c | 77 +++ > .../platform/google/chameleonv3/dprx-dp.c | 82 +++ > .../platform/google/chameleonv3/dprx-dpcd.c | 424 ++++++++++++++ > .../platform/google/chameleonv3/dprx-dprx.c | 262 +++++++++ > .../platform/google/chameleonv3/dprx-edid.c | 39 ++ > .../platform/google/chameleonv3/dprx-i2c.c | 41 ++ > .../platform/google/chameleonv3/dprx-mt.c | 184 ++++++ > .../platform/google/chameleonv3/dprx-sbmsg.c | 162 ++++++ > .../media/platform/google/chameleonv3/dprx.h | 128 +++++ > drivers/media/v4l2-core/v4l2-ioctl.c | 5 + > include/uapi/linux/videodev2.h | 5 + > 22 files changed, 2377 insertions(+) > create mode 100644 drivers/media/platform/google/Kconfig > create mode 100644 drivers/media/platform/google/Makefile > create mode 100644 drivers/media/platform/google/chameleonv3/Kconfig > create mode 100644 drivers/media/platform/google/chameleonv3/Makefile > create mode 100644 drivers/media/platform/google/chameleonv3/chv3-core.c > create mode 100644 drivers/media/platform/google/chameleonv3/chv3-core.h > create mode 100644 drivers/media/platform/google/chameleonv3/chv3-fb.c > create mode 100644 drivers/media/platform/google/chameleonv3/chv3-fb.h > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-aux.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-dp.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-dpcd.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-dprx.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-edid.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-i2c.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-mt.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx-sbmsg.c > create mode 100644 drivers/media/platform/google/chameleonv3/dprx.h >