| Message ID | 20240226-dp-live-fmt-v1-2-b78c3f69c9d8@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Setting live video input format for ZynqMP DPSUB | expand |
Hi Anatoliy, Thank you for the patch. On Mon, Feb 26, 2024 at 08:44:43PM -0800, 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. > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > --- > drivers/gpu/drm/xlnx/zynqmp_disp.c | 37 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/xlnx/zynqmp_disp.h | 10 ++++++++++ > drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 + > 3 files changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c > index e6d26ef60e89..ee99aad915ba 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,14 @@ 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; > + u32 bus_fmt; > u32 buf_fmt; > bool swap; > const u32 *sf; > @@ -364,6 +367,40 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = { > }, > }; > > +/* List of live video layer formats */ > +static const struct zynqmp_disp_format avbuf_live_fmts[] = { > + { > + .drm_fmt = DRM_FORMAT_VYUY, > + .bus_fmt = MEDIA_BUS_FMT_VYUY8_1X16, > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, > + .sf = scaling_factors_888, Is there a reason to have a separate array, instead of populating .bus_fmt in the existing arrays for the formats that can be supported with the live input, and only reporting those from zynqmp_disp_get_input_bus_fmts() ? > + }, > +}; > + > +u32 *zynqmp_disp_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) > +{ > + int i; > + u32 *input_fmts; > + > + input_fmts = kcalloc(ARRAY_SIZE(avbuf_live_fmts), sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) { > + *num_input_fmts = 0; > + return input_fmts; > + } > + > + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) > + input_fmts[i] = avbuf_live_fmts[i].bus_fmt; Extra space. > + *num_input_fmts = ARRAY_SIZE(avbuf_live_fmts); > + > + return input_fmts; > +} > + > static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg) > { > return readl(disp->avbuf.base + reg); > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h > index 9b8b202224d9..c2c8dd4896ba 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h > @@ -27,6 +27,10 @@ > struct device; > struct drm_format_info; > struct drm_plane_state; > +struct drm_bridge; > +struct drm_bridge_state; > +struct drm_connector_state; > +struct drm_crtc_state; > struct platform_device; > struct zynqmp_disp; > struct zynqmp_disp_layer; > @@ -52,6 +56,12 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp, > > u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, > unsigned int *num_formats); > +u32 *zynqmp_disp_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); As this is a bridge operation, I think it would be better located in zynqmp_dp.c. You can possibly expose the avbuf_live_fmts array in zynqmp_disp.h, but that's not really nice as you'll be missing the size. Another option would be to split the function in two, with the part that handles the bridge API implemented in zynqmp_dp.c, and the part that accesses the formats array in zynqmp_disp.c. > 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..9cb7ac9f3097 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -1580,6 +1580,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_disp_get_input_bus_fmts, > }; > > /* -----------------------------------------------------------------------------
Hi Laurent, Thanks for the review. > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Laurent > Pinchart > Sent: Wednesday, February 28, 2024 7:58 AM > 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>; dri- > devel@lists.freedesktop.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 2/4] 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 Mon, Feb 26, 2024 at 08:44:43PM -0800, 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. > > > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > > --- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 37 > > +++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/xlnx/zynqmp_disp.h | 10 ++++++++++ > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 + > > 3 files changed, 48 insertions(+) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > index e6d26ef60e89..ee99aad915ba 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,14 @@ 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; > > + u32 bus_fmt; > > u32 buf_fmt; > > bool swap; > > const u32 *sf; > > @@ -364,6 +367,40 @@ static const struct zynqmp_disp_format > avbuf_gfx_fmts[] = { > > }, > > }; > > > > +/* List of live video layer formats */ static const struct > > +zynqmp_disp_format avbuf_live_fmts[] = { > > + { > > + .drm_fmt = DRM_FORMAT_VYUY, > > + .bus_fmt = MEDIA_BUS_FMT_VYUY8_1X16, > > + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | > > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, > > + .sf = scaling_factors_888, > > Is there a reason to have a separate array, instead of populating .bus_fmt in the > existing arrays for the formats that can be supported with the live input, and only > reporting those from > zynqmp_disp_get_input_bus_fmts() ? > There are multiple reasons for this: 1) those formats share some similarities, although they are different in nature, e.g. memory layout formats vs. video signal formats; 2) corresponding IP registers are different with incompatible layouts; 3) ZynqMP DPSUB documentation clearly distinguishes 3 sets of formats: live video, video packer and graphics packer, ref. https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Video-Formats. I think, having separate format arrays will help to avoid ambiguity. > > + }, > > +}; > > + > > +u32 *zynqmp_disp_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) { > > + int i; > > + u32 *input_fmts; > > + > > + input_fmts = kcalloc(ARRAY_SIZE(avbuf_live_fmts), sizeof(*input_fmts), > GFP_KERNEL); > > + if (!input_fmts) { > > + *num_input_fmts = 0; > > + return input_fmts; > > + } > > + > > + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) > > + input_fmts[i] = avbuf_live_fmts[i].bus_fmt; > > Extra space. > ACK. Thank you. > > + *num_input_fmts = ARRAY_SIZE(avbuf_live_fmts); > > + > > + return input_fmts; > > +} > > + > > static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg) > > { > > return readl(disp->avbuf.base + reg); diff --git > > a/drivers/gpu/drm/xlnx/zynqmp_disp.h > > b/drivers/gpu/drm/xlnx/zynqmp_disp.h > > index 9b8b202224d9..c2c8dd4896ba 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h > > @@ -27,6 +27,10 @@ > > struct device; > > struct drm_format_info; > > struct drm_plane_state; > > +struct drm_bridge; > > +struct drm_bridge_state; > > +struct drm_connector_state; > > +struct drm_crtc_state; > > struct platform_device; > > struct zynqmp_disp; > > struct zynqmp_disp_layer; > > @@ -52,6 +56,12 @@ void zynqmp_disp_blend_set_global_alpha(struct > > zynqmp_disp *disp, > > > > u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, > > unsigned int *num_formats); > > +u32 *zynqmp_disp_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); > > As this is a bridge operation, I think it would be better located in zynqmp_dp.c. > You can possibly expose the avbuf_live_fmts array in zynqmp_disp.h, but that's > not really nice as you'll be missing the size. > Another option would be to split the function in two, with the part that handles > the bridge API implemented in zynqmp_dp.c, and the part that accesses the > formats array in zynqmp_disp.c. > Sure, I'll adjust this. > > 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..9cb7ac9f3097 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > @@ -1580,6 +1580,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_disp_get_input_bus_fmts, > > }; > > > > /* > > ---------------------------------------------------------------------- > > ------- > > -- > Regards, > > Laurent Pinchart Thank you, Anatoliy
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index e6d26ef60e89..ee99aad915ba 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,14 @@ 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; + u32 bus_fmt; u32 buf_fmt; bool swap; const u32 *sf; @@ -364,6 +367,40 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = { }, }; +/* List of live video layer formats */ +static const struct zynqmp_disp_format avbuf_live_fmts[] = { + { + .drm_fmt = DRM_FORMAT_VYUY, + .bus_fmt = MEDIA_BUS_FMT_VYUY8_1X16, + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 | + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422, + .sf = scaling_factors_888, + }, +}; + +u32 *zynqmp_disp_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) +{ + int i; + u32 *input_fmts; + + input_fmts = kcalloc(ARRAY_SIZE(avbuf_live_fmts), sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) { + *num_input_fmts = 0; + return input_fmts; + } + + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) + input_fmts[i] = avbuf_live_fmts[i].bus_fmt; + *num_input_fmts = ARRAY_SIZE(avbuf_live_fmts); + + return input_fmts; +} + static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg) { return readl(disp->avbuf.base + reg); diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h index 9b8b202224d9..c2c8dd4896ba 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h @@ -27,6 +27,10 @@ struct device; struct drm_format_info; struct drm_plane_state; +struct drm_bridge; +struct drm_bridge_state; +struct drm_connector_state; +struct drm_crtc_state; struct platform_device; struct zynqmp_disp; struct zynqmp_disp_layer; @@ -52,6 +56,12 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp, u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer, unsigned int *num_formats); +u32 *zynqmp_disp_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); 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..9cb7ac9f3097 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1580,6 +1580,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_disp_get_input_bus_fmts, }; /* -----------------------------------------------------------------------------
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. Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> --- drivers/gpu/drm/xlnx/zynqmp_disp.c | 37 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/xlnx/zynqmp_disp.h | 10 ++++++++++ drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 + 3 files changed, 48 insertions(+)