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

[Maxime Ripard]

On Thu, Mar 14, 2024 at 07:43:30PM +0000, Klymenko, Anatoliy wrote:
> > > +/*
> > > +---------------------------------------------------------------------
> > > +--------
> > > + * 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.

Sure, but we're still discussing an API to accomodate your use-case
here. Your usecase is one thing, but the API has to be cover all cases,
and there's definitely some CRTCs out there that support multiple output
formats that would benefit from that API.

And it would mimic the drm_bridge API, which is a nice consistency
bonus.

Maxime

Attachment: signature.asc
Description: PGP signature