| Message ID | 20181214162920.20361-1-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/rockchip: Fix YUV buffers color rendering | expand |
On Fri, 2018-12-14 at 13:29 -0300, Ezequiel Garcia wrote: > From: Daniele Castagna <dcastagna@chromium.org> > > Currently, YUV hardware overlays are converted to RGB using > a color space conversion different than BT.601. > > The result is that colors of e.g. NV12 buffers don't match > colors of YUV hardware overlays. > > In order to fix this, enable YUV2YUV and set appropriate coefficients > for formats such as NV12 to be displayed correctly. > > This commit was tested using modetest, gstreamer and chromeos (hardware > accelerated video playback). Before the commit, tests rendering > with NV12 format resulted in colors not displayed correctly. > > Test examples (RK3399 Ficus board connected to HDMI monitor): > > $ modetest 39@32:1920x1080@NV12 > $ gst-launch-1.0 videotestrc ! video/x-raw,format=NV12 ! kmssink > > Signed-off-by: Daniele Castagna <dcastagna@chromium.org> > [ezequiel: rebase on linux-next and massage commit log] > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > v2: > * Addressed feedback from Sean Paul > * Rebased on linux-next > Any feedback on this one? Thanks! Ezequiel > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 ++++++++++++++++++++- > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 13 +++++++ > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 36 ++++++++++++++++++ > 3 files changed, 89 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index fb70fb486fbf..78c7f63a60c0 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -52,6 +52,18 @@ > vop_reg_set(vop, &win->phy->scl->ext->name, \ > win->base, ~0, v, #name) > > +#define VOP_WIN_YUV2YUV_SET(x, win_yuv2yuv, name, v) \ > + do { \ > + if (win_yuv2yuv->name.mask) \ > + vop_reg_set(vop, &win_yuv2yuv->name, 0, ~0, v, #name); \ > + } while (0) > + > +#define VOP_WIN_YUV2YUV_COEFFICIENT_SET(x, win_yuv2yuv, name, v) \ > + do { \ > + if (win_yuv2yuv->phy->name.mask) \ > + vop_reg_set(vop, &win_yuv2yuv->phy->name, win_yuv2yuv->base, ~0, v, #name); \ > + } while (0) > + > #define VOP_INTR_SET_MASK(vop, name, mask, v) \ > vop_reg_set(vop, &vop->data->intr->name, 0, mask, v, #name) > > @@ -84,6 +96,18 @@ > #define to_vop(x) container_of(x, struct vop, crtc) > #define to_vop_win(x) container_of(x, struct vop_win, base) > > +/* > + * The coefficients of the following matrix are all fixed points. > + * The format is S2.10 for the 3x3 part of the matrix, and S9.12 for the offsets. > + * They are all represented in two's complement. > + */ > +static const uint32_t rockchip_bt601_yuv2rgb[] = { > + 0x000004a8, 0x00000000, 0x00000662, > + 0x000004a8, 0x00001e6f, 0x00001cbf, > + 0x000004a8, 0x00000812, 0x00000000, > + 0x00321168, 0x000877cf, 0x002eb127 > +}; > + > enum vop_pending { > VOP_PENDING_FB_UNREF, > }; > @@ -91,6 +115,7 @@ enum vop_pending { > struct vop_win { > struct drm_plane base; > const struct vop_win_data *data; > + const struct vop_win_yuv2yuv_data *yuv2yuv_data; > struct vop *vop; > }; > > @@ -712,6 +737,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane, > struct drm_crtc *crtc = state->crtc; > struct vop_win *vop_win = to_vop_win(plane); > const struct vop_win_data *win = vop_win->data; > + const struct vop_win_yuv2yuv_data *win_yuv2yuv = vop_win->yuv2yuv_data; > struct vop *vop = to_vop(state->crtc); > struct drm_framebuffer *fb = state->fb; > unsigned int actual_w, actual_h; > @@ -727,6 +753,8 @@ static void vop_plane_atomic_update(struct drm_plane *plane, > bool rb_swap; > int win_index = VOP_WIN_TO_INDEX(vop_win); > int format; > + int is_yuv = fb->format->is_yuv; > + int i; > > /* > * can't update plane when vop is disabled. > @@ -767,7 +795,10 @@ static void vop_plane_atomic_update(struct drm_plane *plane, > VOP_WIN_SET(vop, win, format, format); > VOP_WIN_SET(vop, win, yrgb_vir, DIV_ROUND_UP(fb->pitches[0], 4)); > VOP_WIN_SET(vop, win, yrgb_mst, dma_addr); > - if (fb->format->is_yuv) { > + > + VOP_WIN_YUV2YUV_SET(vop, win_yuv2yuv, y2r_en, is_yuv); > + > + if (is_yuv) { > int hsub = drm_format_horz_chroma_subsampling(fb->format->format); > int vsub = drm_format_vert_chroma_subsampling(fb->format->format); > int bpp = fb->format->cpp[1]; > @@ -781,6 +812,13 @@ static void vop_plane_atomic_update(struct drm_plane *plane, > dma_addr = rk_uv_obj->dma_addr + offset + fb->offsets[1]; > VOP_WIN_SET(vop, win, uv_vir, DIV_ROUND_UP(fb->pitches[1], 4)); > VOP_WIN_SET(vop, win, uv_mst, dma_addr); > + > + for (i = 0; i < NUM_YUV2YUV_COEFFICIENTS; i++) { > + VOP_WIN_YUV2YUV_COEFFICIENT_SET(vop, > + win_yuv2yuv, > + y2r_coefficients[i], > + rockchip_bt601_yuv2rgb[i]); > + } > } > > if (win->phy->scl) > @@ -1529,6 +1567,7 @@ static void vop_win_init(struct vop *vop) > > vop_win->data = win_data; > vop_win->vop = vop; > + vop_win->yuv2yuv_data = &vop_data->win_yuv2yuv[i]; > } > } > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > index 0fe40e1983d9..aed467cd81b9 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h > @@ -23,6 +23,8 @@ > #define VOP_MAJOR(version) ((version) >> 8) > #define VOP_MINOR(version) ((version) & 0xff) > > +#define NUM_YUV2YUV_COEFFICIENTS 12 > + > enum vop_data_format { > VOP_FMT_ARGB8888 = 0, > VOP_FMT_RGB888, > @@ -124,6 +126,10 @@ struct vop_scl_regs { > struct vop_reg scale_cbcr_y; > }; > > +struct vop_yuv2yuv_phy { > + struct vop_reg y2r_coefficients[NUM_YUV2YUV_COEFFICIENTS]; > +}; > + > struct vop_win_phy { > const struct vop_scl_regs *scl; > const uint32_t *data_formats; > @@ -146,6 +152,12 @@ struct vop_win_phy { > struct vop_reg channel; > }; > > +struct vop_win_yuv2yuv_data { > + uint32_t base; > + const struct vop_yuv2yuv_phy *phy; > + struct vop_reg y2r_en; > +}; > + > struct vop_win_data { > uint32_t base; > const struct vop_win_phy *phy; > @@ -159,6 +171,7 @@ struct vop_data { > const struct vop_misc *misc; > const struct vop_modeset *modeset; > const struct vop_output *output; > + const struct vop_win_yuv2yuv_data *win_yuv2yuv; > const struct vop_win_data *win; > unsigned int win_size; > > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > index 08fc40af52c8..fe752df4e038 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > @@ -637,6 +637,34 @@ static const struct vop_output rk3399_output = { > .mipi_dual_channel_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 3), > }; > > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win01_data = { > + .y2r_coefficients = { > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 16), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 16), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 16), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 16), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 16, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 20, 0xffffffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 24, 0xffffffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 28, 0xffffffff, 0), > + }, > +}; > + > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win23_data = { }; > + > +static const struct vop_win_yuv2yuv_data rk3399_vop_big_win_yuv2yuv_data[] = { > + { .base = 0x00, .phy = &rk3399_yuv2yuv_win01_data, > + .y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 1) }, > + { .base = 0x60, .phy = &rk3399_yuv2yuv_win01_data, > + .y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 9) }, > + { .base = 0xC0, .phy = &rk3399_yuv2yuv_win23_data }, > + { .base = 0x120, .phy = &rk3399_yuv2yuv_win23_data }, > +}; > + > static const struct vop_data rk3399_vop_big = { > .version = VOP_VERSION(3, 5), > .feature = VOP_FEATURE_OUTPUT_RGB10, > @@ -647,6 +675,7 @@ static const struct vop_data rk3399_vop_big = { > .misc = &rk3368_misc, > .win = rk3368_vop_win_data, > .win_size = ARRAY_SIZE(rk3368_vop_win_data), > + .win_yuv2yuv = rk3399_vop_big_win_yuv2yuv_data, > }; > > static const struct vop_win_data rk3399_vop_lit_win_data[] = { > @@ -656,6 +685,12 @@ static const struct vop_win_data rk3399_vop_lit_win_data[] = { > .type = DRM_PLANE_TYPE_CURSOR}, > }; > > +static const struct vop_win_yuv2yuv_data rk3399_vop_lit_win_yuv2yuv_data[] = { > + { .base = 0x00, .phy = &rk3399_yuv2yuv_win01_data, > + .y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 1)}, > + { .base = 0x60, .phy = &rk3399_yuv2yuv_win23_data }, > +}; > + > static const struct vop_data rk3399_vop_lit = { > .version = VOP_VERSION(3, 6), > .intr = &rk3366_vop_intr, > @@ -665,6 +700,7 @@ static const struct vop_data rk3399_vop_lit = { > .misc = &rk3368_misc, > .win = rk3399_vop_lit_win_data, > .win_size = ARRAY_SIZE(rk3399_vop_lit_win_data), > + .win_yuv2yuv = rk3399_vop_lit_win_yuv2yuv_data, > }; > > static const struct vop_win_data rk3228_vop_win_data[] = {
Hi, sorry, only now got to test this on actual hardware, Am Freitag, 14. Dezember 2018, 17:29:20 CET schrieb Ezequiel Garcia: > From: Daniele Castagna <dcastagna@chromium.org> > > Currently, YUV hardware overlays are converted to RGB using > a color space conversion different than BT.601. > > The result is that colors of e.g. NV12 buffers don't match > colors of YUV hardware overlays. > > In order to fix this, enable YUV2YUV and set appropriate coefficients > for formats such as NV12 to be displayed correctly. > > This commit was tested using modetest, gstreamer and chromeos (hardware > accelerated video playback). Before the commit, tests rendering > with NV12 format resulted in colors not displayed correctly. > > Test examples (RK3399 Ficus board connected to HDMI monitor): > > $ modetest 39@32:1920x1080@NV12 > $ gst-launch-1.0 videotestrc ! video/x-raw,format=NV12 ! kmssink > > Signed-off-by: Daniele Castagna <dcastagna@chromium.org> > [ezequiel: rebase on linux-next and massage commit log] > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > v2: > * Addressed feedback from Sean Paul > * Rebased on linux-next > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 ++++++++++++++++++++- > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 13 +++++++ > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 36 ++++++++++++++++++ > 3 files changed, 89 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index fb70fb486fbf..78c7f63a60c0 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -52,6 +52,18 @@ > vop_reg_set(vop, &win->phy->scl->ext->name, \ > win->base, ~0, v, #name) > > +#define VOP_WIN_YUV2YUV_SET(x, win_yuv2yuv, name, v) \ > + do { \ > + if (win_yuv2yuv->name.mask) \ > + vop_reg_set(vop, &win_yuv2yuv->name, 0, ~0, v, #name); \ > + } while (0) > + > +#define VOP_WIN_YUV2YUV_COEFFICIENT_SET(x, win_yuv2yuv, name, v) \ > + do { \ > + if (win_yuv2yuv->phy->name.mask) \ > + vop_reg_set(vop, &win_yuv2yuv->phy->name, win_yuv2yuv->base, ~0, v, #name); \ > + } while (0) > + While this seems to work on rk3399, it hangs both my rk3328 (rock64) and rk3288 (google-pinky) during rockchip-drm probe. Making this something like if (win_yuv2yuv && win_yuv2yuv->phy->name.mask) \ aka testing for existence of win_yuv2yuv first, makes them boot again, so I guess I ran into a (for whatever reason) silent null-ptr-dereference. > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > index 08fc40af52c8..fe752df4e038 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > @@ -637,6 +637,34 @@ static const struct vop_output rk3399_output = { > .mipi_dual_channel_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 3), > }; > > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win01_data = { > + .y2r_coefficients = { > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 16), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 16), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 16), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 16), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 16, 0xffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 20, 0xffffffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 24, 0xffffffff, 0), > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 28, 0xffffffff, 0), > + }, > +}; > + > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win23_data = { }; looking at the rk3399 TRM it seems that win2+3 also have yuv2rgb coefficient registers. I didn't check in depth but are they so different that they cannot be supported? Aka what is the difference between win0/1 and win2/3 ? Heiko
On Mon, 2019-01-07 at 14:26 +0100, Heiko Stuebner wrote: > Hi, > > sorry, only now got to test this on actual hardware, > > Am Freitag, 14. Dezember 2018, 17:29:20 CET schrieb Ezequiel Garcia: > > From: Daniele Castagna <dcastagna@chromium.org> > > > > Currently, YUV hardware overlays are converted to RGB using > > a color space conversion different than BT.601. > > > > The result is that colors of e.g. NV12 buffers don't match > > colors of YUV hardware overlays. > > > > In order to fix this, enable YUV2YUV and set appropriate coefficients > > for formats such as NV12 to be displayed correctly. > > > > This commit was tested using modetest, gstreamer and chromeos (hardware > > accelerated video playback). Before the commit, tests rendering > > with NV12 format resulted in colors not displayed correctly. > > > > Test examples (RK3399 Ficus board connected to HDMI monitor): > > > > $ modetest 39@32:1920x1080@NV12 > > $ gst-launch-1.0 videotestrc ! video/x-raw,format=NV12 ! kmssink > > > > Signed-off-by: Daniele Castagna <dcastagna@chromium.org> > > [ezequiel: rebase on linux-next and massage commit log] > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > v2: > > * Addressed feedback from Sean Paul > > * Rebased on linux-next > > > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 ++++++++++++++++++++- > > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 13 +++++++ > > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 36 ++++++++++++++++++ > > 3 files changed, 89 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > index fb70fb486fbf..78c7f63a60c0 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > @@ -52,6 +52,18 @@ > > vop_reg_set(vop, &win->phy->scl->ext->name, \ > > win->base, ~0, v, #name) > > > > +#define VOP_WIN_YUV2YUV_SET(x, win_yuv2yuv, name, v) \ > > + do { \ > > + if (win_yuv2yuv->name.mask) \ > > + vop_reg_set(vop, &win_yuv2yuv->name, 0, ~0, v, #name); \ > > + } while (0) > > + > > +#define VOP_WIN_YUV2YUV_COEFFICIENT_SET(x, win_yuv2yuv, name, v) \ > > + do { \ > > + if (win_yuv2yuv->phy->name.mask) \ > > + vop_reg_set(vop, &win_yuv2yuv->phy->name, win_yuv2yuv->base, ~0, v, #name); \ > > + } while (0) > > + > > While this seems to work on rk3399, it hangs both my rk3328 (rock64) > and rk3288 (google-pinky) during rockchip-drm probe. > Oh, shame on me, I should've done that. > Making this something like > > if (win_yuv2yuv && win_yuv2yuv->phy->name.mask) \ > > aka testing for existence of win_yuv2yuv first, makes them boot again, > so I guess I ran into a (for whatever reason) silent null-ptr-dereference. > Sounds good. I'll post a v3. > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > index 08fc40af52c8..fe752df4e038 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > @@ -637,6 +637,34 @@ static const struct vop_output rk3399_output = { > > .mipi_dual_channel_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 3), > > }; > > > > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win01_data = { > > + .y2r_coefficients = { > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 0), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 16), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 0), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 16), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 0), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 16), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 0), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 16), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 16, 0xffff, 0), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 20, 0xffffffff, 0), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 24, 0xffffffff, 0), > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 28, 0xffffffff, 0), > > + }, > > +}; > > + > > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win23_data = { }; > > looking at the rk3399 TRM it seems that win2+3 also have yuv2rgb > coefficient registers. I didn't check in depth but are they so different > that they cannot be supported? > > Aka what is the difference between win0/1 and win2/3 ? > > I think Sandy is the best to answer this. I can't say I'm an expert on the details of this hardware. Thanks, Eze
On Mon, 2019-01-07 at 17:07 -0300, Ezequiel Garcia wrote: > On Mon, 2019-01-07 at 14:26 +0100, Heiko Stuebner wrote: > > Hi, > > > > sorry, only now got to test this on actual hardware, > > > > Am Freitag, 14. Dezember 2018, 17:29:20 CET schrieb Ezequiel Garcia: > > > From: Daniele Castagna <dcastagna@chromium.org> > > > > > > Currently, YUV hardware overlays are converted to RGB using > > > a color space conversion different than BT.601. > > > > > > The result is that colors of e.g. NV12 buffers don't match > > > colors of YUV hardware overlays. > > > > > > In order to fix this, enable YUV2YUV and set appropriate coefficients > > > for formats such as NV12 to be displayed correctly. > > > > > > This commit was tested using modetest, gstreamer and chromeos (hardware > > > accelerated video playback). Before the commit, tests rendering > > > with NV12 format resulted in colors not displayed correctly. > > > > > > Test examples (RK3399 Ficus board connected to HDMI monitor): > > > > > > $ modetest 39@32:1920x1080@NV12 > > > $ gst-launch-1.0 videotestrc ! video/x-raw,format=NV12 ! kmssink > > > > > > Signed-off-by: Daniele Castagna <dcastagna@chromium.org> > > > [ezequiel: rebase on linux-next and massage commit log] > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > --- > > > v2: > > > * Addressed feedback from Sean Paul > > > * Rebased on linux-next > > > > > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 ++++++++++++++++++++- > > > drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 13 +++++++ > > > drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 36 ++++++++++++++++++ > > > 3 files changed, 89 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > > index fb70fb486fbf..78c7f63a60c0 100644 > > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > > > @@ -52,6 +52,18 @@ > > > vop_reg_set(vop, &win->phy->scl->ext->name, \ > > > win->base, ~0, v, #name) > > > > > > +#define VOP_WIN_YUV2YUV_SET(x, win_yuv2yuv, name, v) \ > > > + do { \ > > > + if (win_yuv2yuv->name.mask) \ > > > + vop_reg_set(vop, &win_yuv2yuv->name, 0, ~0, v, #name); \ > > > + } while (0) > > > + > > > +#define VOP_WIN_YUV2YUV_COEFFICIENT_SET(x, win_yuv2yuv, name, v) \ > > > + do { \ > > > + if (win_yuv2yuv->phy->name.mask) \ > > > + vop_reg_set(vop, &win_yuv2yuv->phy->name, win_yuv2yuv->base, ~0, v, #name); \ > > > + } while (0) > > > + > > > > While this seems to work on rk3399, it hangs both my rk3328 (rock64) > > and rk3288 (google-pinky) during rockchip-drm probe. > > > > Oh, shame on me, I should've done that. > > > Making this something like > > > > if (win_yuv2yuv && win_yuv2yuv->phy->name.mask) \ > > > > aka testing for existence of win_yuv2yuv first, makes them boot again, > > so I guess I ran into a (for whatever reason) silent null-ptr-dereference. > > > > Sounds good. I'll post a v3. > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > > index 08fc40af52c8..fe752df4e038 100644 > > > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c > > > @@ -637,6 +637,34 @@ static const struct vop_output rk3399_output = { > > > .mipi_dual_channel_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 3), > > > }; > > > > > > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win01_data = { > > > + .y2r_coefficients = { > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 0), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 16), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 0), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 16), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 0), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 16), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 0), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 16), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 16, 0xffff, 0), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 20, 0xffffffff, 0), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 24, 0xffffffff, 0), > > > + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 28, 0xffffffff, 0), > > > + }, > > > +}; > > > + > > > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win23_data = { }; > > > > looking at the rk3399 TRM it seems that win2+3 also have yuv2rgb > > coefficient registers. I didn't check in depth but are they so different > > that they cannot be supported? > > > > Aka what is the difference between win0/1 and win2/3 ? > > > > > > I think Sandy is the best to answer this. I can't say I'm an expert > on the details of this hardware. > Reading thru the spec, I see what wins 2 and 3 don't support YUV formats, so the feature (whatever it's purpose) is not needed, as there's no YUV colorspace issue. Like I said, I don't have specific knowledge on the hardware. Thanks, Eze
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index fb70fb486fbf..78c7f63a60c0 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -52,6 +52,18 @@ vop_reg_set(vop, &win->phy->scl->ext->name, \ win->base, ~0, v, #name) +#define VOP_WIN_YUV2YUV_SET(x, win_yuv2yuv, name, v) \ + do { \ + if (win_yuv2yuv->name.mask) \ + vop_reg_set(vop, &win_yuv2yuv->name, 0, ~0, v, #name); \ + } while (0) + +#define VOP_WIN_YUV2YUV_COEFFICIENT_SET(x, win_yuv2yuv, name, v) \ + do { \ + if (win_yuv2yuv->phy->name.mask) \ + vop_reg_set(vop, &win_yuv2yuv->phy->name, win_yuv2yuv->base, ~0, v, #name); \ + } while (0) + #define VOP_INTR_SET_MASK(vop, name, mask, v) \ vop_reg_set(vop, &vop->data->intr->name, 0, mask, v, #name) @@ -84,6 +96,18 @@ #define to_vop(x) container_of(x, struct vop, crtc) #define to_vop_win(x) container_of(x, struct vop_win, base) +/* + * The coefficients of the following matrix are all fixed points. + * The format is S2.10 for the 3x3 part of the matrix, and S9.12 for the offsets. + * They are all represented in two's complement. + */ +static const uint32_t rockchip_bt601_yuv2rgb[] = { + 0x000004a8, 0x00000000, 0x00000662, + 0x000004a8, 0x00001e6f, 0x00001cbf, + 0x000004a8, 0x00000812, 0x00000000, + 0x00321168, 0x000877cf, 0x002eb127 +}; + enum vop_pending { VOP_PENDING_FB_UNREF, }; @@ -91,6 +115,7 @@ enum vop_pending { struct vop_win { struct drm_plane base; const struct vop_win_data *data; + const struct vop_win_yuv2yuv_data *yuv2yuv_data; struct vop *vop; }; @@ -712,6 +737,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane, struct drm_crtc *crtc = state->crtc; struct vop_win *vop_win = to_vop_win(plane); const struct vop_win_data *win = vop_win->data; + const struct vop_win_yuv2yuv_data *win_yuv2yuv = vop_win->yuv2yuv_data; struct vop *vop = to_vop(state->crtc); struct drm_framebuffer *fb = state->fb; unsigned int actual_w, actual_h; @@ -727,6 +753,8 @@ static void vop_plane_atomic_update(struct drm_plane *plane, bool rb_swap; int win_index = VOP_WIN_TO_INDEX(vop_win); int format; + int is_yuv = fb->format->is_yuv; + int i; /* * can't update plane when vop is disabled. @@ -767,7 +795,10 @@ static void vop_plane_atomic_update(struct drm_plane *plane, VOP_WIN_SET(vop, win, format, format); VOP_WIN_SET(vop, win, yrgb_vir, DIV_ROUND_UP(fb->pitches[0], 4)); VOP_WIN_SET(vop, win, yrgb_mst, dma_addr); - if (fb->format->is_yuv) { + + VOP_WIN_YUV2YUV_SET(vop, win_yuv2yuv, y2r_en, is_yuv); + + if (is_yuv) { int hsub = drm_format_horz_chroma_subsampling(fb->format->format); int vsub = drm_format_vert_chroma_subsampling(fb->format->format); int bpp = fb->format->cpp[1]; @@ -781,6 +812,13 @@ static void vop_plane_atomic_update(struct drm_plane *plane, dma_addr = rk_uv_obj->dma_addr + offset + fb->offsets[1]; VOP_WIN_SET(vop, win, uv_vir, DIV_ROUND_UP(fb->pitches[1], 4)); VOP_WIN_SET(vop, win, uv_mst, dma_addr); + + for (i = 0; i < NUM_YUV2YUV_COEFFICIENTS; i++) { + VOP_WIN_YUV2YUV_COEFFICIENT_SET(vop, + win_yuv2yuv, + y2r_coefficients[i], + rockchip_bt601_yuv2rgb[i]); + } } if (win->phy->scl) @@ -1529,6 +1567,7 @@ static void vop_win_init(struct vop *vop) vop_win->data = win_data; vop_win->vop = vop; + vop_win->yuv2yuv_data = &vop_data->win_yuv2yuv[i]; } } diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h index 0fe40e1983d9..aed467cd81b9 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h @@ -23,6 +23,8 @@ #define VOP_MAJOR(version) ((version) >> 8) #define VOP_MINOR(version) ((version) & 0xff) +#define NUM_YUV2YUV_COEFFICIENTS 12 + enum vop_data_format { VOP_FMT_ARGB8888 = 0, VOP_FMT_RGB888, @@ -124,6 +126,10 @@ struct vop_scl_regs { struct vop_reg scale_cbcr_y; }; +struct vop_yuv2yuv_phy { + struct vop_reg y2r_coefficients[NUM_YUV2YUV_COEFFICIENTS]; +}; + struct vop_win_phy { const struct vop_scl_regs *scl; const uint32_t *data_formats; @@ -146,6 +152,12 @@ struct vop_win_phy { struct vop_reg channel; }; +struct vop_win_yuv2yuv_data { + uint32_t base; + const struct vop_yuv2yuv_phy *phy; + struct vop_reg y2r_en; +}; + struct vop_win_data { uint32_t base; const struct vop_win_phy *phy; @@ -159,6 +171,7 @@ struct vop_data { const struct vop_misc *misc; const struct vop_modeset *modeset; const struct vop_output *output; + const struct vop_win_yuv2yuv_data *win_yuv2yuv; const struct vop_win_data *win; unsigned int win_size; diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c index 08fc40af52c8..fe752df4e038 100644 --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c @@ -637,6 +637,34 @@ static const struct vop_output rk3399_output = { .mipi_dual_channel_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 3), }; +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win01_data = { + .y2r_coefficients = { + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 0), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 16), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 0), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 16), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 0), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 16), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 0), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 16), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 16, 0xffff, 0), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 20, 0xffffffff, 0), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 24, 0xffffffff, 0), + VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 28, 0xffffffff, 0), + }, +}; + +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win23_data = { }; + +static const struct vop_win_yuv2yuv_data rk3399_vop_big_win_yuv2yuv_data[] = { + { .base = 0x00, .phy = &rk3399_yuv2yuv_win01_data, + .y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 1) }, + { .base = 0x60, .phy = &rk3399_yuv2yuv_win01_data, + .y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 9) }, + { .base = 0xC0, .phy = &rk3399_yuv2yuv_win23_data }, + { .base = 0x120, .phy = &rk3399_yuv2yuv_win23_data }, +}; + static const struct vop_data rk3399_vop_big = { .version = VOP_VERSION(3, 5), .feature = VOP_FEATURE_OUTPUT_RGB10, @@ -647,6 +675,7 @@ static const struct vop_data rk3399_vop_big = { .misc = &rk3368_misc, .win = rk3368_vop_win_data, .win_size = ARRAY_SIZE(rk3368_vop_win_data), + .win_yuv2yuv = rk3399_vop_big_win_yuv2yuv_data, }; static const struct vop_win_data rk3399_vop_lit_win_data[] = { @@ -656,6 +685,12 @@ static const struct vop_win_data rk3399_vop_lit_win_data[] = { .type = DRM_PLANE_TYPE_CURSOR}, }; +static const struct vop_win_yuv2yuv_data rk3399_vop_lit_win_yuv2yuv_data[] = { + { .base = 0x00, .phy = &rk3399_yuv2yuv_win01_data, + .y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 1)}, + { .base = 0x60, .phy = &rk3399_yuv2yuv_win23_data }, +}; + static const struct vop_data rk3399_vop_lit = { .version = VOP_VERSION(3, 6), .intr = &rk3366_vop_intr, @@ -665,6 +700,7 @@ static const struct vop_data rk3399_vop_lit = { .misc = &rk3368_misc, .win = rk3399_vop_lit_win_data, .win_size = ARRAY_SIZE(rk3399_vop_lit_win_data), + .win_yuv2yuv = rk3399_vop_lit_win_yuv2yuv_data, }; static const struct vop_win_data rk3228_vop_win_data[] = {