From: Sascha Hauer <s.hauer@pengutronix.de>
To: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
Cc: devicetree@vger.kernel.org,
Benjamin Gaignard <benjamin.gaignard@collabora.com>,
Peter Geis <pgwipeout@gmail.com>,
Sandy Huang <hjc@rock-chips.com>,
dri-devel@lists.freedesktop.org,
linux-rockchip@lists.infradead.org,
Michael Riesch <michael.riesch@wolfvision.net>,
kernel@pengutronix.de, Andy Yan <andy.yan@rock-chips.com>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 22/22] drm: rockchip: Add VOP2 driver
Date: Mon, 3 Jan 2022 15:55:03 +0100 [thread overview]
Message-ID: <20220103145503.GH6003@pengutronix.de> (raw)
In-Reply-To: <1761858.GBYTvM79DV@archbook>
Hi Nicolas,
Thanks for the review. I have changed the points I don't comment on here
according to your suggestions.
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/delay.h>
> > +#include <linux/swab.h>
> > +#include <linux/bitfield.h>
> > +#include <drm/drm_debugfs.h>
> > +#include <uapi/linux/videodev2.h>
> > +#include <drm/drm_vblank.h>
> > +#include <dt-bindings/soc/rockchip,vop2.h>
>
> Alphabetically sort these includes? Not sure on what the
> convention is in the DRM subsystem.
Me neither. The first random file I looked at in drivers/gpu/drm/ has
the includes sorted alphabetically, so I'll do the same.
>
> > +static void vop2_cfg_done(struct vop2_video_port *vp)
> > +{
> > + struct vop2 *vop2 = vp->vop2;
> > + uint32_t val;
> > +
> > + val = vop2_readl(vop2, RK3568_REG_CFG_DONE);
> > +
> > + val &= 0x7;
>
> GENMASK(2, 0) instead of 0x7, should that be a constant somewhere?
>
> > +
> > + vop2_writel(vop2, RK3568_REG_CFG_DONE,
> > + val | BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
Replaced that with the following which doesn't need a mask:
regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE,
BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
> > +}
> > +
> > +static void vop2_win_disable(struct vop2_win *win)
> > +{
> > + vop2_win_write(win, VOP2_WIN_ENABLE, 0);
> > +
> > + if (vop2_cluster_window(win))
> > + vop2_win_write(win, VOP2_WIN_CLUSTER_ENABLE, 0);
> > +}
> > +
> > +static uint32_t vop2_afbc_transform_offset(struct drm_plane_state *pstate,
> > + bool afbc_half_block_en)
> > +{
> > + struct drm_rect *src = &pstate->src;
> > + struct drm_framebuffer *fb = pstate->fb;
> > + uint32_t bpp = fb->format->cpp[0] * 8;
> > + uint32_t vir_width = (fb->pitches[0] << 3) / bpp;
> > + uint32_t width = drm_rect_width(src) >> 16;
> > + uint32_t height = drm_rect_height(src) >> 16;
> > + uint32_t act_xoffset = src->x1 >> 16;
> > + uint32_t act_yoffset = src->y1 >> 16;
> > + uint32_t align16_crop = 0;
> > + uint32_t align64_crop = 0;
> > + uint32_t height_tmp = 0;
> > + uint32_t transform_tmp = 0;
> > + uint8_t transform_xoffset = 0;
> > + uint8_t transform_yoffset = 0;
> > + uint8_t top_crop = 0;
> > + uint8_t top_crop_line_num = 0;
> > + uint8_t bottom_crop_line_num = 0;
> > +
> > + /* 16 pixel align */
> > + if (height & 0xf)
> > + align16_crop = 16 - (height & 0xf);
> > +
> > + height_tmp = height + align16_crop;
> > +
> > + /* 64 pixel align */
> > + if (height_tmp & 0x3f)
> > + align64_crop = 64 - (height_tmp & 0x3f);
> > +
> > + top_crop_line_num = top_crop << 2;
> > + if (top_crop == 0)
> > + bottom_crop_line_num = align16_crop + align64_crop;
> > + else if (top_crop == 1)
> > + bottom_crop_line_num = align16_crop + align64_crop + 12;
> > + else if (top_crop == 2)
> > + bottom_crop_line_num = align16_crop + align64_crop + 8;
>
> top_crop == 0 is always taken from what I can gather, rest is
> dead code. Is this intentional? It doesn't seem intentional.
It's the same in the downstream driver. I don't know what top_crop is
good for. I just removed the dead code for now.
> > + transform_xoffset = transform_tmp & 0xf;
> > + transform_tmp = vir_width - width - act_xoffset;
> > + transform_yoffset = transform_tmp & 0xf;
> > + break;
> > + case 0:
> > + transform_tmp = act_xoffset;
> > + transform_xoffset = transform_tmp & 0xf;
> > + transform_tmp = top_crop_line_num + act_yoffset;
> > +
> > + if (afbc_half_block_en)
> > + transform_yoffset = transform_tmp & 0x7;
> > + else
> > + transform_yoffset = transform_tmp & 0xf;
> > +
> > + break;
> > + }
> > +
> > + return (transform_xoffset & 0xf) | ((transform_yoffset & 0xf) << 16);
> > +}
>
> 0xf, 0x3f, 0x7 might be more clear as GENMASK.
> if (afbc_half_block_en) branches can be moved out of cases and
> assign a transform mask variable instead.
Sure. Other than that the function can be simplified a bit more.
>
> > +
> > +/*
> > + * A Cluster window has 2048 x 16 line buffer, which can
> > + * works at 2048 x 16(Full) or 4096 x 8 (Half) mode.
> > + * for Cluster_lb_mode register:
> > + * 0: half mode, for plane input width range 2048 ~ 4096
> > + * 1: half mode, for cluster work at 2 * 2048 plane mode
> > + * 2: half mode, for rotate_90/270 mode
> > + *
> > + */
> > +static int vop2_get_cluster_lb_mode(struct vop2_win *win, struct drm_plane_state *pstate)
> > +{
> > + if ((pstate->rotation & DRM_MODE_ROTATE_270) || (pstate->rotation & DRM_MODE_ROTATE_90))
> > + return 2;
> > + else
> > + return 0;
> > +}
>
> What about 1?
1 is used in the downstream driver for cluster sub windows. These are
currently not supported, I don't know yet where this is leading to.
>
> > +
> > +/*
> > + * bli_sd_factor = (src - 1) / (dst - 1) << 12;
> > + * avg_sd_factor:
> > + * bli_su_factor:
> > + * bic_su_factor:
> > + * = (src - 1) / (dst - 1) << 16;
> > + *
> > + * gt2 enable: dst get one line from two line of the src
> > + * gt4 enable: dst get one line from four line of the src.
> > + *
> > + */
> > +static uint16_t vop2_scale_factor(enum scale_mode mode,
> > + int32_t filter_mode,
> > + uint32_t src, uint32_t dst)
> > +{
> > + uint32_t fac;
> > + int i;
> > +
> > + if (mode == SCALE_NONE)
> > + return 0;
> > +
> > + /*
> > + * A workaround to avoid zero div.
> > + */
> > + if (dst == 1 || src == 1) {
> > + dst++;
> > + src++;
> > + }
> > +
> > + if ((mode == SCALE_DOWN) && (filter_mode == VOP2_SCALE_DOWN_BIL)) {
> > + fac = ((src - 1) << 12) / (dst - 1);
> > + for (i = 0; i < 100; i++) {
> > + if (fac * (dst - 1) >> 12 < (src - 1))
> > + break;
> > + fac -= 1;
> > + DRM_DEBUG("down fac cali: src:%d, dst:%d, fac:0x%x\n", src, dst, fac);
> > + }
> > + } else {
> > + fac = ((src - 1) << 16) / (dst - 1);
> > + for (i = 0; i < 100; i++) {
> > + if (fac * (dst - 1) >> 16 < (src - 1))
> > + break;
> > + fac -= 1;
> > + DRM_DEBUG("up fac cali: src:%d, dst:%d, fac:0x%x\n", src, dst, fac);
> > + }
> > + }
> > +
> > + return fac;
> > +}
>
> src = 0 or dst = 0 causes an uint underflow here.
Right. Looking at it these loops are rather unnecessary and duplicating
the code also. The whole function goes down to:
static uint16_t vop2_scale_factor(uint32_t src, uint32_t dst)
{
uint32_t fac;
int shift;
if (src == dst)
return 0;
if (dst < 2)
return U16_MAX;
if (src < 2)
return 0;
if (src > dst)
shift = 12;
else
shift = 16;
src--;
dst--;
fac = DIV_ROUND_UP(src << shift, dst) - 1;
if (fac > U16_MAX)
return U16_MAX;
return fac;
}
> > +static void vop2_enable(struct vop2 *vop2)
> > +{
> > + int ret;
> > + uint32_t v;
> > +
> > + ret = pm_runtime_get_sync(vop2->dev);
> > + if (ret < 0) {
> > + drm_err(vop2->drm, "failed to get pm runtime: %d\n", ret);
> > + return;
> > + }
> > +
> > + ret = vop2_core_clks_prepare_enable(vop2);
> > + if (ret) {
> > + pm_runtime_put_sync(vop2->dev);
> > + return;
> > + }
> > +
> > + if (vop2->data->soc_id == 3566)
> > + vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
> > +
> > + vop2_writel(vop2, RK3568_REG_CFG_DONE, RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
> > +
> > + /*
> > + * Disable auto gating, this is a workaround to
> > + * avoid display image shift when a window enabled.
> > + */
> > + v = vop2_readl(vop2, RK3568_SYS_AUTO_GATING_CTRL);
> > + v &= ~RK3568_SYS_AUTO_GATING_CTRL__AUTO_GATING_EN;
> > + vop2_writel(vop2, RK3568_SYS_AUTO_GATING_CTRL, v);
> > +
> > + vop2_writel(vop2, RK3568_SYS0_INT_CLR,
> > + VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
> > + vop2_writel(vop2, RK3568_SYS0_INT_EN,
> > + VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
> > + vop2_writel(vop2, RK3568_SYS1_INT_CLR,
> > + VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
> > + vop2_writel(vop2, RK3568_SYS1_INT_EN,
> > + VOP2_INT_BUS_ERRPR << 16 | VOP2_INT_BUS_ERRPR);
> > +}
>
> Does reg writes but doesn't have the spinlock.
I didn't care about it at all when working on this driver, so we can be
sure it's at the wrong places right now.
I think we can just drop the register spinlock. The driver uses regmap
now, so read-modify-write accesses are covered by the regmap internal
locking as long as we use regmap_update_bits(). The remaining accesses
are either orthogonal anyway or are covered by the vop2_lock mutex.
> > + vop2_lock(vop2);
> > +
> > + ret = clk_prepare_enable(vp->dclk);
> > + if (ret < 0) {
> > + drm_err(vop2->drm, "failed to enable dclk for video port%d - %d\n",
> > + vp->id, ret);
> > + return;
>
> vop2_lock is never unlocked in this branch
I'll move the call to vop2_lock() below the clk_prepare_enable().
> > + }
> > +
> > + pm_runtime_put(vop2->dev);
> > +
> > + return ret;
> > +}
>
> Does reg writes, does the ISR need the reg spinlock here? I'm not
> sure.
>
> In general there appear to be a lot of issues with the register
> lock, so either I'm misunderstanding what it's supposed to protect
> or the code is very thread unsafe.
Yeah, the original driver had the spinlock and as said I kept it but
didn't maintain it. Let's remove it for now.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
next prev parent reply other threads:[~2022-01-03 14:55 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-20 11:06 [PATCH v3 00/22] drm/rockchip: RK356x VOP2 support Sascha Hauer
2021-12-20 11:06 ` [PATCH 01/22] drm/rockchip: dw_hdmi: Do not leave clock enabled in error case Sascha Hauer
2021-12-20 11:06 ` [PATCH 02/22] drm/rockchip: dw_hdmi: rename vpll clock to reference clock Sascha Hauer
2021-12-20 11:06 ` [PATCH 03/22] drm/rockchip: dw_hdmi: add rk3568 support Sascha Hauer
2021-12-20 11:06 ` [PATCH 04/22] drm/rockchip: dw_hdmi: add regulator support Sascha Hauer
2021-12-20 11:06 ` [PATCH 05/22] drm/rockchip: dw_hdmi: Add support for hclk Sascha Hauer
2021-12-20 11:06 ` [PATCH 06/22] dt-bindings: display: rockchip: dw-hdmi: Add compatible for rk3568 HDMI Sascha Hauer
2021-12-20 11:06 ` [PATCH 07/22] dt-bindings: display: rockchip: dw-hdmi: Make unwedge pinctrl optional Sascha Hauer
2021-12-20 14:27 ` Rob Herring
2021-12-20 11:06 ` [PATCH 08/22] dt-bindings: display: rockchip: dw-hdmi: use "ref" as clock name Sascha Hauer
2021-12-20 14:27 ` Rob Herring
2021-12-21 14:31 ` Rob Herring
2021-12-22 10:47 ` Sascha Hauer
2021-12-22 13:52 ` Rob Herring
2021-12-22 19:39 ` Heiko Stübner
2021-12-22 19:44 ` Nicolas Frattaroli
2021-12-22 19:56 ` Rob Herring
2021-12-20 11:06 ` [PATCH 09/22] dt-bindings: display: rockchip: dw-hdmi: Add regulator support Sascha Hauer
2021-12-20 14:27 ` Rob Herring
2021-12-20 11:06 ` [PATCH 10/22] dt-bindings: display: rockchip: dw-hdmi: Add additional clock Sascha Hauer
2021-12-20 11:06 ` [PATCH 11/22] dt-bindings: display: rockchip: Add binding for VOP2 Sascha Hauer
2021-12-20 14:27 ` Rob Herring
2021-12-21 14:33 ` Rob Herring
2021-12-20 11:06 ` [PATCH 12/22] arm64: dts: rockchip: rk3399: reorder hmdi clocks Sascha Hauer
2021-12-20 11:06 ` [PATCH 13/22] arm64: dts: rockchip: rk3399: rename HDMI ref clock to 'ref' Sascha Hauer
2021-12-20 11:06 ` [PATCH 14/22] arm64: dts: rockchip: rk356x: Add VOP2 nodes Sascha Hauer
2021-12-20 11:06 ` [PATCH 15/22] arm64: dts: rockchip: rk356x: Add HDMI nodes Sascha Hauer
2021-12-20 11:06 ` [PATCH 16/22] arm64: dts: rockchip: rk3568-evb: Enable VOP2 and hdmi Sascha Hauer
2021-12-20 11:06 ` [PATCH 17/22] arm64: dts: rockchip: enable vop2 and hdmi tx on quartz64a Sascha Hauer
2021-12-20 11:06 ` [PATCH 18/22] clk: rk3568: drop CLK_SET_RATE_PARENT from dclk_vop* Sascha Hauer
2021-12-20 11:06 ` [PATCH 19/22] clk: rk3568: Add CLK_SET_RATE_PARENT to the HDMI reference clock Sascha Hauer
2021-12-20 11:06 ` [PATCH 20/22] drm/encoder: Add of_graph port to struct drm_encoder Sascha Hauer
2021-12-21 17:31 ` Heiko Stübner
2021-12-20 11:06 ` [PATCH 21/22] drm/rockchip: Make VOP driver optional Sascha Hauer
2021-12-20 11:06 ` [PATCH 22/22] drm: rockchip: Add VOP2 driver Sascha Hauer
2021-12-20 14:16 ` Nicolas Frattaroli
2021-12-20 16:53 ` Nicolas Frattaroli
2021-12-20 18:51 ` Sascha Hauer
2021-12-20 23:20 ` kernel test robot
2021-12-21 13:44 ` Nicolas Frattaroli
2021-12-22 17:07 ` Nicolas Frattaroli
2022-01-03 14:55 ` Sascha Hauer [this message]
2022-01-04 11:07 ` Andy Yan
2022-01-05 12:20 ` Sascha Hauer
2021-12-20 11:51 ` [PATCH v3 00/22] drm/rockchip: RK356x VOP2 support Nicolas Frattaroli
2022-01-19 11:29 ` Piotr Oniszczuk
2022-01-21 10:32 ` Sascha Hauer
2022-01-21 15:43 ` Piotr Oniszczuk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220103145503.GH6003@pengutronix.de \
--to=s.hauer@pengutronix.de \
--cc=andy.yan@rock-chips.com \
--cc=benjamin.gaignard@collabora.com \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=frattaroli.nicolas@gmail.com \
--cc=hjc@rock-chips.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=michael.riesch@wolfvision.net \
--cc=pgwipeout@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).