On Wed, Nov 28, 2018 at 10:05 AM Ezequiel Garcia [off-list ref] wrote:

On Tue, 2018-11-27 at 19:09 +0900, Tomasz Figa wrote:

quoted

On Fri, Nov 23, 2018 at 5:24 AM Ezequiel Garcia [off-list ref] wrote:
[snip]

quoted

quoted

quoted

+const struct rockchip_vpu_variant rk3288_vpu_variant = {
+       .enc_offset = 0x0,
+       .enc_fmts = rk3288_vpu_enc_fmts,
+       .num_enc_fmts = ARRAY_SIZE(rk3288_vpu_enc_fmts),
+       .codec_ops = rk3288_vpu_codec_ops,
+       .codec = RK_VPU_CODEC_JPEG,
+       .vepu_irq = rk3288_vepu_irq,
+       .init = rk3288_vpu_hw_init,
+       .clk_names = {"aclk", "hclk"},

nit: Spaces inside the brackets.

You mean you this style is prefered?

.clk_names = { "aclk", "hclk" },

Grepping thru sources, it seems there is no convention on this,
so it's your call.

I thought this is a part of CodingStyle, but it doesn't seem to
mention it. I personally prefer to have the spaces there, but we can
go with your personal preferences here. :)

OK.

quoted

[snip]

quoted

quoted

quoted

+        * by .vidioc_s_fmt_vid_cap_mplane() callback
+        */
+       reg = VEPU_REG_IN_IMG_CTRL_ROW_LEN(pix_fmt->width);
+       vepu_write_relaxed(vpu, reg, VEPU_REG_INPUT_LUMA_INFO);
+
+       reg = VEPU_REG_IN_IMG_CTRL_OVRFLR_D4(0) |
+             VEPU_REG_IN_IMG_CTRL_OVRFLB(0);

For reference, this register controls the input crop, as the offset
from the right/bottom within the last macroblock. The offset from the
right must be divided by 4 and so the crop must be aligned to 4 pixels
horizontally.

OK, I'll add a comment.

I meant to refer to the comment Hans had, about input images of
resolutions that are not of full macroblocks, so the comment would
probably go to the TODO file together with Hans's note.

Got it.

quoted

[snip]

quoted

quoted

quoted

+static inline u32 vepu_read(struct rockchip_vpu_dev *vpu, u32 reg)
+{
+       u32 val = readl(vpu->enc_base + reg);
+
+       vpu_debug(6, "MARK: get reg[%03d]: %08x\n", reg / 4, val);

I remember seeing this "MARK" in the logs when debugging. I don't
think it's desired here.

How about printing "%s(%03d) = %08x\n" for reads and "%s(%08x,
%03d)\n" for writes?

Actually, I missed the 0x prefix for hex values and possibly also
changing the decimal register offset to hexadecimal:
"%s(0x%04x) = 0x%08x\n"

quoted

Makes sense, but why a %s string format?

For __func__, so that we end up with messages like:

vepu_write(0x12345678, 0x0123)
vepu_read(0x0123) = 0x12345678

vepu_debug already uses __func__, so it would look like this:

[   27.125090] vepu_write_relaxed:215: 0x001f = 0x01010101
[   27.127413] vepu_write:221: 0x0069 = 0xfc000000
[   27.129673] vepu_write_relaxed:215: 0x0036 = 0x00001000
[   27.132079] vepu_write:221: 0x0067 = 0x00f01461
[   27.135042] vepu_read:229: 0x006d = 0x00001003
[   27.137450] vepu_read:229: 0x0035 = 0x00020318
[   27.139778] vepu_write:221: 0x006d = 0x00000000
[   27.142144] vepu_write:221: 0x0036 = 0x00000000

Unless we use a different debug macro for i/o access.

Okay, I missed that part of vepu_debug(). We can drop the %s and
explicit __func__ from the message then.

quoted

[snip]

quoted

quoted

quoted

+       dst->field = src->field;
+       dst->timecode = src->timecode;

Time code is only valid if the buffer has V4L2_BUF_FLAG_TIMECODE set.
I don't think there is any use case for mem2mem devices for it.

Right. Other mem2mem drivers seem to pass thru the timecode like this:

        if (in_vb->flags & V4L2_BUF_FLAG_TIMECODE)
                out_vb->timecode = in_vb->timecode;

It fails a v4l2-compliance test without it.

Hmm, I don't see the spec defining it to be copied by a mem2mem device
and I wonder if it's actually of any use for those. Hans, could you
comment on this?

I asked Hans about this and he said timecode should behave as timestamp,
and be carried from the output queue to the capture queue.

This helper will take care of it: https://patchwork.linuxtv.org/patch/52961/

Fair enough.

Best regards,
Tomasz