Hi Laurent,
Thanks for the review.
> -----Original Message-----
> From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Laurent
> Pinchart
> Sent: Wednesday, February 28, 2024 7:58 AM
> To: Klymenko, Anatoliy <Anatoliy.Klymenko@xxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard
> <mripard@xxxxxxxxxx>; 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>; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> > ---
> > 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
