Re: [PATCH] v4l: vsp1: Move subdev operations from HGO to common histogram code

[Laurent Pinchart]

Hi Niklas,

On Tuesday 06 Sep 2016 12:28:00 Niklas Söderlund wrote:
> On 2016-09-05 18:13:39 +0300, Laurent Pinchart wrote:
> > The code will be shared with the HGT entity, move it to the generic
> > histogram implementation.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_drv.c   |   7 +-
> >  drivers/media/platform/vsp1/vsp1_hgo.c   | 308 ++---
> >  drivers/media/platform/vsp1/vsp1_hgo.h   |   7 +-
> >  drivers/media/platform/vsp1/vsp1_histo.c | 334 +++--
> >  drivers/media/platform/vsp1/vsp1_histo.h |  25 ++-
> >  5 files changed, 355 insertions(+), 326 deletions(-)

[snip]

> > @@ -268,7 +559,16 @@ int vsp1_histogram_init(struct vsp1_device *vsp1,
> > struct vsp1_histogram *histo,> 
> > INIT_LIST_HEAD(>irqqueue);
> > init_waitqueue_head(>wait_queue);
> > 
> > -   /* Initialize the media entity... */
> > +   /* Initialize the VSP entity... */
> > +   histo->entity.ops = ops;
> > +   histo->entity.type = type;
> > +
> > +   ret = vsp1_entity_init(vsp1, >entity, name, 2, _ops,
> > +  MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* ... and the media entity... */
> > ret = media_entity_pads_init(>video.entity, 1, >pad);
> > if (ret < 0)
> > return ret;
> 
> You forgot to update the histo video device name to match the subdevice
> name here. Something like this makes vsp-tests work for me again.
> 
> @@ -577,7 +577,7 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, struct
> vsp1_histogram *histo, histo->video.v4l2_dev = >v4l2_dev;
> histo->video.fops = _v4l2_fops;
> snprintf(histo->video.name, sizeof(histo->video.name),
> -"%s histo", name);
> +"%s histo", histo->entity.subdev.name);
> histo->video.vfl_type = VFL_TYPE_GRABBER;
> histo->video.release = video_device_release_empty;
> histo->video.ioctl_ops = _v4l2_ioctl_ops;
> 
> Without this fix the names listed using media-ctl -p show:
> 
> - entity 1: hgo histo (1 pad, 1 link)
> 
> Instead as it did before:
> 
> - entity 1: fe928000.vsp1 hgo histo (1 pad, 1 link)
> 
> Other then that
> 
> Tested-by: Niklas Söderlund 

Thank you. I'll fix that in the next version.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] v4l: vsp1: Move subdev operations from HGO to common histogram code

[Niklas Söderlund]

Hi Laurent,

Thanks for your patch.

On 2016-09-05 18:13:39 +0300, Laurent Pinchart wrote:
> The code will be shared with the HGT entity, move it to the generic
> histogram implementation.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/media/platform/vsp1/vsp1_drv.c   |   7 +-
>  drivers/media/platform/vsp1/vsp1_hgo.c   | 308 ++--
>  drivers/media/platform/vsp1/vsp1_hgo.h   |   7 +-
>  drivers/media/platform/vsp1/vsp1_histo.c | 334 
> +--
>  drivers/media/platform/vsp1/vsp1_histo.h |  25 ++-
>  5 files changed, 355 insertions(+), 326 deletions(-)
> 



>  int vsp1_histogram_init(struct vsp1_device *vsp1, struct vsp1_histogram 
> *histo,
> - const char *name, size_t data_size, u32 format)
> + enum vsp1_entity_type type, const char *name,
> + const struct vsp1_entity_operations *ops,
> + const unsigned int *formats, unsigned int num_formats,
> + size_t data_size, u32 meta_format)
>  {
>   int ret;
>  
> - histo->vsp1 = vsp1;
> + histo->formats = formats;
> + histo->num_formats = num_formats;
>   histo->data_size = data_size;
> - histo->format = format;
> + histo->meta_format = meta_format;
>  
>   histo->pad.flags = MEDIA_PAD_FL_SINK;
>   histo->video.vfl_dir = VFL_DIR_RX;
> @@ -268,7 +559,16 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, struct 
> vsp1_histogram *histo,
>   INIT_LIST_HEAD(>irqqueue);
>   init_waitqueue_head(>wait_queue);
>  
> - /* Initialize the media entity... */
> + /* Initialize the VSP entity... */
> + histo->entity.ops = ops;
> + histo->entity.type = type;
> +
> + ret = vsp1_entity_init(vsp1, >entity, name, 2, _ops,
> +MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> + if (ret < 0)
> + return ret;
> +
> + /* ... and the media entity... */
>   ret = media_entity_pads_init(>video.entity, 1, >pad);
>   if (ret < 0)
>   return ret;


You forgot to update the histo video device name to match the subdevice 
name here. Something like this makes vsp-tests work for me again.

@@ -577,7 +577,7 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, struct 
vsp1_histogram *histo,
histo->video.v4l2_dev = >v4l2_dev;
histo->video.fops = _v4l2_fops;
snprintf(histo->video.name, sizeof(histo->video.name),
-"%s histo", name);
+"%s histo", histo->entity.subdev.name);
histo->video.vfl_type = VFL_TYPE_GRABBER;
histo->video.release = video_device_release_empty;
histo->video.ioctl_ops = _v4l2_ioctl_ops;

Without this fix the names listed using media-ctl -p show:

- entity 1: hgo histo (1 pad, 1 link)

Instead as it did before:

- entity 1: fe928000.vsp1 hgo histo (1 pad, 1 link)

Other then that

Tested-by: Niklas Söderlund 

> @@ -293,10 +593,10 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, 
> struct vsp1_histogram *histo,
>   histo->queue.ops = _video_queue_qops;
>   histo->queue.mem_ops = _vmalloc_memops;
>   histo->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> - histo->queue.dev = histo->vsp1->dev;
> + histo->queue.dev = vsp1->dev;
>   ret = vb2_queue_init(>queue);
>   if (ret < 0) {
> - dev_err(histo->vsp1->dev, "failed to initialize vb2 queue\n");
> + dev_err(vsp1->dev, "failed to initialize vb2 queue\n");
>   goto error;
>   }
>  
> @@ -304,7 +604,7 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, struct 
> vsp1_histogram *histo,
>   histo->video.queue = >queue;
>   ret = video_register_device(>video, VFL_TYPE_GRABBER, -1);
>   if (ret < 0) {
> - dev_err(histo->vsp1->dev, "failed to register video device\n");
> + dev_err(vsp1->dev, "failed to register video device\n");
>   goto error;
>   }
>  
> @@ -314,11 +614,3 @@ error:
>   vsp1_histogram_cleanup(histo);
>   return ret;
>  }
> -
> -void vsp1_histogram_cleanup(struct vsp1_histogram *histo)
> -{
> - if (video_is_registered(>video))
> - video_unregister_device(>video);
> -
> - media_entity_cleanup(>video.entity);
> -}



