From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.ideasonboard.com ([95.142.166.194]:47040 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757157Ab1JCVjZ (ORCPT ); Mon, 3 Oct 2011 17:39:25 -0400 From: Laurent Pinchart To: Guennadi Liakhovetski Subject: Re: [PATCH 7/9] V4L: soc-camera: add a Media Controller wrapper Date: Mon, 3 Oct 2011 22:44:22 +0200 Cc: Linux Media Mailing List , Deepthy Ravi References: <1317313137-4403-1-git-send-email-g.liakhovetski@gmx.de> <201110031305.41253.laurent.pinchart@ideasonboard.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201110032244.22764.laurent.pinchart@ideasonboard.com> Sender: linux-media-owner@vger.kernel.org List-ID: Hi Guennadi, On Monday 03 October 2011 17:29:23 Guennadi Liakhovetski wrote: > Hi Laurent > > Thanks for the reviews! You're welcome. > On Mon, 3 Oct 2011, Laurent Pinchart wrote: > > On Thursday 29 September 2011 18:18:55 Guennadi Liakhovetski wrote: > > > This wrapper adds a Media Controller implementation to soc-camera > > > drivers. To really benefit from it individual host drivers should > > > implement support for values of enum soc_camera_target other than > > > SOCAM_TARGET_PIPELINE in their .set_fmt() and .try_fmt() methods. > > > > [snip] > > > > > diff --git a/drivers/media/video/soc_entity.c > > > b/drivers/media/video/soc_entity.c new file mode 100644 > > > index 0000000..3a04700 > > > --- /dev/null > > > +++ b/drivers/media/video/soc_entity.c > > > @@ -0,0 +1,284 @@ > > > > [snip] > > > > > +static int bus_sd_pad_g_fmt(struct v4l2_subdev *sd, struct > > > v4l2_subdev_fh *fh, > > > + struct v4l2_subdev_format *sd_fmt) > > > +{ > > > + struct soc_camera_device *icd = v4l2_get_subdevdata(sd); > > > + struct v4l2_mbus_framefmt *f = &sd_fmt->format; > > > + > > > + if (sd_fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > > > + sd_fmt->format = *v4l2_subdev_get_try_format(fh, sd_fmt->pad); > > > + return 0; > > > + } > > > + > > > + if (sd_fmt->pad == SOC_HOST_BUS_PAD_SINK) { > > > + f->width = icd->host_input_width; > > > + f->height = icd->host_input_height; > > > + } else { > > > + f->width = icd->user_width; > > > + f->height = icd->user_height; > > > + } > > > + f->field = icd->field; > > > + f->code = icd->current_fmt->code; > > > + f->colorspace = icd->colorspace; > > > > Can soc-camera hosts perform format conversion ? If so you will likely > > need to store the mbus code for the input and output separately, > > possibly in v4l2_mbus_format fields. You could then simplify the > > [gs]_fmt functions by implementing similar to the __*_get_format > > functions in the OMAP3 ISP driver. > > They can, yes. But, under soc-camera conversions are performed between > mediabus codes and fourcc formats. Upon pipeline construction (probing) a > table of format conversions is built, where hosts generate one or more > translation entries for all client formats, that they support. The only > example of a more complex translations so far is MIPI CSI-2, but even > there we have decided to identify CSI-2 formats using the same media-bus > codes, as what you "get" "between" the CSI-2 block and the DMA engine. For > the only CSI-2 capable soc-camera host so far - the CEU driver - this is > also a very natural representation, because there the CSI-2 block is > indeed an additional pipeline stage, uniquely translating CSI-2 to > media-bus codes, that are then fed to the CEU parallel port. How does that work with the MC API then ? If the bridge can, let's say, convert between raw bayer and YUV, shouldn't the format at the bridge input be raw bayer and at the bridge output YUV ? > > > + return 0; > > > +} > > > + > > > +static int bus_sd_pad_s_fmt(struct v4l2_subdev *sd, struct > > > v4l2_subdev_fh *fh, > > > + struct v4l2_subdev_format *sd_fmt) > > > +{ > > > + struct soc_camera_device *icd = v4l2_get_subdevdata(sd); > > > + struct v4l2_mbus_framefmt *mf = &sd_fmt->format; > > > + struct v4l2_format vf = { > > > + .type = V4L2_BUF_TYPE_VIDEO_CAPTURE, > > > + }; > > > + enum soc_camera_target tgt = sd_fmt->pad == SOC_HOST_BUS_PAD_SINK ? > > > + SOCAM_TARGET_HOST_IN : SOCAM_TARGET_HOST_OUT; > > > + int ret; > > > + > > > + se_mbus_to_v4l2(icd, mf, &vf); > > > + > > > + if (sd_fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > > > + struct v4l2_mbus_framefmt *try_fmt = > > > + v4l2_subdev_get_try_format(fh, sd_fmt->pad); > > > + ret = soc_camera_try_fmt(icd, &vf, tgt); > > > + if (!ret) { > > > + se_v4l2_to_mbus(icd, &vf, try_fmt); > > > + sd_fmt->format = *try_fmt; > > > + } > > > + return ret; > > > + } > > > + > > > + ret = soc_camera_set_fmt(icd, &vf, tgt); > > > + if (!ret) > > > + se_v4l2_to_mbus(icd, &vf, &sd_fmt->format); > > > + > > > + return ret; > > > +} > > > + > > > +static int bus_sd_pad_enum_mbus_code(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_fh *fh, > > > + struct v4l2_subdev_mbus_code_enum *ce) > > > +{ > > > + struct soc_camera_device *icd = v4l2_get_subdevdata(sd); > > > + > > > + if (ce->index >= icd->num_user_formats) > > > + return -EINVAL; > > > + > > > + ce->code = icd->user_formats[ce->index].code; > > > + return 0; > > > +} > > > + > > > +static const struct v4l2_subdev_pad_ops se_bus_sd_pad_ops = { > > > + .get_fmt = bus_sd_pad_g_fmt, > > > + .set_fmt = bus_sd_pad_s_fmt, > > > + .enum_mbus_code = bus_sd_pad_enum_mbus_code, > > > +}; > > > + > > > +static const struct v4l2_subdev_ops se_bus_sd_ops = { > > > + .pad = &se_bus_sd_pad_ops, > > > +}; > > > + > > > +static const struct media_entity_operations se_bus_me_ops = { > > > +}; > > > + > > > +static const struct media_entity_operations se_vdev_me_ops = { > > > +}; > > > > NULL operations are allowed, you don't have to use an empty structure. > > Ok > > > > + > > > +int soc_camera_mc_streamon(struct soc_camera_device *icd) > > > +{ > > > + struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > > > + struct v4l2_subdev *bus_sd = &ici->bus_sd; > > > + struct media_entity *bus_me = &bus_sd->entity; > > > + struct v4l2_subdev *sd = soc_camera_to_subdev(icd); > > > + struct v4l2_mbus_framefmt mf; > > > + int ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf); > > > + if (WARN_ON(ret < 0)) > > > + return ret; > > > + if (icd->host_input_width != mf.width || > > > + icd->host_input_height != mf.height || > > > + icd->current_fmt->code != mf.code) > > > + return -EINVAL; > > > > Shouldn't you also check that the source pad format matches the video > > node format ? > > I think, that's true by construction. It is already cheked in > soc_camera_set_fmt(): > > } else if (!icd->current_fmt || > icd->current_fmt->host_fmt->fourcc != pix->pixelformat) { > dev_err(icd->pdev, > "Host driver hasn't set up current format correctly!\n"); > return -EINVAL; But aren't applications allowed to configure the format at the bridge output pad first ? > > > + > > > + media_entity_pipeline_start(bus_me, &ici->pipe); > > > + return 0; > > > +} > > > + > > > +void soc_camera_mc_streamoff(struct soc_camera_device *icd) > > > +{ > > > + struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > > > + struct v4l2_subdev *bus_sd = &ici->bus_sd; > > > + struct media_entity *bus_me = &bus_sd->entity; > > > + media_entity_pipeline_stop(bus_me); > > > +} > > > + > > > +int soc_camera_mc_install(struct soc_camera_device *icd) > > > +{ > > > + struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > > > + struct v4l2_subdev *bus_sd = &ici->bus_sd; > > > + struct media_entity *bus_me = &bus_sd->entity; > > > + struct media_pad *bus_pads = ici->bus_pads; > > > + struct media_pad *vdev_pads = ici->vdev_pads; > > > + struct video_device *vdev = icd->vdev; > > > + struct media_entity *vdev_me = &vdev->entity; > > > + struct v4l2_subdev *sd = soc_camera_to_subdev(icd); > > > + int ret; > > > + > > > + if (!ici->v4l2_dev.mdev || soc_entity_native_mc(icd)) > > > + return 0; > > > + > > > + /* Configure the video bus subdevice, entity, and pads */ > > > + v4l2_subdev_init(bus_sd, &se_bus_sd_ops); > > > + v4l2_set_subdevdata(bus_sd, icd); > > > + bus_sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > > + snprintf(bus_sd->name, sizeof(bus_sd->name), "%s input", > > > ici->drv_name); + > > > + bus_pads[SOC_HOST_BUS_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > > > + bus_pads[SOC_HOST_BUS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > > > + bus_me->ops = &se_bus_me_ops; > > > + > > > + ret = media_entity_init(bus_me, 2, bus_pads, 0); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* Configure the video-device entity */ > > > + vdev_pads[SOC_HOST_VDEV_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > > > + vdev_me->ops = &se_vdev_me_ops; > > > + > > > + ret = media_entity_init(vdev_me, 1, vdev_pads, 0); > > > + if (ret < 0) > > > + goto evmei; > > > + > > > + /* Link the two entities */ > > > + ret = media_entity_create_link(bus_me, SOC_HOST_BUS_PAD_SOURCE, > > > + vdev_me, SOC_HOST_VDEV_PAD_SINK, > > > + MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED); > > > + if (ret < 0) > > > + goto elink; > > > + > > > + ret = v4l2_device_register_subdev(&ici->v4l2_dev, bus_sd); > > > + if (ret < 0) > > > + goto eregsd; > > > + > > > + ret = v4l2_device_register_subdev_nodes(&ici->v4l2_dev); > > > + if (ret < 0) > > > + goto eregsdn; > > > + > > > + /* > > > + * Link the client: make it immutable too for now, since there is no > > > + * meaningful mapping for the .link_setup() method to the soc-camera > > > + * API > > > + */ > > > + ret = media_entity_create_link(&sd->entity, 0, > > > + bus_me, SOC_HOST_BUS_PAD_SINK, > > > + MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED); > > > + if (ret < 0) > > > + goto eclink; > > > > Qhat do you think about moving this above subdev registration ? > > I don't yet:-) But yes, perhaps, this can be done. -- Regards, Laurent Pinchart