Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <a3faa3c6b82d8fde19bb87b90ca133ca914ea526.camel@collabora.com>
Subject: Re: [PATCH v1 2/2] media: hantro: Add scaling lists feature
From:   Ezequiel Garcia <ezequiel@collabora.com>
To:     Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        hverkuil@xs4all.nl, p.zabel@pengutronix.de, mchehab@kernel.org,
        shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com,
        gregkh@linuxfoundation.org, mripard@kernel.org,
        paul.kocialkowski@bootlin.com, wens@csie.org,
        jernej.skrabec@siol.net, emil.l.velikov@gmail.com,
        andrzej.p@collabora.com, jc@kynesim.co.uk,
        jernej.skrabec@gmail.com, nicolas@ndufresne.ca, cphealy@gmail.com
Cc:     kernel@pengutronix.de, linux-imx@nxp.com,
        linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Date:   Tue, 03 Aug 2021 16:27:49 -0300
In-Reply-To: <1c3d2d66902978e3533074b4d95c8da220cab255.camel@collabora.com>
References: <20210715151223.656453-1-benjamin.gaignard@collabora.com>
         <20210715151223.656453-3-benjamin.gaignard@collabora.com>
         <1c3d2d66902978e3533074b4d95c8da220cab255.camel@collabora.com>
Organization: Collabora
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.3-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 2021-08-03 at 16:15 -0300, Ezequiel Garcia wrote:
> On Thu, 2021-07-15 at 17:12 +0200, Benjamin Gaignard wrote:
> > If the bitstream embedded scaling lists allow the driver to use
> > them for decode the frames.
> > The scaling lists are expected to be in raster scan order (i.e. not up
> > right diagonal scan order)
> > Allocate the memory needed to store lists.
> > 
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> 

Forgot to mention that I couldn't apply this patch,
so my R-b assumes this is tested and passes the usual tests.

Thanks!
Ezequiel

> Thanks!
> 
> > ---
> >  drivers/staging/media/hantro/hantro_drv.c     |  8 +--
> >  .../staging/media/hantro/hantro_g2_hevc_dec.c | 52 +++++++++++++++++++
> >  drivers/staging/media/hantro/hantro_hevc.c    | 21 ++++++++
> >  drivers/staging/media/hantro/hantro_hw.h      |  3 ++
> >  4 files changed, 81 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index 8ad074a464fe..5610b7821a54 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -267,9 +267,6 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> >                     sps->bit_depth_luma_minus8 != 2)
> >                         /* Only 8-bit or 10-bit is supported */
> >                         return -EINVAL;
> > -               if (sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED)
> > -                       /* No scaling support */
> > -                       return -EINVAL;
> >         }
> >         return 0;
> >  }
> > @@ -451,6 +448,11 @@ static const struct hantro_ctrl controls[] = {
> >                 .cfg = {
> >                         .id = V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS,
> >                 },
> > +       }, {
> > +               .codec = HANTRO_HEVC_DECODER,
> > +               .cfg = {
> > +                       .id = V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX,
> > +               },
> >         }, {
> >                 .codec = HANTRO_HEVC_DECODER,
> >                 .cfg = {
> > diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> > index 90de74aa6b13..f95135ad553c 100644
> > --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> > +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> > @@ -608,6 +608,56 @@ static void set_buffers(struct hantro_ctx *ctx)
> >         hantro_write_addr(vpu, G2_TILE_BSD, ctx->hevc_dec.tile_bsd.dma);
> >  }
> >  
> > +static void prepare_scaling_list_buffer(struct hantro_ctx *ctx)
> > +{
> > +       struct hantro_dev *vpu = ctx->dev;
> > +       const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls;
> > +       const struct v4l2_ctrl_hevc_scaling_matrix *sc = ctrls->scaling;
> > +       const struct v4l2_ctrl_hevc_sps *sps = ctrls->sps;
> > +       u8 *p = ((u8 *)ctx->hevc_dec.scaling_lists.cpu);
> > +       unsigned int scaling_list_enabled;
> > +       unsigned int i, j, k;
> > +
> > +       scaling_list_enabled = !!(sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED);
> > +       hantro_reg_write(vpu, &g2_scaling_list_e, scaling_list_enabled);
> > +
> > +       if (!scaling_list_enabled)
> > +               return;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(sc->scaling_list_dc_coef_16x16); i++)
> > +               *p++ = sc->scaling_list_dc_coef_16x16[i];
> > +
> > +       for (i = 0; i < ARRAY_SIZE(sc->scaling_list_dc_coef_32x32); i++)
> > +               *p++ = sc->scaling_list_dc_coef_32x32[i];
> > +
> > +       /* 128-bit boundary */
> > +       p += 8;
> > +
> > +       /* write scaling lists column by column */
> > +
> > +       for (i = 0; i < 6; i++)
> > +               for (j = 0; j < 4; j++)
> > +                       for (k = 0; k < 4; k++)
> > +                               *p++ = sc->scaling_list_4x4[i][4 * k + j];
> > +
> > +       for (i = 0; i < 6; i++)
> > +               for (j = 0; j < 8; j++)
> > +                       for (k = 0; k < 8; k++)
> > +                               *p++ = sc->scaling_list_8x8[i][8 * k + j];
> > +
> > +       for (i = 0; i < 6; i++)
> > +               for (j = 0; j < 8; j++)
> > +                       for (k = 0; k < 8; k++)
> > +                               *p++ = sc->scaling_list_16x16[i][8 * k + j];
> > +
> > +       for (i = 0; i < 2; i++)
> > +               for (j = 0; j < 8; j++)
> > +                       for (k = 0; k < 8; k++)
> > +                               *p++ = sc->scaling_list_32x32[i][8 * k + j];
> > +
> > +       hantro_write_addr(vpu, HEVC_SCALING_LIST, ctx->hevc_dec.scaling_lists.dma);
> > +}
> > +
> >  static void hantro_g2_check_idle(struct hantro_dev *vpu)
> >  {
> >         int i;
> > @@ -668,6 +718,8 @@ int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx)
> >         set_buffers(ctx);
> >         prepare_tile_info_buffer(ctx);
> >  
> > +       prepare_scaling_list_buffer(ctx);
> > +
> >         hantro_end_prepare_run(ctx);
> >  
> >         hantro_reg_write(vpu, &g2_mode, HEVC_DEC_MODE);
> > diff --git a/drivers/staging/media/hantro/hantro_hevc.c b/drivers/staging/media/hantro/hantro_hevc.c
> > index 4e816ea73018..95f765d9ff4e 100644
> > --- a/drivers/staging/media/hantro/hantro_hevc.c
> > +++ b/drivers/staging/media/hantro/hantro_hevc.c
> > @@ -20,6 +20,8 @@
> >  /* tile border coefficients of filter */
> >  #define VERT_SAO_RAM_SIZE 48 /* bytes per pixel */
> >  
> > +#define SCALING_LIST_SIZE (16 * 64)
> > +
> >  #define MAX_TILE_COLS 20
> >  #define MAX_TILE_ROWS 22
> >  
> > @@ -296,6 +298,11 @@ int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx)
> >         if (WARN_ON(!ctrls->decode_params))
> >                 return -EINVAL;
> >  
> > +       ctrls->scaling =
> > +               hantro_get_ctrl(ctx, V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX);
> > +       if (WARN_ON(!ctrls->scaling))
> > +               return -EINVAL;
> > +
> >         ctrls->sps =
> >                 hantro_get_ctrl(ctx, V4L2_CID_MPEG_VIDEO_HEVC_SPS);
> >         if (WARN_ON(!ctrls->sps))
> > @@ -324,6 +331,12 @@ void hantro_hevc_dec_exit(struct hantro_ctx *ctx)
> >                                   hevc_dec->tile_sizes.dma);
> >         hevc_dec->tile_sizes.cpu = NULL;
> >  
> > +       if (hevc_dec->scaling_lists.cpu)
> > +               dma_free_coherent(vpu->dev, hevc_dec->scaling_lists.size,
> > +                                 hevc_dec->scaling_lists.cpu,
> > +                                 hevc_dec->scaling_lists.dma);
> > +       hevc_dec->scaling_lists.cpu = NULL;
> > +
> >         if (hevc_dec->tile_filter.cpu)
> >                 dma_free_coherent(vpu->dev, hevc_dec->tile_filter.size,
> >                                   hevc_dec->tile_filter.cpu,
> > @@ -367,6 +380,14 @@ int hantro_hevc_dec_init(struct hantro_ctx *ctx)
> >  
> >         hevc_dec->tile_sizes.size = size;
> >  
> > +       hevc_dec->scaling_lists.cpu = dma_alloc_coherent(vpu->dev, SCALING_LIST_SIZE,
> > +                                                        &hevc_dec->scaling_lists.dma,
> > +                                                        GFP_KERNEL);
> > +       if (!hevc_dec->scaling_lists.cpu)
> > +               return -ENOMEM;
> > +
> > +       hevc_dec->scaling_lists.size = SCALING_LIST_SIZE;
> > +
> >         hantro_hevc_ref_init(ctx);
> >  
> >         return 0;
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > index d8126f8178f5..1becc22af0f9 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -108,6 +108,7 @@ struct hantro_h264_dec_hw_ctx {
> >   */
> >  struct hantro_hevc_dec_ctrls {
> >         const struct v4l2_ctrl_hevc_decode_params *decode_params;
> > +       const struct v4l2_ctrl_hevc_scaling_matrix *scaling;
> >         const struct v4l2_ctrl_hevc_sps *sps;
> >         const struct v4l2_ctrl_hevc_pps *pps;
> >         u32 hevc_hdr_skip_length;
> > @@ -120,6 +121,7 @@ struct hantro_hevc_dec_ctrls {
> >   * @tile_sao:          Tile SAO buffer
> >   * @tile_bsd:          Tile BSD control buffer
> >   * @ref_bufs:          Internal reference buffers
> > + * @scaling_lists:     Scaling lists buffer
> >   * @ref_bufs_poc:      Internal reference buffers picture order count
> >   * @ref_bufs_used:     Bitfield of used reference buffers
> >   * @ctrls:             V4L2 controls attached to a run
> > @@ -131,6 +133,7 @@ struct hantro_hevc_dec_hw_ctx {
> >         struct hantro_aux_buf tile_sao;
> >         struct hantro_aux_buf tile_bsd;
> >         struct hantro_aux_buf ref_bufs[NUM_REF_PICTURES];
> > +       struct hantro_aux_buf scaling_lists;
> >         int ref_bufs_poc[NUM_REF_PICTURES];
> >         u32 ref_bufs_used;
> >         struct hantro_hevc_dec_ctrls ctrls;
> 

-- 
Kindly,
Ezequiel