| Message ID | 20240321-dp-live-fmt-v3-0-d5090d796b7e@amd.com |
|---|---|
| Headers | show |
| Series | Setting live video input format for ZynqMP DPSUB | expand |
On 21/03/2024 21:43, Anatoliy Klymenko wrote: > DO NOT MERGE. REFERENCE ONLY. Why? What are you doing here and why nothing about this is explained? > > Add binding for AMD/Xilinx Video Timing Controller and Test Pattern > Generator. > > Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node. Still not tested. Do not send untested code to the lists. NAK Best regards, Krzysztof
On 21/03/2024 21:43, Anatoliy Klymenko wrote: > diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h > new file mode 100644 > index 000000000000..60fc6e11dabc > --- /dev/null > +++ b/include/dt-bindings/media/media-bus-format.h > @@ -0,0 +1,177 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > +/* > + * Media Bus API header > + * > + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. That's not true. Your SPDX tells something entirely different. Anyway, you did not explain why you need to copy anything anywhere. Specifically, random hex values *are not bindings*. Best regards, Krzysztof
On Fri, Mar 22, 2024 at 06:59:18AM +0100, Krzysztof Kozlowski wrote: > On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h > > new file mode 100644 > > index 000000000000..60fc6e11dabc > > --- /dev/null > > +++ b/include/dt-bindings/media/media-bus-format.h > > @@ -0,0 +1,177 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > +/* > > + * Media Bus API header > > + * > > + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > That's not true. Your SPDX tells something entirely different. > > Anyway, you did not explain why you need to copy anything anywhere. I assume by "copy anything anywhere" you mean "why did you copy a linux uapi header into the bindings? > Specifically, random hex values *are not bindings*. > > Best regards, > Krzysztof >
Hi Krzysztof, Thanks a lot for the review. > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, March 21, 2024 10:59 PM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie > <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, Michal > <michal.simek@amd.com>; Andrzej Hajda <andrzej.hajda@intel.com>; Neil > Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas > Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Rob > Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt- > bindings/media/media-bus-format.h > > new file mode 100644 > > index 000000000000..60fc6e11dabc > > --- /dev/null > > +++ b/include/dt-bindings/media/media-bus-format.h > > @@ -0,0 +1,177 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > +/* > > + * Media Bus API header > > + * > > + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > That's not true. Your SPDX tells something entirely different. > Thank you - I'll see how to fix it. > Anyway, you did not explain why you need to copy anything anywhere. > > Specifically, random hex values *are not bindings*. > The same media bus format values are being used by the reference driver in patch #9. And, as far as I know, we cannot use headers from Linux API headers directly (at least I noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). What would be the best approach to reusing the same defines on DT and driver sides from your point of view? Symlink maybe? > Best regards, > Krzysztof Thank you, Anatoliy
On 21/03/2024 21:43, Anatoliy Klymenko wrote: > Implement live video input format setting for ZynqMP DPSUB. > > ZynqMP DPSUB can operate in 2 modes: DMA-based and live. > > In the live mode, DPSUB receives a live video signal from FPGA-based CRTC. > DPSUB acts as a DRM encoder bridge in such a scenario. To properly tune > into the incoming video signal, DPSUB should be programmed with the proper > media bus format. This patch series addresses this task. > > Patch 1/9: Set the DPSUB layer mode of operation prior to enabling the > layer. Allows to use layer operational mode before its enablement. > > Patch 2/9: Update some IP register defines. > > Patch 3/9: Factor out some code into a helper function. > > Patch 4/9: Announce supported input media bus formats via > drm_bridge_funcs.atomic_get_input_bus_fmts callback. > > Patch 5/9: Minimize usage of a global flag. Minor improvement. > > Patch 6/9: Program DPSUB live video input format based on selected bus > config in the new atomic bridge state. > > Patch 7/9: New optional CRTC atomic helper proposal that will allow CRTC > to participate in DRM bridge chain format negotiation and impose format > restrictions. Incorporate this callback into the DRM bridge format > negotiation process. > > Patch 8/9: DT bindings documentation for Video Timing Controller and Test > Pattern Generator IPs. > > Patch 9/9: Reference FPGA CRTC driver based on AMD/Xilinx Test Pattern > Generator (TPG) IP. Add driver for the AMD/Xilinx Video Timing Controller > (VTC), which supplements TPG. None of the last users of your API can be merged, therefore this API should be considered as without users. We do not add API which does not have any in-tree users. Best regards, Krzysztof
On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > Hi Krzysztof, > > Thanks a lot for the review. > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Thursday, March 21, 2024 10:59 PM >> To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>; Laurent Pinchart >> <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst >> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; >> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie >> <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, Michal >> <michal.simek@amd.com>; Andrzej Hajda <andrzej.hajda@intel.com>; Neil >> Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas >> Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Rob >> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; >> Mauro Carvalho Chehab <mchehab@kernel.org> >> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- >> devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- >> media@vger.kernel.org >> Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings >> >> Caution: This message originated from an External Source. Use proper caution >> when opening attachments, clicking links, or responding. >> >> >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: >>> diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt- >> bindings/media/media-bus-format.h >>> new file mode 100644 >>> index 000000000000..60fc6e11dabc >>> --- /dev/null >>> +++ b/include/dt-bindings/media/media-bus-format.h >>> @@ -0,0 +1,177 @@ >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ >>> +/* >>> + * Media Bus API header >>> + * >>> + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >> >> That's not true. Your SPDX tells something entirely different. >> > > Thank you - I'll see how to fix it. > >> Anyway, you did not explain why you need to copy anything anywhere. >> >> Specifically, random hex values *are not bindings*. >> > > The same media bus format values are being used by the reference driver in patch #9. And, as far as I know, we cannot use headers from Linux API headers directly (at least I I don't understand what does it mean. You can use in your driver whatever headers you wish, I don't care about them. noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). What would be the best approach to reusing the same defines on DT and driver sides from your point of view? Symlink maybe? > Wrap your messages to match mailing list discussion style. There are no defines used in DT. If there are, show me them in *THIS* or other *upstreamed* (being upstreamed) patchset. Whatever you have out of tree or "DO NOT MERGE" does not matter and does not justify anything. Best regards, Krzysztof
On 22/03/2024 19:05, Conor Dooley wrote: > On Fri, Mar 22, 2024 at 06:59:18AM +0100, Krzysztof Kozlowski wrote: >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: >>> diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h >>> new file mode 100644 >>> index 000000000000..60fc6e11dabc >>> --- /dev/null >>> +++ b/include/dt-bindings/media/media-bus-format.h >>> @@ -0,0 +1,177 @@ >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ >>> +/* >>> + * Media Bus API header >>> + * >>> + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >> >> That's not true. Your SPDX tells something entirely different. >> >> Anyway, you did not explain why you need to copy anything anywhere. > > I assume by "copy anything anywhere" you mean "why did you copy a linux > uapi header into the bindings? Yes, I trimmed context too much. The reasoning of copying some UAPI and claiming it is a binding was: "Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node." so as seen *there is no reason*. Commit msg should explain why we are doing things. Best regards, Krzysztof
On Sat, Mar 23, 2024 at 11:22:22AM +0100, Krzysztof Kozlowski wrote: > On 22/03/2024 19:05, Conor Dooley wrote: > > On Fri, Mar 22, 2024 at 06:59:18AM +0100, Krzysztof Kozlowski wrote: > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > >>> diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt-bindings/media/media-bus-format.h > >>> new file mode 100644 > >>> index 000000000000..60fc6e11dabc > >>> --- /dev/null > >>> +++ b/include/dt-bindings/media/media-bus-format.h > >>> @@ -0,0 +1,177 @@ > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > >>> +/* > >>> + * Media Bus API header > >>> + * > >>> + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > >>> + * > >>> + * This program is free software; you can redistribute it and/or modify > >>> + * it under the terms of the GNU General Public License version 2 as > >>> + * published by the Free Software Foundation. > >> > >> That's not true. Your SPDX tells something entirely different. > >> > >> Anyway, you did not explain why you need to copy anything anywhere. > > > > I assume by "copy anything anywhere" you mean "why did you copy a linux > > uapi header into the bindings? > > Yes, I trimmed context too much. > > The reasoning of copying some UAPI and claiming it is a binding was: > "Copy media-bus-formats.h into dt-bindings/media to suplement TPG DT node." > so as seen *there is no reason*. > > Commit msg should explain why we are doing things. Oh for sure. I was just wondering if you were complaining about the UAPI header or if that comment was about the copyright notice. If it had been the latter I was gonna point out the former :)
Hi Krzysztof, Thank you for the feedback. > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Saturday, March 23, 2024 3:21 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; > David Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, > Michal <michal.simek@amd.com>; Andrzej Hajda > <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>; > Robert Foss <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; Jernej > Skrabec <jernej.skrabec@gmail.com>; Rob Herring <robh+dt@kernel.org>; > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. > > > On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > > Hi Krzysztof, > > > > Thanks a lot for the review. > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Thursday, March 21, 2024 10:59 PM > >> To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com>; Laurent Pinchart > >> <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > >> <maarten.lankhorst@linux.intel.com>; Maxime Ripard > >> <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; > David > >> Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, > >> Michal <michal.simek@amd.com>; Andrzej Hajda > >> <andrzej.hajda@intel.com>; Neil Armstrong > >> <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas > >> Karlman <jonas@kwiboo.se>; Jernej Skrabec > <jernej.skrabec@gmail.com>; > >> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > >> <conor+dt@kernel.org>; Mauro Carvalho Chehab <mchehab@kernel.org> > >> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- > >> devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; > >> linux- kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > >> media@vger.kernel.org > >> Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG > >> bindings > >> > >> Caution: This message originated from an External Source. Use proper > >> caution when opening attachments, clicking links, or responding. > >> > >> > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > >>> diff --git a/include/dt-bindings/media/media-bus-format.h > >>> b/include/dt- > >> bindings/media/media-bus-format.h > >>> new file mode 100644 > >>> index 000000000000..60fc6e11dabc > >>> --- /dev/null > >>> +++ b/include/dt-bindings/media/media-bus-format.h > >>> @@ -0,0 +1,177 @@ > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > >>> +/* > >>> + * Media Bus API header > >>> + * > >>> + * Copyright (C) 2009, Guennadi Liakhovetski > >>> +<g.liakhovetski@gmx.de> > >>> + * > >>> + * This program is free software; you can redistribute it and/or > >>> +modify > >>> + * it under the terms of the GNU General Public License version 2 > >>> +as > >>> + * published by the Free Software Foundation. > >> > >> That's not true. Your SPDX tells something entirely different. > >> > > > > Thank you - I'll see how to fix it. > > > >> Anyway, you did not explain why you need to copy anything anywhere. > >> > >> Specifically, random hex values *are not bindings*. > >> > > > > The same media bus format values are being used by the reference > > driver in patch #9. And, as far as I know, we cannot use headers from > > Linux API headers directly (at least I > > I don't understand what does it mean. You can use in your driver whatever > headers you wish, I don't care about them. > > > noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). > What would be the best approach to reusing the same defines on DT and > driver sides from your point of view? Symlink maybe? > > > > Wrap your messages to match mailing list discussion style. There are no > defines used in DT. If there are, show me them in *THIS* or other > *upstreamed* (being upstreamed) patchset. > Sorry, I didn't explain properly what I'm trying to achieve. I need to create a DT node property that represents video signal format, one of MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would be nice to reuse the same symbolic values in the device tree. What is the best approach here? Should I create a separate header in include/dt-bindings with the same or similar (to avoid multiple definition errors) defines, or is it better to create a symlink to media-bus-format.h like include/dt-bindings/linux-event-codes.h? > Whatever you have out of tree or "DO NOT MERGE" does not matter and > does not justify anything. > > > Best regards, > Krzysztof Thank you, Anatoliy
On Fri, Mar 29, 2024 at 12:38:33AM +0000, Klymenko, Anatoliy wrote: > Thank you for the feedback. > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > >>> diff --git a/include/dt-bindings/media/media-bus-format.h > > >>> b/include/dt- > > >> bindings/media/media-bus-format.h > > >>> new file mode 100644 > > >>> index 000000000000..60fc6e11dabc > > >>> --- /dev/null > > >>> +++ b/include/dt-bindings/media/media-bus-format.h > > >>> @@ -0,0 +1,177 @@ > > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > >>> +/* > > >>> + * Media Bus API header > > >>> + * > > >>> + * Copyright (C) 2009, Guennadi Liakhovetski > > >>> +<g.liakhovetski@gmx.de> > > >>> + * > > >>> + * This program is free software; you can redistribute it and/or > > >>> +modify > > >>> + * it under the terms of the GNU General Public License version 2 > > >>> +as > > >>> + * published by the Free Software Foundation. > > >> > > >> That's not true. Your SPDX tells something entirely different. > > >> > > > > > > Thank you - I'll see how to fix it. > > > > > >> Anyway, you did not explain why you need to copy anything anywhere. > > >> > > >> Specifically, random hex values *are not bindings*. > > >> > > > > > > The same media bus format values are being used by the reference > > > driver in patch #9. And, as far as I know, we cannot use headers from > > > Linux API headers directly (at least I > > > > I don't understand what does it mean. You can use in your driver whatever > > headers you wish, I don't care about them. > > > > > > noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). > > What would be the best approach to reusing the same defines on DT and > > driver sides from your point of view? Symlink maybe? > > > > > > > Wrap your messages to match mailing list discussion style. There are no > > defines used in DT. If there are, show me them in *THIS* or other > > *upstreamed* (being upstreamed) patchset. > > > > Sorry, I didn't explain properly what I'm trying to achieve. I need to > create a DT node property that represents video signal format, one of > MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would be > nice to reuse the same symbolic values in the device tree. What is the > best approach here? Should I create a separate header in > include/dt-bindings with the same or similar (to avoid multiple > definition errors) defines, or is it better to create a symlink to > media-bus-format.h like include/dt-bindings/linux-event-codes.h? Isn't there already a property for this, described in Documentation/devicetree/bindings/media/xilinx/video.txt ?
> -----Original Message----- > From: Conor Dooley <conor@kernel.org> > Sent: Friday, March 29, 2024 8:47 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; > David Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, > Michal <michal.simek@amd.com>; Andrzej Hajda > <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>; > Robert Foss <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; Jernej > Skrabec <jernej.skrabec@gmail.com>; Rob Herring <robh+dt@kernel.org>; > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Mauro Carvalho Chehab <mchehab@kernel.org>; > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > On Fri, Mar 29, 2024 at 12:38:33AM +0000, Klymenko, Anatoliy wrote: > > Thank you for the feedback. > > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > > On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > > > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > > >>> diff --git a/include/dt-bindings/media/media-bus-format.h > > > >>> b/include/dt- > > > >> bindings/media/media-bus-format.h > > > >>> new file mode 100644 > > > >>> index 000000000000..60fc6e11dabc > > > >>> --- /dev/null > > > >>> +++ b/include/dt-bindings/media/media-bus-format.h > > > >>> @@ -0,0 +1,177 @@ > > > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > > >>> +/* > > > >>> + * Media Bus API header > > > >>> + * > > > >>> + * Copyright (C) 2009, Guennadi Liakhovetski > > > >>> +<g.liakhovetski@gmx.de> > > > >>> + * > > > >>> + * This program is free software; you can redistribute it and/or > > > >>> +modify > > > >>> + * it under the terms of the GNU General Public License version 2 > > > >>> +as > > > >>> + * published by the Free Software Foundation. > > > >> > > > >> That's not true. Your SPDX tells something entirely different. > > > >> > > > > > > > > Thank you - I'll see how to fix it. > > > > > > > >> Anyway, you did not explain why you need to copy anything anywhere. > > > >> > > > >> Specifically, random hex values *are not bindings*. > > > >> > > > > > > > > The same media bus format values are being used by the reference > > > > driver in patch #9. And, as far as I know, we cannot use headers from > > > > Linux API headers directly (at least I > > > > > > I don't understand what does it mean. You can use in your driver > whatever > > > headers you wish, I don't care about them. > > > > > > > > > noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). > > > What would be the best approach to reusing the same defines on DT and > > > driver sides from your point of view? Symlink maybe? > > > > > > > > > > Wrap your messages to match mailing list discussion style. There are no > > > defines used in DT. If there are, show me them in *THIS* or other > > > *upstreamed* (being upstreamed) patchset. > > > > > > > Sorry, I didn't explain properly what I'm trying to achieve. I need to > > create a DT node property that represents video signal format, one of > > MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would > be > > nice to reuse the same symbolic values in the device tree. What is the > > best approach here? Should I create a separate header in > > include/dt-bindings with the same or similar (to avoid multiple > > definition errors) defines, or is it better to create a symlink to > > media-bus-format.h like include/dt-bindings/linux-event-codes.h? > > Isn't there already a property for this, described in > Documentation/devicetree/bindings/media/xilinx/video.txt > ? Those properties are very similar, indeed but not exactly the same. The one you noticed maps Xilinx video data format on AXI4-Stream transport that covers color space and chroma subsampling only. The rest of the signal attributes are either conventional or left out. MEDIA_BUS_FMT_* collection is more specific and embeds additional information about video signals, like bits per component and component ordering.
On 30/03/2024 03:02, Klymenko, Anatoliy wrote: >>>> >>> >>> Sorry, I didn't explain properly what I'm trying to achieve. I need to >>> create a DT node property that represents video signal format, one of >>> MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would >> be >>> nice to reuse the same symbolic values in the device tree. What is the >>> best approach here? Should I create a separate header in There is no user of this new header, so I don't agree. Please send either full work or link your other upstreamed patchset. Anything sent as "DO NOT MERGE" does not count because it is not an user. Without the DTS user I claim that you do not bind here anything... >>> include/dt-bindings with the same or similar (to avoid multiple >>> definition errors) defines, or is it better to create a symlink to >>> media-bus-format.h like include/dt-bindings/linux-event-codes.h? Copying or symlinking entire header into bindings does not help us to understand what is exactly a binding here. For example, maybe you encode runtime information into DT (don't do this) and that's why you need these defines... Or maybe your block has some capabilities. Dunno, patch was not tested, is defined as do not merge and is not explaining any of these. Therefore, please provide complete set of users ready to be merged, test your patches, provide rationale why this is supposed to be a binding and why do you think it represents hardware configuration, not OS policy or runtime configuration. Best regards, Krzysztof
Implement live video input format setting for ZynqMP DPSUB. ZynqMP DPSUB can operate in 2 modes: DMA-based and live. In the live mode, DPSUB receives a live video signal from FPGA-based CRTC. DPSUB acts as a DRM encoder bridge in such a scenario. To properly tune into the incoming video signal, DPSUB should be programmed with the proper media bus format. This patch series addresses this task. Patch 1/9: Set the DPSUB layer mode of operation prior to enabling the layer. Allows to use layer operational mode before its enablement. Patch 2/9: Update some IP register defines. Patch 3/9: Factor out some code into a helper function. Patch 4/9: Announce supported input media bus formats via drm_bridge_funcs.atomic_get_input_bus_fmts callback. Patch 5/9: Minimize usage of a global flag. Minor improvement. Patch 6/9: Program DPSUB live video input format based on selected bus config in the new atomic bridge state. Patch 7/9: New optional CRTC atomic helper proposal that will allow CRTC to participate in DRM bridge chain format negotiation and impose format restrictions. Incorporate this callback into the DRM bridge format negotiation process. Patch 8/9: DT bindings documentation for Video Timing Controller and Test Pattern Generator IPs. Patch 9/9: Reference FPGA CRTC driver based on AMD/Xilinx Test Pattern Generator (TPG) IP. Add driver for the AMD/Xilinx Video Timing Controller (VTC), which supplements TPG. To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> To: Maxime Ripard <mripard@kernel.org> To: Thomas Zimmermann <tzimmermann@suse.de> To: David Airlie <airlied@gmail.com> To: Daniel Vetter <daniel@ffwll.ch> To: Michal Simek <michal.simek@amd.com> To: Andrzej Hajda <andrzej.hajda@intel.com> To: Neil Armstrong <neil.armstrong@linaro.org> To: Robert Foss <rfoss@kernel.org> To: Jonas Karlman <jonas@kwiboo.se> To: Jernej Skrabec <jernej.skrabec@gmail.com> To: Rob Herring <robh+dt@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Conor Dooley <conor+dt@kernel.org> To: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-media@vger.kernel.org Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> Changes in v3: - Add connected live layer helper - Include reference DRM format in zynqmp_disp_format for live layerss. - Add default bus format list for non-live case. - Explain removal of redundant checks in the commit message. - Minor fixes and improvements from review comments. Link to v2: https://lore.kernel.org/r/20240312-dp-live-fmt-v2-0-a9c35dc5c50d@amd.com Changes in v2: - Factor out register defines update into separate patch. - Add some improvements minimizing ithe usage of a global flag. - Reuse existing format setting API instead of introducing new versions. - Add warning around NULL check on new bridge state within atomic enable callback. - Add drm_helper_crtc_select_output_bus_format() that wraps drm_crtc_helper_funcs.select_output_bus_format(). - Update API comments per review recommendations. - Address some minor review comments. - Add reference CRTC driver that demonstrates the usage of the proposed drm_crtc_helper_funcs.select_output_bus_format() API. - Link to v1: https://lore.kernel.org/r/20240226-dp-live-fmt-v1-0-b78c3f69c9d8@amd.com --- Anatoliy Klymenko (9): drm: xlnx: zynqmp_dpsub: Set layer mode during creation drm: xlnx: zynqmp_dpsub: Update live format defines drm: xlnx: zynqmp_dpsub: Add connected live layer helper drm: xlnx: zynqmp_dpsub: Anounce supported input formats drm: xlnx: zynqmp_dpsub: Minimize usage of global flag drm: xlnx: zynqmp_dpsub: Set input live format drm/atomic-helper: Add select_output_bus_format callback dt-bindings: xlnx: Add VTC and TPG bindings drm: xlnx: Intoduce TPG CRTC driver .../bindings/display/xlnx/xlnx,v-tpg.yaml | 87 +++ .../devicetree/bindings/display/xlnx/xlnx,vtc.yaml | 65 ++ drivers/gpu/drm/drm_bridge.c | 14 +- drivers/gpu/drm/drm_crtc_helper.c | 36 + drivers/gpu/drm/xlnx/Kconfig | 21 + drivers/gpu/drm/xlnx/Makefile | 4 + drivers/gpu/drm/xlnx/xlnx_tpg.c | 846 +++++++++++++++++++++ drivers/gpu/drm/xlnx/xlnx_vtc.c | 452 +++++++++++ drivers/gpu/drm/xlnx/xlnx_vtc.h | 101 +++ drivers/gpu/drm/xlnx/xlnx_vtc_list.c | 160 ++++ drivers/gpu/drm/xlnx/zynqmp_disp.c | 170 ++++- drivers/gpu/drm/xlnx/zynqmp_disp.h | 19 +- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 81 +- drivers/gpu/drm/xlnx/zynqmp_kms.c | 6 +- include/drm/drm_crtc_helper.h | 5 + include/drm/drm_modeset_helper_vtables.h | 30 + include/dt-bindings/media/media-bus-format.h | 177 +++++ 18 files changed, 2198 insertions(+), 84 deletions(-) --- base-commit: bfa4437fd3938ae2e186e7664b2db65bb8775670 change-id: 20240226-dp-live-fmt-6415773b5a68 Best regards,