Hi Hans,
On Mon, Sep 30, 2013 at 4:02 PM, Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxx>
>>>> ---
>>>> 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.
Regards,
Shaik Ameer Basha
>
> Regards,
>
> Hans
>
>>
>> Thanks,
>> Shaik Ameer Basha
>>
>>>
>>> The selection API for m2m devices will be discussed during the upcoming V4L2 mini-summit
>>> since the API may actually need some adjustments to have it work the way it should.
>>>
>>> As requested above, if you can explain the exact functionality you are trying to
>>> implement here, then I can look over this code carefully and see how it should be done.
>>>
>>> Thanks!
>>>
>>> Hans
>>>
>> [...]
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
