lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <MW4PR12MB71655692853EE2D33A2A49AEE65F2@MW4PR12MB7165.namprd12.prod.outlook.com>
Date: Thu, 29 Feb 2024 20:07:13 +0000
From: "Klymenko, Anatoliy" <Anatoliy.Klymenko@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
	<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
	<airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, "Simek, Michal"
	<michal.simek@....com>, Andrzej Hajda <andrzej.hajda@...el.com>, Neil
 Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, Jonas
 Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/4] drm: xlnx: zynqmp_dpsub: Anounce supported input
 formats

Hi Laurent,

Thanks for the review.

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@...ts.freedesktop.org> On Behalf Of Laurent
> Pinchart
> Sent: Wednesday, February 28, 2024 7:58 AM
> To: Klymenko, Anatoliy <Anatoliy.Klymenko@....com>
> Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>; Maxime Ripard
> <mripard@...nel.org>; Thomas Zimmermann <tzimmermann@...e.de>; David
> Airlie <airlied@...il.com>; Daniel Vetter <daniel@...ll.ch>; Simek, Michal
> <michal.simek@....com>; Andrzej Hajda <andrzej.hajda@...el.com>; Neil
> Armstrong <neil.armstrong@...aro.org>; Robert Foss <rfoss@...nel.org>; Jonas
> Karlman <jonas@...boo.se>; Jernej Skrabec <jernej.skrabec@...il.com>; dri-
> devel@...ts.freedesktop.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.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@....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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