From: "Klymenko, Anatoliy" <Anatoliy.Klymenko@amd.com> To: Maxime Ripard <mripard@kernel.org> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 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>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org> Subject: RE: [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver Date: Thu, 14 Mar 2024 19:43:30 +0000 [thread overview] Message-ID: <MW4PR12MB7165A15E233957E3914AA297E6292@MW4PR12MB7165.namprd12.prod.outlook.com> (raw) In-Reply-To: <20240314-esoteric-delicate-sidewinder-5dc4db@houat> Hi Maxime, Thank you for the review. > -----Original Message----- > From: Maxime Ripard <mripard@kernel.org> > Sent: Thursday, March 14, 2024 5:05 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; 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>; 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 v2 8/8] drm: xlnx: Intoduce TPG CRTC driver > > Hi, > > On Tue, Mar 12, 2024 at 05:55:05PM -0700, Anatoliy Klymenko wrote: > > DO NOT MERGE. REFERENCE ONLY. > > > > Add CRTC driver based on AMD/Xilinx Video Test Pattern Generator IP. > > TPG based FPGA design represents minimalistic harness useful for > > testing links between FPGA based CRTC and external DRM encoders, both > > FPGA and hardened IP based. > > > > Add driver for AMD/Xilinx Video Timing Controller. The VTC, working in > > generator mode, suplements TPG with video timing signals. > > > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > > As I said previously, we don't want to have unused APIs, so this patch should be in > a good enough state to be merged if we want to merge the whole API. > This is understandable, but even having this API just reviewed by the community will open the path forward for aligning AMD/Xilinx downstream DRM drivers with the upstream kernel. > > +/* > > +--------------------------------------------------------------------- > > +-------- > > + * DRM CRTC > > + */ > > + > > +static enum drm_mode_status xlnx_tpg_crtc_mode_valid(struct drm_crtc > *crtc, > > + const struct > drm_display_mode *mode) { > > + return MODE_OK; > > +} > > + > > +static int xlnx_tpg_crtc_check(struct drm_crtc *crtc, > > + struct drm_atomic_state *state) { > > + struct drm_crtc_state *crtc_state = > drm_atomic_get_new_crtc_state(state, crtc); > > + int ret; > > + > > + if (!crtc_state->enable) > > + goto out; > > + > > + ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state); > > + if (ret) > > + return ret; > > + > > +out: > > + return drm_atomic_add_affected_planes(state, crtc); } > > + > > [...] > > > + > > +static u32 xlnx_tpg_crtc_select_output_bus_format(struct drm_crtc *crtc, > > + struct drm_crtc_state > *crtc_state, > > + const u32 *in_bus_fmts, > > + unsigned int > num_in_bus_fmts) { > > + struct xlnx_tpg *tpg = crtc_to_tpg(crtc); > > + unsigned int i; > > + > > + for (i = 0; i < num_in_bus_fmts; ++i) > > + if (in_bus_fmts[i] == tpg->output_bus_format) > > + return tpg->output_bus_format; > > + > > + return 0; > > +} > > + > > +static const struct drm_crtc_helper_funcs xlnx_tpg_crtc_helper_funcs = { > > + .mode_valid = xlnx_tpg_crtc_mode_valid, > > + .atomic_check = xlnx_tpg_crtc_check, > > + .atomic_enable = xlnx_tpg_crtc_enable, > > + .atomic_disable = xlnx_tpg_crtc_disable, > > + .select_output_bus_format = xlnx_tpg_crtc_select_output_bus_format, > > +}; > > From that code, it's not clear to me how the CRTC is going to be able to get what > the format is. > It's coming from DT "bus-format" property. The idea is that this property will reflect the FPGA design variation synthesized. > It looks like you hardcode it here, but what if there's several that would fit the > bill? Is the CRTC expected to store it into its private structure? > It's impractical from the resources utilization point of view to support multiple runtime options for FPGA-based CRTCs output signal format, so the bus-format will be runtime fixed but can vary between differently synthesized instances. > If so, I would expect it to be in the crtc state, and atomic_enable to just reuse > whatever is in the state. > This could be totally valid for different kinds of CRTCs, although for this particular case, the bus-fomat choice is runtime immutable. > Maxime Thank you, Anatoliy
WARNING: multiple messages have this Message-ID (diff)
From: "Klymenko, Anatoliy" <Anatoliy.Klymenko@amd.com> To: Maxime Ripard <mripard@kernel.org> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 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>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org> Subject: RE: [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver Date: Thu, 14 Mar 2024 19:43:30 +0000 [thread overview] Message-ID: <MW4PR12MB7165A15E233957E3914AA297E6292@MW4PR12MB7165.namprd12.prod.outlook.com> (raw) In-Reply-To: <20240314-esoteric-delicate-sidewinder-5dc4db@houat> Hi Maxime, Thank you for the review. > -----Original Message----- > From: Maxime Ripard <mripard@kernel.org> > Sent: Thursday, March 14, 2024 5:05 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; 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>; 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 v2 8/8] drm: xlnx: Intoduce TPG CRTC driver > > Hi, > > On Tue, Mar 12, 2024 at 05:55:05PM -0700, Anatoliy Klymenko wrote: > > DO NOT MERGE. REFERENCE ONLY. > > > > Add CRTC driver based on AMD/Xilinx Video Test Pattern Generator IP. > > TPG based FPGA design represents minimalistic harness useful for > > testing links between FPGA based CRTC and external DRM encoders, both > > FPGA and hardened IP based. > > > > Add driver for AMD/Xilinx Video Timing Controller. The VTC, working in > > generator mode, suplements TPG with video timing signals. > > > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > > As I said previously, we don't want to have unused APIs, so this patch should be in > a good enough state to be merged if we want to merge the whole API. > This is understandable, but even having this API just reviewed by the community will open the path forward for aligning AMD/Xilinx downstream DRM drivers with the upstream kernel. > > +/* > > +--------------------------------------------------------------------- > > +-------- > > + * DRM CRTC > > + */ > > + > > +static enum drm_mode_status xlnx_tpg_crtc_mode_valid(struct drm_crtc > *crtc, > > + const struct > drm_display_mode *mode) { > > + return MODE_OK; > > +} > > + > > +static int xlnx_tpg_crtc_check(struct drm_crtc *crtc, > > + struct drm_atomic_state *state) { > > + struct drm_crtc_state *crtc_state = > drm_atomic_get_new_crtc_state(state, crtc); > > + int ret; > > + > > + if (!crtc_state->enable) > > + goto out; > > + > > + ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state); > > + if (ret) > > + return ret; > > + > > +out: > > + return drm_atomic_add_affected_planes(state, crtc); } > > + > > [...] > > > + > > +static u32 xlnx_tpg_crtc_select_output_bus_format(struct drm_crtc *crtc, > > + struct drm_crtc_state > *crtc_state, > > + const u32 *in_bus_fmts, > > + unsigned int > num_in_bus_fmts) { > > + struct xlnx_tpg *tpg = crtc_to_tpg(crtc); > > + unsigned int i; > > + > > + for (i = 0; i < num_in_bus_fmts; ++i) > > + if (in_bus_fmts[i] == tpg->output_bus_format) > > + return tpg->output_bus_format; > > + > > + return 0; > > +} > > + > > +static const struct drm_crtc_helper_funcs xlnx_tpg_crtc_helper_funcs = { > > + .mode_valid = xlnx_tpg_crtc_mode_valid, > > + .atomic_check = xlnx_tpg_crtc_check, > > + .atomic_enable = xlnx_tpg_crtc_enable, > > + .atomic_disable = xlnx_tpg_crtc_disable, > > + .select_output_bus_format = xlnx_tpg_crtc_select_output_bus_format, > > +}; > > From that code, it's not clear to me how the CRTC is going to be able to get what > the format is. > It's coming from DT "bus-format" property. The idea is that this property will reflect the FPGA design variation synthesized. > It looks like you hardcode it here, but what if there's several that would fit the > bill? Is the CRTC expected to store it into its private structure? > It's impractical from the resources utilization point of view to support multiple runtime options for FPGA-based CRTCs output signal format, so the bus-format will be runtime fixed but can vary between differently synthesized instances. > If so, I would expect it to be in the crtc state, and atomic_enable to just reuse > whatever is in the state. > This could be totally valid for different kinds of CRTCs, although for this particular case, the bus-fomat choice is runtime immutable. > Maxime Thank you, Anatoliy _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-03-14 19:43 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-03-13 0:54 [PATCH v2 0/8] Setting live video input format for ZynqMP DPSUB Anatoliy Klymenko 2024-03-13 0:54 ` Anatoliy Klymenko 2024-03-13 0:54 ` [PATCH v2 1/8] drm: xlnx: zynqmp_dpsub: Set layer mode during creation Anatoliy Klymenko 2024-03-13 0:54 ` Anatoliy Klymenko 2024-03-13 0:54 ` [PATCH v2 2/8] drm: xlnx: zynqmp_dpsub: Update live format defines Anatoliy Klymenko 2024-03-13 0:54 ` Anatoliy Klymenko 2024-03-18 23:16 ` Laurent Pinchart 2024-03-18 23:16 ` Laurent Pinchart 2024-03-13 0:55 ` [PATCH v2 3/8] drm: xlnx: zynqmp_dpsub: Anounce supported input formats Anatoliy Klymenko 2024-03-13 0:55 ` Anatoliy Klymenko 2024-03-19 0:05 ` Laurent Pinchart 2024-03-19 0:05 ` Laurent Pinchart 2024-03-20 0:57 ` Klymenko, Anatoliy 2024-03-20 0:57 ` Klymenko, Anatoliy 2024-03-13 0:55 ` [PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global flag Anatoliy Klymenko 2024-03-13 0:55 ` Anatoliy Klymenko 2024-03-19 0:12 ` Laurent Pinchart 2024-03-19 0:12 ` Laurent Pinchart 2024-03-20 1:12 ` Klymenko, Anatoliy 2024-03-20 1:12 ` Klymenko, Anatoliy 2024-03-13 0:55 ` [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format Anatoliy Klymenko 2024-03-13 0:55 ` Anatoliy Klymenko 2024-03-19 0:35 ` Laurent Pinchart 2024-03-19 0:35 ` Laurent Pinchart 2024-03-20 1:25 ` Klymenko, Anatoliy 2024-03-20 1:25 ` Klymenko, Anatoliy 2024-03-13 0:55 ` [PATCH v2 6/8] drm/atomic-helper: Add select_output_bus_format callback Anatoliy Klymenko 2024-03-13 0:55 ` Anatoliy Klymenko 2024-03-13 0:55 ` [PATCH v2 7/8] dt-bindings: xlnx: Add VTC and TPG bindings Anatoliy Klymenko 2024-03-13 0:55 ` Anatoliy Klymenko 2024-03-13 2:30 ` Rob Herring 2024-03-13 2:30 ` Rob Herring 2024-03-13 7:27 ` Krzysztof Kozlowski 2024-03-13 7:27 ` Krzysztof Kozlowski 2024-03-13 0:55 ` [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver Anatoliy Klymenko 2024-03-13 0:55 ` Anatoliy Klymenko 2024-03-14 12:05 ` Maxime Ripard 2024-03-14 12:05 ` Maxime Ripard 2024-03-14 19:43 ` Klymenko, Anatoliy [this message] 2024-03-14 19:43 ` Klymenko, Anatoliy 2024-03-15 15:24 ` Maxime Ripard 2024-03-15 15:24 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=MW4PR12MB7165A15E233957E3914AA297E6292@MW4PR12MB7165.namprd12.prod.outlook.com \ --to=anatoliy.klymenko@amd.com \ --cc=airlied@gmail.com \ --cc=andrzej.hajda@intel.com \ --cc=conor+dt@kernel.org \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=jernej.skrabec@gmail.com \ --cc=jonas@kwiboo.se \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mchehab@kernel.org \ --cc=michal.simek@amd.com \ --cc=mripard@kernel.org \ --cc=neil.armstrong@linaro.org \ --cc=rfoss@kernel.org \ --cc=robh+dt@kernel.org \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.