Re: [Patch 1/2] media: v4l: ti-vpe: Add CAL v4l2 camera capture driver

[Benoit Parrot]

Hans,

I am almost done with my review comment but I am stuck here.

Benoit Parrot <bparrot@xxxxxx> wrote on Tue [2015-Jun-23 14:36:47 -0500]:
> > > +
> > > +static int cal_get_external_info(struct cal_ctx *ctx)
> > > +{
> > > +	struct v4l2_ext_control ctrl_ext;
> > > +	struct v4l2_ext_controls ctrls_ext;
> > > +	int ret;
> > > +
> > > +	ctx_dbg(3, ctx, "%s\n", __func__);
> > > +
> > > +	memset(&ctrls_ext, 0, sizeof(ctrls_ext));
> > > +	memset(&ctrl_ext, 0, sizeof(ctrl_ext));
> > > +
> > > +	ctrl_ext.id = V4L2_CID_PIXEL_RATE;
> > > +
> > > +	ctrls_ext.count = 1;
> > > +	ctrls_ext.controls = &ctrl_ext;
> > > +
> > > +	ret = v4l2_g_ext_ctrls(&ctx->ctrl_handler, &ctrls_ext);
> > 
> > Use v4l2_ctrl_g_ctrl_int64() instead: much simpler. You do need to store the
> > v4l2_ctrl pointer for the pixel rate control when you create it, but that's no
> > problem. (or use v4l2_ctrl_find, but I prefer to just cache the pointer).
> 
> Hmm, that's how I had it in our local 3.14 branch originally but I kept
> getting 0 instead of the actual pixel rate.
> I'll revert it back and test again against linux-media/master.
> 
> > 
> > > +	if (ret < 0) {
> > > +		ctx_err(ctx, "no pixel rate control in subdev: %s\n",
> > > +			ctx->sensor->name);
> > > +		return -EPIPE;
> > > +	}
> > > +
> > > +	ctx->external_rate = ctrl_ext.value64;
> > > +	ctx_dbg(3, ctx, "sensor Pixel Rate: %d\n", ctx->external_rate);
> > > +
> > > +	return 0;
> > > +}

Alright, in order to comply with this comment I rewrote the above function
as follows:

static int cal_get_external_info(struct cal_ctx *ctx)
{
	struct v4l2_ctrl *ctrl;
	s64 val;
	ctx_dbg(3, ctx, "%s\n", __func__);

	ctrl = v4l2_ctrl_find(&ctx->ctrl_handler, V4L2_CID_PIXEL_RATE);
//	ctrl = v4l2_ctrl_find(ctx->sensor->ctrl_handler, V4L2_CID_PIXEL_RATE);
	if (ctrl == NULL) {
		ctx_err(ctx, "no pixel rate control in subdev: %s\n",
			ctx->sensor->name);
		return -EPIPE;
	}

//	ctx->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);
	val = v4l2_ctrl_g_ctrl_int64(ctrl);
	ctx_dbg(3, ctx, "sensor Pixel Rate: s64 %lld\n", val);
	ctx->external_rate = (unsigned int)val;
	ctx_dbg(3, ctx, "sensor Pixel Rate: %d\n", ctx->external_rate);

       return 0;
}

But in all cases the value I get from v4l2_ctrl_g_ctrl_int64 is always zero!
Now I have tripple check with debug code that the subdev is in fact setting
the p_cur and p_new value to something useful (i.e. not 0).

I have traced it down to the get_ctrl() call inside of v4l2_ctrl_g_ctrl_int64.

static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
  {
          struct v4l2_ctrl *master = ctrl->cluster[0];
          int ret = 0;
          int i;
  
	printk("BENOIT: ==> get_ctrl: ctrl name:%s cur: %lld new: %lld\n",
        ctrl->name, *ctrl->p_cur.p_s64, *ctrl->p_new.p_s64);
          /* Compound controls are not supported. The new_to_user() and
           * cur_to_user() calls below would need to be modified not to access
           * userspace memory when called from get_ctrl().
           */
	printk("BENOIT: ==> get_ctrl: ctrl name:%s is_int: %d\n", ctrl->name, ctrl->is_int);
          if (!ctrl->is_int)
                  return -EINVAL;

**** It always exits here, because for int64_integer the is_int is not set!
**** which means on the way back out the value reported will always be 0!
  
	printk("BENOIT: ==> get_ctrl: ctrl name:%s flags: 0x%08lx\n", ctrl->name, ctrl->flags);
          if (ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
                  return -EACCES;
  
	printk("BENOIT: ==> get_ctrl: ctrl name:%s before ctrl_lock:\n", ctrl->name);
          v4l2_ctrl_lock(master);
          /* g_volatile_ctrl will update the current control values */
          if (ctrl->flags & V4L2_CTRL_FLAG_VOLATILE) {
                  for (i = 0; i < master->ncontrols; i++)
                          cur_to_new(master->cluster[i]);
                  ret = call_op(master, g_volatile_ctrl);
                  new_to_user(c, ctrl);
          } else {
                  cur_to_user(c, ctrl);
          }
	printk("BENOIT: <== get_ctrl: ctrl name:%s cur: %lld new: %lld\n",
        	ctrl->name, *ctrl->p_cur.p_s64, *ctrl->p_new.p_s64);
          v4l2_ctrl_unlock(master);
          return ret;
  }

As anyone ever used this successfully to read a V4L2_CID_PIXEL_RATE control value?

Regards,
Benoit

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/