Re: [Linux-kernel] [PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap
On 29/05/15 11:10, Hans Verkuil wrote: On 05/29/2015 03:08 AM, Ben Hutchings wrote: On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote: On 21/05/15 06:58, Hans Verkuil wrote: On 05/20/2015 06:39 PM, William Towle wrote: Fill in bus_info field and zero reserved field. Signed-off-by: Rob Taylor rob.tay...@codethink.co.uk Reviewed-by: William Towle william.to...@codethink.co.uk --- drivers/media/platform/soc_camera/soc_camera.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c index fd7497e..583c5e6 100644 --- a/drivers/media/platform/soc_camera/soc_camera.c +++ b/drivers/media/platform/soc_camera/soc_camera.c @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void *priv, WARN_ON(priv != file-private_data); strlcpy(cap-driver, ici-drv_name, sizeof(cap-driver)); + strlcpy(cap-bus_info, platform:soc_camera, sizeof(cap-bus_info)); + memset(cap-reserved, 0, sizeof(cap-reserved)); Why the memset? That shouldn't be needed. v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in v4l-utils v1.6.2 [1]) William, you should use the latest v4l-utils compiled from the git repo. Unlikely to be related to this, though. I'm puzzled by that. Isn't this function called by v4l_querycap(), which is called by video_usercopy()? And video_usercopy() zeroes the entire structure before doing so, or at least it appears to be intended to. Right. So I don't understand this. Can you dig a bit deeper why this would be needed here? It should not be necessary at all, so if reserved is non-zero, then someone is writing data where it shouldn't. I've just done some digging (including rolling back to when I saw the issue). Turns out the message I was attempting to 'fix' here was from an ancient v4l2-compliance, and clearly the outcome of a late night hacking... The memset is indeed not needed. Thanks Rob -- 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 13/20] media: soc_camera: v4l2-compliance fixes for querycap
On 21/05/15 06:58, Hans Verkuil wrote: On 05/20/2015 06:39 PM, William Towle wrote: Fill in bus_info field and zero reserved field. Signed-off-by: Rob Taylor rob.tay...@codethink.co.uk Reviewed-by: William Towle william.to...@codethink.co.uk --- drivers/media/platform/soc_camera/soc_camera.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c index fd7497e..583c5e6 100644 --- a/drivers/media/platform/soc_camera/soc_camera.c +++ b/drivers/media/platform/soc_camera/soc_camera.c @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void *priv, WARN_ON(priv != file-private_data); strlcpy(cap-driver, ici-drv_name, sizeof(cap-driver)); +strlcpy(cap-bus_info, platform:soc_camera, sizeof(cap-bus_info)); +memset(cap-reserved, 0, sizeof(cap-reserved)); Why the memset? That shouldn't be needed. v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in v4l-utils v1.6.2 [1]) Thanks, Rob [1] http://git.linuxtv.org/cgit.cgi/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-compliance.cpp?id=v4l-utils-1.6.2#n308 Regards, Hans return ici-ops-querycap(ici, cap); } -- 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 15/20] media: rcar_vin: Don't advertise support for USERPTR
On 21/05/15 07:03, Hans Verkuil wrote: On 05/20/2015 06:39 PM, William Towle wrote: rcar_vin requires physically contiguous buffer, so shouldn't advertise support for USERPTR. Signed-off-by: Rob Taylor rob.tay...@codethink.co.uk Reviewed-by: William Towle william.to...@codethink.co.uk --- drivers/media/platform/soc_camera/rcar_vin.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c index 222002a..b530503 100644 --- a/drivers/media/platform/soc_camera/rcar_vin.c +++ b/drivers/media/platform/soc_camera/rcar_vin.c @@ -1824,7 +1824,7 @@ static int rcar_vin_init_videobuf2(struct vb2_queue *vq, struct soc_camera_host *ici = to_soc_camera_host(icd-parent); vq-type = V4L2_BUF_TYPE_VIDEO_CAPTURE; -vq-io_modes = VB2_MMAP | VB2_USERPTR; +vq-io_modes = VB2_MMAP; NACK. USERPTR can be used, but the user pointer must point to physically contig memory (and this is checked). There are cases where the system will carve out phys. contig. memory and userspace has pointers to that. I'm pretty sure some of this is used by systems where soc-camera is run. Unfortunately, userspace has currently no way of knowing such userptr restrictions. That's a failing of the API. It's ugly as hell, but it is in use today and can't be dropped. Ah, that is ugly - I assumed because there wasn't an interface to query restrictions it would be necessary to drop support. We can drop this patch for now. It seems rcar_vin doesn't pass compliance with USERPTR, but we'll have to look into fixing that at a later date. Thanks, Rob Regards, Hans vq-drv_priv = icd; vq-ops = rcar_vin_vb2_ops; vq-mem_ops = vb2_dma_contig_memops; -- 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 17/20] media: adv7604: Support V4L_FIELD_INTERLACED
On 21/05/15 07:10, Hans Verkuil wrote: On 05/20/2015 06:39 PM, William Towle wrote: When hardware reports interlaced input, correctly set field to V4L_FIELD_INTERLACED ini adv76xx_fill_format. Signed-off-by: Rob Taylor rob.tay...@codethink.co.uk Reviewed-by: William Towle william.to...@codethink.co.uk --- drivers/media/i2c/adv7604.c |7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c index 4bde3e1..d77ee1f 100644 --- a/drivers/media/i2c/adv7604.c +++ b/drivers/media/i2c/adv7604.c @@ -1791,7 +1791,12 @@ static void adv76xx_fill_format(struct adv76xx_state *state, format-width = state-timings.bt.width; format-height = state-timings.bt.height; -format-field = V4L2_FIELD_NONE; + +if (state-timings.bt.interlaced) +format-field= V4L2_FIELD_INTERLACED; No, this should be FIELD_ALTERNATE. FIELD_INTERLACED means that the two fields are interlaced into a single frame buffer, with FIELD_ALTERNATE each buffer contains one field. And when capturing v4l2_buffer should return which field (TOP/BOTTOM) the buffer contains. It also complicates cropping/composing: the crop rectangle is in frame coordinates, composing uses field coordinates. The vivid driver handles this correctly and can be used as a reference. OK, that makes sense. I think for now we'll just drop this patch from the series until we can test this properly - its a bit undercooked.. Also, no space before the '='. Please add. You might be interested in this patch series as well: http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/90578 Ah yes, this does look useful. I'm thinking of doing some interlaced tests myself, possibly this weekend, using the adv7604. That would be great, happy to test anything on our side. Thanks Rob Regards, Hans +else +format-field= V4L2_FIELD_NONE; + format-colorspace = V4L2_COLORSPACE_SRGB; if (state-timings.bt.flags V4L2_DV_FL_IS_CE_VIDEO) -- 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