-- 
Regards,
Niklas Söderlund
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] v4l: vsp1: Move subdev operations from HGO to common histogram code

[Laurent Pinchart]

The code will be shared with the HGT entity, move it to the generic
histogram implementation.

Signed-off-by: Laurent Pinchart 
---
 drivers/media/platform/vsp1/vsp1_drv.c   |   7 +-
 drivers/media/platform/vsp1/vsp1_hgo.c   | 308 ++--
 drivers/media/platform/vsp1/vsp1_hgo.h   |   7 +-
 drivers/media/platform/vsp1/vsp1_histo.c | 334 +--
 drivers/media/platform/vsp1/vsp1_histo.h |  25 ++-
 5 files changed, 355 insertions(+), 326 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
b/drivers/media/platform/vsp1/vsp1_drv.c
index 0b53280be150..1d6e87105752 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -151,8 +151,8 @@ static int vsp1_uapi_create_links(struct vsp1_device *vsp1)
}
 
if (vsp1->hgo) {
-   ret = media_create_pad_link(>hgo->entity.subdev.entity,
-   HGO_PAD_SOURCE,
+   ret = 
media_create_pad_link(>hgo->histo.entity.subdev.entity,
+   HISTO_PAD_SOURCE,
>hgo->histo.video.entity, 0,
MEDIA_LNK_FL_ENABLED |
MEDIA_LNK_FL_IMMUTABLE);
@@ -298,7 +298,8 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
goto done;
}
 
-   list_add_tail(>hgo->entity.list_dev, >entities);
+   list_add_tail(>hgo->histo.entity.list_dev,
+ >entities);
}
 
