RE: [PATCH v2 3/8] drm: xlnx: zynqmp_dpsub: Anounce supported input formats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

Thanks a lot for the review.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Monday, March 18, 2024 5:05 PM
> 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>; Rob
> Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> media@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 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@xxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxx>
> 
> > +     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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux