Re: [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]