/* The LIF is only supported when used in conjunction with the DU, in
diff --git a/drivers/media/platform/vsp1/vsp1_hgo.c 
b/drivers/media/platform/vsp1/vsp1_hgo.c
index 94bb46f3e12b..2b257ad492db 100644
--- a/drivers/media/platform/vsp1/vsp1_hgo.c
+++ b/drivers/media/platform/vsp1/vsp1_hgo.c
@@ -21,8 +21,6 @@
 #include "vsp1_dl.h"
 #include "vsp1_hgo.h"
 
-#define HGO_MIN_SIZE   4U
-#define HGO_MAX_SIZE   8192U
 #define HGO_DATA_SIZE  ((2 + 256) * 4)
 
 /* 
-
@@ -31,7 +29,7 @@
 
 static inline u32 vsp1_hgo_read(struct vsp1_hgo *hgo, u32 reg)
 {
-   return vsp1_read(hgo->entity.vsp1, reg);
+   return vsp1_read(hgo->histo.entity.vsp1, reg);
 }
 
 static inline void vsp1_hgo_write(struct vsp1_hgo *hgo, struct vsp1_dl_list 
*dl,
@@ -63,7 +61,8 @@ void vsp1_hgo_frame_end(struct vsp1_entity *entity)
*data++ = vsp1_hgo_read(hgo, VI6_HGO_G_SUM);
 
for (i = 0; i < 256; ++i) {
-   vsp1_write(hgo->entity.vsp1, VI6_HGO_EXT_HIST_ADDR, i);
+   vsp1_write(hgo->histo.entity.vsp1,
+  VI6_HGO_EXT_HIST_ADDR, i);
*data++ = vsp1_hgo_read(hgo, VI6_HGO_EXT_HIST_DATA);
}
 
@@ -129,272 +128,6 @@ static const struct v4l2_ctrl_config hgo_num_bins_control 
= {
 };
 
 /* 
-
- * V4L2 Subdevice Operations
- */
-
-static int hgo_enum_mbus_code(struct v4l2_subdev *subdev,
-  struct v4l2_subdev_pad_config *cfg,
-  struct v4l2_subdev_mbus_code_enum *code)
-{
-   static const unsigned int codes[] = {
-   MEDIA_BUS_FMT_ARGB_1X32,
-   MEDIA_BUS_FMT_AHSV_1X32,
-   MEDIA_BUS_FMT_AYUV8_1X32,
-   };
-
-   if (code->pad == HGO_PAD_SOURCE) {
-   code->code = MEDIA_BUS_FMT_FIXED;
-   return 0;
-   }
-
-   return vsp1_subdev_enum_mbus_code(subdev, cfg, code, codes,
- ARRAY_SIZE(codes));
-}
-
-static int hgo_enum_frame_size(struct v4l2_subdev *subdev,
-   struct v4l2_subdev_pad_config *cfg,
-   struct v4l2_subdev_frame_size_enum *fse)
-{
-   if (fse->pad != HGO_PAD_SINK)
-   return -EINVAL;
-
-   return vsp1_subdev_enum_frame_size(subdev, cfg, fse, HGO_MIN_SIZE,
-  HGO_MIN_SIZE, HGO_MAX_SIZE,
-  HGO_MAX_SIZE);
-}
-
-static int hgo_get_selection(struct v4l2_subdev *subdev,
-struct v4l2_subdev_pad_config *cfg,
-struct v4l2_subdev_selection *sel)
-{
-   struct vsp1_hgo *hgo = to_hgo(subdev);
-   struct v4l2_subdev_pad_config *config;
-   struct v4l2_mbus_framefmt *format;
-   struct v4l2_rect *crop;
-
-   if (sel->pad != HGO_PAD_SINK)
-   return -EINVAL;
-
-   config = vsp1_entity_get_pad_config(>entity, cfg, sel->which);
-   if (!config)
-   return -EINVAL;
-
-   switch (sel->target) {
-