Hi Hans, On Friday 24 July 2015 15:35:24 Hans Verkuil wrote: > Hi Helen, > > Thank you for creating this driver! Much appreciated! > > I do have some comments, see my notes below... > > On 07/18/2015 04:42 PM, Helen Fornazier wrote: > > First version of the Virtual Media Controller. > > Add a simple version of the core of the driver, the capture and > > sensor nodes in the topology, generating a grey image in a hardcoded > > format. > > > > Signed-off-by: Helen Fornazier <helen.fornazier@xxxxxxxxx> > > --- > > > > The Virtual Media Controller is meant to provide a test tool which > > simulates a configurable video pipeline exposing the configuration of its > > individual nodes (such as sensors, debayers, scalers, inputs, outputs) > > throught the subdevice API. > > > > This first version generates a grey image with a fixed format within a > > thread in the sensor, but it will be integrated with the Test Pattern > > Generator from the Vivid driver in the future. > > > > For more information: http://kernelnewbies.org/LaurentPinchart > > > > The patch is based on 'media/master' branch and available at > > > > https://github.com/helen-fornazier/opw-staging > > vmc/review/video-pipe > > > > drivers/media/Kconfig | 1 + > > drivers/media/Makefile | 2 +- > > drivers/media/vmc/Kconfig | 6 + > > drivers/media/vmc/Makefile | 4 + > > drivers/media/vmc/vmc-capture.c | 528 +++++++++++++++++++++++++++++++++++ > > drivers/media/vmc/vmc-capture.h | 50 ++++ > > drivers/media/vmc/vmc-core.c | 595 +++++++++++++++++++++++++++++++++++ > > drivers/media/vmc/vmc-core.h | 55 ++++ > > drivers/media/vmc/vmc-sensor.c | 275 +++++++++++++++++++ > > drivers/media/vmc/vmc-sensor.h | 40 +++ > > 10 files changed, 1555 insertions(+), 1 deletion(-) > > create mode 100644 drivers/media/vmc/Kconfig > > As was mentioned before, this virtual driver should go into the platform > directory. > > I would also prefer that is was renamed to vimc to be consistent with the > vim2m and vivid drivers (all with 'vi' prefixes). > > > create mode 100644 drivers/media/vmc/Makefile > > create mode 100644 drivers/media/vmc/vmc-capture.c > > create mode 100644 drivers/media/vmc/vmc-capture.h > > create mode 100644 drivers/media/vmc/vmc-core.c > > create mode 100644 drivers/media/vmc/vmc-core.h > > create mode 100644 drivers/media/vmc/vmc-sensor.c > > create mode 100644 drivers/media/vmc/vmc-sensor.h [snip] > > diff --git a/drivers/media/vmc/vmc-capture.c > > b/drivers/media/vmc/vmc-capture.c new file mode 100644 > > index 0000000..9367973 > > --- /dev/null > > +++ b/drivers/media/vmc/vmc-capture.c [snip] > > +static void vmc_cap_buf_queue(struct vb2_buffer *vb2) > > +{ > > + struct vmc_cap_device *vcap = vb2_get_drv_priv(vb2->vb2_queue); > > + struct vmc_cap_buffer *buf = container_of(vb2, > > + struct vmc_cap_buffer, vb2); > > + > > + /* If the buffer doesn't have enough size, mark it as error */ > > + if (vb2->v4l2_planes[0].length < vcap->format.sizeimage) { > > + vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR); > > + return; > > + } > > No need for this check, this will never happen. > > > + > > + spin_lock(&vcap->qlock); > > + > > + list_add_tail(&buf->list, &vcap->buf_list); > > + > > + spin_unlock(&vcap->qlock); > > +} > > + > > +static int vmc_cap_queue_setup(struct vb2_queue *vq, > > + const struct v4l2_format *fmt, > > + unsigned int *nbuffers, unsigned int *nplanes, > > + unsigned int sizes[], void *alloc_ctxs[]) > > +{ > > + struct vmc_cap_device *vcap = vb2_get_drv_priv(vq); > > + > > + /*We don't support multiplanes for now */ > > + *nplanes = 1; > > + > > + sizes[0] = fmt ? fmt->fmt.pix.sizeimage : vcap->format.sizeimage; > > If fmt != NULL then you need to check if fmt->fmt.pix.sizeimage >= > vcap->format.sizeimage and return -EINVAL if that is not the case. Isn't CREATE_BUFS allowed to create buffers smaller (or larger) than required for the currently configured format ? > > + > > + return 0; > > +} [snip] > I didn't review the MC-specific parts in-depth. I assume that Laurent did > that already, I did that to some extend, but getting upstream review is also an important part of the learning experience ;-) I mentor Helen on the VMC project, but it belongs to her, not to me. > and in addition there will be upcoming changes in the MC API based on the > workshop results next week. > > In general I have the feeling that the amount of code needed to handle and > setup entities, links, etc. is too large. That's not your problem, that's > something we as V4L2 core developers need to address. I've seen this in > other drivers as well. I suspect we need more helper functions in the core > code. > > I may be wrong, though. And in any case, any in-depth analysis of this will > also have to wait until the workshop results are in. > > Anyway, thank you for working on this. I didn't see much wrong here, and the > few issues I noticed should be easy to fix. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
