Received-SPF: pass (google.com: domain of linux-kernel+bounces-103738-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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
Thread-Topic: [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver
Thread-Index: AQHadOEjzq/Vahr1mEq0V8Suc/Gl2bE3Je0AgAB2zWA=
Date: Thu, 14 Mar 2024 19:43:30 +0000
Message-ID:
 <MW4PR12MB7165A15E233957E3914AA297E6292@MW4PR12MB7165.namprd12.prod.outlook.com>
References: <20240312-dp-live-fmt-v2-0-a9c35dc5c50d@amd.com>
 <20240312-dp-live-fmt-v2-8-a9c35dc5c50d@amd.com>
 <20240314-esoteric-delicate-sidewinder-5dc4db@houat>
In-Reply-To: <20240314-esoteric-delicate-sidewinder-5dc4db@houat>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?dFiiNdazaCUapWHiqwROeDqHEUoiVhfWBirzm9VArvBPCOpE/Tpe1KnHngyN?=
 =?us-ascii?Q?iMmB0TlmkbOzcPmfxFbgnx+FGYF8kOcxBZTUDFQNNE0obnrsa5Dri+X7UNzJ?=
 =?us-ascii?Q?rc1MoH8JMIwDyiqD9ZhE8ehL7mEerhJRn6J5qwDMewLS1/Zmc/ouBP+Jaedu?=
 =?us-ascii?Q?GdwwNrdaPszIDR+MfHd9F6UPQIrUB9DqbkIWQoQ9oVEqBwqV+dfSyI5ZkKIQ?=
 =?us-ascii?Q?oZ60M2uefZQhotHCluHRJJT9ld88EB7NtlV5j/gRjqZF1tSZlHiCiefT+KuM?=
 =?us-ascii?Q?UvrdD7TgdCSLkG/MMBJhCqP2TRJru+me8/Zq6abdhgosFp9gEoXtymtNV9fG?=
 =?us-ascii?Q?0XgA5KseKnemUaGD8/7Zc3vWJb8y4zjge6MITMxgNmnXzksvKA9MXIrPx2Tf?=
 =?us-ascii?Q?VaLhJm8BbjtkLJ+ApSZoUSkkK2k+WwAHykkBkjfaCYk2TshIg/4cBnhnwKBJ?=
 =?us-ascii?Q?eChU+GdqTx5ke5yFWPszw1PO5472DeHDlqCLyT7+GsmXw34OMVNRrvr7iw3s?=
 =?us-ascii?Q?xfBHPHMhUh+HbtWi1jfo2nHsP+CnP6jqJGrqdPjJqNQoZkG4ZRKF0K36FGKV?=
 =?us-ascii?Q?wNAM3hkSxMTiUZliz+CzOB5Lk+Ui0JY23Kp2rJhR/lwpBlxZ9oF9tMng5zIR?=
 =?us-ascii?Q?ixWC3m7yUIkDauMyx15Y/Jc77JkeDyyYXhbjdiIpYmI7HY02u6G2iCvo7qnM?=
 =?us-ascii?Q?c36KluFiGQwrs4hXz9BFXeCxr2/9uPPrYuZVvcylTqRSIJiuII/O0YICHoY/?=
 =?us-ascii?Q?tHz+s2AjWcXsYAK/tQ+QTQyJqLy4fKQmSSUZKtrpLp7KDfX44/ZAF/0/Tuv9?=
 =?us-ascii?Q?2C8PtsWmtJWZdQlRR+VJpoHQvgOKJmrKqMDrpT6fowPIxTND2u9n1LPOI9xs?=
 =?us-ascii?Q?dicbJjXQe/By+YsZ8qFEkJiQ5tbjnloivZvXc08a27C5LOS+e1uIDkhbfUIZ?=
 =?us-ascii?Q?jF9EB105L32el3bhNrp4V2sDufqY8Dcv2WQMgvulTAyjxt5V+IcYFnXZonxG?=
 =?us-ascii?Q?iIQYolt/Th3mYzY2h9KYOnAjEWPJtKdggde3XeOwMSvFcRR9ZZZgIxKj83AK?=
 =?us-ascii?Q?24br8R4apeeDXwVKz1Zd2Z3pw9Ht5+YZ1RQ8lynrAz0m+mp1DguCpeV0EFK4?=
 =?us-ascii?Q?soTfFkRE9oFhK/4Rb8AE94vnEv47RNN0FeFBRI+yrINNMWq/H02iPp7O3myV?=
 =?us-ascii?Q?tjspmAEC8fi8XRgJ0M9KapSKDZqBjgMPsB6FUXZCrDN+IRVw0n93tcMZvNys?=
 =?us-ascii?Q?Gb0edZFgVtevW5D5IjU8NTTy91Bm82xjYN1aLhNEzaDZ88IU04PrrXBsHVzn?=
 =?us-ascii?Q?Nu3jlbvSxPIhO0synx7TKVamx3Xxgdu6ZvZk5vdTMoJRYzcnfxEx5eFluovi?=
 =?us-ascii?Q?YA8Qopqjp/vQCUELJjGkGWguF9Ltu+888M2qlaDeK4FKLJpECLkyMmFvwNsE?=
 =?us-ascii?Q?KgfyLHz3mHiaOeQpo3GVI77gg3BUm4HQI5Uf2Yihj0YaCvpHKHwGcVLQMG3M?=
 =?us-ascii?Q?8H+Sey2wGBWfceJvZvrIVXyZtFKmj95rDzYiNMKCogZCBqmOErwALgMc/3zv?=
 =?us-ascii?Q?lbKhnprmRRerhRYBz44=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MW4PR12MB7165.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cd9377ab-cc12-4c8c-ea45-08dc445f06ff
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Mar 2024 19:43:30.9641
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: YKSD4FK5LMdM/MuO2vhxf7wUiJKk2PGwD+I4lI14Z+cE/jcHgPMDG+G0TNOs1y9dG9epROBRLCthtkykrBnANA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB7668

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 Lankhor=
st
> <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>; Ro=
bert
> 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
>=20
> Hi,
>=20
> 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>
>=20
> As I said previously, we don't want to have unused APIs, so this patch sh=
ould be in
> a good enough state to be merged if we want to merge the whole API.
>=20

This is understandable, but even having this API just reviewed by the commu=
nity will open the path forward for aligning AMD/Xilinx downstream DRM driv=
ers 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 =3D
> drm_atomic_get_new_crtc_state(state, crtc);
> > +	int ret;
> > +
> > +	if (!crtc_state->enable)
> > +		goto out;
> > +
> > +	ret =3D drm_atomic_helper_check_crtc_primary_plane(crtc_state);
> > +	if (ret)
> > +		return ret;
> > +
> > +out:
> > +	return drm_atomic_add_affected_planes(state, crtc); }
> > +
>=20
> [...]
>=20
> > +
> > +static u32 xlnx_tpg_crtc_select_output_bus_format(struct drm_crtc *crt=
c,
> > +						  struct drm_crtc_state
> *crtc_state,
> > +						  const u32 *in_bus_fmts,
> > +						  unsigned int
> num_in_bus_fmts) {
> > +	struct xlnx_tpg *tpg =3D crtc_to_tpg(crtc);
> > +	unsigned int i;
> > +
> > +	for (i =3D 0; i < num_in_bus_fmts; ++i)
> > +		if (in_bus_fmts[i] =3D=3D tpg->output_bus_format)
> > +			return tpg->output_bus_format;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_crtc_helper_funcs xlnx_tpg_crtc_helper_funcs =
=3D {
> > +	.mode_valid =3D xlnx_tpg_crtc_mode_valid,
> > +	.atomic_check =3D xlnx_tpg_crtc_check,
> > +	.atomic_enable =3D xlnx_tpg_crtc_enable,
> > +	.atomic_disable =3D xlnx_tpg_crtc_disable,
> > +	.select_output_bus_format =3D xlnx_tpg_crtc_select_output_bus_format,
> > +};
>=20
> From that code, it's not clear to me how the CRTC is going to be able to =
get what
> the format is.
>=20

It's coming from DT "bus-format" property. The idea is that this property w=
ill reflect the FPGA design variation synthesized.=20

> It looks like you hardcode it here, but what if there's several that woul=
d fit the
> bill? Is the CRTC expected to store it into its private structure?
>=20

It's impractical from the resources utilization point of view to support mu=
ltiple runtime options for FPGA-based CRTCs output signal format, so the bu=
s-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 ju=
st reuse
> whatever is in the state.
>=20

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