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: <20240314-esoteric-delicate-sidewinder-5dc4db@houat>
Date: Thu, 14 Mar 2024 13:05:02 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Anatoliy Klymenko <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>, 
	Michal Simek <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.

> +/* -----------------------------------------------------------------------------
> + * 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 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?

If so, I would expect it to be in the crtc state, and atomic_enable to
just reuse whatever is in the state.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)