Re: [PATCH v3 3/4] [media] exynos-scaler: Add m2m functionality for the SCALER driver

[Hans Verkuil <hverkuil@xs4all.nl>]

On 09/30/2013 12:48 PM, Shaik Ameer Basha wrote:
> Hi Hans,
> 
> 
> On Mon, Sep 30, 2013 at 4:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 09/30/2013 11:32 AM, Shaik Ameer Basha wrote:
>>> Hi Hans,
>>>
>>> Thanks for pointing it out.
>>>
>>>
>>> On Mon, Sep 30, 2013 at 1:38 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>> Hi Shaik,
>>>>
>>>> I have a few questions regarding the selection part...
>>>>
>>>> On 09/12/2013 03:09 PM, Shaik Ameer Basha wrote:
>>>>> This patch adds the Makefile and memory to memory (m2m) interface
>>>>> functionality for the SCALER driver.
>>>>>
>>>>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>>>>> ---
>>>>>  drivers/media/platform/Kconfig                    |    8 +
>>>>>  drivers/media/platform/Makefile                   |    1 +
>>>>>  drivers/media/platform/exynos-scaler/Makefile     |    3 +
>>>>>  drivers/media/platform/exynos-scaler/scaler-m2m.c |  781 +++++++++++++++++++++
>>>>>  4 files changed, 793 insertions(+)
>>>>>  create mode 100644 drivers/media/platform/exynos-scaler/Makefile
>>>>>  create mode 100644 drivers/media/platform/exynos-scaler/scaler-m2m.c
>>>>>
>>>>
>>>
>>>
>>> [...]
>>>
>>>
>>>>> +
>>>>> +static int scaler_m2m_s_selection(struct file *file, void *fh,
>>>>> +                             struct v4l2_selection *s)
>>>>> +{
>>>>> +     struct scaler_frame *frame;
>>>>> +     struct scaler_ctx *ctx = fh_to_ctx(fh);
>>>>> +     struct v4l2_crop cr;
>>>>> +     struct scaler_variant *variant = ctx->scaler_dev->variant;
>>>>> +     int ret;
>>>>> +
>>>>> +     cr.type = s->type;
>>>>> +     cr.c = s->r;
>>>>> +
>>>>> +     if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) &&
>>>>> +         (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     ret = scaler_try_crop(ctx, &cr);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     if (s->flags & V4L2_SEL_FLAG_LE &&
>>>>> +         !is_rectangle_enclosed(&cr.c, &s->r))
>>>>> +             return -ERANGE;
>>>>> +
>>>>> +     if (s->flags & V4L2_SEL_FLAG_GE &&
>>>>> +         !is_rectangle_enclosed(&s->r, &cr.c))
>>>>> +             return -ERANGE;
>>>>> +
>>>>> +     s->r = cr.c;
>>>>> +
>>>>> +     switch (s->target) {
>>>>> +     case V4L2_SEL_TGT_COMPOSE_BOUNDS:
>>>>> +     case V4L2_SEL_TGT_COMPOSE_DEFAULT:
>>>>> +     case V4L2_SEL_TGT_COMPOSE:
>>>>> +             frame = &ctx->s_frame;
>>>>> +             break;
>>>>> +
>>>>> +     case V4L2_SEL_TGT_CROP_BOUNDS:
>>>>> +     case V4L2_SEL_TGT_CROP:
>>>>> +     case V4L2_SEL_TGT_CROP_DEFAULT:
>>>>> +             frame = &ctx->d_frame;
>>>>> +             break;
>>>>
>>>> Similar problems as with g_selection above. Tomasz mentioned to me that the selection
>>>> API is not implemented correctly in m2m Samsung drivers. It looks like this code is
>>>> copied-and-pasted from other drivers, so it seems he was right.
>>>
>>> Sorry, after going through the documentation, I have to agree with you...
>>> As you mentioned, this part of the code was copied while implementing
>>> the G-Scaler driver :)
>>>
>>> I will change the above implementation for M2M devices (GScaler and
>>> SCALER) as below,
>>> I will only allow all V4L2_SEL_TGT_COMPOSE_* target requests if
>>> 's->type' is equal to "V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE".
>>> and all V4L2_SEL_TGT_CROP_* target requests if 's->type' is equal to
>>> "V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE".
>>>
>>> I hope with the above two checkings taken in to care, there should not
>>> be any issues with using selection APIs here.
>>
>> Well, that depends on what the hardware does.
>>
>> Using compose with a capture buffer means that the frame as delivered by
>> the hardware is composed into a larger buffer. E.g. the hardware gives
>> you 1280x720 which is composed into a buffer of size 1920x1080.
>>
>> Using crop with an output buffer means that the hardware gets a cropped
>> part of a larger frame. E.g. you give a 1280x720 crop from a larger 1920x1080
>> buffer.
>>
>> I suspect however, that in this case the hardware does the opposite for
>> capture: you really want to crop with a capture buffer (e.g. the hardware
>> delivers a 1280x720 frame which is cropped before DMA to 640x360).
>>
>> I'm not sure what you want to do with an output buffer: cropping or composing.
>>
>> Tomasz mentioned that the M2M + selection API was screwy, and this seems to
>> be to be the case indeed.
>>
>> Which is also why I would like to know exactly what this hardware does.
> 
> This hardware is just a M2M device.
> It accepts one source buffer at a time and does some operations on
> that and saves to the destination buffer.
> Operations like Rotation, Cropping, Scaling, Color Space Conversion
> etc are possible.
> 
> Here when I provide the Output buffer (source buffer), I can apply all
> V4L2_SEL_TGT_CROP_* targets on it.
> That means I can select the whole buffer for processing or apply some
> crop and select that area for further processing.
> 
> similarly, On the capture buffer (output buffer), I can apply
> V4L2_SEL_TGT_COMPOSE_* targets.
> That means I can compose the final output to the complete capture
> frame (dst frame), or I can choose some part of the destination frame.

OK, in that case your proposed changes are indeed correct. Sorry, I had
to check this carefully since it is so easy to get confused :-)

Regards,

	Hans