On 5/10/19 3:52 PM, Michael Tretter wrote:
> On Fri, 10 May 2019 12:58:43 +0200, Hans Verkuil wrote:
>> On 5/10/19 12:28 PM, Michael Tretter wrote:
>>> On Fri, 10 May 2019 10:28:53 +0200, Hans Verkuil wrote:
>>>> On 5/3/19 2:20 PM, Michael Tretter wrote:
>>>>> Add a V4L2 mem-to-mem driver for Allegro DVT video IP cores as found in
>>>>> the EV family of the Xilinx ZynqMP SoC. The Zynq UltraScale+ Device
>>>>> Technical Reference Manual uses the term VCU (Video Codec Unit) for the
>>>>> encoder, decoder and system integration block.
>>>>>
>>>>> This driver takes care of interacting with the MicroBlaze MCU that
>>>>> controls the actual IP cores. The IP cores and MCU are integrated in the
>>>>> FPGA. The xlnx_vcu driver is responsible for configuring the clocks and
>>>>> providing information about the codec configuration.
>>>>>
>>>>> The driver currently only supports the H.264 video encoder.
>>>>>
>>>>> Signed-off-by: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
>>>>> ---
>>
>> <snip>
>>
>>>>> +static int allegro_try_fmt_vid_out(struct file *file, void *fh,
>>>>> + struct v4l2_format *f)
>>>>> +{
>>>>> + f->fmt.pix.field = V4L2_FIELD_NONE;
>>>>> +
>>>>> + f->fmt.pix.width = clamp_t(__u32, f->fmt.pix.width,
>>>>> + ALLEGRO_WIDTH_MIN, ALLEGRO_WIDTH_MAX);
>>>>> + f->fmt.pix.height = clamp_t(__u32, f->fmt.pix.height,
>>>>> + ALLEGRO_HEIGHT_MIN, ALLEGRO_HEIGHT_MAX);
>>>>
>>>> Shouldn't this be rounded up to the macroblock size? Or is the encoder
>>>> smart enough to do the padding internally?
>>>
>>> The driver sends a message with the visible size of the raw frames
>>> (without macroblock alignment) to the encoder firmware. Therefore, the
>>> encoder firmware is responsible for handling the padding to macroblock
>>> size.
>>
>> Please add a comment describing this. It is unusual for encoders to be
>> able to do this so it is good to document this.
>
> OK.
>
>>
>>>
>>> Furthermore, the encoder requires that the stride is 32 byte aligned.
>>> Therefore, we naturally have a macroblock alignment regarding the
>>> width, but not regarding the height. This limitation is already
>>> included in the bytesperline field.
>>
>> Ack.
>>
>>>
>>>>
>>>>> +
>>>>> + f->fmt.pix.pixelformat = V4L2_PIX_FMT_NV12;
>>>>> + f->fmt.pix.bytesperline = round_up(f->fmt.pix.width, 32);
>>>>> + f->fmt.pix.sizeimage =
>>>>> + f->fmt.pix.bytesperline * f->fmt.pix.height * 3 / 2;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int allegro_s_fmt_vid_out(struct file *file, void *fh,
>>>>> + struct v4l2_format *f)
>>>>> +{
>>>>> + struct allegro_channel *channel = fh_to_channel(fh);
>>>>> + int err;
>>>>> +
>>>>> + err = allegro_try_fmt_vid_out(file, fh, f);
>>>>> + if (err)
>>>>> + return err;
>>>>> +
>>>>> + channel->width = f->fmt.pix.width;
>>>>> + channel->height = f->fmt.pix.height;
>>>>> + channel->stride = f->fmt.pix.bytesperline;
>>>>> + channel->sizeimage_raw = f->fmt.pix.sizeimage;
>>>>> +
>>>>> + channel->colorspace = f->fmt.pix.colorspace;
>>>>> + channel->ycbcr_enc = f->fmt.pix.ycbcr_enc;
>>>>> + channel->quantization = f->fmt.pix.quantization;
>>>>> + channel->xfer_func = f->fmt.pix.xfer_func;
>>>>> +
>>>>> + channel->level =
>>>>> + select_minimum_h264_level(channel->width, channel->height);
>>>>> + channel->sizeimage_encoded =
>>>>> + estimate_stream_size(channel->width, channel->height);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int allegro_g_selection(struct file *file, void *priv,
>>>>> + struct v4l2_selection *s)
>>>>> +{
>>>>> + struct v4l2_fh *fh = file->private_data;
>>>>> + struct allegro_channel *channel = fh_to_channel(fh);
>>>>> +
>>>>> + if (!V4L2_TYPE_IS_OUTPUT(s->type))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + switch (s->target) {
>>>>> + case V4L2_SEL_TGT_CROP:
>>>>> + case V4L2_SEL_TGT_CROP_DEFAULT:
>>>>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>>>>> + s->r.left = 0;
>>>>> + s->r.top = 0;
>>>>> + s->r.width = channel->width;
>>>>> + s->r.height = channel->height;
>>>>
>>>> I don't think this is quite right. The CROP target should return the visible
>>>> width/height (e.g. 1920x1080) whereas the other two targets should return the
>>>> coded width/height (e.g. 1920x1088 when rounded to the macroblock alignment).
>>>>
>>>> Note: if the hardware doesn't require that the raw frame is macroblock aligned,
>>>> then I need to think a bit more about how the selection handling should be
>>>> done.
>>>
>>> The driver internally calculates the coded width/height in macroblocks
>>> and cropping and writes it to the SPS. Currently, this isn't exposed to
>>> userspace, because I don't see a need to tell the userspace about that.
>>>
>>> If there is a reason to expose this to userspace, I am fine with
>>> implementing that.
>>
>> There really is no need for the selection API at all. Just drop both
>> G and S_SELECTION from the driver. Let me know if the compliance test
>> fails for drivers without selection support, I'll have to fix the test
>> in that case.
>
> The compliance test for VIDIOC_S_FMT fails with the following message
> if G_SELECTION is not implemented:
>
> fail: v4l2-test-formats.cpp(836): sel.r.width != fmt.g_width()
> test VIDIOC_S_FMT: FAIL
>
Try this patch:
Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
---
diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
index fc497e3c..544ecb5c 100644
--- a/utils/v4l2-compliance/v4l2-test-formats.cpp
+++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
@@ -828,7 +828,11 @@ static int testM2MFormats(struct node *node)
.type = fmt.g_type(),
.target = V4L2_SEL_TGT_CROP,
};
- node->g_selection(sel);
+ if (node->g_selection(sel) == ENOTTY) {
+ fail_on_test(fmt_cap.g_width() != fmt.g_width());
+ fail_on_test(fmt_cap.g_height() != fmt.g_height());
+ return 0;
+ }
fail_on_test(sel.r.top || sel.r.left);
fail_on_test(sel.r.width != fmt.g_width());
fail_on_test(sel.r.height != fmt.g_height());
------------------------------------------------------------
Regards,
Hans
