Re: [PATCH v7] media: imx: add mem2mem device

[Hans Verkuil <hverkuil@xs4all.nl>]

On 1/18/19 12:18 PM, Philipp Zabel wrote:
> Hi Hans,
> 
> On Fri, 2019-01-18 at 10:30 +0100, Hans Verkuil wrote:
>> On 1/17/19 4:50 PM, Philipp Zabel wrote:
> [...]
>>> +
>>> +static const struct video_device ipu_csc_scaler_videodev_template = {
>>> +	.name		= "ipu0_ic_pp mem2mem",
>>
>> I would expect to see something like 'imx-media-csc-scaler' as the name.
>> Wouldn't that be more descriptive?
> 
> Yes, thank you. I'll change this as well.
> 
> [...]
>>> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
>>> index 4b344a4a3706..fee2ece0a6f8 100644
>>> --- a/drivers/staging/media/imx/imx-media-dev.c
>>> +++ b/drivers/staging/media/imx/imx-media-dev.c
>>> @@ -318,12 +318,36 @@ static int imx_media_probe_complete(struct v4l2_async_notifier *notifier)
>>>  		goto unlock;
>>>  
>>>  	ret = v4l2_device_register_subdev_nodes(&imxmd->v4l2_dev);
>>> -unlock:
>>> -	mutex_unlock(&imxmd->mutex);
>>>  	if (ret)
>>> -		return ret;
>>> +		goto unlock;
>>> +
>>> +	imxmd->m2m_vdev = imx_media_csc_scaler_device_init(imxmd);
>>> +	if (IS_ERR(imxmd->m2m_vdev)) {
>>> +		ret = PTR_ERR(imxmd->m2m_vdev);
>>> +		goto unlock;
>>> +	}
>>>  
>>> -	return media_device_register(&imxmd->md);
>>> +	ret = imx_media_csc_scaler_device_register(imxmd->m2m_vdev);
>>> +	if (ret)
>>> +		goto m2m_remove;
>>> +
>>> +	mutex_unlock(&imxmd->mutex);
>>> +
>>> +	ret = media_device_register(&imxmd->md);
>>> +	if (ret) {
>>> +		mutex_lock(&imxmd->mutex);
>>> +		goto m2m_unreg;
>>> +	}
>>
>> I am missing a call to v4l2_m2m_register_media_controller() to ensure that this
>> device shows up in the media controller.
> 
> I can do that, but what would be the purpose of it showing up in the
> media controller?
> There is nothing to be configured, no interaction with the rest of the
> graph, and the processing subdevice wouldn't even correspond to an
> actual hardware unit. I assumed this would clutter the media controller
> for no good reason.

Just because you can't change routing doesn't mean you can't expose it in the
media controller topology. It makes sense to show in the topology that you
have this block.

That said, I can't decide whether or not to add this. For a standalone m2m
device I would not require it, but in this case you already have a media
device.

I guess it is easy enough to add later, so leave this for now.

> 
> [...]
>>> @@ -262,6 +265,13 @@ void imx_media_capture_device_set_format(struct imx_media_video_dev *vdev,
>>>  					 struct v4l2_pix_format *pix);
>>>  void imx_media_capture_device_error(struct imx_media_video_dev *vdev);
>>>  
>>> +/* imx-media-mem2mem.c */
>>> +struct imx_media_video_dev *
>>> +imx_media_csc_scaler_device_init(struct imx_media_dev *dev);
>>> +void imx_media_csc_scaler_device_remove(struct imx_media_video_dev *vdev);
>>> +int imx_media_csc_scaler_device_register(struct imx_media_video_dev *vdev);
>>> +void imx_media_csc_scaler_device_unregister(struct imx_media_video_dev *vdev);
>>> +
>>>  /* subdev group ids */
>>>  #define IMX_MEDIA_GRP_ID_CSI2      BIT(8)
>>>  #define IMX_MEDIA_GRP_ID_CSI_BIT   9
>>
>> How did you test the rotate control? I'm asking because I would expect to see code
>> that checks this control in the *_fmt ioctls: rotating 90 or 270 degrees would mean
>> that the reported width and height are swapped for the capture queue. And I see no
>> sign that that is done. For the same reason this should also impact the g/s_selection
>> code.
> 
> I'm probably misunderstanding something.
> 
> Which "reported width and height" have to be swapped compared to what?
> Since this is a scaler, the capture queue has its own width and height,
> independent of the output queue width and height.
> What currently happens is that the chosen capture width and height stay
> the same upon rotation, so the image is stretched into the configured
> dimensions.
> 
> The V4L2_CID_ROTATE documentation [1] states:
> 
>     Rotates the image by specified angle. Common angles are 90, 270 and
>     180. Rotating the image to 90 and 270 will reverse the height and
>     width of the display window. It is necessary to set the new height
>     and width of the picture using the VIDIOC_S_FMT ioctl according to
>     the rotation angle selected.
> 
> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/control.html#control-ids
> 
> I didn't understand what the "display window" is in the context of a
> mem2mem scaler/rotator/CSC converter. Is this intended to mean that
> aspect ratio should be kept as intact as possible, and that every time
> V4L2_CID_ROTATE changes between 0/180 and 90/270, an automatic S_FMT
> should be issued on the capture queue with width and height switched
> compared to the currently set value? This might still slightly modify
> width and height due to alignment restrictions.

Most drivers that implement rotate do not have a scaler, so rotating a
640x480 image would result in a 480x640 result. Hence for an m2m device
the output queue would have format 640x480 and the capture queue 480x640.

So the question becomes: what if you can both rotate and scale, what
do you do when you change the rotate control?

I would expect as a user of this API that if I first scale 640x480 to
320x240, then rotate 90 degrees, that the capture format is now 240x320.

In other words, rotating comes after scaling.

But even if you keep the current behavior I suspect you still need to
update the format due to alignment restriction. Either due to 4:2:2
formats or due to the 'resizer cannot downsize more than 4:1' limitation.

E.g. in the latter case it is fine to downscale 640x480 to 640x120,
but if you now rotate 90 degrees, then you can no longer downscale
480x640 to 640x120 (640 / 120 > 4).

At least, if I understand the code correctly.

There may also be a corner case with rotate and MIN_W/MIN_H resolutions
(MAX_W == MAX_H, so that's probably fine).

It might be a good idea to set MIN_H equal to MIN_W to simplify the rotate
code.

Regards,

	Hans