Re: [PATCH v2 08/11] rockchip/vpu: Support the Request API

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

 



On Thu, 2019-03-28 at 16:20 +0900, Tomasz Figa wrote:
> On Tue, Mar 5, 2019 at 4:27 AM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
> > Introduce support for the Request API. Although the JPEG encoder
> > does not mandate using the Request API, it's perfectly possible to
> > use it, if the application wants to.
> > 
> > In addition, add helpers that will be used by video decoders.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > ---
> >  .../rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c     |  6 ++++
> >  .../media/rockchip/vpu/rockchip_vpu_common.h  |  3 ++
> >  .../media/rockchip/vpu/rockchip_vpu_drv.c     | 29 ++++++++++++++++++-
> >  .../media/rockchip/vpu/rockchip_vpu_enc.c     | 18 ++++++++++++
> >  4 files changed, 55 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c b/drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c
> > index fe92c40d0a84..e11ee152b8ce 100644
> > --- a/drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c
> > +++ b/drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c
> > @@ -113,11 +113,15 @@ void rk3399_vpu_jpeg_enc_run(struct rockchip_vpu_ctx *ctx)
> >         struct rockchip_vpu_dev *vpu = ctx->dev;
> >         struct vb2_v4l2_buffer *src_buf, *dst_buf;
> >         struct rockchip_vpu_jpeg_ctx jpeg_ctx;
> > +       struct media_request *src_req;
> >         u32 reg;
> > 
> >         src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> >         dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > 
> > +       src_req = src_buf->vb2_buf.req_obj.req;
> > +       v4l2_ctrl_request_setup(src_req, &ctx->ctrl_handler);
> > +
> >         memset(&jpeg_ctx, 0, sizeof(jpeg_ctx));
> >         jpeg_ctx.buffer = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> >         jpeg_ctx.width = ctx->dst_fmt.width;
> > @@ -153,6 +157,8 @@ void rk3399_vpu_jpeg_enc_run(struct rockchip_vpu_ctx *ctx)
> >                 | VEPU_REG_ENCODE_FORMAT_JPEG
> >                 | VEPU_REG_ENCODE_ENABLE;
> > 
> > +       v4l2_ctrl_request_complete(src_req, &ctx->ctrl_handler);
> > +
> >         /* Kick the watchdog and start encoding */
> >         schedule_delayed_work(&vpu->watchdog_work, msecs_to_jiffies(2000));
> >         vepu_write(vpu, reg, VEPU_REG_ENCODE_START);
> > diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_common.h b/drivers/staging/media/rockchip/vpu/rockchip_vpu_common.h
> > index ca77668d9579..ac018136a7bc 100644
> > --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_common.h
> > +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_common.h
> > @@ -26,4 +26,7 @@ void rockchip_vpu_enc_reset_src_fmt(struct rockchip_vpu_dev *vpu,
> >  void rockchip_vpu_enc_reset_dst_fmt(struct rockchip_vpu_dev *vpu,
> >                                     struct rockchip_vpu_ctx *ctx);
> > 
> > +void *rockchip_vpu_get_ctrl(struct rockchip_vpu_ctx *ctx, u32 id);
> > +dma_addr_t rockchip_vpu_get_ref(struct vb2_queue *q, u64 ts);
> > +
> >  #endif /* ROCKCHIP_VPU_COMMON_H_ */
> > diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> > index af2481ca2228..937e9cfb4568 100644
> > --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> > +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> > @@ -36,6 +36,24 @@ module_param_named(debug, rockchip_vpu_debug, int, 0644);
> >  MODULE_PARM_DESC(debug,
> >                  "Debug level - higher value produces more verbose messages");
> > 
> > +void *rockchip_vpu_get_ctrl(struct rockchip_vpu_ctx *ctx, u32 id)
> > +{
> > +       struct v4l2_ctrl *ctrl;
> > +
> > +       ctrl = v4l2_ctrl_find(&ctx->ctrl_handler, id);
> > +       return ctrl ? ctrl->p_cur.p : NULL;
> > +}
> > +
> > +dma_addr_t rockchip_vpu_get_ref(struct vb2_queue *q, u64 ts)
> > +{
> > +       int index;
> > +
> > +       index = vb2_find_timestamp(q, ts, 0);
> > +       if (index >= 0)
> > +               return vb2_dma_contig_plane_dma_addr(q->bufs[index], 0);
> > +       return 0;
> > +}
> > +
> 
> Hmm, I assume these 2 are the "helpers that will be used by video
> decoders"? Probably overly nitpicking, but these are relatively small,
> so why couldn't they be just added in the patch that actually adds the
> code using them? Otherwise, if for some reason we separate the series,
> we end up with dead code.
> 

I can't remember why these are here. It was obviously intentional,
as the commit log shows!

Anyway, yeah, I'll move them to the patch that makes use of it.

> >  static void rockchip_vpu_job_finish(struct rockchip_vpu_dev *vpu,
> >                                     struct rockchip_vpu_ctx *ctx,
> >                                     unsigned int bytesused,
> > @@ -164,6 +182,7 @@ enc_queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
> >         src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >         src_vq->lock = &ctx->dev->vpu_mutex;
> >         src_vq->dev = ctx->dev->v4l2_dev.dev;
> > +       src_vq->supports_requests = true;
> > 
> >         ret = vb2_queue_init(src_vq);
> >         if (ret)
> > @@ -328,6 +347,11 @@ static const struct of_device_id of_rockchip_vpu_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, of_rockchip_vpu_match);
> > 
> > +static const struct media_device_ops rockchip_m2m_media_ops = {
> > +       .req_validate = vb2_request_validate,
> > +       .req_queue = v4l2_m2m_request_queue,
> > +};
> > +
> >  static int rockchip_vpu_video_device_register(struct rockchip_vpu_dev *vpu)
> >  {
> >         const struct of_device_id *match;
> > @@ -610,8 +634,11 @@ static int rockchip_vpu_probe(struct platform_device *pdev)
> >         }
> > 
> >         vpu->mdev.dev = vpu->dev;
> > -       strlcpy(vpu->mdev.model, DRIVER_NAME, sizeof(vpu->mdev.model));
> > +       strscpy(vpu->mdev.model, DRIVER_NAME, sizeof(vpu->mdev.model));
> > +       strscpy(vpu->mdev.bus_info, "platform:" DRIVER_NAME,
> > +               sizeof(vpu->mdev.bus_info));
> 
> Unrelated changes?
> 

Totally.

> >         media_device_init(&vpu->mdev);
> > +       vpu->mdev.ops = &rockchip_m2m_media_ops;
> >         vpu->v4l2_dev.mdev = &vpu->mdev;
> > 
> >         ret = rockchip_vpu_video_device_register(vpu);
> > diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
> > index 2b28403314bc..1b5a675ef24f 100644
> > --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
> > +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
> > @@ -555,14 +555,32 @@ static void rockchip_vpu_stop_streaming(struct vb2_queue *q)
> >                         vbuf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> >                 if (!vbuf)
> >                         break;
> > +               v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, &ctx->ctrl_handler);
> >                 v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> >         }
> >  }
> > 
> > +static void rockchip_vpu_buf_request_complete(struct vb2_buffer *vb)
> > +{
> > +       struct rockchip_vpu_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > +       v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->ctrl_handler);
> > +}
> > +
> > +static int rockchip_vpu_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > +       struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +
> > +       vbuf->field = V4L2_FIELD_NONE;
> 
> Hmm, "validate" in the name of this callback would suggest that we
> should just check the contents, not change them. Hans, what was the
> intention when adding this callback? Are we missing the const
> specifier in the argument?
> 

Hans already replied to this.

Thanks,
Eze




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux