| Message ID | 20190613192244.5447-1-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC/WIP] drm/rockchip: Support CRTC gamma LUT | expand |
Note that userspace may provide any size of gamma lut. Have a look at i915/intel_color.c:intel_color_check which filters out only the allowed sizes. Consider having a special allowance for 256-sized LUTs since that's what most legacy userspace will set, and it seems like a waste to create a 10-bit LUT for RGBA8 color. -ilia On Thu, Jun 13, 2019 at 3:23 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > Add CRTC gamma LUT configuration on RK3288 and RK3399. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > This patch seems to work well on RK3288, but produces > a distorted output on RK3399. I was hoping > someone could have any idea, so we can support both > platforms. > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 87 +++++++++++++++++++++ > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 2 + > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 4 + > drivers/gpu/drm/rockchip/rockchip_vop_reg.h | 1 + > 4 files changed, 94 insertions(+) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index 12ed5265a90b..8381679c1045 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -38,6 +38,8 @@ > #include "rockchip_drm_vop.h" > #include "rockchip_rgb.h" > > +#define VOP_GAMMA_LUT_SIZE 1024 > + > #define VOP_WIN_SET(vop, win, name, v) \ > vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name) > #define VOP_SCL_SET(vop, win, name, v) \ > @@ -137,6 +139,7 @@ struct vop { > > uint32_t *regsbak; > void __iomem *regs; > + void __iomem *lut_regs; > > /* physical map length of vop register */ > uint32_t len; > @@ -1153,6 +1156,46 @@ static void vop_wait_for_irq_handler(struct vop *vop) > synchronize_irq(vop->irq); > } > > +static bool vop_dsp_lut_is_enable(struct vop *vop) > +{ > + return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); > +} > + > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc, > + struct drm_crtc_state *state) > +{ > + struct drm_color_lut *lut; > + int i, idle, ret; > + > + if (!state->gamma_lut) > + return; > + lut = state->gamma_lut->data; > + > + spin_lock(&vop->reg_lock); > + VOP_REG_SET(vop, common, dsp_lut_en, 0); > + vop_cfg_done(vop); > + spin_unlock(&vop->reg_lock); > + > + ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop, > + idle, !idle, 5, 10 * 30000); > + if (ret) > + return; > + > + spin_lock(&vop->reg_lock); > + for (i = 0; i < crtc->gamma_size; i++) { > + u32 word; > + > + word = (drm_color_lut_extract(lut[i].red, 10) << 20) | > + (drm_color_lut_extract(lut[i].green, 10) << 10) | > + drm_color_lut_extract(lut[i].blue, 10); > + writel(word, vop->lut_regs + i * 4); > + } > + > + VOP_REG_SET(vop, common, dsp_lut_en, 1); > + vop_cfg_done(vop); > + spin_unlock(&vop->reg_lock); > +} > + > static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > struct drm_crtc_state *old_crtc_state) > { > @@ -1201,6 +1244,9 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb); > set_bit(VOP_PENDING_FB_UNREF, &vop->pending); > } > + > + if (vop->lut_regs && crtc->state->color_mgmt_changed) > + vop_crtc_gamma_set(vop, crtc, crtc->state); > } > > static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = { > @@ -1323,6 +1369,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = { > .disable_vblank = vop_crtc_disable_vblank, > .set_crc_source = vop_crtc_set_crc_source, > .verify_crc_source = vop_crtc_verify_crc_source, > + .gamma_set = drm_atomic_helper_legacy_gamma_set, > }; > > static void vop_fb_unref_worker(struct drm_flip_work *work, void *val) > @@ -1480,6 +1527,8 @@ static int vop_create_crtc(struct vop *vop) > goto err_cleanup_planes; > > drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs); > + drm_mode_crtc_set_gamma_size(crtc, VOP_GAMMA_LUT_SIZE); > + drm_crtc_enable_color_mgmt(crtc, 0, false, VOP_GAMMA_LUT_SIZE); > > /* > * Create drm_planes for overlay windows with possible_crtcs restricted > @@ -1744,6 +1793,41 @@ int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout) > } > EXPORT_SYMBOL(rockchip_drm_wait_vact_end); > > +static int vop_gamma_lut_request(struct device *dev, > + struct resource *res, struct vop *vop) > +{ > + resource_size_t offset = vop->data->gamma_lut_addr_off; > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; > + > + /* > + * Some SoCs (e.g. RK3288) have the gamma LUT address after > + * the MMU registers, which means we can't request and ioremap > + * the entire register set. Other (e.g. RK3399) have gamma LUT > + * address before MMU. > + * > + * Therefore, we need to request and ioremap those that haven't > + * been already. > + */ > + if (vop->len >= (offset + size)) { > + vop->lut_regs = vop->regs + offset; > + return 0; > + } > + > + if (!devm_request_mem_region(dev, res->start + offset, > + size, dev_name(dev))) { > + dev_warn(dev, "can't request gamma lut region\n"); > + return -EBUSY; > + } > + > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); > + if (!vop->lut_regs) { > + dev_err(dev, "can't ioremap gamma lut address\n"); > + devm_release_mem_region(dev, res->start + offset, size); > + return -ENOMEM; > + } > + return 0; > +} > + > static int vop_bind(struct device *dev, struct device *master, void *data) > { > struct platform_device *pdev = to_platform_device(dev); > @@ -1776,6 +1860,9 @@ static int vop_bind(struct device *dev, struct device *master, void *data) > if (IS_ERR(vop->regs)) > return PTR_ERR(vop->regs); > > + if (vop->data->gamma_lut_addr_off) > + vop_gamma_lut_request(dev, res, vop); > + > vop->regsbak = devm_kzalloc(dev, vop->len, GFP_KERNEL); > if (!vop->regsbak) > return -ENOMEM; > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > index 2149a889c29d..12d5bde0d0bc 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > @@ -67,6 +67,7 @@ struct vop_common { > struct vop_reg dither_down_mode; > struct vop_reg dither_down_en; > struct vop_reg dither_up; > + struct vop_reg dsp_lut_en; > struct vop_reg gate_en; > struct vop_reg mmu_en; > struct vop_reg out_mode; > @@ -170,6 +171,7 @@ struct vop_data { > const struct vop_win_yuv2yuv_data *win_yuv2yuv; > const struct vop_win_data *win; > unsigned int win_size; > + off_t gamma_lut_addr_off; > > #define VOP_FEATURE_OUTPUT_RGB10 BIT(0) > #define VOP_FEATURE_INTERNAL_RGB BIT(1) > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > index 7b9c74750f6d..63fbb384893b 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > @@ -593,6 +593,7 @@ static const struct vop_common rk3288_common = { > .dither_down_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 2), > .pre_dither_down = VOP_REG(RK3288_DSP_CTRL1, 0x1, 1), > .dither_up = VOP_REG(RK3288_DSP_CTRL1, 0x1, 6), > + .dsp_lut_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 0), > .data_blank = VOP_REG(RK3288_DSP_CTRL0, 0x1, 19), > .dsp_blank = VOP_REG(RK3288_DSP_CTRL0, 0x3, 18), > .out_mode = VOP_REG(RK3288_DSP_CTRL0, 0xf, 0), > @@ -641,6 +642,7 @@ static const struct vop_data rk3288_vop = { > .output = &rk3288_output, > .win = rk3288_vop_win_data, > .win_size = ARRAY_SIZE(rk3288_vop_win_data), > + .gamma_lut_addr_off = RK3288_GAMMA_LUT_ADDR, > }; > > static const int rk3368_vop_intrs[] = { > @@ -811,6 +813,7 @@ static const struct vop_data rk3399_vop_big = { > .win = rk3368_vop_win_data, > .win_size = ARRAY_SIZE(rk3368_vop_win_data), > .win_yuv2yuv = rk3399_vop_big_win_yuv2yuv_data, > + .gamma_lut_addr_off = RK3399_GAMMA_LUT_ADDR, > }; > > static const struct vop_win_data rk3399_vop_lit_win_data[] = { > @@ -836,6 +839,7 @@ static const struct vop_data rk3399_vop_lit = { > .win = rk3399_vop_lit_win_data, > .win_size = ARRAY_SIZE(rk3399_vop_lit_win_data), > .win_yuv2yuv = rk3399_vop_lit_win_yuv2yuv_data, > + .gamma_lut_addr_off = RK3399_GAMMA_LUT_ADDR, > }; > > static const struct vop_win_data rk3228_vop_win_data[] = { > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > index 6e9fa5815d4d..490318382f74 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > @@ -113,6 +113,7 @@ > #define RK3288_DSP_VACT_ST_END 0x0194 > #define RK3288_DSP_VS_ST_END_F1 0x0198 > #define RK3288_DSP_VACT_ST_END_F1 0x019c > +#define RK3288_GAMMA_LUT_ADDR 0x1000 > /* register definition end */ > > /* rk3368 register definition */ > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 13 Jun 2019 16:22:44 -0300 Ezequiel Garcia <ezequiel@collabora.com> wrote: > +static int vop_gamma_lut_request(struct device *dev, > + struct resource *res, struct vop *vop) > +{ > + resource_size_t offset = vop->data->gamma_lut_addr_off; > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; > + > + /* > + * Some SoCs (e.g. RK3288) have the gamma LUT address after > + * the MMU registers, which means we can't request and ioremap > + * the entire register set. Other (e.g. RK3399) have gamma LUT > + * address before MMU. > + * > + * Therefore, we need to request and ioremap those that haven't > + * been already. > + */ > + if (vop->len >= (offset + size)) { > + vop->lut_regs = vop->regs + offset; > + return 0; > + } > + > + if (!devm_request_mem_region(dev, res->start + offset, > + size, dev_name(dev))) { > + dev_warn(dev, "can't request gamma lut region\n"); > + return -EBUSY; > + } > + > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); > + if (!vop->lut_regs) { > + dev_err(dev, "can't ioremap gamma lut address\n"); > + devm_release_mem_region(dev, res->start + offset, size); > + return -ENOMEM; > + } Can't we patch the resource just after calling plaform_get_resource() (and before calling devm_ioremap_resource()) so we don't have to add these devm_request_mem_region()+devm_ioremap() calls here? > + return 0; > +}
Hi Boris, Am Freitag, 14. Juni 2019, 15:53:20 CEST schrieb Boris Brezillon: > On Thu, 13 Jun 2019 16:22:44 -0300 > Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > > +static int vop_gamma_lut_request(struct device *dev, > > + struct resource *res, struct vop *vop) > > +{ > > + resource_size_t offset = vop->data->gamma_lut_addr_off; > > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; > > + > > + /* > > + * Some SoCs (e.g. RK3288) have the gamma LUT address after > > + * the MMU registers, which means we can't request and ioremap > > + * the entire register set. Other (e.g. RK3399) have gamma LUT > > + * address before MMU. > > + * > > + * Therefore, we need to request and ioremap those that haven't > > + * been already. > > + */ > > + if (vop->len >= (offset + size)) { > > + vop->lut_regs = vop->regs + offset; > > + return 0; > > + } > > + > > + if (!devm_request_mem_region(dev, res->start + offset, > > + size, dev_name(dev))) { > > + dev_warn(dev, "can't request gamma lut region\n"); > > + return -EBUSY; > > + } > > + > > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); > > + if (!vop->lut_regs) { > > + dev_err(dev, "can't ioremap gamma lut address\n"); > > + devm_release_mem_region(dev, res->start + offset, size); > > + return -ENOMEM; > > + } > > Can't we patch the resource just after calling plaform_get_resource() > (and before calling devm_ioremap_resource()) so we don't have to add > these devm_request_mem_region()+devm_ioremap() calls here? The issue is that on the older rk3288 socs the vops memory map has the mmu registers (which get mapped separately) in between the core and lut registers.
On Fri, 14 Jun 2019 16:03:28 +0200 Heiko Stübner <heiko@sntech.de> wrote: > Hi Boris, > > Am Freitag, 14. Juni 2019, 15:53:20 CEST schrieb Boris Brezillon: > > On Thu, 13 Jun 2019 16:22:44 -0300 > > Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > > > > > +static int vop_gamma_lut_request(struct device *dev, > > > + struct resource *res, struct vop *vop) > > > +{ > > > + resource_size_t offset = vop->data->gamma_lut_addr_off; > > > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; > > > + > > > + /* > > > + * Some SoCs (e.g. RK3288) have the gamma LUT address after > > > + * the MMU registers, which means we can't request and ioremap > > > + * the entire register set. Other (e.g. RK3399) have gamma LUT > > > + * address before MMU. > > > + * > > > + * Therefore, we need to request and ioremap those that haven't > > > + * been already. > > > + */ > > > + if (vop->len >= (offset + size)) { > > > + vop->lut_regs = vop->regs + offset; > > > + return 0; > > > + } > > > + > > > + if (!devm_request_mem_region(dev, res->start + offset, > > > + size, dev_name(dev))) { > > > + dev_warn(dev, "can't request gamma lut region\n"); > > > + return -EBUSY; > > > + } > > > + > > > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); > > > + if (!vop->lut_regs) { > > > + dev_err(dev, "can't ioremap gamma lut address\n"); > > > + devm_release_mem_region(dev, res->start + offset, size); > > > + return -ENOMEM; > > > + } > > > > Can't we patch the resource just after calling plaform_get_resource() > > (and before calling devm_ioremap_resource()) so we don't have to add > > these devm_request_mem_region()+devm_ioremap() calls here? > > The issue is that on the older rk3288 socs the vops memory map has > the mmu registers (which get mapped separately) in between the core > and lut registers. Sorry for the noise, I thought the MMU was registered directly by the VOP driver and we could avoid the 3 ioremap and merge things into a single one, but it seems the MMU driver is a separate thing.
Hi, On Thu, Jun 13, 2019 at 12:23 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > @@ -1744,6 +1793,41 @@ int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout) > } > EXPORT_SYMBOL(rockchip_drm_wait_vact_end); > > +static int vop_gamma_lut_request(struct device *dev, > + struct resource *res, struct vop *vop) > +{ > + resource_size_t offset = vop->data->gamma_lut_addr_off; > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; > + > + /* > + * Some SoCs (e.g. RK3288) have the gamma LUT address after > + * the MMU registers, which means we can't request and ioremap > + * the entire register set. Other (e.g. RK3399) have gamma LUT > + * address before MMU. > + * > + * Therefore, we need to request and ioremap those that haven't > + * been already. > + */ > + if (vop->len >= (offset + size)) { > + vop->lut_regs = vop->regs + offset; > + return 0; > + } > + > + if (!devm_request_mem_region(dev, res->start + offset, > + size, dev_name(dev))) { > + dev_warn(dev, "can't request gamma lut region\n"); > + return -EBUSY; > + } > + > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); > + if (!vop->lut_regs) { > + dev_err(dev, "can't ioremap gamma lut address\n"); > + devm_release_mem_region(dev, res->start + offset, size); > + return -ENOMEM; > + } I'm curious here. I was always under the impression that you were supposed to specify all of your memory regions in the device tree. ...but here the device tree on rk3288 says: vopb: vop@ff930000 { compatible = "rockchip,rk3288-vop"; reg = <0x0 0xff930000 0x0 0x19c>; ... }; ...and we're now mapping 4096 bytes starting at 0xff931000. Is that really legit? Wouldn't it be better to put this extra memory range in the dts? Hrm, but then I guess you need to figure out what to do about older device trees. Do you disable the gamma LUT feature? ...or do you do exactly what the code here is doing and just map it anyway? I guess you could just keep the code here (and it'll work fine), but maybe in parallel we should add it to the .dts file and bindings? --- I will say that, though I don't know much (anything?) about gamma LUTs, I ran the Chrome OS "gamma_test" program and saw a pretty RGB gradient on the both the internal screen and HDMI monitor on my rk3288-veyron-jerry. Thus: Tested-by: Douglas Anderson <dianders@chromium.org>
Hi Ezequiel, one small question, as I'm working on supporting gamma LUT for rcar-du as well, and there's one point not totally clear to me On Thu, Jun 13, 2019 at 04:22:44PM -0300, Ezequiel Garcia wrote: > Add CRTC gamma LUT configuration on RK3288 and RK3399. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > This patch seems to work well on RK3288, but produces > a distorted output on RK3399. I was hoping > someone could have any idea, so we can support both > platforms. > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 87 +++++++++++++++++++++ > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 2 + > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 4 + > drivers/gpu/drm/rockchip/rockchip_vop_reg.h | 1 + > 4 files changed, 94 insertions(+) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index 12ed5265a90b..8381679c1045 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -38,6 +38,8 @@ > #include "rockchip_drm_vop.h" > #include "rockchip_rgb.h" > > +#define VOP_GAMMA_LUT_SIZE 1024 > + > #define VOP_WIN_SET(vop, win, name, v) \ > vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name) > #define VOP_SCL_SET(vop, win, name, v) \ > @@ -137,6 +139,7 @@ struct vop { > > uint32_t *regsbak; > void __iomem *regs; > + void __iomem *lut_regs; > > /* physical map length of vop register */ > uint32_t len; > @@ -1153,6 +1156,46 @@ static void vop_wait_for_irq_handler(struct vop *vop) > synchronize_irq(vop->irq); > } > > +static bool vop_dsp_lut_is_enable(struct vop *vop) > +{ > + return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); > +} > + > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc, > + struct drm_crtc_state *state) > +{ > + struct drm_color_lut *lut; > + int i, idle, ret; > + > + if (!state->gamma_lut) > + return; > + lut = state->gamma_lut->data; > + > + spin_lock(&vop->reg_lock); > + VOP_REG_SET(vop, common, dsp_lut_en, 0); > + vop_cfg_done(vop); > + spin_unlock(&vop->reg_lock); > + > + ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop, > + idle, !idle, 5, 10 * 30000); > + if (ret) > + return; > + > + spin_lock(&vop->reg_lock); > + for (i = 0; i < crtc->gamma_size; i++) { > + u32 word; > + > + word = (drm_color_lut_extract(lut[i].red, 10) << 20) | > + (drm_color_lut_extract(lut[i].green, 10) << 10) | > + drm_color_lut_extract(lut[i].blue, 10); > + writel(word, vop->lut_regs + i * 4); > + } > + > + VOP_REG_SET(vop, common, dsp_lut_en, 1); > + vop_cfg_done(vop); > + spin_unlock(&vop->reg_lock); > +} > + > static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > struct drm_crtc_state *old_crtc_state) > { > @@ -1201,6 +1244,9 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb); > set_bit(VOP_PENDING_FB_UNREF, &vop->pending); > } > + > + if (vop->lut_regs && crtc->state->color_mgmt_changed) > + vop_crtc_gamma_set(vop, crtc, crtc->state); Which one is the right point when to call LUT update functions? I initially added my callback in atomic_flush as you did here, mostly because I've found examples in other drivers in drm and went in cargo cult mode. I've been then suggested by Laurent that atomic_flush() might not be the right place where to do so, as it gets called after the plane updates (iirc, the DRM atomic API is not something I'm that familiar with yet). So I moved my LUT update function in the atomic_commit_tail callback, which is meant to actually commit a CRTC to the hw. What's your opinion on this? Thanks j > } > > static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = { > @@ -1323,6 +1369,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = { > .disable_vblank = vop_crtc_disable_vblank, > .set_crc_source = vop_crtc_set_crc_source, > .verify_crc_source = vop_crtc_verify_crc_source, > + .gamma_set = drm_atomic_helper_legacy_gamma_set, > }; > > static void vop_fb_unref_worker(struct drm_flip_work *work, void *val) > @@ -1480,6 +1527,8 @@ static int vop_create_crtc(struct vop *vop) > goto err_cleanup_planes; > > drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs); > + drm_mode_crtc_set_gamma_size(crtc, VOP_GAMMA_LUT_SIZE); > + drm_crtc_enable_color_mgmt(crtc, 0, false, VOP_GAMMA_LUT_SIZE); > > /* > * Create drm_planes for overlay windows with possible_crtcs restricted > @@ -1744,6 +1793,41 @@ int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout) > } > EXPORT_SYMBOL(rockchip_drm_wait_vact_end); > > +static int vop_gamma_lut_request(struct device *dev, > + struct resource *res, struct vop *vop) > +{ > + resource_size_t offset = vop->data->gamma_lut_addr_off; > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; > + > + /* > + * Some SoCs (e.g. RK3288) have the gamma LUT address after > + * the MMU registers, which means we can't request and ioremap > + * the entire register set. Other (e.g. RK3399) have gamma LUT > + * address before MMU. > + * > + * Therefore, we need to request and ioremap those that haven't > + * been already. > + */ > + if (vop->len >= (offset + size)) { > + vop->lut_regs = vop->regs + offset; > + return 0; > + } > + > + if (!devm_request_mem_region(dev, res->start + offset, > + size, dev_name(dev))) { > + dev_warn(dev, "can't request gamma lut region\n"); > + return -EBUSY; > + } > + > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); > + if (!vop->lut_regs) { > + dev_err(dev, "can't ioremap gamma lut address\n"); > + devm_release_mem_region(dev, res->start + offset, size); > + return -ENOMEM; > + } > + return 0; > +} > + > static int vop_bind(struct device *dev, struct device *master, void *data) > { > struct platform_device *pdev = to_platform_device(dev); > @@ -1776,6 +1860,9 @@ static int vop_bind(struct device *dev, struct device *master, void *data) > if (IS_ERR(vop->regs)) > return PTR_ERR(vop->regs); > > + if (vop->data->gamma_lut_addr_off) > + vop_gamma_lut_request(dev, res, vop); > + > vop->regsbak = devm_kzalloc(dev, vop->len, GFP_KERNEL); > if (!vop->regsbak) > return -ENOMEM; > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > index 2149a889c29d..12d5bde0d0bc 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > @@ -67,6 +67,7 @@ struct vop_common { > struct vop_reg dither_down_mode; > struct vop_reg dither_down_en; > struct vop_reg dither_up; > + struct vop_reg dsp_lut_en; > struct vop_reg gate_en; > struct vop_reg mmu_en; > struct vop_reg out_mode; > @@ -170,6 +171,7 @@ struct vop_data { > const struct vop_win_yuv2yuv_data *win_yuv2yuv; > const struct vop_win_data *win; > unsigned int win_size; > + off_t gamma_lut_addr_off; > > #define VOP_FEATURE_OUTPUT_RGB10 BIT(0) > #define VOP_FEATURE_INTERNAL_RGB BIT(1) > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > index 7b9c74750f6d..63fbb384893b 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > @@ -593,6 +593,7 @@ static const struct vop_common rk3288_common = { > .dither_down_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 2), > .pre_dither_down = VOP_REG(RK3288_DSP_CTRL1, 0x1, 1), > .dither_up = VOP_REG(RK3288_DSP_CTRL1, 0x1, 6), > + .dsp_lut_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 0), > .data_blank = VOP_REG(RK3288_DSP_CTRL0, 0x1, 19), > .dsp_blank = VOP_REG(RK3288_DSP_CTRL0, 0x3, 18), > .out_mode = VOP_REG(RK3288_DSP_CTRL0, 0xf, 0), > @@ -641,6 +642,7 @@ static const struct vop_data rk3288_vop = { > .output = &rk3288_output, > .win = rk3288_vop_win_data, > .win_size = ARRAY_SIZE(rk3288_vop_win_data), > + .gamma_lut_addr_off = RK3288_GAMMA_LUT_ADDR, > }; > > static const int rk3368_vop_intrs[] = { > @@ -811,6 +813,7 @@ static const struct vop_data rk3399_vop_big = { > .win = rk3368_vop_win_data, > .win_size = ARRAY_SIZE(rk3368_vop_win_data), > .win_yuv2yuv = rk3399_vop_big_win_yuv2yuv_data, > + .gamma_lut_addr_off = RK3399_GAMMA_LUT_ADDR, > }; > > static const struct vop_win_data rk3399_vop_lit_win_data[] = { > @@ -836,6 +839,7 @@ static const struct vop_data rk3399_vop_lit = { > .win = rk3399_vop_lit_win_data, > .win_size = ARRAY_SIZE(rk3399_vop_lit_win_data), > .win_yuv2yuv = rk3399_vop_lit_win_yuv2yuv_data, > + .gamma_lut_addr_off = RK3399_GAMMA_LUT_ADDR, > }; > > static const struct vop_win_data rk3228_vop_win_data[] = { > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > index 6e9fa5815d4d..490318382f74 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > @@ -113,6 +113,7 @@ > #define RK3288_DSP_VACT_ST_END 0x0194 > #define RK3288_DSP_VS_ST_END_F1 0x0198 > #define RK3288_DSP_VACT_ST_END_F1 0x019c > +#define RK3288_GAMMA_LUT_ADDR 0x1000 > /* register definition end */ > > /* rk3368 register definition */ > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, 2019-06-17 at 12:06 +0200, Jacopo Mondi wrote: > Hi Ezequiel, > one small question, as I'm working on supporting gamma LUT for > rcar-du as well, and there's one point not totally clear to me > > > On Thu, Jun 13, 2019 at 04:22:44PM -0300, Ezequiel Garcia wrote: > > Add CRTC gamma LUT configuration on RK3288 and RK3399. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > This patch seems to work well on RK3288, but produces > > a distorted output on RK3399. I was hoping > > someone could have any idea, so we can support both > > platforms. > > > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 87 +++++++++++++++++++++ > > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 2 + > > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 4 + > > drivers/gpu/drm/rockchip/rockchip_vop_reg.h | 1 + > > 4 files changed, 94 insertions(+) > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > index 12ed5265a90b..8381679c1045 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > @@ -38,6 +38,8 @@ > > #include "rockchip_drm_vop.h" > > #include "rockchip_rgb.h" > > > > +#define VOP_GAMMA_LUT_SIZE 1024 > > + > > #define VOP_WIN_SET(vop, win, name, v) \ > > vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name) > > #define VOP_SCL_SET(vop, win, name, v) \ > > @@ -137,6 +139,7 @@ struct vop { > > > > uint32_t *regsbak; > > void __iomem *regs; > > + void __iomem *lut_regs; > > > > /* physical map length of vop register */ > > uint32_t len; > > @@ -1153,6 +1156,46 @@ static void vop_wait_for_irq_handler(struct vop *vop) > > synchronize_irq(vop->irq); > > } > > > > +static bool vop_dsp_lut_is_enable(struct vop *vop) > > +{ > > + return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); > > +} > > + > > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc, > > + struct drm_crtc_state *state) > > +{ > > + struct drm_color_lut *lut; > > + int i, idle, ret; > > + > > + if (!state->gamma_lut) > > + return; > > + lut = state->gamma_lut->data; > > + > > + spin_lock(&vop->reg_lock); > > + VOP_REG_SET(vop, common, dsp_lut_en, 0); > > + vop_cfg_done(vop); > > + spin_unlock(&vop->reg_lock); > > + > > + ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop, > > + idle, !idle, 5, 10 * 30000); > > + if (ret) > > + return; > > + > > + spin_lock(&vop->reg_lock); > > + for (i = 0; i < crtc->gamma_size; i++) { > > + u32 word; > > + > > + word = (drm_color_lut_extract(lut[i].red, 10) << 20) | > > + (drm_color_lut_extract(lut[i].green, 10) << 10) | > > + drm_color_lut_extract(lut[i].blue, 10); > > + writel(word, vop->lut_regs + i * 4); > > + } > > + > > + VOP_REG_SET(vop, common, dsp_lut_en, 1); > > + vop_cfg_done(vop); > > + spin_unlock(&vop->reg_lock); > > +} > > + > > static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > > struct drm_crtc_state *old_crtc_state) > > { > > @@ -1201,6 +1244,9 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > > drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb); > > set_bit(VOP_PENDING_FB_UNREF, &vop->pending); > > } > > + > > + if (vop->lut_regs && crtc->state->color_mgmt_changed) > > + vop_crtc_gamma_set(vop, crtc, crtc->state); > > Which one is the right point when to call LUT update functions? > > I initially added my callback in atomic_flush as you did here, mostly > because I've found examples in other drivers in drm and went in > cargo cult mode. I've been then suggested by Laurent that atomic_flush() > might not be the right place where to do so, as it gets called after > the plane updates (iirc, the DRM atomic API is not something I'm that > familiar with yet). > > So I moved my LUT update function in the atomic_commit_tail callback, > which is meant to actually commit a CRTC to the hw. > > What's your opinion on this? > I have to admit this is not exactly clear to me either. Let me make sure I understand the issue. You are concerned about getting some tearing if the CRTC gamma LUT is affected in the atomic_flush? If that's the case, it shouldn't be too hard to confirm (I think). Thanks, Eze
On Thu, 2019-06-13 at 15:36 -0400, Ilia Mirkin wrote: > Note that userspace may provide any size of gamma lut. Have a look at > i915/intel_color.c:intel_color_check which filters out only the > allowed sizes. Consider having a special allowance for 256-sized LUTs > since that's what most legacy userspace will set, and it seems like a > waste to create a 10-bit LUT for RGBA8 color. > Right. I will add a check for the gamma lut size. Unfortunately, this hardware seems to only support 10-bit, 1024-sized LUTs. The spec does mention a support 8-bit, 256-entries, but it's not at all clear how configure that. Thanks for the feedback, Ezequiel > -ilia > > On Thu, Jun 13, 2019 at 3:23 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > Add CRTC gamma LUT configuration on RK3288 and RK3399. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > This patch seems to work well on RK3288, but produces > > a distorted output on RK3399. I was hoping > > someone could have any idea, so we can support both > > platforms. > > > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 87 +++++++++++++++++++++ > > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 2 + > > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 4 + > > drivers/gpu/drm/rockchip/rockchip_vop_reg.h | 1 + > > 4 files changed, 94 insertions(+) > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > index 12ed5265a90b..8381679c1045 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > @@ -38,6 +38,8 @@ > > #include "rockchip_drm_vop.h" > > #include "rockchip_rgb.h" > > > > +#define VOP_GAMMA_LUT_SIZE 1024 > > + > > #define VOP_WIN_SET(vop, win, name, v) \ > > vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name) > > #define VOP_SCL_SET(vop, win, name, v) \ > > @@ -137,6 +139,7 @@ struct vop { > > > > uint32_t *regsbak; > > void __iomem *regs; > > + void __iomem *lut_regs; > > > > /* physical map length of vop register */ > > uint32_t len; > > @@ -1153,6 +1156,46 @@ static void vop_wait_for_irq_handler(struct vop *vop) > > synchronize_irq(vop->irq); > > } > > > > +static bool vop_dsp_lut_is_enable(struct vop *vop) > > +{ > > + return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); > > +} > > + > > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc, > > + struct drm_crtc_state *state) > > +{ > > + struct drm_color_lut *lut; > > + int i, idle, ret; > > + > > + if (!state->gamma_lut) > > + return; > > + lut = state->gamma_lut->data; > > + > > + spin_lock(&vop->reg_lock); > > + VOP_REG_SET(vop, common, dsp_lut_en, 0); > > + vop_cfg_done(vop); > > + spin_unlock(&vop->reg_lock); > > + > > + ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop, > > + idle, !idle, 5, 10 * 30000); > > + if (ret) > > + return; > > + > > + spin_lock(&vop->reg_lock); > > + for (i = 0; i < crtc->gamma_size; i++) { > > + u32 word; > > + > > + word = (drm_color_lut_extract(lut[i].red, 10) << 20) | > > + (drm_color_lut_extract(lut[i].green, 10) << 10) | > > + drm_color_lut_extract(lut[i].blue, 10); > > + writel(word, vop->lut_regs + i * 4); > > + } > > + > > + VOP_REG_SET(vop, common, dsp_lut_en, 1); > > + vop_cfg_done(vop); > > + spin_unlock(&vop->reg_lock); > > +} > > + > > static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > > struct drm_crtc_state *old_crtc_state) > > { > > @@ -1201,6 +1244,9 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > > drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb); > > set_bit(VOP_PENDING_FB_UNREF, &vop->pending); > > } > > + > > + if (vop->lut_regs && crtc->state->color_mgmt_changed) > > + vop_crtc_gamma_set(vop, crtc, crtc->state); > > } > > > > static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = { > > @@ -1323,6 +1369,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = { > > .disable_vblank = vop_crtc_disable_vblank, > > .set_crc_source = vop_crtc_set_crc_source, > > .verify_crc_source = vop_crtc_verify_crc_source, > > + .gamma_set = drm_atomic_helper_legacy_gamma_set, > > }; > > > > static void vop_fb_unref_worker(struct drm_flip_work *work, void *val) > > @@ -1480,6 +1527,8 @@ static int vop_create_crtc(struct vop *vop) > > goto err_cleanup_planes; > > > > drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs); > > + drm_mode_crtc_set_gamma_size(crtc, VOP_GAMMA_LUT_SIZE); > > + drm_crtc_enable_color_mgmt(crtc, 0, false, VOP_GAMMA_LUT_SIZE); > > > > /* > > * Create drm_planes for overlay windows with possible_crtcs restricted > > @@ -1744,6 +1793,41 @@ int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout) > > } > > EXPORT_SYMBOL(rockchip_drm_wait_vact_end); > > > > +static int vop_gamma_lut_request(struct device *dev, > > + struct resource *res, struct vop *vop) > > +{ > > + resource_size_t offset = vop->data->gamma_lut_addr_off; > > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; > > + > > + /* > > + * Some SoCs (e.g. RK3288) have the gamma LUT address after > > + * the MMU registers, which means we can't request and ioremap > > + * the entire register set. Other (e.g. RK3399) have gamma LUT > > + * address before MMU. > > + * > > + * Therefore, we need to request and ioremap those that haven't > > + * been already. > > + */ > > + if (vop->len >= (offset + size)) { > > + vop->lut_regs = vop->regs + offset; > > + return 0; > > + } > > + > > + if (!devm_request_mem_region(dev, res->start + offset, > > + size, dev_name(dev))) { > > + dev_warn(dev, "can't request gamma lut region\n"); > > + return -EBUSY; > > + } > > + > > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); > > + if (!vop->lut_regs) { > > + dev_err(dev, "can't ioremap gamma lut address\n"); > > + devm_release_mem_region(dev, res->start + offset, size); > > + return -ENOMEM; > > + } > > + return 0; > > +} > > + > > static int vop_bind(struct device *dev, struct device *master, void *data) > > { > > struct platform_device *pdev = to_platform_device(dev); > > @@ -1776,6 +1860,9 @@ static int vop_bind(struct device *dev, struct device *master, void *data) > > if (IS_ERR(vop->regs)) > > return PTR_ERR(vop->regs); > > > > + if (vop->data->gamma_lut_addr_off) > > + vop_gamma_lut_request(dev, res, vop); > > + > > vop->regsbak = devm_kzalloc(dev, vop->len, GFP_KERNEL); > > if (!vop->regsbak) > > return -ENOMEM; > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > > index 2149a889c29d..12d5bde0d0bc 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > > @@ -67,6 +67,7 @@ struct vop_common { > > struct vop_reg dither_down_mode; > > struct vop_reg dither_down_en; > > struct vop_reg dither_up; > > + struct vop_reg dsp_lut_en; > > struct vop_reg gate_en; > > struct vop_reg mmu_en; > > struct vop_reg out_mode; > > @@ -170,6 +171,7 @@ struct vop_data { > > const struct vop_win_yuv2yuv_data *win_yuv2yuv; > > const struct vop_win_data *win; > > unsigned int win_size; > > + off_t gamma_lut_addr_off; > > > > #define VOP_FEATURE_OUTPUT_RGB10 BIT(0) > > #define VOP_FEATURE_INTERNAL_RGB BIT(1) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > index 7b9c74750f6d..63fbb384893b 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > @@ -593,6 +593,7 @@ static const struct vop_common rk3288_common = { > > .dither_down_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 2), > > .pre_dither_down = VOP_REG(RK3288_DSP_CTRL1, 0x1, 1), > > .dither_up = VOP_REG(RK3288_DSP_CTRL1, 0x1, 6), > > + .dsp_lut_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 0), > > .data_blank = VOP_REG(RK3288_DSP_CTRL0, 0x1, 19), > > .dsp_blank = VOP_REG(RK3288_DSP_CTRL0, 0x3, 18), > > .out_mode = VOP_REG(RK3288_DSP_CTRL0, 0xf, 0), > > @@ -641,6 +642,7 @@ static const struct vop_data rk3288_vop = { > > .output = &rk3288_output, > > .win = rk3288_vop_win_data, > > .win_size = ARRAY_SIZE(rk3288_vop_win_data), > > + .gamma_lut_addr_off = RK3288_GAMMA_LUT_ADDR, > > }; > > > > static const int rk3368_vop_intrs[] = { > > @@ -811,6 +813,7 @@ static const struct vop_data rk3399_vop_big = { > > .win = rk3368_vop_win_data, > > .win_size = ARRAY_SIZE(rk3368_vop_win_data), > > .win_yuv2yuv = rk3399_vop_big_win_yuv2yuv_data, > > + .gamma_lut_addr_off = RK3399_GAMMA_LUT_ADDR, > > }; > > > > static const struct vop_win_data rk3399_vop_lit_win_data[] = { > > @@ -836,6 +839,7 @@ static const struct vop_data rk3399_vop_lit = { > > .win = rk3399_vop_lit_win_data, > > .win_size = ARRAY_SIZE(rk3399_vop_lit_win_data), > > .win_yuv2yuv = rk3399_vop_lit_win_yuv2yuv_data, > > + .gamma_lut_addr_off = RK3399_GAMMA_LUT_ADDR, > > }; > > > > static const struct vop_win_data rk3228_vop_win_data[] = { > > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > > index 6e9fa5815d4d..490318382f74 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h > > @@ -113,6 +113,7 @@ > > #define RK3288_DSP_VACT_ST_END 0x0194 > > #define RK3288_DSP_VS_ST_END_F1 0x0198 > > #define RK3288_DSP_VACT_ST_END_F1 0x019c > > +#define RK3288_GAMMA_LUT_ADDR 0x1000 > > /* register definition end */ > > > > /* rk3368 register definition */ > > -- > > 2.20.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, 2019-06-14 at 13:05 -0700, Doug Anderson wrote: > Hi, > > On Thu, Jun 13, 2019 at 12:23 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > @@ -1744,6 +1793,41 @@ int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout) > > } > > EXPORT_SYMBOL(rockchip_drm_wait_vact_end); > > > > +static int vop_gamma_lut_request(struct device *dev, > > + struct resource *res, struct vop *vop) > > +{ > > + resource_size_t offset = vop->data->gamma_lut_addr_off; > > + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; > > + > > + /* > > + * Some SoCs (e.g. RK3288) have the gamma LUT address after > > + * the MMU registers, which means we can't request and ioremap > > + * the entire register set. Other (e.g. RK3399) have gamma LUT > > + * address before MMU. > > + * > > + * Therefore, we need to request and ioremap those that haven't > > + * been already. > > + */ > > + if (vop->len >= (offset + size)) { > > + vop->lut_regs = vop->regs + offset; > > + return 0; > > + } > > + > > + if (!devm_request_mem_region(dev, res->start + offset, > > + size, dev_name(dev))) { > > + dev_warn(dev, "can't request gamma lut region\n"); > > + return -EBUSY; > > + } > > + > > + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); > > + if (!vop->lut_regs) { > > + dev_err(dev, "can't ioremap gamma lut address\n"); > > + devm_release_mem_region(dev, res->start + offset, size); > > + return -ENOMEM; > > + } > > I'm curious here. I was always under the impression that you were > supposed to specify all of your memory regions in the device tree. > ...but here the device tree on rk3288 says: > > vopb: vop@ff930000 { > compatible = "rockchip,rk3288-vop"; > reg = <0x0 0xff930000 0x0 0x19c>; > ... > }; > > ...and we're now mapping 4096 bytes starting at 0xff931000. Is that > really legit? Wouldn't it be better to put this extra memory range in > the dts? > > Hrm, but then I guess you need to figure out what to do about older > device trees. Do you disable the gamma LUT feature? ...or do you do > exactly what the code here is doing and just map it anyway? I guess > you could just keep the code here (and it'll work fine), but maybe in > parallel we should add it to the .dts file and bindings? > Maybe we can see how it would look adding the LUT as a separate (optional) resource in the devicetree, and dropping support for RK3399, which doesn't seem to work. I'm taking a look at Jacopo's question on atomic_flush vs. atomic_commit_tail, and will prepare a v2. > --- > > I will say that, though I don't know much (anything?) about gamma > LUTs, I ran the Chrome OS "gamma_test" program and saw a pretty RGB > gradient on the both the internal screen and HDMI monitor on my > rk3288-veyron-jerry. Thus: > > Tested-by: Douglas Anderson <dianders@chromium.org> Thanks, Ezequiel
On Tue, Jun 18, 2019 at 9:36 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > On Thu, 2019-06-13 at 15:36 -0400, Ilia Mirkin wrote: > > Note that userspace may provide any size of gamma lut. Have a look at > > i915/intel_color.c:intel_color_check which filters out only the > > allowed sizes. Consider having a special allowance for 256-sized LUTs > > since that's what most legacy userspace will set, and it seems like a > > waste to create a 10-bit LUT for RGBA8 color. > > > > Right. I will add a check for the gamma lut size. > > Unfortunately, this hardware seems to only support 10-bit, 1024-sized LUTs. > > The spec does mention a support 8-bit, 256-entries, but it's not at all > clear how configure that. It's up to you, and the more experienced drm reviewers, but even if you can't figure out how to bend the hardware to your will (which is worth a bit of exploration), you can still emulate it by just repeating all the values 4x. IMHO 256-sized LUTs are worth supporting when possible. Cheers, -ilia
On Tue, 2019-06-18 at 02:15 -0300, Ezequiel Garcia wrote: > On Mon, 2019-06-17 at 12:06 +0200, Jacopo Mondi wrote: > > Hi Ezequiel, > > one small question, as I'm working on supporting gamma LUT for > > rcar-du as well, and there's one point not totally clear to me > > > > > > On Thu, Jun 13, 2019 at 04:22:44PM -0300, Ezequiel Garcia wrote: > > > Add CRTC gamma LUT configuration on RK3288 and RK3399. > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > --- > > > This patch seems to work well on RK3288, but produces > > > a distorted output on RK3399. I was hoping > > > someone could have any idea, so we can support both > > > platforms. > > > > > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 87 +++++++++++++++++++++ > > > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 2 + > > > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 4 + > > > drivers/gpu/drm/rockchip/rockchip_vop_reg.h | 1 + > > > 4 files changed, 94 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > > index 12ed5265a90b..8381679c1045 100644 > > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > > @@ -38,6 +38,8 @@ > > > #include "rockchip_drm_vop.h" > > > #include "rockchip_rgb.h" > > > > > > +#define VOP_GAMMA_LUT_SIZE 1024 > > > + > > > #define VOP_WIN_SET(vop, win, name, v) \ > > > vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name) > > > #define VOP_SCL_SET(vop, win, name, v) \ > > > @@ -137,6 +139,7 @@ struct vop { > > > > > > uint32_t *regsbak; > > > void __iomem *regs; > > > + void __iomem *lut_regs; > > > > > > /* physical map length of vop register */ > > > uint32_t len; > > > @@ -1153,6 +1156,46 @@ static void vop_wait_for_irq_handler(struct vop *vop) > > > synchronize_irq(vop->irq); > > > } > > > > > > +static bool vop_dsp_lut_is_enable(struct vop *vop) > > > +{ > > > + return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); > > > +} > > > + > > > +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc, > > > + struct drm_crtc_state *state) > > > +{ > > > + struct drm_color_lut *lut; > > > + int i, idle, ret; > > > + > > > + if (!state->gamma_lut) > > > + return; > > > + lut = state->gamma_lut->data; > > > + > > > + spin_lock(&vop->reg_lock); > > > + VOP_REG_SET(vop, common, dsp_lut_en, 0); > > > + vop_cfg_done(vop); > > > + spin_unlock(&vop->reg_lock); > > > + > > > + ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop, > > > + idle, !idle, 5, 10 * 30000); > > > + if (ret) > > > + return; > > > + > > > + spin_lock(&vop->reg_lock); > > > + for (i = 0; i < crtc->gamma_size; i++) { > > > + u32 word; > > > + > > > + word = (drm_color_lut_extract(lut[i].red, 10) << 20) | > > > + (drm_color_lut_extract(lut[i].green, 10) << 10) | > > > + drm_color_lut_extract(lut[i].blue, 10); > > > + writel(word, vop->lut_regs + i * 4); > > > + } > > > + > > > + VOP_REG_SET(vop, common, dsp_lut_en, 1); > > > + vop_cfg_done(vop); > > > + spin_unlock(&vop->reg_lock); > > > +} > > > + > > > static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > > > struct drm_crtc_state *old_crtc_state) > > > { > > > @@ -1201,6 +1244,9 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, > > > drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb); > > > set_bit(VOP_PENDING_FB_UNREF, &vop->pending); > > > } > > > + > > > + if (vop->lut_regs && crtc->state->color_mgmt_changed) > > > + vop_crtc_gamma_set(vop, crtc, crtc->state); > > > > Which one is the right point when to call LUT update functions? > > > > I initially added my callback in atomic_flush as you did here, mostly > > because I've found examples in other drivers in drm and went in > > cargo cult mode. I've been then suggested by Laurent that atomic_flush() > > might not be the right place where to do so, as it gets called after > > the plane updates (iirc, the DRM atomic API is not something I'm that > > familiar with yet). > > > > So I moved my LUT update function in the atomic_commit_tail callback, > > which is meant to actually commit a CRTC to the hw. > > > > What's your opinion on this? > > > > I have to admit this is not exactly clear to me either. > > Let me make sure I understand the issue. You are concerned about > getting some tearing if the CRTC gamma LUT is affected > in the atomic_flush? > > If that's the case, it shouldn't be too hard to confirm (I think). > As we suspected, indeed setting the gamma lut in atomic_flush is exposed to tearing, if the atomic API is used. As Laurent suggested you, setting from atomic_commit_tail seems correct, as an example you can see the malidp driver. I'm preparing a v2 patch. Thanks, Ezequiel
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 12ed5265a90b..8381679c1045 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -38,6 +38,8 @@ #include "rockchip_drm_vop.h" #include "rockchip_rgb.h" +#define VOP_GAMMA_LUT_SIZE 1024 + #define VOP_WIN_SET(vop, win, name, v) \ vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name) #define VOP_SCL_SET(vop, win, name, v) \ @@ -137,6 +139,7 @@ struct vop { uint32_t *regsbak; void __iomem *regs; + void __iomem *lut_regs; /* physical map length of vop register */ uint32_t len; @@ -1153,6 +1156,46 @@ static void vop_wait_for_irq_handler(struct vop *vop) synchronize_irq(vop->irq); } +static bool vop_dsp_lut_is_enable(struct vop *vop) +{ + return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); +} + +static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + struct drm_color_lut *lut; + int i, idle, ret; + + if (!state->gamma_lut) + return; + lut = state->gamma_lut->data; + + spin_lock(&vop->reg_lock); + VOP_REG_SET(vop, common, dsp_lut_en, 0); + vop_cfg_done(vop); + spin_unlock(&vop->reg_lock); + + ret = readx_poll_timeout(vop_dsp_lut_is_enable, vop, + idle, !idle, 5, 10 * 30000); + if (ret) + return; + + spin_lock(&vop->reg_lock); + for (i = 0; i < crtc->gamma_size; i++) { + u32 word; + + word = (drm_color_lut_extract(lut[i].red, 10) << 20) | + (drm_color_lut_extract(lut[i].green, 10) << 10) | + drm_color_lut_extract(lut[i].blue, 10); + writel(word, vop->lut_regs + i * 4); + } + + VOP_REG_SET(vop, common, dsp_lut_en, 1); + vop_cfg_done(vop); + spin_unlock(&vop->reg_lock); +} + static void vop_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) { @@ -1201,6 +1244,9 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb); set_bit(VOP_PENDING_FB_UNREF, &vop->pending); } + + if (vop->lut_regs && crtc->state->color_mgmt_changed) + vop_crtc_gamma_set(vop, crtc, crtc->state); } static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = { @@ -1323,6 +1369,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = { .disable_vblank = vop_crtc_disable_vblank, .set_crc_source = vop_crtc_set_crc_source, .verify_crc_source = vop_crtc_verify_crc_source, + .gamma_set = drm_atomic_helper_legacy_gamma_set, }; static void vop_fb_unref_worker(struct drm_flip_work *work, void *val) @@ -1480,6 +1527,8 @@ static int vop_create_crtc(struct vop *vop) goto err_cleanup_planes; drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs); + drm_mode_crtc_set_gamma_size(crtc, VOP_GAMMA_LUT_SIZE); + drm_crtc_enable_color_mgmt(crtc, 0, false, VOP_GAMMA_LUT_SIZE); /* * Create drm_planes for overlay windows with possible_crtcs restricted @@ -1744,6 +1793,41 @@ int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout) } EXPORT_SYMBOL(rockchip_drm_wait_vact_end); +static int vop_gamma_lut_request(struct device *dev, + struct resource *res, struct vop *vop) +{ + resource_size_t offset = vop->data->gamma_lut_addr_off; + resource_size_t size = VOP_GAMMA_LUT_SIZE * 4; + + /* + * Some SoCs (e.g. RK3288) have the gamma LUT address after + * the MMU registers, which means we can't request and ioremap + * the entire register set. Other (e.g. RK3399) have gamma LUT + * address before MMU. + * + * Therefore, we need to request and ioremap those that haven't + * been already. + */ + if (vop->len >= (offset + size)) { + vop->lut_regs = vop->regs + offset; + return 0; + } + + if (!devm_request_mem_region(dev, res->start + offset, + size, dev_name(dev))) { + dev_warn(dev, "can't request gamma lut region\n"); + return -EBUSY; + } + + vop->lut_regs = devm_ioremap(dev, res->start + offset, size); + if (!vop->lut_regs) { + dev_err(dev, "can't ioremap gamma lut address\n"); + devm_release_mem_region(dev, res->start + offset, size); + return -ENOMEM; + } + return 0; +} + static int vop_bind(struct device *dev, struct device *master, void *data) { struct platform_device *pdev = to_platform_device(dev); @@ -1776,6 +1860,9 @@ static int vop_bind(struct device *dev, struct device *master, void *data) if (IS_ERR(vop->regs)) return PTR_ERR(vop->regs); + if (vop->data->gamma_lut_addr_off) + vop_gamma_lut_request(dev, res, vop); + vop->regsbak = devm_kzalloc(dev, vop->len, GFP_KERNEL); if (!vop->regsbak) return -ENOMEM; diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h index 2149a889c29d..12d5bde0d0bc 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h @@ -67,6 +67,7 @@ struct vop_common { struct vop_reg dither_down_mode; struct vop_reg dither_down_en; struct vop_reg dither_up; + struct vop_reg dsp_lut_en; struct vop_reg gate_en; struct vop_reg mmu_en; struct vop_reg out_mode; @@ -170,6 +171,7 @@ struct vop_data { const struct vop_win_yuv2yuv_data *win_yuv2yuv; const struct vop_win_data *win; unsigned int win_size; + off_t gamma_lut_addr_off; #define VOP_FEATURE_OUTPUT_RGB10 BIT(0) #define VOP_FEATURE_INTERNAL_RGB BIT(1) diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c index 7b9c74750f6d..63fbb384893b 100644 --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c @@ -593,6 +593,7 @@ static const struct vop_common rk3288_common = { .dither_down_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 2), .pre_dither_down = VOP_REG(RK3288_DSP_CTRL1, 0x1, 1), .dither_up = VOP_REG(RK3288_DSP_CTRL1, 0x1, 6), + .dsp_lut_en = VOP_REG(RK3288_DSP_CTRL1, 0x1, 0), .data_blank = VOP_REG(RK3288_DSP_CTRL0, 0x1, 19), .dsp_blank = VOP_REG(RK3288_DSP_CTRL0, 0x3, 18), .out_mode = VOP_REG(RK3288_DSP_CTRL0, 0xf, 0), @@ -641,6 +642,7 @@ static const struct vop_data rk3288_vop = { .output = &rk3288_output, .win = rk3288_vop_win_data, .win_size = ARRAY_SIZE(rk3288_vop_win_data), + .gamma_lut_addr_off = RK3288_GAMMA_LUT_ADDR, }; static const int rk3368_vop_intrs[] = { @@ -811,6 +813,7 @@ static const struct vop_data rk3399_vop_big = { .win = rk3368_vop_win_data, .win_size = ARRAY_SIZE(rk3368_vop_win_data), .win_yuv2yuv = rk3399_vop_big_win_yuv2yuv_data, + .gamma_lut_addr_off = RK3399_GAMMA_LUT_ADDR, }; static const struct vop_win_data rk3399_vop_lit_win_data[] = { @@ -836,6 +839,7 @@ static const struct vop_data rk3399_vop_lit = { .win = rk3399_vop_lit_win_data, .win_size = ARRAY_SIZE(rk3399_vop_lit_win_data), .win_yuv2yuv = rk3399_vop_lit_win_yuv2yuv_data, + .gamma_lut_addr_off = RK3399_GAMMA_LUT_ADDR, }; static const struct vop_win_data rk3228_vop_win_data[] = { diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h index 6e9fa5815d4d..490318382f74 100644 --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h @@ -113,6 +113,7 @@ #define RK3288_DSP_VACT_ST_END 0x0194 #define RK3288_DSP_VS_ST_END_F1 0x0198 #define RK3288_DSP_VACT_ST_END_F1 0x019c +#define RK3288_GAMMA_LUT_ADDR 0x1000 /* register definition end */ /* rk3368 register definition */
Add CRTC gamma LUT configuration on RK3288 and RK3399. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- This patch seems to work well on RK3288, but produces a distorted output on RK3399. I was hoping someone could have any idea, so we can support both platforms. drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 87 +++++++++++++++++++++ drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 2 + drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 4 + drivers/gpu/drm/rockchip/rockchip_vop_reg.h | 1 + 4 files changed, 94 insertions(+)