[bug] omap3isp: missing support for ENUM_FMT

Commit Message

Pavel Machek April 26, 2017, 9:19 p.m. UTC

Hi!

Currently, ispvideo.c does not support enum_format. This causes
problems for example for libv4l2.

Now, I'm pretty sure patch below is not the right fix. But it fixes
libv4l2 problem for me.

Pointer to right solution welcome.

Regards,
									Pavel


On Sat 2017-03-04 20:44:50, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Saturday 04 Mar 2017 17:39:46 Sakari Ailus wrote:
> > On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> > > On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> > >> 
> > >>>> Ok, how about this one?
> > >>>> omap3isp: add rest of CSI1 support
> > >>>> 
> > >>>> CSI1 needs one more bit to be set up. Do just that.
> > >>>> 
> > >>>> It is not as straightforward as I'd like, see the comments in the
> > >>>> code for explanation.
> > >>
> > >> ...
> > >> 
> > >>>> +	if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > >>>> +		struct media_pad *pad;
> > >>>> +		struct v4l2_subdev *sensor;
> > >>>> +		const struct isp_ccp2_cfg *buscfg;
> > >>>> +
> > >>>> +		pad = media_entity_remote_pad(&ccp2
> > >>>> ->pads[CCP2_PAD_SINK]);
> > >>>> +		sensor = media_entity_to_v4l2_subdev(pad->entity);
> > >>>> +		/* Struct isp_bus_cfg has union inside */
> > >>>> +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)
> > >>>> ->bus.ccp2;
> > >>>> +
> > >>>> +		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > >>>> +					ISP_INTERFACE_CCP2B_PHY1,
> > >>> > +					enable, !!buscfg->phy_layer,
> > >>> > +					buscfg->strobe_clk_pol);
> > >>> 
> > >>> You should do this through omap3isp_csiphy_acquire(), and not call
> > >>> csiphy_routing_cfg_3430() directly from here.
> > >> 
> > >> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> > >> assumptions hard-coded :-(.
> > >> 
> > >> This will probably fail.
> > >> 
> > >> 	        rval = omap3isp_csi2_reset(phy->csi2);
> > >> 	        if (rval < 0)
> > >> 		                goto done;
> > > 
> > > Could you try to two patches I've applied on the ccp2 branch (I'll remove
> > > them if there are issues).
> > > 
> > > That's compile tested for now only.
> > 
> > One more thing. What's needed for configuring the PHY for CCP2?
> > 
> > For instance, is the CSI-2 PHY regulator still needed in
> > omap3isp_csiphy_acquire()? One way to do this might go to see the original
> > driver for N900; I don't have the TRM at hand right now.
> 
> The OMAP34xx TRM and data manual both mention separate VDDS power supplies for 
> the CSIb and CSI2 I/O complexes.
> 
> vdds_csi2		CSI2 Complex I/O
> vdds_csib		CSIb Complex I/O
> 
> On OMAP36xx, we instead have
> 
> vdda_csiphy1		Input power for camera PHY buffer
> vdda_csiphy2		Input power for camera PHY buffer
> 
> We need to enable the vds_csib regulator to operate the CSI1/CCP2 PHY, but 
> that regulator gets enabled in ispccp2.c as that module is powered by the 
> vdds_csib supply on OMAP34xx. However, it won't hurt to do so, and the code 
> could be simpler if we manage the regulators the same way on OMAP34xx and 
> OMAP36xx.
>

Comments

Sakari Ailus April 28, 2017, 7:59 a.m. UTC | #1

Hi Pavel,

On Wed, Apr 26, 2017 at 11:19:33PM +0200, Pavel Machek wrote:
> Hi!
> 
> Currently, ispvideo.c does not support enum_format. This causes
> problems for example for libv4l2.
> 
> Now, I'm pretty sure patch below is not the right fix. But it fixes
> libv4l2 problem for me.
> 
> Pointer to right solution welcome.

The issue with providing ENUM_FMT support on MC-enabled drivers is that the
media bus format has an effect to which pixel formats are available.

What has been discussed in the past but what remains unimplemented is to add
the media bus code to v4l2_fmtdesc for user to choose. That would allow
meaningful ENUM_FMT support.

For users such as libv4l2, i.e. code == 0 --- it should just tell the user
what it can do with the active media bus format.

Patch

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 218e6d7..2ce0327 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -772,6 +772,44 @@  isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
 }
 
 static int
+isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *format)
+{
+	struct isp_video *video = video_drvdata(file);
+	struct v4l2_subdev_format fmt;
+	struct v4l2_subdev *subdev;
+	u32 pad;
+	int ret;
+
+	printk("ispvideo: enum_fmt\n");
+
+	subdev = isp_video_remote_subdev(video, &pad);
+	if (subdev == NULL) {
+		printk("No subdev\n");
+		//return -EINVAL;
+	}
+
+	//isp_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
+	if (format->index)
+		return -EINVAL;
+	format->type = video->type;
+	format->flags = 0;
+	strcpy(format->description, "subdev description");
+	format->pixelformat = V4L2_PIX_FMT_SGRBG10;
+
+	printk("Returning SRGBG10\n");
+#if 0	
+	fmt.pad = pad;
+	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret)
+		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
+
+	isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
+#endif
+	return 0;
+}
+
+static int
 isp_video_get_selection(struct file *file, void *fh, struct v4l2_selection *sel)
 {
 	struct isp_video *video = video_drvdata(file);
@@ -1276,6 +1314,7 @@  static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
 	.vidioc_g_fmt_vid_cap		= isp_video_get_format,
 	.vidioc_s_fmt_vid_cap		= isp_video_set_format,
 	.vidioc_try_fmt_vid_cap		= isp_video_try_format,
+	.vidioc_enum_fmt_vid_cap        = isp_video_enum_format,
 	.vidioc_g_fmt_vid_out		= isp_video_get_format,
 	.vidioc_s_fmt_vid_out		= isp_video_set_format,
 	.vidioc_try_fmt_vid_out		= isp_video_try_format,