| Message ID | 20240312-dp-live-fmt-v2-0-a9c35dc5c50d@amd.com |
|---|---|
| Headers | show |
| Series | Setting live video input format for ZynqMP DPSUB | expand |
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
Hi Maxime, Thank you for the review. > -----Original Message----- > From: Maxime Ripard <mripard@kernel.org> > Sent: Thursday, March 14, 2024 5:05 AM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Thomas Zimmermann > <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>; Daniel Vetter > <daniel@ffwll.ch>; Simek, Michal <michal.simek@amd.com>; Andrzej Hajda > <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>; Robert > Foss <rfoss@kernel.org>; Jonas Karlman <jonas@kwiboo.se>; Jernej Skrabec > <jernej.skrabec@gmail.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley > <conor+dt@kernel.org>; Mauro Carvalho Chehab <mchehab@kernel.org>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.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@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. > 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
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
Hi Anatoliy, Thank you for the patch. On Tue, Mar 12, 2024 at 05:54:59PM -0700, Anatoliy Klymenko wrote: > Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register > layout. > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > index f92a006d5070..fa3935384834 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > @@ -165,10 +165,10 @@ > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 0x1 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 0x2 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4) > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4) > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8) > #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400 >
Hi Anatoliy, Thank you for the patch. On Tue, Mar 12, 2024 at 05:55:00PM -0700, Anatoliy Klymenko wrote: > DPSUB in bridge mode supports multiple input media bus formats. > > Announce the list of supported input media bus formats via > drm_bridge.atomic_get_input_bus_fmts callback. > Introduce a set of live input formats, supported by DPSUB. > Rename zynqmp_disp_layer_drm_formats() to zynqmp_disp_layer_formats() to > reflect semantics for both live and non-live layer format lists. > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > --- > drivers/gpu/drm/xlnx/zynqmp_disp.c | 67 +++++++++++++++++++++++++++++++++----- > drivers/gpu/drm/xlnx/zynqmp_disp.h | 4 +-- > drivers/gpu/drm/xlnx/zynqmp_dp.c | 26 +++++++++++++++ > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- > 4 files changed, 88 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c > index e6d26ef60e89..af851190f447 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > @@ -18,6 +18,7 @@ > #include <linux/dma/xilinx_dpdma.h> > #include <linux/dma-mapping.h> > #include <linux/dmaengine.h> > +#include <linux/media-bus-format.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/platform_device.h> > @@ -77,12 +78,16 @@ enum zynqmp_dpsub_layer_mode { > /** > * struct zynqmp_disp_format - Display subsystem format information > * @drm_fmt: DRM format (4CC) > + * @bus_fmt: Media bus format > * @buf_fmt: AV buffer format > * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats > * @sf: Scaling factors for color components > */ > struct zynqmp_disp_format { > - u32 drm_fmt; > + union { > + u32 drm_fmt; > + u32 bus_fmt; > + }; I'm not a big fan of the union, but I can live with it. > u32 buf_fmt; > bool swap; > const u32 *sf; > @@ -182,6 +187,12 @@ static const u32 scaling_factors_565[] = { > ZYNQMP_DISP_AV_BUF_5BIT_SF, > }; > > +static const u32 scaling_factors_666[] = { > + ZYNQMP_DISP_AV_BUF_6BIT_SF, > + ZYNQMP_DISP_AV_BUF_6BIT_SF, > + ZYNQMP_DISP_AV_BUF_6BIT_SF, > +}; > + > static const u32 scaling_factors_888[] = { > ZYNQMP_DISP_AV_BUF_8BIT_SF, > ZYNQMP_DISP_AV_BUF_8BIT_SF, > @@ -364,6 +375,36 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = { > }, > }; > > +/* List of live video layer formats */ > +static const struct zynqmp_disp_format avbuf_live_fmts[] = { > + { > + .bus_fmt = MEDIA_BUS_FMT_RGB666_1X18, > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 | > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, > + .sf = scaling_factors_666, > + }, { > + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X24, > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, > + .sf = scaling_factors_888, > + }, { > + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16, > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, > + .sf = scaling_factors_888, > + }, { > + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24, > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444, > + .sf = scaling_factors_888, > + }, { > + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20, > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 | > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, > + .sf = scaling_factors_101010, > + }, > +}; > + > static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg) > { > return readl(disp->avbuf.base + reg); > @@ -883,16 +924,17 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer, > } > > /** > - * zynqmp_disp_layer_drm_formats - Return the DRM formats supported by the layer > + * zynqmp_disp_layer_formats - Return DRM or media bus formats supported by > + * the layer > * @layer: The layer > * @num_formats: Pointer to the returned number of formats > * > - * Return: A newly allocated u32 array that stores all the DRM formats > + * Return: A newly allocated u32 array that stores all DRM or media bus formats > * supported by the layer. The number of formats in the array is returned > * through the num_formats argument. > */ > -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, > - unsigned int *num_formats) > +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer, > + unsigned int *num_formats) > { > unsigned int i; > u32 *formats; > @@ -1131,6 +1173,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp) > .num_channels = 1, > }, > }; > + static const struct zynqmp_disp_layer_info live_layer_info = { > + .formats = avbuf_live_fmts, > + .num_formats = ARRAY_SIZE(avbuf_live_fmts), > + .num_channels = 0, > + }; > > unsigned int i; > int ret; > @@ -1140,12 +1187,16 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp *disp) > > layer->id = i; > layer->disp = disp; > - layer->info = &layer_info[i]; > /* For now assume dpsub works in either live or non-live mode for both layers. While are it, could you please turn this into /* * For now assume dpsub works in either live or non-live mode for both layers. with a blank line just above it ? > * Hybrid mode is not supported yet. > */ > - layer->mode = disp->dpsub->dma_enabled ? ZYNQMP_DPSUB_LAYER_NONLIVE > - : ZYNQMP_DPSUB_LAYER_LIVE; > + if (disp->dpsub->dma_enabled) { > + layer->mode = ZYNQMP_DPSUB_LAYER_NONLIVE; > + layer->info = &layer_info[i]; > + } else { > + layer->mode = ZYNQMP_DPSUB_LAYER_LIVE; > + layer->info = &live_layer_info; > + } > > ret = zynqmp_disp_layer_request_dma(disp, layer); > if (ret) > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h > index 9b8b202224d9..88c285a12e23 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h > @@ -50,8 +50,8 @@ int zynqmp_disp_setup_clock(struct zynqmp_disp *disp, > void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp, > bool enable, u32 alpha); > > -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, > - unsigned int *num_formats); > +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer, > + unsigned int *num_formats); > void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer); > void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer); > void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c > index 04b6bcac3b07..a0d169ac48c0 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -1568,6 +1568,31 @@ static const struct drm_edid *zynqmp_dp_bridge_edid_read(struct drm_bridge *brid > return drm_edid_read_ddc(connector, &dp->aux.ddc); > } > > +static u32 * > +zynqmp_dp_bridge_get_input_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + struct zynqmp_dp *dp = bridge_to_dp(bridge); > + struct zynqmp_disp_layer *layer; > + enum zynqmp_dpsub_layer_id layer_id; > + > + if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO)) > + layer_id = ZYNQMP_DPSUB_LAYER_VID; > + else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX)) > + layer_id = ZYNQMP_DPSUB_LAYER_GFX; > + else { > + *num_input_fmts = 0; > + return NULL; > + } You need curly braces around all branches if one of them has multiple statements. Given that the above pattern is repeated twice already, a helper function that returns the layer pointer would be useful. Then you could simply write layer = ...(dp); if (!layer) { *num_input_fmts = 0; return NULL; } With these small issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + layer = dp->dpsub->layers[layer_id]; > + > + return zynqmp_disp_layer_formats(layer, num_input_fmts); > +} > + > static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = { > .attach = zynqmp_dp_bridge_attach, > .detach = zynqmp_dp_bridge_detach, > @@ -1580,6 +1605,7 @@ static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = { > .atomic_check = zynqmp_dp_bridge_atomic_check, > .detect = zynqmp_dp_bridge_detect, > .edid_read = zynqmp_dp_bridge_edid_read, > + .atomic_get_input_bus_fmts = zynqmp_dp_bridge_get_input_bus_fmts, > }; > > /* ----------------------------------------------------------------------------- > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c > index 43bf416b33d5..bf9fba01df0e 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c > @@ -152,7 +152,7 @@ static int zynqmp_dpsub_create_planes(struct zynqmp_dpsub *dpsub) > unsigned int num_formats; > u32 *formats; > > - formats = zynqmp_disp_layer_drm_formats(layer, &num_formats); > + formats = zynqmp_disp_layer_formats(layer, &num_formats); > if (!formats) > return -ENOMEM; > >
Hi Anatoliy, Thank you for the patch. On Tue, Mar 12, 2024 at 05:55:01PM -0700, Anatoliy Klymenko wrote: > Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer > context. This flag signals whether the driver runs in DRM CRTC or DRM > bridge mode, assuming that all display layers share the same live or > non-live mode of operation. Using per-layer mode instead of global flag > will siplify future support of the hybrid scenario. s/siplify/simplify/ > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > --- > drivers/gpu/drm/xlnx/zynqmp_disp.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c > index af851190f447..dd48fa60fa9a 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > @@ -975,7 +975,7 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer) > { > unsigned int i; > > - if (layer->disp->dpsub->dma_enabled) { > + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) { > for (i = 0; i < layer->drm_fmt->num_planes; i++) > dmaengine_terminate_sync(layer->dmas[i].chan); > } > @@ -1001,7 +1001,7 @@ void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, > > zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt); > > - if (!layer->disp->dpsub->dma_enabled) > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) > return; > > /* > @@ -1039,7 +1039,7 @@ int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer, > const struct drm_format_info *info = layer->drm_fmt; > unsigned int i; > > - if (!layer->disp->dpsub->dma_enabled) > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) > return 0; The above changes look nice. > > for (i = 0; i < info->num_planes; i++) { > @@ -1089,7 +1089,7 @@ static void zynqmp_disp_layer_release_dma(struct zynqmp_disp *disp, > { > unsigned int i; > > - if (!layer->info || !disp->dpsub->dma_enabled) > + if (!layer->info) This, however, doesn't seem right, as this function is called unconditionally from the remove path. The change below seems weird too. If I'm missing something, it should at least be explained in the commit message. > return; > > for (i = 0; i < layer->info->num_channels; i++) { > @@ -1132,9 +1132,6 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp *disp, > unsigned int i; > int ret; > > - if (!disp->dpsub->dma_enabled) > - return 0; > - > for (i = 0; i < layer->info->num_channels; i++) { > struct zynqmp_disp_layer_dma *dma = &layer->dmas[i]; > char dma_channel_name[16]; >
Hi Anatoliy, Thank you for the patch. On Tue, Mar 12, 2024 at 05:55:02PM -0700, Anatoliy Klymenko wrote: > Program live video input format according to selected media bus format. > > In the bridge mode of operation, DPSUB is connected to FPGA CRTC which > almost certainly supports a single media bus format as its output. Expect > this to be delivered via the new bridge atomic state. Program DPSUB > registers accordingly. No line breaks within paragraphs. Add a blank line if you want to paragraphs, remove the line break otherwise. > Update zynqmp_disp_layer_set_format() API to fit both live and non-live > layer types. > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > --- > drivers/gpu/drm/xlnx/zynqmp_disp.c | 93 +++++++++++++++++++++++++++++++------- > drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +- > drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++++-- > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- > 4 files changed, 87 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c > index dd48fa60fa9a..0cacd597f4b8 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > @@ -383,7 +383,7 @@ static const struct zynqmp_disp_format avbuf_live_fmts[] = { > ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, > .sf = scaling_factors_666, > }, { > - .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X24, > + .bus_fmt = MEDIA_BUS_FMT_RBG888_1X24, Does this belong to a previous patch ? > .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, > .sf = scaling_factors_888, > @@ -433,19 +433,28 @@ static void zynqmp_disp_avbuf_set_format(struct zynqmp_disp *disp, > const struct zynqmp_disp_format *fmt) > { > unsigned int i; > - u32 val; > + u32 val, reg; > > - val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT); > - val &= zynqmp_disp_layer_is_video(layer) > - ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK > - : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK; > - val |= fmt->buf_fmt; > - zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val); > + layer->disp_fmt = fmt; > + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) { > + reg = ZYNQMP_DISP_AV_BUF_FMT; > + val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT); > + val &= zynqmp_disp_layer_is_video(layer) > + ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK > + : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK; > + val |= fmt->buf_fmt; > + } else { > + reg = zynqmp_disp_layer_is_video(layer) > + ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG > + : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG; > + val = fmt->buf_fmt; > + } > + zynqmp_disp_avbuf_write(disp, reg, val); > > for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) { > - unsigned int reg = zynqmp_disp_layer_is_video(layer) > - ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i) > - : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i); > + reg = zynqmp_disp_layer_is_video(layer) > + ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i) > + : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i); > > zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]); > } > @@ -984,23 +993,73 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer) > zynqmp_disp_blend_layer_disable(layer->disp, layer); > } > > +struct zynqmp_disp_bus_to_drm { > + u32 bus_fmt; > + u32 drm_fmt; > +}; > + > +/** > + * zynqmp_disp_reference_drm_format - Get reference DRM format corresponding > + * to the given media bus format. > + * @bus_format: Media bus format > + * > + * Map media bus format to some DRM format that represents the same color space > + * and chroma subsampling as the @bus_format video signal. These DRM format > + * properties are required to program the blender. > + * > + * Return: DRM format code corresponding to @bus_format > + */ > +static u32 zynqmp_disp_reference_drm_format(u32 bus_format) > +{ > + static const struct zynqmp_disp_bus_to_drm format_map[] = { > + { > + .bus_fmt = MEDIA_BUS_FMT_RGB666_1X18, > + .drm_fmt = DRM_FORMAT_RGB565, > + }, { > + .bus_fmt = MEDIA_BUS_FMT_RBG888_1X24, > + .drm_fmt = DRM_FORMAT_RGB888, > + }, { > + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16, > + .drm_fmt = DRM_FORMAT_YUV422, > + }, { > + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24, > + .drm_fmt = DRM_FORMAT_YUV444, > + }, { > + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20, > + .drm_fmt = DRM_FORMAT_P210, > + }, Hmmmm... Looking at this, I think you should have both bus_fmt and drm_fmt in zynqmp_disp_format. It seems it would simplify the code flow and make things more readable. If you would like me to give it a try, please let me know. > + }; > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(format_map); ++i) > + if (format_map[i].bus_fmt == bus_format) > + return format_map[i].drm_fmt; > + > + return DRM_FORMAT_INVALID; > +} > + > /** > * zynqmp_disp_layer_set_format - Set the layer format > * @layer: The layer > - * @info: The format info > + * @drm_or_bus_format: DRM or media bus format > * > * Set the format for @layer to @info. The layer must be disabled. > */ > void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, > - const struct drm_format_info *info) > + u32 drm_or_bus_format) > { > unsigned int i; > > - layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format); > - layer->drm_fmt = info; > - > + layer->disp_fmt = zynqmp_disp_layer_find_format(layer, drm_or_bus_format); > zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt); > > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) > + drm_or_bus_format = zynqmp_disp_reference_drm_format(drm_or_bus_format); > + > + layer->drm_fmt = drm_format_info(drm_or_bus_format); > + if (!layer->drm_fmt) > + return; > + > if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) > return; > > @@ -1008,7 +1067,7 @@ void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, > * Set pconfig for each DMA channel to indicate they're part of a > * video group. > */ > - for (i = 0; i < info->num_planes; i++) { > + for (i = 0; i < layer->drm_fmt->num_planes; i++) { > struct zynqmp_disp_layer_dma *dma = &layer->dmas[i]; > struct xilinx_dpdma_peripheral_config pconfig = { > .video_group = true, > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h > index 88c285a12e23..9f9a5f50ffbc 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h > @@ -55,7 +55,7 @@ u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer, > void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer); > void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer); > void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, > - const struct drm_format_info *info); > + u32 drm_or_bus_format); > int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer, > struct drm_plane_state *state); > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c > index a0d169ac48c0..fc6b1d783c28 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp, > { > enum zynqmp_dpsub_layer_id layer_id; > struct zynqmp_disp_layer *layer; > - const struct drm_format_info *info; > + struct drm_bridge_state *bridge_state; > + u32 bus_fmt; > > if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO)) > layer_id = ZYNQMP_DPSUB_LAYER_VID; > @@ -1291,10 +1292,14 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp, > return; > > layer = dp->dpsub->layers[layer_id]; > + bridge_state = drm_atomic_get_new_bridge_state(old_bridge_state->base.state, > + old_bridge_state->bridge); > + if (WARN_ON(!bridge_state)) > + return; > + > + bus_fmt = bridge_state->input_bus_cfg.format; > + zynqmp_disp_layer_set_format(layer, bus_fmt); > > - /* TODO: Make the format configurable. */ > - info = drm_format_info(DRM_FORMAT_YUV422); > - zynqmp_disp_layer_set_format(layer, info); > zynqmp_disp_layer_enable(layer); > > if (layer_id == ZYNQMP_DPSUB_LAYER_GFX) > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c > index bf9fba01df0e..d96b3f3f2e3a 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c > @@ -111,7 +111,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane, > if (old_state->fb) > zynqmp_disp_layer_disable(layer); > > - zynqmp_disp_layer_set_format(layer, new_state->fb->format); > + zynqmp_disp_layer_set_format(layer, new_state->fb->format->format); > } > > zynqmp_disp_layer_update(layer, new_state); >
Hi Laurent, Thanks a lot for the review. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Monday, March 18, 2024 5:05 PM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, Michal > <michal.simek@amd.com>; Andrzej Hajda <andrzej.hajda@intel.com>; Neil > Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas > Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Rob > Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Mauro Carvalho Chehab <mchehab@kernel.org>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v2 3/8] drm: xlnx: zynqmp_dpsub: Anounce supported input > formats > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > Hi Anatoliy, > > Thank you for the patch. > > On Tue, Mar 12, 2024 at 05:55:00PM -0700, Anatoliy Klymenko wrote: > > DPSUB in bridge mode supports multiple input media bus formats. > > > > Announce the list of supported input media bus formats via > > drm_bridge.atomic_get_input_bus_fmts callback. > > Introduce a set of live input formats, supported by DPSUB. > > Rename zynqmp_disp_layer_drm_formats() to zynqmp_disp_layer_formats() > > to reflect semantics for both live and non-live layer format lists. > > > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > > --- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 67 > > +++++++++++++++++++++++++++++++++----- > > drivers/gpu/drm/xlnx/zynqmp_disp.h | 4 +-- > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 26 +++++++++++++++ > > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- > > 4 files changed, 88 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > index e6d26ef60e89..af851190f447 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > @@ -18,6 +18,7 @@ > > #include <linux/dma/xilinx_dpdma.h> > > #include <linux/dma-mapping.h> > > #include <linux/dmaengine.h> > > +#include <linux/media-bus-format.h> > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/platform_device.h> > > @@ -77,12 +78,16 @@ enum zynqmp_dpsub_layer_mode { > > /** > > * struct zynqmp_disp_format - Display subsystem format information > > * @drm_fmt: DRM format (4CC) > > + * @bus_fmt: Media bus format > > * @buf_fmt: AV buffer format > > * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats > > * @sf: Scaling factors for color components > > */ > > struct zynqmp_disp_format { > > - u32 drm_fmt; > > + union { > > + u32 drm_fmt; > > + u32 bus_fmt; > > + }; > > I'm not a big fan of the union, but I can live with it. > I'm trying to represent the duality of the layer formats - non-live described by the DRM fourcc, and live by the bus format. > > u32 buf_fmt; > > bool swap; > > const u32 *sf; > > @@ -182,6 +187,12 @@ static const u32 scaling_factors_565[] = { > > ZYNQMP_DISP_AV_BUF_5BIT_SF, > > }; > > > > +static const u32 scaling_factors_666[] = { > > + ZYNQMP_DISP_AV_BUF_6BIT_SF, > > + ZYNQMP_DISP_AV_BUF_6BIT_SF, > > + ZYNQMP_DISP_AV_BUF_6BIT_SF, > > +}; > > + > > static const u32 scaling_factors_888[] = { > > ZYNQMP_DISP_AV_BUF_8BIT_SF, > > ZYNQMP_DISP_AV_BUF_8BIT_SF, > > @@ -364,6 +375,36 @@ static const struct zynqmp_disp_format > avbuf_gfx_fmts[] = { > > }, > > }; > > > > +/* List of live video layer formats */ static const struct > > +zynqmp_disp_format avbuf_live_fmts[] = { > > + { > > + .bus_fmt = MEDIA_BUS_FMT_RGB666_1X18, > > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 | > > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, > > + .sf = scaling_factors_666, > > + }, { > > + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X24, > > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, > > + .sf = scaling_factors_888, > > + }, { > > + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16, > > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, > > + .sf = scaling_factors_888, > > + }, { > > + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24, > > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444, > > + .sf = scaling_factors_888, > > + }, { > > + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20, > > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 | > > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, > > + .sf = scaling_factors_101010, > > + }, > > +}; > > + > > static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg) > > { > > return readl(disp->avbuf.base + reg); @@ -883,16 +924,17 @@ > > zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer, } > > > > /** > > - * zynqmp_disp_layer_drm_formats - Return the DRM formats supported > > by the layer > > + * zynqmp_disp_layer_formats - Return DRM or media bus formats > > + supported by > > + * the layer > > * @layer: The layer > > * @num_formats: Pointer to the returned number of formats > > * > > - * Return: A newly allocated u32 array that stores all the DRM > > formats > > + * Return: A newly allocated u32 array that stores all DRM or media > > + bus formats > > * supported by the layer. The number of formats in the array is returned > > * through the num_formats argument. > > */ > > -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, > > - unsigned int *num_formats) > > +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer, > > + unsigned int *num_formats) > > { > > unsigned int i; > > u32 *formats; > > @@ -1131,6 +1173,11 @@ static int zynqmp_disp_create_layers(struct > zynqmp_disp *disp) > > .num_channels = 1, > > }, > > }; > > + static const struct zynqmp_disp_layer_info live_layer_info = { > > + .formats = avbuf_live_fmts, > > + .num_formats = ARRAY_SIZE(avbuf_live_fmts), > > + .num_channels = 0, > > + }; > > > > unsigned int i; > > int ret; > > @@ -1140,12 +1187,16 @@ static int zynqmp_disp_create_layers(struct > > zynqmp_disp *disp) > > > > layer->id = i; > > layer->disp = disp; > > - layer->info = &layer_info[i]; > > /* For now assume dpsub works in either live or non-live mode for both > layers. > > While are it, could you please turn this into > > /* > * For now assume dpsub works in either live or non-live mode for both > layers. > > with a blank line just above it ? > Sure, thank you. > > * Hybrid mode is not supported yet. > > */ > > - layer->mode = disp->dpsub->dma_enabled ? > ZYNQMP_DPSUB_LAYER_NONLIVE > > - : ZYNQMP_DPSUB_LAYER_LIVE; > > + if (disp->dpsub->dma_enabled) { > > + layer->mode = ZYNQMP_DPSUB_LAYER_NONLIVE; > > + layer->info = &layer_info[i]; > > + } else { > > + layer->mode = ZYNQMP_DPSUB_LAYER_LIVE; > > + layer->info = &live_layer_info; > > + } > > > > ret = zynqmp_disp_layer_request_dma(disp, layer); > > if (ret) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h > > b/drivers/gpu/drm/xlnx/zynqmp_disp.h > > index 9b8b202224d9..88c285a12e23 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h > > @@ -50,8 +50,8 @@ int zynqmp_disp_setup_clock(struct zynqmp_disp > > *disp, void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp, > > bool enable, u32 alpha); > > > > -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, > > - unsigned int *num_formats); > > +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer, > > + unsigned int *num_formats); > > void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer); void > > zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer); void > > zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, diff > > --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > index 04b6bcac3b07..a0d169ac48c0 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > @@ -1568,6 +1568,31 @@ static const struct drm_edid > *zynqmp_dp_bridge_edid_read(struct drm_bridge *brid > > return drm_edid_read_ddc(connector, &dp->aux.ddc); } > > > > +static u32 * > > +zynqmp_dp_bridge_get_input_bus_fmts(struct drm_bridge *bridge, > > + struct drm_bridge_state *bridge_state, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state, > > + u32 output_fmt, > > + unsigned int *num_input_fmts) { > > + struct zynqmp_dp *dp = bridge_to_dp(bridge); > > + struct zynqmp_disp_layer *layer; > > + enum zynqmp_dpsub_layer_id layer_id; > > + > > + if (dp->dpsub->connected_ports & > BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO)) > > + layer_id = ZYNQMP_DPSUB_LAYER_VID; > > + else if (dp->dpsub->connected_ports & > BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX)) > > + layer_id = ZYNQMP_DPSUB_LAYER_GFX; > > + else { > > + *num_input_fmts = 0; > > + return NULL; > > + } > > You need curly braces around all branches if one of them has multiple statements. > Hmm, checkpatch gave me a CHECK hint on that. How did I miss it? Will fix. Thank you. > Given that the above pattern is repeated twice already, a helper function that > returns the layer pointer would be useful. Then you could simply write > > layer = ...(dp); > if (!layer) { > *num_input_fmts = 0; > return NULL; > } > Makes sense - I'll fix that. > With these small issues addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + layer = dp->dpsub->layers[layer_id]; > > + > > + return zynqmp_disp_layer_formats(layer, num_input_fmts); } > > + > > static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = { > > .attach = zynqmp_dp_bridge_attach, > > .detach = zynqmp_dp_bridge_detach, @@ -1580,6 +1605,7 @@ static > > const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = { > > .atomic_check = zynqmp_dp_bridge_atomic_check, > > .detect = zynqmp_dp_bridge_detect, > > .edid_read = zynqmp_dp_bridge_edid_read, > > + .atomic_get_input_bus_fmts = > > + zynqmp_dp_bridge_get_input_bus_fmts, > > }; > > > > /* > > ---------------------------------------------------------------------- > > ------- diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c > > b/drivers/gpu/drm/xlnx/zynqmp_kms.c > > index 43bf416b33d5..bf9fba01df0e 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c > > @@ -152,7 +152,7 @@ static int zynqmp_dpsub_create_planes(struct > zynqmp_dpsub *dpsub) > > unsigned int num_formats; > > u32 *formats; > > > > - formats = zynqmp_disp_layer_drm_formats(layer, &num_formats); > > + formats = zynqmp_disp_layer_formats(layer, > > + &num_formats); > > if (!formats) > > return -ENOMEM; > > > > > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Thank you for the review. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Monday, March 18, 2024 5:13 PM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, Michal > <michal.simek@amd.com>; Andrzej Hajda <andrzej.hajda@intel.com>; Neil > Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas > Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Rob > Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Mauro Carvalho Chehab <mchehab@kernel.org>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global > flag > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > Hi Anatoliy, > > Thank you for the patch. > > On Tue, Mar 12, 2024 at 05:55:01PM -0700, Anatoliy Klymenko wrote: > > Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer > > context. This flag signals whether the driver runs in DRM CRTC or DRM > > bridge mode, assuming that all display layers share the same live or > > non-live mode of operation. Using per-layer mode instead of global > > flag will siplify future support of the hybrid scenario. > > s/siplify/simplify/ > > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > > --- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > index af851190f447..dd48fa60fa9a 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > @@ -975,7 +975,7 @@ void zynqmp_disp_layer_disable(struct > > zynqmp_disp_layer *layer) { > > unsigned int i; > > > > - if (layer->disp->dpsub->dma_enabled) { > > + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) { > > for (i = 0; i < layer->drm_fmt->num_planes; i++) > > dmaengine_terminate_sync(layer->dmas[i].chan); > > } > > @@ -1001,7 +1001,7 @@ void zynqmp_disp_layer_set_format(struct > > zynqmp_disp_layer *layer, > > > > zynqmp_disp_avbuf_set_format(layer->disp, layer, > > layer->disp_fmt); > > > > - if (!layer->disp->dpsub->dma_enabled) > > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) > > return; > > > > /* > > @@ -1039,7 +1039,7 @@ int zynqmp_disp_layer_update(struct > zynqmp_disp_layer *layer, > > const struct drm_format_info *info = layer->drm_fmt; > > unsigned int i; > > > > - if (!layer->disp->dpsub->dma_enabled) > > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) > > return 0; > > The above changes look nice. > > > > > for (i = 0; i < info->num_planes; i++) { @@ -1089,7 +1089,7 @@ > > static void zynqmp_disp_layer_release_dma(struct zynqmp_disp *disp, { > > unsigned int i; > > > > - if (!layer->info || !disp->dpsub->dma_enabled) > > + if (!layer->info) > > This, however, doesn't seem right, as this function is called unconditionally from > the remove path. The change below seems weird too. > If I'm missing something, it should at least be explained in the commit message. > Actually, this whole condition should be removed, as now we're setting layer.info for all types of layers. On top of that, we're setting the number of DMA channels to zero for the live layers, which in turn prevents any DMA channel initialization or release. You are right - that probably should be mentioned explicitly in the commit message. I'll update it. > > return; > > > > for (i = 0; i < layer->info->num_channels; i++) { @@ -1132,9 > > +1132,6 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp > *disp, > > unsigned int i; > > int ret; > > > > - if (!disp->dpsub->dma_enabled) > > - return 0; > > - > > for (i = 0; i < layer->info->num_channels; i++) { > > struct zynqmp_disp_layer_dma *dma = &layer->dmas[i]; > > char dma_channel_name[16]; > > > > -- > Regards, > > Laurent Pinchart Thank you, Anatoliy
Hi Laurent, Thanks a lot for your review. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Monday, March 18, 2024 5:35 PM > To: Klymenko, Anatoliy <Anatoliy.Klymenko@amd.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard > <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Simek, Michal > <michal.simek@amd.com>; Andrzej Hajda <andrzej.hajda@intel.com>; Neil > Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Jonas > Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Rob > Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Mauro Carvalho Chehab <mchehab@kernel.org>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > Hi Anatoliy, > > Thank you for the patch. > > On Tue, Mar 12, 2024 at 05:55:02PM -0700, Anatoliy Klymenko wrote: > > Program live video input format according to selected media bus format. > > > > In the bridge mode of operation, DPSUB is connected to FPGA CRTC which > > almost certainly supports a single media bus format as its output. > > Expect this to be delivered via the new bridge atomic state. Program > > DPSUB registers accordingly. > > No line breaks within paragraphs. Add a blank line if you want to paragraphs, > remove the line break otherwise. > Got it - will fix. Thank you. > > Update zynqmp_disp_layer_set_format() API to fit both live and > > non-live layer types. > > > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > > --- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 93 > > +++++++++++++++++++++++++++++++------- > > drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +- > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++++-- > > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- > > 4 files changed, 87 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > index dd48fa60fa9a..0cacd597f4b8 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > @@ -383,7 +383,7 @@ static const struct zynqmp_disp_format > avbuf_live_fmts[] = { > > ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, > > .sf = scaling_factors_666, > > }, { > > - .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X24, > > + .bus_fmt = MEDIA_BUS_FMT_RBG888_1X24, > > Does this belong to a previous patch ? Yep, slipped between my fingers during the rebase. I will update this in the next version. > > > .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > > ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB, > > .sf = scaling_factors_888, > > @@ -433,19 +433,28 @@ static void zynqmp_disp_avbuf_set_format(struct > zynqmp_disp *disp, > > const struct zynqmp_disp_format > > *fmt) { > > unsigned int i; > > - u32 val; > > + u32 val, reg; > > > > - val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT); > > - val &= zynqmp_disp_layer_is_video(layer) > > - ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK > > - : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK; > > - val |= fmt->buf_fmt; > > - zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val); > > + layer->disp_fmt = fmt; > > + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) { > > + reg = ZYNQMP_DISP_AV_BUF_FMT; > > + val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT); > > + val &= zynqmp_disp_layer_is_video(layer) > > + ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK > > + : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK; > > + val |= fmt->buf_fmt; > > + } else { > > + reg = zynqmp_disp_layer_is_video(layer) > > + ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG > > + : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG; > > + val = fmt->buf_fmt; > > + } > > + zynqmp_disp_avbuf_write(disp, reg, val); > > > > for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) { > > - unsigned int reg = zynqmp_disp_layer_is_video(layer) > > - ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i) > > - : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i); > > + reg = zynqmp_disp_layer_is_video(layer) > > + ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i) > > + : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i); > > > > zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]); > > } > > @@ -984,23 +993,73 @@ void zynqmp_disp_layer_disable(struct > zynqmp_disp_layer *layer) > > zynqmp_disp_blend_layer_disable(layer->disp, layer); } > > > > +struct zynqmp_disp_bus_to_drm { > > + u32 bus_fmt; > > + u32 drm_fmt; > > +}; > > + > > +/** > > + * zynqmp_disp_reference_drm_format - Get reference DRM format > > +corresponding > > + * to the given media bus format. > > + * @bus_format: Media bus format > > + * > > + * Map media bus format to some DRM format that represents the same > > +color space > > + * and chroma subsampling as the @bus_format video signal. These DRM > > +format > > + * properties are required to program the blender. > > + * > > + * Return: DRM format code corresponding to @bus_format */ static > > +u32 zynqmp_disp_reference_drm_format(u32 bus_format) { > > + static const struct zynqmp_disp_bus_to_drm format_map[] = { > > + { > > + .bus_fmt = MEDIA_BUS_FMT_RGB666_1X18, > > + .drm_fmt = DRM_FORMAT_RGB565, > > + }, { > > + .bus_fmt = MEDIA_BUS_FMT_RBG888_1X24, > > + .drm_fmt = DRM_FORMAT_RGB888, > > + }, { > > + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16, > > + .drm_fmt = DRM_FORMAT_YUV422, > > + }, { > > + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24, > > + .drm_fmt = DRM_FORMAT_YUV444, > > + }, { > > + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20, > > + .drm_fmt = DRM_FORMAT_P210, > > + }, > > Hmmmm... Looking at this, I think you should have both bus_fmt and drm_fmt in > zynqmp_disp_format. It seems it would simplify the code flow and make things > more readable. If you would like me to give it a try, please let me know. > I was trying to preserve symmetry in struct zynqmp_disp_format, having only relevant information there for both layer modes, but this ended up in this ugly lookup. Probably the proper fix would be adding blender register values to this struct, but that would be a rather big change. So, I think your proposal is a good compromise. This will require a few layer.mode checks though. Thank you. > > + }; > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(format_map); ++i) > > + if (format_map[i].bus_fmt == bus_format) > > + return format_map[i].drm_fmt; > > + > > + return DRM_FORMAT_INVALID; > > +} > > + > > /** > > * zynqmp_disp_layer_set_format - Set the layer format > > * @layer: The layer > > - * @info: The format info > > + * @drm_or_bus_format: DRM or media bus format > > * > > * Set the format for @layer to @info. The layer must be disabled. > > */ > > void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, > > - const struct drm_format_info *info) > > + u32 drm_or_bus_format) > > { > > unsigned int i; > > > > - layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format); > > - layer->drm_fmt = info; > > - > > + layer->disp_fmt = zynqmp_disp_layer_find_format(layer, > > + drm_or_bus_format); > > zynqmp_disp_avbuf_set_format(layer->disp, layer, > > layer->disp_fmt); > > > > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) > > + drm_or_bus_format = > > + zynqmp_disp_reference_drm_format(drm_or_bus_format); > > + > > + layer->drm_fmt = drm_format_info(drm_or_bus_format); > > + if (!layer->drm_fmt) > > + return; > > + > > if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE) > > return; > > > > @@ -1008,7 +1067,7 @@ void zynqmp_disp_layer_set_format(struct > zynqmp_disp_layer *layer, > > * Set pconfig for each DMA channel to indicate they're part of a > > * video group. > > */ > > - for (i = 0; i < info->num_planes; i++) { > > + for (i = 0; i < layer->drm_fmt->num_planes; i++) { > > struct zynqmp_disp_layer_dma *dma = &layer->dmas[i]; > > struct xilinx_dpdma_peripheral_config pconfig = { > > .video_group = true, diff --git > > a/drivers/gpu/drm/xlnx/zynqmp_disp.h > > b/drivers/gpu/drm/xlnx/zynqmp_disp.h > > index 88c285a12e23..9f9a5f50ffbc 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h > > @@ -55,7 +55,7 @@ u32 *zynqmp_disp_layer_formats(struct > > zynqmp_disp_layer *layer, void zynqmp_disp_layer_enable(struct > > zynqmp_disp_layer *layer); void zynqmp_disp_layer_disable(struct > > zynqmp_disp_layer *layer); void zynqmp_disp_layer_set_format(struct > zynqmp_disp_layer *layer, > > - const struct drm_format_info *info); > > + u32 drm_or_bus_format); > > int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer, > > struct drm_plane_state *state); > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > index a0d169ac48c0..fc6b1d783c28 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > @@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct > > zynqmp_dp *dp, { > > enum zynqmp_dpsub_layer_id layer_id; > > struct zynqmp_disp_layer *layer; > > - const struct drm_format_info *info; > > + struct drm_bridge_state *bridge_state; > > + u32 bus_fmt; > > > > if (dp->dpsub->connected_ports & > BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO)) > > layer_id = ZYNQMP_DPSUB_LAYER_VID; @@ -1291,10 +1292,14 > > @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp, > > return; > > > > layer = dp->dpsub->layers[layer_id]; > > + bridge_state = drm_atomic_get_new_bridge_state(old_bridge_state- > >base.state, > > + old_bridge_state->bridge); > > + if (WARN_ON(!bridge_state)) > > + return; > > + > > + bus_fmt = bridge_state->input_bus_cfg.format; > > + zynqmp_disp_layer_set_format(layer, bus_fmt); > > > > - /* TODO: Make the format configurable. */ > > - info = drm_format_info(DRM_FORMAT_YUV422); > > - zynqmp_disp_layer_set_format(layer, info); > > zynqmp_disp_layer_enable(layer); > > > > if (layer_id == ZYNQMP_DPSUB_LAYER_GFX) diff --git > > a/drivers/gpu/drm/xlnx/zynqmp_kms.c > > b/drivers/gpu/drm/xlnx/zynqmp_kms.c > > index bf9fba01df0e..d96b3f3f2e3a 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c > > @@ -111,7 +111,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct > drm_plane *plane, > > if (old_state->fb) > > zynqmp_disp_layer_disable(layer); > > > > - zynqmp_disp_layer_set_format(layer, new_state->fb->format); > > + zynqmp_disp_layer_set_format(layer, > > + new_state->fb->format->format); > > } > > > > zynqmp_disp_layer_update(layer, new_state); > > > > -- > Regards, > > Laurent Pinchart Thank you, Anatoliy
Implement live video input format setting for ZynqMP DPSUB. ZynqMP DPSUB can operate in 2 modes: DMA-based and live. In the live mode, DPSUB receives a live video signal from FPGA-based CRTC. DPSUB acts as a DRM encoder bridge in such a scenario. To properly tune into the incoming video signal, DPSUB should be programmed with the proper media bus format. This patch series addresses this task. Patch 1/8: Set the DPSUB layer mode of operation prior to enabling the layer. Allows to use layer operational mode before its enablement. Patch 2/8: Update some IP register defines. Patch 3/8: Announce supported input media bus formats via drm_bridge_funcs.atomic_get_input_bus_fmts callback. Patch 4/8: Minimize usage of a global flag. Minor improvement. Patch 5/8: Program DPSUB live video input format based on selected bus config in the new atomic bridge state. Patch 6/8: New optional CRTC atomic helper proposal that will allow CRTC to participate in DRM bridge chain format negotiation and impose format restrictions. Incorporate this callback into the DRM bridge format negotiation process. Patch 7/8: DT bindings documentation for Video Timing Controller and Test Pattern Generator IPs. Patch 8/8: Reference FPGA CRTC driver based on AMD/Xilinx Test Pattern Generator (TPG) IP. Add driver for the AMD/Xilinx Video Timing Controller (VTC), which supplements TPG. To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> To: Maxime Ripard <mripard@kernel.org> To: Thomas Zimmermann <tzimmermann@suse.de> To: David Airlie <airlied@gmail.com> To: Daniel Vetter <daniel@ffwll.ch> To: Michal Simek <michal.simek@amd.com> To: Andrzej Hajda <andrzej.hajda@intel.com> To: Neil Armstrong <neil.armstrong@linaro.org> To: Robert Foss <rfoss@kernel.org> To: Jonas Karlman <jonas@kwiboo.se> To: Jernej Skrabec <jernej.skrabec@gmail.com> To: Rob Herring <robh+dt@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Conor Dooley <conor+dt@kernel.org> To: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-media@vger.kernel.org Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> Changes in v2: - Factor out register defines update into separate patch. - Add some improvements minimizing ithe usage of a global flag. - Reuse existing format setting API instead of introducing new versions. - Add warning around NULL check on new bridge state within atomic enable callback. - Add drm_helper_crtc_select_output_bus_format() that wraps drm_crtc_helper_funcs.select_output_bus_format(). - Update API comments per review recommendations. - Address some minor review comments. - Add reference CRTC driver that demonstrates the usage of the proposed drm_crtc_helper_funcs.select_output_bus_format() API. - Link to v1: https://lore.kernel.org/r/20240226-dp-live-fmt-v1-0-b78c3f69c9d8@amd.com --- Anatoliy Klymenko (8): drm: xlnx: zynqmp_dpsub: Set layer mode during creation drm: xlnx: zynqmp_dpsub: Update live format defines drm: xlnx: zynqmp_dpsub: Anounce supported input formats drm: xlnx: zynqmp_dpsub: Minimize usage of global flag drm: xlnx: zynqmp_dpsub: Set input live format drm/atomic-helper: Add select_output_bus_format callback dt-bindings: xlnx: Add VTC and TPG bindings drm: xlnx: Intoduce TPG CRTC driver .../bindings/display/xlnx/xlnx,v-tpg.yaml | 87 +++ .../devicetree/bindings/display/xlnx/xlnx,vtc.yaml | 65 ++ drivers/gpu/drm/drm_bridge.c | 14 +- drivers/gpu/drm/drm_crtc_helper.c | 36 + drivers/gpu/drm/xlnx/Kconfig | 21 + drivers/gpu/drm/xlnx/Makefile | 4 + drivers/gpu/drm/xlnx/xlnx_tpg.c | 854 +++++++++++++++++++++ drivers/gpu/drm/xlnx/xlnx_vtc.c | 452 +++++++++++ drivers/gpu/drm/xlnx/xlnx_vtc.h | 101 +++ drivers/gpu/drm/xlnx/xlnx_vtc_list.c | 160 ++++ drivers/gpu/drm/xlnx/zynqmp_disp.c | 187 ++++- drivers/gpu/drm/xlnx/zynqmp_disp.h | 19 +- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 41 +- drivers/gpu/drm/xlnx/zynqmp_kms.c | 6 +- include/drm/drm_crtc_helper.h | 5 + include/drm/drm_modeset_helper_vtables.h | 30 + include/dt-bindings/media/media-bus-format.h | 177 +++++ 18 files changed, 2204 insertions(+), 63 deletions(-) --- base-commit: bfa4437fd3938ae2e186e7664b2db65bb8775670 change-id: 20240226-dp-live-fmt-6415773b5a68 Best regards,