LinuxTV Patchwork [v2,07/11] rockchip/vpu: Open-code media controller register

login
register
mail settings
Submitter Ezequiel Garcia
Date March 4, 2019, 7:25 p.m.
Message ID <20190304192529.14200-8-ezequiel@collabora.com>
Download mbox | patch
Permalink /patch/54822/
State Superseded
Delegated to: Hans Verkuil
Headers show

Comments

Ezequiel Garcia - March 4, 2019, 7:25 p.m.
In preparation to support decoders, using a single memory-to-memory
device, we need to roll our own media controller entities registration.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../staging/media/rockchip/vpu/rockchip_vpu.h |  35 ++++
 .../media/rockchip/vpu/rockchip_vpu_drv.c     | 181 ++++++++++++++++--
 2 files changed, 204 insertions(+), 12 deletions(-)
Tomasz Figa - March 28, 2019, 7:11 a.m.
On Tue, Mar 5, 2019 at 4:27 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> In preparation to support decoders, using a single memory-to-memory
> device, we need to roll our own media controller entities registration.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../staging/media/rockchip/vpu/rockchip_vpu.h |  35 ++++
>  .../media/rockchip/vpu/rockchip_vpu_drv.c     | 181 ++++++++++++++++--
>  2 files changed, 204 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu.h b/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> index 76ee24abc141..084f58cadda1 100644
> --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> @@ -71,6 +71,38 @@ enum rockchip_vpu_codec_mode {
>         RK_VPU_MODE_JPEG_ENC,
>  };
>
> +/*
> + * struct rockchip_vpu_mc - media controller data
> + *
> + * @source:            &struct media_entity pointer with the source entity
> + *                     Used only when the M2M device is registered via
> + *                     v4l2_m2m_unregister_media_controller().

_register? (and few other places below)

But I'm really confused, because this patch exactly removes the uses
of v4l2_m2m_(un)register_media_controller() in this driver.

> + * @source_pad:                &struct media_pad with the source pad.
> + *                     Used only when the M2M device is registered via
> + *                     v4l2_m2m_unregister_media_controller().
> + * @sink:              &struct media_entity pointer with the sink entity
> + *                     Used only when the M2M device is registered via
> + *                     v4l2_m2m_unregister_media_controller().
> + * @sink_pad:          &struct media_pad with the sink pad.
> + *                     Used only when the M2M device is registered via
> + *                     v4l2_m2m_unregister_media_controller().
> + * @proc:              &struct media_entity pointer with the M2M device itself.
> + * @proc_pads:         &struct media_pad with the @proc pads.
> + *                     Used only when the M2M device is registered via
> + *                     v4l2_m2m_unregister_media_controller().
> + * @intf_devnode:      &struct media_intf devnode pointer with the interface
> + *                     with controls the M2M device.
> + */
> +struct rockchip_vpu_mc {
> +       struct media_entity     *source;
> +       struct media_pad        source_pad;
> +       struct media_entity     sink;
> +       struct media_pad        sink_pad;
> +       struct media_entity     proc;
> +       struct media_pad        proc_pads[2];
> +       struct media_intf_devnode *intf_devnode;
> +};
> +
>  /**
>   * struct rockchip_vpu_dev - driver data
>   * @v4l2_dev:          V4L2 device to register video devices for.
> @@ -78,6 +110,8 @@ enum rockchip_vpu_codec_mode {
>   * @mdev:              media device associated to this device.
>   * @vfd_enc:           Video device for encoder.
>   * @pdev:              Pointer to VPU platform device.
> + * @mc:                        Array of media controller topology structs

Is it just me or there is something wrong with indentation here?

> + *                     for encoder and decoder.
>   * @dev:               Pointer to device for convenient logging using
>   *                     dev_ macros.
>   * @clocks:            Array of clock handles.
> @@ -95,6 +129,7 @@ struct rockchip_vpu_dev {
>         struct media_device mdev;
>         struct video_device *vfd_enc;
>         struct platform_device *pdev;
> +       struct rockchip_vpu_mc mc[2];
>         struct device *dev;
>         struct clk_bulk_data clocks[ROCKCHIP_VPU_MAX_CLOCKS];
>         void __iomem *base;
> diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> index 1a6dd36c71ab..af2481ca2228 100644
> --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> @@ -332,7 +332,7 @@ static int rockchip_vpu_video_device_register(struct rockchip_vpu_dev *vpu)
>  {
>         const struct of_device_id *match;
>         struct video_device *vfd;
> -       int function, ret;
> +       int ret;
>
>         match = of_match_node(of_rockchip_vpu_match, vpu->dev->of_node);
>         vfd = video_device_alloc();
> @@ -359,21 +359,169 @@ static int rockchip_vpu_video_device_register(struct rockchip_vpu_dev *vpu)
>         }
>         v4l2_info(&vpu->v4l2_dev, "registered as /dev/video%d\n", vfd->num);
>
> -       function = MEDIA_ENT_F_PROC_VIDEO_ENCODER;
> -       ret = v4l2_m2m_register_media_controller(vpu->m2m_dev, vfd, function);
> -       if (ret) {
> -               v4l2_err(&vpu->v4l2_dev, "Failed to init mem2mem media controller\n");
> -               goto err_unreg_video;
> -       }
>         return 0;
> -
> -err_unreg_video:
> -       video_unregister_device(vfd);
>  err_free_dev:
>         video_device_release(vfd);
>         return ret;
>  }
>
> +static int rockchip_vpu_register_entity(struct media_device *mdev,
> +       struct media_entity *entity, const char *entity_name,
> +       struct media_pad *pads, int num_pads, int function,
> +       struct video_device *vdev)
> +{
> +       unsigned int len;
> +       char *name;
> +       int ret;
> +
> +       entity->obj_type = MEDIA_ENTITY_TYPE_BASE;
> +       if (function == MEDIA_ENT_F_IO_V4L) {
> +               entity->info.dev.major = VIDEO_MAJOR;
> +               entity->info.dev.minor = vdev->minor;
> +       }
> +       len = strlen(vdev->name) + 2 + strlen(entity_name);
> +       name = kmalloc(len, GFP_KERNEL);
> +       if (!name)
> +               return -ENOMEM;
> +       snprintf(name, len, "%s-%s", vdev->name, entity_name);

How about using kasprintf()?

> +       entity->name = name;
> +       entity->function = function;
> +
> +       ret = media_entity_pads_init(entity, num_pads, pads);
> +       if (ret)
> +               return ret;
> +       ret = media_device_register_entity(mdev, entity);
> +       if (ret)
> +               return ret;

How about the memory allocated for entity->name?

> +
> +       return 0;
> +}
> +
> +static int rockchip_register_mc(struct media_device *mdev,
> +                               struct rockchip_vpu_mc *mc,
> +                               struct video_device *vdev,
> +                               int function)
> +{
> +       struct media_link *link;
> +       int ret;
> +
> +       /* Create the three encoder entities with their pads */
> +       mc->source = &vdev->entity;
> +       mc->source_pad.flags = MEDIA_PAD_FL_SOURCE;
> +       ret = rockchip_vpu_register_entity(mdev, mc->source,
> +                       "source", &mc->source_pad, 1, MEDIA_ENT_F_IO_V4L, vdev);
> +       if (ret)
> +               return ret;
> +
> +       mc->proc_pads[0].flags = MEDIA_PAD_FL_SINK;
> +       mc->proc_pads[1].flags = MEDIA_PAD_FL_SOURCE;
> +       ret = rockchip_vpu_register_entity(mdev, &mc->proc,
> +                       "proc", mc->proc_pads, 2, function, vdev);
> +       if (ret)
> +               goto err_rel_entity0;
> +
> +       mc->sink_pad.flags = MEDIA_PAD_FL_SINK;
> +       ret = rockchip_vpu_register_entity(mdev, &mc->sink,
> +                       "sink", &mc->sink_pad, 1, MEDIA_ENT_F_IO_V4L, vdev);
> +       if (ret)
> +               goto err_rel_entity1;
> +
> +       /* Connect the three entities */
> +       ret = media_create_pad_link(mc->source, 0, &mc->proc, 1,
> +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> +       if (ret)
> +               goto err_rel_entity2;
> +
> +       ret = media_create_pad_link(&mc->proc, 0, &mc->sink, 0,
> +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> +       if (ret)
> +               goto err_rm_links0;
> +
> +       /* Create video interface */
> +       mc->intf_devnode = media_devnode_create(mdev,
> +                       MEDIA_INTF_T_V4L_VIDEO, 0,
> +                       VIDEO_MAJOR, vdev->minor);
> +       if (!mc->intf_devnode) {
> +               ret = -ENOMEM;
> +               goto err_rm_links1;
> +       }
> +
> +       /* Connect the two DMA engines to the interface */
> +       link = media_create_intf_link(mc->source,
> +                       &mc->intf_devnode->intf,
> +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> +       if (!link) {
> +               ret = -ENOMEM;
> +               goto err_rm_devnode;
> +       }
> +
> +       link = media_create_intf_link(&mc->sink,
> +                       &mc->intf_devnode->intf,
> +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> +       if (!link) {
> +               ret = -ENOMEM;
> +               goto err_rm_intf_link;
> +       }
> +       return 0;
> +
> +err_rm_intf_link:
> +       media_remove_intf_links(&mc->intf_devnode->intf);

Do we need to explicitly remove the links here? The entity removal
functions remove the links implicitly.

> +err_rm_devnode:
> +       media_devnode_remove(mc->intf_devnode);
> +err_rm_links1:
> +       media_entity_remove_links(&mc->sink);
> +err_rm_links0:
> +       media_entity_remove_links(&mc->proc);
> +       media_entity_remove_links(mc->source);
> +err_rel_entity2:
> +       media_device_unregister_entity(&mc->proc);
> +       kfree(mc->proc.name);
> +err_rel_entity1:
> +       media_device_unregister_entity(&mc->sink);
> +       kfree(mc->sink.name);
> +err_rel_entity0:
> +       media_device_unregister_entity(mc->source);
> +       kfree(mc->source->name);
> +       return ret;
> +}
> +
> +static void rockchip_unregister_mc(struct rockchip_vpu_mc *mc)
> +{
> +       media_remove_intf_links(&mc->intf_devnode->intf);
> +       media_devnode_remove(mc->intf_devnode);
> +       media_entity_remove_links(mc->source);
> +       media_entity_remove_links(&mc->sink);
> +       media_entity_remove_links(&mc->proc);
> +       media_device_unregister_entity(mc->source);
> +       media_device_unregister_entity(&mc->sink);
> +       media_device_unregister_entity(&mc->proc);
> +       kfree(mc->source->name);
> +       kfree(mc->sink.name);
> +       kfree(mc->proc.name);
> +}
> +
> +static int rockchip_register_media_controller(struct rockchip_vpu_dev *vpu)
> +{
> +       int ret;
> +
> +       /* We have one memory-to-memory device, to hold a single queue

Coding style:

/*
 * We have one [...]

Best regards,
Tomasz
Ezequiel Garcia - March 28, 2019, 8:05 p.m.
On Thu, 2019-03-28 at 16:11 +0900, Tomasz Figa wrote:
> On Tue, Mar 5, 2019 at 4:27 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > In preparation to support decoders, using a single memory-to-memory
> > device, we need to roll our own media controller entities registration.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../staging/media/rockchip/vpu/rockchip_vpu.h |  35 ++++
> >  .../media/rockchip/vpu/rockchip_vpu_drv.c     | 181 ++++++++++++++++--
> >  2 files changed, 204 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu.h b/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> > index 76ee24abc141..084f58cadda1 100644
> > --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> > +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
> > @@ -71,6 +71,38 @@ enum rockchip_vpu_codec_mode {
> >         RK_VPU_MODE_JPEG_ENC,
> >  };
> > 
> > +/*
> > + * struct rockchip_vpu_mc - media controller data
> > + *
> > + * @source:            &struct media_entity pointer with the source entity
> > + *                     Used only when the M2M device is registered via
> > + *                     v4l2_m2m_unregister_media_controller().
> 
> _register? (and few other places below)
> 
> But I'm really confused, because this patch exactly removes the uses
> of v4l2_m2m_(un)register_media_controller() in this driver.
> 

Those comments are bad copy paste, should be dropped.

> > + * @source_pad:                &struct media_pad with the source pad.
> > + *                     Used only when the M2M device is registered via
> > + *                     v4l2_m2m_unregister_media_controller().
> > + * @sink:              &struct media_entity pointer with the sink entity
> > + *                     Used only when the M2M device is registered via
> > + *                     v4l2_m2m_unregister_media_controller().
> > + * @sink_pad:          &struct media_pad with the sink pad.
> > + *                     Used only when the M2M device is registered via
> > + *                     v4l2_m2m_unregister_media_controller().
> > + * @proc:              &struct media_entity pointer with the M2M device itself.
> > + * @proc_pads:         &struct media_pad with the @proc pads.
> > + *                     Used only when the M2M device is registered via
> > + *                     v4l2_m2m_unregister_media_controller().
> > + * @intf_devnode:      &struct media_intf devnode pointer with the interface
> > + *                     with controls the M2M device.
> > + */
> > +struct rockchip_vpu_mc {
> > +       struct media_entity     *source;
> > +       struct media_pad        source_pad;
> > +       struct media_entity     sink;
> > +       struct media_pad        sink_pad;
> > +       struct media_entity     proc;
> > +       struct media_pad        proc_pads[2];
> > +       struct media_intf_devnode *intf_devnode;
> > +};
> > +
> >  /**
> >   * struct rockchip_vpu_dev - driver data
> >   * @v4l2_dev:          V4L2 device to register video devices for.
> > @@ -78,6 +110,8 @@ enum rockchip_vpu_codec_mode {
> >   * @mdev:              media device associated to this device.
> >   * @vfd_enc:           Video device for encoder.
> >   * @pdev:              Pointer to VPU platform device.
> > + * @mc:                        Array of media controller topology structs
> 
> Is it just me or there is something wrong with indentation here?
> 

It seems to be fine here -- it's all tabs.

> > + *                     for encoder and decoder.
> >   * @dev:               Pointer to device for convenient logging using
> >   *                     dev_ macros.
> >   * @clocks:            Array of clock handles.
> > @@ -95,6 +129,7 @@ struct rockchip_vpu_dev {
> >         struct media_device mdev;
> >         struct video_device *vfd_enc;
> >         struct platform_device *pdev;
> > +       struct rockchip_vpu_mc mc[2];
> >         struct device *dev;
> >         struct clk_bulk_data clocks[ROCKCHIP_VPU_MAX_CLOCKS];
> >         void __iomem *base;
> > diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> > index 1a6dd36c71ab..af2481ca2228 100644
> > --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> > +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
> > @@ -332,7 +332,7 @@ static int rockchip_vpu_video_device_register(struct rockchip_vpu_dev *vpu)
> >  {
> >         const struct of_device_id *match;
> >         struct video_device *vfd;
> > -       int function, ret;
> > +       int ret;
> > 
> >         match = of_match_node(of_rockchip_vpu_match, vpu->dev->of_node);
> >         vfd = video_device_alloc();
> > @@ -359,21 +359,169 @@ static int rockchip_vpu_video_device_register(struct rockchip_vpu_dev *vpu)
> >         }
> >         v4l2_info(&vpu->v4l2_dev, "registered as /dev/video%d\n", vfd->num);
> > 
> > -       function = MEDIA_ENT_F_PROC_VIDEO_ENCODER;
> > -       ret = v4l2_m2m_register_media_controller(vpu->m2m_dev, vfd, function);
> > -       if (ret) {
> > -               v4l2_err(&vpu->v4l2_dev, "Failed to init mem2mem media controller\n");
> > -               goto err_unreg_video;
> > -       }
> >         return 0;
> > -
> > -err_unreg_video:
> > -       video_unregister_device(vfd);
> >  err_free_dev:
> >         video_device_release(vfd);
> >         return ret;
> >  }
> > 
> > +static int rockchip_vpu_register_entity(struct media_device *mdev,
> > +       struct media_entity *entity, const char *entity_name,
> > +       struct media_pad *pads, int num_pads, int function,
> > +       struct video_device *vdev)
> > +{
> > +       unsigned int len;
> > +       char *name;
> > +       int ret;
> > +
> > +       entity->obj_type = MEDIA_ENTITY_TYPE_BASE;
> > +       if (function == MEDIA_ENT_F_IO_V4L) {
> > +               entity->info.dev.major = VIDEO_MAJOR;
> > +               entity->info.dev.minor = vdev->minor;
> > +       }
> > +       len = strlen(vdev->name) + 2 + strlen(entity_name);
> > +       name = kmalloc(len, GFP_KERNEL);
> > +       if (!name)
> > +               return -ENOMEM;
> > +       snprintf(name, len, "%s-%s", vdev->name, entity_name);
> 
> How about using kasprintf()?
> 

Indeed.

> > +       entity->name = name;
> > +       entity->function = function;
> > +
> > +       ret = media_entity_pads_init(entity, num_pads, pads);
> > +       if (ret)
> > +               return ret;
> > +       ret = media_device_register_entity(mdev, entity);
> > +       if (ret)
> > +               return ret;
> 
> How about the memory allocated for entity->name?
> 

Oops.

> > +
> > +       return 0;
> > +}
> > +
> > +static int rockchip_register_mc(struct media_device *mdev,
> > +                               struct rockchip_vpu_mc *mc,
> > +                               struct video_device *vdev,
> > +                               int function)
> > +{
> > +       struct media_link *link;
> > +       int ret;
> > +
> > +       /* Create the three encoder entities with their pads */
> > +       mc->source = &vdev->entity;
> > +       mc->source_pad.flags = MEDIA_PAD_FL_SOURCE;
> > +       ret = rockchip_vpu_register_entity(mdev, mc->source,
> > +                       "source", &mc->source_pad, 1, MEDIA_ENT_F_IO_V4L, vdev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       mc->proc_pads[0].flags = MEDIA_PAD_FL_SINK;
> > +       mc->proc_pads[1].flags = MEDIA_PAD_FL_SOURCE;
> > +       ret = rockchip_vpu_register_entity(mdev, &mc->proc,
> > +                       "proc", mc->proc_pads, 2, function, vdev);
> > +       if (ret)
> > +               goto err_rel_entity0;
> > +
> > +       mc->sink_pad.flags = MEDIA_PAD_FL_SINK;
> > +       ret = rockchip_vpu_register_entity(mdev, &mc->sink,
> > +                       "sink", &mc->sink_pad, 1, MEDIA_ENT_F_IO_V4L, vdev);
> > +       if (ret)
> > +               goto err_rel_entity1;
> > +
> > +       /* Connect the three entities */
> > +       ret = media_create_pad_link(mc->source, 0, &mc->proc, 1,
> > +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > +       if (ret)
> > +               goto err_rel_entity2;
> > +
> > +       ret = media_create_pad_link(&mc->proc, 0, &mc->sink, 0,
> > +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > +       if (ret)
> > +               goto err_rm_links0;
> > +
> > +       /* Create video interface */
> > +       mc->intf_devnode = media_devnode_create(mdev,
> > +                       MEDIA_INTF_T_V4L_VIDEO, 0,
> > +                       VIDEO_MAJOR, vdev->minor);
> > +       if (!mc->intf_devnode) {
> > +               ret = -ENOMEM;
> > +               goto err_rm_links1;
> > +       }
> > +
> > +       /* Connect the two DMA engines to the interface */
> > +       link = media_create_intf_link(mc->source,
> > +                       &mc->intf_devnode->intf,
> > +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > +       if (!link) {
> > +               ret = -ENOMEM;
> > +               goto err_rm_devnode;
> > +       }
> > +
> > +       link = media_create_intf_link(&mc->sink,
> > +                       &mc->intf_devnode->intf,
> > +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > +       if (!link) {
> > +               ret = -ENOMEM;
> > +               goto err_rm_intf_link;
> > +       }
> > +       return 0;
> > +
> > +err_rm_intf_link:
> > +       media_remove_intf_links(&mc->intf_devnode->intf);
> 
> Do we need to explicitly remove the links here? The entity removal
> functions remove the links implicitly.
> 

You mean the media_devnode_remove, right? In that case, seems you are right.

> > +err_rm_devnode:
> > +       media_devnode_remove(mc->intf_devnode);
> > +err_rm_links1:
> > +       media_entity_remove_links(&mc->sink);
> > +err_rm_links0:
> > +       media_entity_remove_links(&mc->proc);
> > +       media_entity_remove_links(mc->source);
> > +err_rel_entity2:
> > +       media_device_unregister_entity(&mc->proc);
> > +       kfree(mc->proc.name);
> > +err_rel_entity1:
> > +       media_device_unregister_entity(&mc->sink);
> > +       kfree(mc->sink.name);
> > +err_rel_entity0:
> > +       media_device_unregister_entity(mc->source);
> > +       kfree(mc->source->name);
> > +       return ret;
> > +}
> > +
> > +static void rockchip_unregister_mc(struct rockchip_vpu_mc *mc)
> > +{
> > +       media_remove_intf_links(&mc->intf_devnode->intf);
> > +       media_devnode_remove(mc->intf_devnode);
> > +       media_entity_remove_links(mc->source);
> > +       media_entity_remove_links(&mc->sink);
> > +       media_entity_remove_links(&mc->proc);
> > +       media_device_unregister_entity(mc->source);
> > +       media_device_unregister_entity(&mc->sink);
> > +       media_device_unregister_entity(&mc->proc);
> > +       kfree(mc->source->name);
> > +       kfree(mc->sink.name);
> > +       kfree(mc->proc.name);
> > +}
> > +
> > +static int rockchip_register_media_controller(struct rockchip_vpu_dev *vpu)
> > +{
> > +       int ret;
> > +
> > +       /* We have one memory-to-memory device, to hold a single queue
> 
> Coding style:
> 
> /*
>  * We have one [...]

Right.

Thanks,
Eze
Tomasz Figa - March 29, 2019, 7:43 a.m.
On Fri, Mar 29, 2019 at 5:05 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> On Thu, 2019-03-28 at 16:11 +0900, Tomasz Figa wrote:
> > On Tue, Mar 5, 2019 at 4:27 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
[snip]
> > > + * @source_pad:                &struct media_pad with the source pad.
> > > + *                     Used only when the M2M device is registered via
> > > + *                     v4l2_m2m_unregister_media_controller().
> > > + * @sink:              &struct media_entity pointer with the sink entity
> > > + *                     Used only when the M2M device is registered via
> > > + *                     v4l2_m2m_unregister_media_controller().
> > > + * @sink_pad:          &struct media_pad with the sink pad.
> > > + *                     Used only when the M2M device is registered via
> > > + *                     v4l2_m2m_unregister_media_controller().
> > > + * @proc:              &struct media_entity pointer with the M2M device itself.
> > > + * @proc_pads:         &struct media_pad with the @proc pads.
> > > + *                     Used only when the M2M device is registered via
> > > + *                     v4l2_m2m_unregister_media_controller().
> > > + * @intf_devnode:      &struct media_intf devnode pointer with the interface
> > > + *                     with controls the M2M device.
> > > + */
> > > +struct rockchip_vpu_mc {
> > > +       struct media_entity     *source;
> > > +       struct media_pad        source_pad;
> > > +       struct media_entity     sink;
> > > +       struct media_pad        sink_pad;
> > > +       struct media_entity     proc;
> > > +       struct media_pad        proc_pads[2];
> > > +       struct media_intf_devnode *intf_devnode;
> > > +};
> > > +
> > >  /**
> > >   * struct rockchip_vpu_dev - driver data
> > >   * @v4l2_dev:          V4L2 device to register video devices for.
> > > @@ -78,6 +110,8 @@ enum rockchip_vpu_codec_mode {
> > >   * @mdev:              media device associated to this device.
> > >   * @vfd_enc:           Video device for encoder.
> > >   * @pdev:              Pointer to VPU platform device.
> > > + * @mc:                        Array of media controller topology structs
> >
> > Is it just me or there is something wrong with indentation here?
> >
>
> It seems to be fine here -- it's all tabs.

Hmm, never mind then.

> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int rockchip_register_mc(struct media_device *mdev,
> > > +                               struct rockchip_vpu_mc *mc,
> > > +                               struct video_device *vdev,
> > > +                               int function)
> > > +{
> > > +       struct media_link *link;
> > > +       int ret;
> > > +
> > > +       /* Create the three encoder entities with their pads */
> > > +       mc->source = &vdev->entity;
> > > +       mc->source_pad.flags = MEDIA_PAD_FL_SOURCE;
> > > +       ret = rockchip_vpu_register_entity(mdev, mc->source,
> > > +                       "source", &mc->source_pad, 1, MEDIA_ENT_F_IO_V4L, vdev);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       mc->proc_pads[0].flags = MEDIA_PAD_FL_SINK;
> > > +       mc->proc_pads[1].flags = MEDIA_PAD_FL_SOURCE;
> > > +       ret = rockchip_vpu_register_entity(mdev, &mc->proc,
> > > +                       "proc", mc->proc_pads, 2, function, vdev);
> > > +       if (ret)
> > > +               goto err_rel_entity0;
> > > +
> > > +       mc->sink_pad.flags = MEDIA_PAD_FL_SINK;
> > > +       ret = rockchip_vpu_register_entity(mdev, &mc->sink,
> > > +                       "sink", &mc->sink_pad, 1, MEDIA_ENT_F_IO_V4L, vdev);
> > > +       if (ret)
> > > +               goto err_rel_entity1;
> > > +
> > > +       /* Connect the three entities */
> > > +       ret = media_create_pad_link(mc->source, 0, &mc->proc, 1,
> > > +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > > +       if (ret)
> > > +               goto err_rel_entity2;
> > > +
> > > +       ret = media_create_pad_link(&mc->proc, 0, &mc->sink, 0,
> > > +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > > +       if (ret)
> > > +               goto err_rm_links0;
> > > +
> > > +       /* Create video interface */
> > > +       mc->intf_devnode = media_devnode_create(mdev,
> > > +                       MEDIA_INTF_T_V4L_VIDEO, 0,
> > > +                       VIDEO_MAJOR, vdev->minor);
> > > +       if (!mc->intf_devnode) {
> > > +               ret = -ENOMEM;
> > > +               goto err_rm_links1;
> > > +       }
> > > +
> > > +       /* Connect the two DMA engines to the interface */
> > > +       link = media_create_intf_link(mc->source,
> > > +                       &mc->intf_devnode->intf,
> > > +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > > +       if (!link) {
> > > +               ret = -ENOMEM;
> > > +               goto err_rm_devnode;
> > > +       }
> > > +
> > > +       link = media_create_intf_link(&mc->sink,
> > > +                       &mc->intf_devnode->intf,
> > > +                       MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > > +       if (!link) {
> > > +               ret = -ENOMEM;
> > > +               goto err_rm_intf_link;
> > > +       }
> > > +       return 0;
> > > +
> > > +err_rm_intf_link:
> > > +       media_remove_intf_links(&mc->intf_devnode->intf);
> >
> > Do we need to explicitly remove the links here? The entity removal
> > functions remove the links implicitly.
> >
>
> You mean the media_devnode_remove, right? In that case, seems you are right.

Yep. And also media_device_unregister_entity().

Best regards,
Tomasz

Patch

diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu.h b/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
index 76ee24abc141..084f58cadda1 100644
--- a/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
+++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu.h
@@ -71,6 +71,38 @@  enum rockchip_vpu_codec_mode {
 	RK_VPU_MODE_JPEG_ENC,
 };
 
+/*
+ * struct rockchip_vpu_mc - media controller data
+ *
+ * @source:		&struct media_entity pointer with the source entity
+ *			Used only when the M2M device is registered via
+ *			v4l2_m2m_unregister_media_controller().
+ * @source_pad:		&struct media_pad with the source pad.
+ *			Used only when the M2M device is registered via
+ *			v4l2_m2m_unregister_media_controller().
+ * @sink:		&struct media_entity pointer with the sink entity
+ *			Used only when the M2M device is registered via
+ *			v4l2_m2m_unregister_media_controller().
+ * @sink_pad:		&struct media_pad with the sink pad.
+ *			Used only when the M2M device is registered via
+ *			v4l2_m2m_unregister_media_controller().
+ * @proc:		&struct media_entity pointer with the M2M device itself.
+ * @proc_pads:		&struct media_pad with the @proc pads.
+ *			Used only when the M2M device is registered via
+ *			v4l2_m2m_unregister_media_controller().
+ * @intf_devnode:	&struct media_intf devnode pointer with the interface
+ *			with controls the M2M device.
+ */
+struct rockchip_vpu_mc {
+	struct media_entity	*source;
+	struct media_pad	source_pad;
+	struct media_entity	sink;
+	struct media_pad	sink_pad;
+	struct media_entity	proc;
+	struct media_pad	proc_pads[2];
+	struct media_intf_devnode *intf_devnode;
+};
+
 /**
  * struct rockchip_vpu_dev - driver data
  * @v4l2_dev:		V4L2 device to register video devices for.
@@ -78,6 +110,8 @@  enum rockchip_vpu_codec_mode {
  * @mdev:		media device associated to this device.
  * @vfd_enc:		Video device for encoder.
  * @pdev:		Pointer to VPU platform device.
+ * @mc:			Array of media controller topology structs
+ *			for encoder and decoder.
  * @dev:		Pointer to device for convenient logging using
  *			dev_ macros.
  * @clocks:		Array of clock handles.
@@ -95,6 +129,7 @@  struct rockchip_vpu_dev {
 	struct media_device mdev;
 	struct video_device *vfd_enc;
 	struct platform_device *pdev;
+	struct rockchip_vpu_mc mc[2];
 	struct device *dev;
 	struct clk_bulk_data clocks[ROCKCHIP_VPU_MAX_CLOCKS];
 	void __iomem *base;
diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
index 1a6dd36c71ab..af2481ca2228 100644
--- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
+++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
@@ -332,7 +332,7 @@  static int rockchip_vpu_video_device_register(struct rockchip_vpu_dev *vpu)
 {
 	const struct of_device_id *match;
 	struct video_device *vfd;
-	int function, ret;
+	int ret;
 
 	match = of_match_node(of_rockchip_vpu_match, vpu->dev->of_node);
 	vfd = video_device_alloc();
@@ -359,21 +359,169 @@  static int rockchip_vpu_video_device_register(struct rockchip_vpu_dev *vpu)
 	}
 	v4l2_info(&vpu->v4l2_dev, "registered as /dev/video%d\n", vfd->num);
 
-	function = MEDIA_ENT_F_PROC_VIDEO_ENCODER;
-	ret = v4l2_m2m_register_media_controller(vpu->m2m_dev, vfd, function);
-	if (ret) {
-		v4l2_err(&vpu->v4l2_dev, "Failed to init mem2mem media controller\n");
-		goto err_unreg_video;
-	}
 	return 0;
-
-err_unreg_video:
-	video_unregister_device(vfd);
 err_free_dev:
 	video_device_release(vfd);
 	return ret;
 }
 
+static int rockchip_vpu_register_entity(struct media_device *mdev,
+	struct media_entity *entity, const char *entity_name,
+	struct media_pad *pads, int num_pads, int function,
+	struct video_device *vdev)
+{
+	unsigned int len;
+	char *name;
+	int ret;
+
+	entity->obj_type = MEDIA_ENTITY_TYPE_BASE;
+	if (function == MEDIA_ENT_F_IO_V4L) {
+		entity->info.dev.major = VIDEO_MAJOR;
+		entity->info.dev.minor = vdev->minor;
+	}
+	len = strlen(vdev->name) + 2 + strlen(entity_name);
+	name = kmalloc(len, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+	snprintf(name, len, "%s-%s", vdev->name, entity_name);
+	entity->name = name;
+	entity->function = function;
+
+	ret = media_entity_pads_init(entity, num_pads, pads);
+	if (ret)
+		return ret;
+	ret = media_device_register_entity(mdev, entity);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rockchip_register_mc(struct media_device *mdev,
+				struct rockchip_vpu_mc *mc,
+				struct video_device *vdev,
+				int function)
+{
+	struct media_link *link;
+	int ret;
+
+	/* Create the three encoder entities with their pads */
+	mc->source = &vdev->entity;
+	mc->source_pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = rockchip_vpu_register_entity(mdev, mc->source,
+			"source", &mc->source_pad, 1, MEDIA_ENT_F_IO_V4L, vdev);
+	if (ret)
+		return ret;
+
+	mc->proc_pads[0].flags = MEDIA_PAD_FL_SINK;
+	mc->proc_pads[1].flags = MEDIA_PAD_FL_SOURCE;
+	ret = rockchip_vpu_register_entity(mdev, &mc->proc,
+			"proc", mc->proc_pads, 2, function, vdev);
+	if (ret)
+		goto err_rel_entity0;
+
+	mc->sink_pad.flags = MEDIA_PAD_FL_SINK;
+	ret = rockchip_vpu_register_entity(mdev, &mc->sink,
+			"sink", &mc->sink_pad, 1, MEDIA_ENT_F_IO_V4L, vdev);
+	if (ret)
+		goto err_rel_entity1;
+
+	/* Connect the three entities */
+	ret = media_create_pad_link(mc->source, 0, &mc->proc, 1,
+			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+	if (ret)
+		goto err_rel_entity2;
+
+	ret = media_create_pad_link(&mc->proc, 0, &mc->sink, 0,
+			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+	if (ret)
+		goto err_rm_links0;
+
+	/* Create video interface */
+	mc->intf_devnode = media_devnode_create(mdev,
+			MEDIA_INTF_T_V4L_VIDEO, 0,
+			VIDEO_MAJOR, vdev->minor);
+	if (!mc->intf_devnode) {
+		ret = -ENOMEM;
+		goto err_rm_links1;
+	}
+
+	/* Connect the two DMA engines to the interface */
+	link = media_create_intf_link(mc->source,
+			&mc->intf_devnode->intf,
+			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+	if (!link) {
+		ret = -ENOMEM;
+		goto err_rm_devnode;
+	}
+
+	link = media_create_intf_link(&mc->sink,
+			&mc->intf_devnode->intf,
+			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+	if (!link) {
+		ret = -ENOMEM;
+		goto err_rm_intf_link;
+	}
+	return 0;
+
+err_rm_intf_link:
+	media_remove_intf_links(&mc->intf_devnode->intf);
+err_rm_devnode:
+	media_devnode_remove(mc->intf_devnode);
+err_rm_links1:
+	media_entity_remove_links(&mc->sink);
+err_rm_links0:
+	media_entity_remove_links(&mc->proc);
+	media_entity_remove_links(mc->source);
+err_rel_entity2:
+	media_device_unregister_entity(&mc->proc);
+	kfree(mc->proc.name);
+err_rel_entity1:
+	media_device_unregister_entity(&mc->sink);
+	kfree(mc->sink.name);
+err_rel_entity0:
+	media_device_unregister_entity(mc->source);
+	kfree(mc->source->name);
+	return ret;
+}
+
+static void rockchip_unregister_mc(struct rockchip_vpu_mc *mc)
+{
+	media_remove_intf_links(&mc->intf_devnode->intf);
+	media_devnode_remove(mc->intf_devnode);
+	media_entity_remove_links(mc->source);
+	media_entity_remove_links(&mc->sink);
+	media_entity_remove_links(&mc->proc);
+	media_device_unregister_entity(mc->source);
+	media_device_unregister_entity(&mc->sink);
+	media_device_unregister_entity(&mc->proc);
+	kfree(mc->source->name);
+	kfree(mc->sink.name);
+	kfree(mc->proc.name);
+}
+
+static int rockchip_register_media_controller(struct rockchip_vpu_dev *vpu)
+{
+	int ret;
+
+	/* We have one memory-to-memory device, to hold a single queue
+	 * of memory-to-memory serialized jobs.
+	 * There is a set of pads and processing entities for the encoder,
+	 * and another set for the decoder.
+	 * Also, there are two V4L interface, one for each set of entities.
+	 */
+
+	if (vpu->vfd_enc) {
+		ret = rockchip_register_mc(&vpu->mdev, &vpu->mc[0],
+					   vpu->vfd_enc,
+					   MEDIA_ENT_F_PROC_VIDEO_ENCODER);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int rockchip_vpu_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
@@ -472,12 +620,21 @@  static int rockchip_vpu_probe(struct platform_device *pdev)
 		goto err_m2m_rel;
 	}
 
+	ret = rockchip_register_media_controller(vpu);
+	if (ret) {
+		v4l2_err(&vpu->v4l2_dev, "Failed to register media controller\n");
+		goto err_video_dev_unreg;
+	}
+
 	ret = media_device_register(&vpu->mdev);
 	if (ret) {
 		v4l2_err(&vpu->v4l2_dev, "Failed to register mem2mem media device\n");
-		goto err_video_dev_unreg;
+		goto err_mc_unreg;
 	}
 	return 0;
+err_mc_unreg:
+	if (vpu->vfd_enc)
+		rockchip_unregister_mc(&vpu->mc[0]);
 err_video_dev_unreg:
 	if (vpu->vfd_enc) {
 		video_unregister_device(vpu->vfd_enc);
@@ -500,10 +657,10 @@  static int rockchip_vpu_remove(struct platform_device *pdev)
 	v4l2_info(&vpu->v4l2_dev, "Removing %s\n", pdev->name);
 
 	media_device_unregister(&vpu->mdev);
-	v4l2_m2m_unregister_media_controller(vpu->m2m_dev);
 	v4l2_m2m_release(vpu->m2m_dev);
 	media_device_cleanup(&vpu->mdev);
 	if (vpu->vfd_enc) {
+		rockchip_unregister_mc(&vpu->mc[0]);
 		video_unregister_device(vpu->vfd_enc);
 		video_device_release(vpu->vfd_enc);
 	}

Privacy Policy