Hi Maxime, Thank you for the review. > -----Original Message----- > From: Maxime Ripard <mripard@xxxxxxxxxx> > Sent: Thursday, March 14, 2024 5:05 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@xxxxxxx> > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>; Maarten Lankhorst > <maarten.lankhorst@xxxxxxxxxxxxxxx>; Thomas Zimmermann > <tzimmermann@xxxxxxx>; David Airlie <airlied@xxxxxxxxx>; Daniel Vetter > <daniel@xxxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>; Andrzej Hajda > <andrzej.hajda@xxxxxxxxx>; Neil Armstrong <neil.armstrong@xxxxxxxxxx>; Robert > Foss <rfoss@xxxxxxxxxx>; Jonas Karlman <jonas@xxxxxxxxx>; Jernej Skrabec > <jernej.skrabec@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley > <conor+dt@xxxxxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > media@xxxxxxxxxxxxxxx > 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@xxxxxxx> > > 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