linux-kernel - RE: [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <MW4PR12MB7165A15E233957E3914AA297E6292@MW4PR12MB7165.namprd12.prod.outlook.com>
Date: Thu, 14 Mar 2024 19:43:30 +0000
From: "Klymenko, Anatoliy" <Anatoliy.Klymenko@....com>
To: Maxime Ripard <mripard@...nel.org>
CC: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, "Simek,
 Michal" <michal.simek@....com>, Andrzej Hajda <andrzej.hajda@...el.com>, Neil
 Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, Jonas
 Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, Rob
 Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>
Subject: RE: [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver

Hi Maxime,

Thank you for the review.

> -----Original Message-----
> From: Maxime Ripard <mripard@...nel.org>
> Sent: Thursday, March 14, 2024 5:05 AM
> To: Klymenko, Anatoliy <Anatoliy.Klymenko@....com>
> Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>; Maarten Lankhorst
> <maarten.lankhorst@...ux.intel.com>; Thomas Zimmermann
> <tzimmermann@...e.de>; David Airlie <airlied@...il.com>; Daniel Vetter
> <daniel@...ll.ch>; Simek, Michal <michal.simek@....com>; Andrzej Hajda
> <andrzej.hajda@...el.com>; Neil Armstrong <neil.armstrong@...aro.org>; Robert
> Foss <rfoss@...nel.org>; Jonas Karlman <jonas@...boo.se>; Jernej Skrabec
> <jernej.skrabec@...il.com>; Rob Herring <robh+dt@...nel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley
> <conor+dt@...nel.org>; Mauro Carvalho Chehab <mchehab@...nel.org>; dri-
> devel@...ts.freedesktop.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org; linux-
> media@...r.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@....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