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

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.

[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.

[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.

[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/
 

quoted

quoted

quoted

+       dst->vb2_buf.timestamp = src->vb2_buf.timestamp;
+       dst->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+       dst->flags |= src->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;

Not V4L2_BUF_FLAG_TIMESTAMP_COPY?

I believe v4l core should take care of it in __fill_v4l2_buffer,
as timestamp_flags is set when the vb2_queue structs are init'ed.

Ah, okay, I confused this with V4L2_BUF_FLAG_TIMESTAMP_MASK.

quoted

quoted

quoted

+
+       if (bytesused) {

Should we check whether bytesused (read from hardware) is not bigger
than size of the buffer?

Good catch, makes sense. OTOH, if bytesused is bigger than the dst
buffer, it is also bigger than the bounce buffer. I guess the IOMMU
helps prevents nasty issues?

Yes, in that case it would likely trigger an IOMMU fault.

Another thing is whether there isn't some special bit somewhere high
in the register we're reading from that we're not aware of, which
would make the bytesused bigger than the buffer.

[snip]

quoted

quoted

quoted

+void rockchip_vpu_irq_done(struct rockchip_vpu_dev *vpu,
+                          unsigned int bytesused,
+                          enum vb2_buffer_state result)
+{
+       struct rockchip_vpu_ctx *ctx =
+               (struct rockchip_vpu_ctx *)v4l2_m2m_get_curr_priv(vpu->m2m_dev);

I don't think we need to cast from void *?

Right.

quoted

quoted

+
+       /* Atomic watchdog cancel. The worker may still be
+        * running after calling this.
+        */

Wrong multi-line comment style.

Right.

quoted

quoted

+       cancel_delayed_work(&vpu->watchdog_work);
+       if (ctx)
+               rockchip_vpu_job_finish(vpu, ctx, bytesused, result);
+}
+
+void rockchip_vpu_watchdog(struct work_struct *work)
+{
+       struct rockchip_vpu_dev *vpu;
+       struct rockchip_vpu_ctx *ctx;
+
+       vpu = container_of(to_delayed_work(work),
+                          struct rockchip_vpu_dev, watchdog_work);
+       ctx = (struct rockchip_vpu_ctx *)v4l2_m2m_get_curr_priv(vpu->m2m_dev);

Ditto.

quoted

+       if (ctx) {

Is !ctx possible here?

Yes, it's possible because cancel_delayed_work doesn't flush
the worker, so the top-half competes with the watchdog delayed
worker.

Ah, right. What prevents a race between rockchip_vpu_irq_done() and
rockchip_vpu_watchdog() and both running rockchip_job_finish() on the
same context?

Right, missed that. It would be possible to end up calling
rockchip_job_finish twice.

Perhaps in rockchip_vpu_irq_done() we could check the return value of
cancel_delayed_work() and if it's false, just give up and let the
watchdog handle the current ctx?

Yes, that should work.

[..]

quoted

quoted

quoted

+static int __maybe_unused rockchip_vpu_runtime_resume(struct device *dev)
+{
+       struct rockchip_vpu_dev *vpu = dev_get_drvdata(dev);
+
+       return clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);

Something for the TODO list: We should disable the clocks as soon as
the hardware becomes idle, because it's super cheap and the delay
between the idle and autosuspend is quite significant.

You mean getting rid of autosuspend, right?

Not exactly. The autosuspend is to avoid too frequent *power*
transitions, which could be costly. Actually, even if we get rid of
it, the genpd code will still throttle runtime suspends based on the
PM domain suspend/resume latency it measured.

The only way to control the clocks without throttling is managing them
explicitly, just before the call to pm_runtime_put_autosuspend() or
after pm_runtime_get_sync().

OK.

[..]

quoted

Right. However, it seems to me we still need to set a value,
and return it to userspace when userspace doesn't provide any.

There was another discussion about this wrt the venus driver and the
conclusion is to specify this for codecs as follows:

    For compressed formats (and only those!) userspace can set sizeimage to a
    proposed value. The driver may either ignore it and just set its own value,
    or modify it to satisfy HW requirements. The returned value will be used
    by REQBUFS when it allocates buffers.

So I guess the driver could reset it to default if it doesn't sound
reasonable (0?).

OK.

[snip]

quoted

quoted

quoted

+void rockchip_vpu_enc_reset_src_fmt(struct rockchip_vpu_dev *vpu,
+                                   struct rockchip_vpu_ctx *ctx)
+{
+       struct v4l2_pix_format_mplane *fmt = &ctx->src_fmt;
+       unsigned int width, height;
+
+       ctx->vpu_src_fmt = rockchip_vpu_get_default_fmt(ctx, false);
+
+       memset(fmt, 0, sizeof(*fmt));
+
+       width = clamp(fmt->width, ctx->vpu_dst_fmt->frmsize.min_width,
+                     ctx->vpu_dst_fmt->frmsize.max_width);
+       height = clamp(fmt->height, ctx->vpu_dst_fmt->frmsize.min_height,
+                      ctx->vpu_dst_fmt->frmsize.max_height);
+       fmt->field = V4L2_FIELD_NONE;
+       fmt->colorspace = ctx->colorspace;
+       fmt->ycbcr_enc = ctx->ycbcr_enc;
+       fmt->xfer_func = ctx->xfer_func;
+       fmt->quantization = ctx->quantization;

Ditto.

quoted

+
+       fill_pixfmt_mp(fmt, ctx->vpu_src_fmt->fourcc, width, height);
+}

These two don't seem to be very encoder-specific. In particular,
rockchip_vpu_enc_reset_dst_fmt() is not even used by the encoder after
the context is initialized, but it's going to be used by the decoder
to reset the raw format when the coded format changes (similarly to
rockchip_vpu_enc_reset_src_fmt() for encoder).

Yeah, I'm aware there might be some commong logic here, but since
the driver is an encoder only for now, I decided to let it be.

However, I've made sure the driver is ready to support
decoding and other variants, with fairly simple changes.

Fair enough.

[snip]

quoted

quoted

quoted

+void rockchip_vpu_jpeg_render(struct rockchip_vpu_jpeg_ctx *ctx)

nit: I'm not sure "render" is the right word here. Maybe it's just me,
but I associate it with rendering of visible graphics. Perhaps
"assemble" would be better?

I don't think render is gfx only, but.. it might be confusing, so
how about "rockchip_vpu_jpeg_header_prepare" or "rockchip_vpu_jpeg_header_set" ?

This function might be moved to lib/jpeg.c, so getting the name
right is not that much of a nitpick.

rockchip_vpu_jpeg_header_assemble()? (My preference for "assemble"
comes from building the header from the pieces as the function does.)

OK, let's go with _assemble.

Thanks,
Ezequiel