Re: [RESENT PATCH V0 0/4] media: some framework interface extension for new feature of Mediatek Camsys driver

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Louis Kuo <louis.kuo@mediatek.com>,
	sakari.ailus@linux.intel.com, laurent.pinchart@ideasonboard.com,
	mchehab@kernel.org, matthias.bgg@gmail.com, arnd@arndb.de,
	sergey.senozhatsky@gmail.com, helen.koike@collabora.com,
	niklas.soderlund+renesas@ragnatech.se, yepeilin.cs@gmail.com
Cc: frederic.chen@mediatek.com, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [RESENT PATCH V0 0/4] media: some framework interface extension for new feature of Mediatek Camsys driver
Date: Mon, 11 Oct 2021 11:56:32 +0200	[thread overview]
Message-ID: <4109bf9b-7210-6b1c-1614-abb1bbd74c6c@xs4all.nl> (raw)
In-Reply-To: <61393c9e-6f28-a357-8ea1-3590e883f107@xs4all.nl>

On 14/06/2021 12:56, Hans Verkuil wrote:
> Hi Louis,
> 
> On 07/05/2021 09:46, Louis Kuo wrote:
>> Hello,
>>
>> This is the first version of the patch series extending V4L2 and media
>> framework to support some advanced camera function, for example, to change
>> the sensor when ISP is still streaming. A typical scenario is the wide-angle
>> sensor and telephoto sensor switching in camera application. When the user
>> is using the zooming UI, the application needs to switch the sensor from
>> wide-angle sensor to telephoto sensor smoothly.
>>
>> To finish the function, we may need to modify the links of a pipeline and
>> the format of pad and video device per request. Currently, the link,
>> pad and video device format and selection settings are not involved in
>> media request's design. Therefore, we try to extend the related interface
>> to support the request-based operations. In the early version, we added
>> request fd to the parameters of MEDIA_IOC_SETUP_LINK,
>> VIDIOC_S_FMT, VIDIOC_SUBDEV_S_SELECTION, VIDIOC_SUBDEV_S_FMT.
>> The driver uses media_request_get_by_fd() to retrieve the media request
>> and save the pending change in it, so that we can apply the pending change
>> in req_queue() callback then.
>>
>> Here is an example:
>>
>> int mtk_cam_vidioc_s_selection(struct file *file, void *fh,
>> 				struct v4l2_selection *s)
>> {
>> 	struct mtk_cam_device *cam = video_drvdata(file);
>> 	struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
>> 	struct mtk_cam_request_stream_data *stream_data;
>> 	struct mtk_cam_request *cam_req;
>> 	struct media_request *req;
>> 	s32 fd;
>>
>> 	fd = s->request_fd;
>> 	if (fd < 0)
>> 		return -EINVAL;
>>
>> 	req = media_request_get_by_fd(&cam->media_dev, fd);
>>
>> 	/* .... */
>>  
>> 	cam_req = to_mtk_cam_req(req);
>> 	stream_data = &cam_req->stream_data[node->uid.pipe_id];
>> 	stream_data->vdev_selection_update |= (1 << node->desc.id);
>> 	stream_data->vdev_selection[node->desc.id] = *s;
>>
>> 	/* .... */
>>
>> 	media_request_put(req);
>>
>> 	return 0;
>> }
>>
>> I posted interface change to discuss first and would like some
>> review comments.
>>
>> Thank you very much.
> 
> Just adding a request_fd in several places is the easy bit. The much
> harder part is where to store that information, and even harder is an
> outstanding issue with the request framework:
> 
> Currently the request framework is only used with decoder drivers, so
> there are no subdev drivers involved. I suspect that there is a fair
> amount of work to do to make it work well if part of the request configuration
> is for subdev drivers.
> 
> Ideally I would like to see a proof-of-concept with the vimc driver.
> 
> I think getting this right is quite a lot of work. The public API part
> is just a minor part of that since the public API was designed with support
> for this in mind. It's the internal kernel support that is lacking.
> 
> If you want to pursue this (and that would be great!), then start with
> vimc and initially just support controls in a request. The core problem
> is likely to be how to keep track of the request data if it is spread
> out between the bridge driver and subdev drivers, and that can be tested
> with just supporting controls.
> 
> Adding support for formats and selection rectangles is, I think, much less
> difficult and can be addressed later. Changing the topology in a request
> is a separate issue as well, and I would suggest that you postpone that.
> There is some low-level work going on that might make this easier in the
> near future (1), we'll have to wait and see.

Just FYI: I have not heard anything about this since my reply, so I am marking
this series as RFC in patchwork.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
> (1): https://patchwork.linuxtv.org/project/linux-media/cover/20210524104408.599645-1-tomi.valkeinen@ideasonboard.com/
> 
>>
>>   media: v4l2-core: extend the v4l2 format to support request
>>   media: subdev: support which in v4l2_subdev_frame_interval
>>   media: v4l2-ctrl: Add ISP Camsys user control
>>   media: pixfmt: Add ISP Camsys formats
>>
>>  drivers/media/mc/mc-device.c         |   7 +-
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 153 ++++++++++++++++++++++++++-
>>  include/media/media-entity.h         |   3 +
>>  include/uapi/linux/media.h           |   3 +-
>>  include/uapi/linux/v4l2-controls.h   |   4 +
>>  include/uapi/linux/v4l2-subdev.h     |   8 +-
>>  include/uapi/linux/videodev2.h       | 109 ++++++++++++++++++-
>>  7 files changed, 275 insertions(+), 12 deletions(-)
>>
>>
> 