Hi Chen-Yu, The series looks good, thanks for picking up this task. Just a one comment. On Fri, Jan 07, 2022 at 05:34:48PM +0800, Chen-Yu Tsai wrote: > In the earlier submissions of the Hantro/Rockchip JPEG encoder driver, a > wmb() was inserted before the final register write that starts the > encoder. In v11, it was removed and the second-to-last register write > was changed to a non-relaxed write, which has an implicit wmb() [1]. > The rockchip_vpu2 (then rk3399_vpu) variant is even weirder as there > is another writel_relaxed() following the non-relaxed one. > > Turns out only the last writel() needs to be non-relaxed. Device I/O > mappings already guarantee strict ordering to the same endpoint, and > the writel() triggering the hardware would force all writes to memory > to be observed before the writel() to the hardware is observed. > > [1] https://lore.kernel.org/linux-media/CAAFQd5ArFG0hU6MgcyLd+_UOP3+T_U-aw2FXv6sE7fGqVCVGqw@xxxxxxxxxxxxxx/ > > Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx> > --- > drivers/staging/media/hantro/hantro_h1_jpeg_enc.c | 3 +-- > drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c | 3 +-- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > index 1450013d3685..03db1c3444f8 100644 > --- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > +++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > @@ -123,8 +123,7 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx) > | H1_REG_AXI_CTRL_INPUT_SWAP32 > | H1_REG_AXI_CTRL_OUTPUT_SWAP8 > | H1_REG_AXI_CTRL_INPUT_SWAP8; > - /* Make sure that all registers are written at this point. */ > - vepu_write(vpu, reg, H1_REG_AXI_CTRL); > + vepu_write_relaxed(vpu, reg, H1_REG_AXI_CTRL); > As far as I can remember, this logic comes from really old Chromium Kernels. You might be right, and this barrier isn't needed... but then OTOH the comment is here for a reason, so maybe it is needed (or was needed on some RK3288 SoC revision). I don't have RK3288 boards near me, but in any case, I'm not sure we'd be able to test this easily (maybe there are issues that only trigger under a certain load). I'd personally avoid this one change, but if you are confident enough with it that's fine too. Thanks! Ezequiel > reg = H1_REG_ENC_CTRL_WIDTH(MB_WIDTH(ctx->src_fmt.width)) > | H1_REG_ENC_CTRL_HEIGHT(MB_HEIGHT(ctx->src_fmt.height)) > diff --git a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > index 4df16f59fb97..b931fc5fa1a9 100644 > --- a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > +++ b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > @@ -152,8 +152,7 @@ int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx) > | VEPU_REG_INPUT_SWAP8 > | VEPU_REG_INPUT_SWAP16 > | VEPU_REG_INPUT_SWAP32; > - /* Make sure that all registers are written at this point. */ > - vepu_write(vpu, reg, VEPU_REG_DATA_ENDIAN); > + vepu_write_relaxed(vpu, reg, VEPU_REG_DATA_ENDIAN); > > reg = VEPU_REG_AXI_CTRL_BURST_LEN(16); > vepu_write_relaxed(vpu, reg, VEPU_REG_AXI_CTRL); > -- > 2.34.1.575.g55b058a8bb-goog >
