Hi Jyri,
On Thu, Sep 05, 2019 at 12:24:37PM +0300, Jyri Sarha wrote:
> On 03/09/2019 18:32, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 03:53:57PM +0300, Tomi Valkeinen wrote:
> >> From: Jyri Sarha <jsarha@xxxxxx>
> >>
> >> Adds support for COLOR_ENCODING and COLOR_RANGE properties to
> >> omap_plane.c and dispc.c. The supported encodings and ranges are:
> >>
> >> For COLOR_ENCODING:
> >> - YCbCt BT.601 (default)
> >
> > Did you mean YCbCr ?
> > >> - YCbCt BT.709
> >>
> >> For COLOR_RANGE:
> >> - YCbCt limited range
> >> - YCbCt full range (default)
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> >> ---
> >> drivers/gpu/drm/omapdrm/dss/dispc.c | 119 ++++++++++++++++++++------
> >> drivers/gpu/drm/omapdrm/dss/omapdss.h | 4 +
> >> drivers/gpu/drm/omapdrm/omap_plane.c | 30 +++++++
> >> 3 files changed, 127 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> index 7d87d60edb80..40ddb28ee7aa 100644
> >> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> @@ -892,32 +892,91 @@ static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
> >> #undef CVAL
> >> }
> >>
> >> -static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
> >> +/* YUV -> RGB, ITU-R BT.601, full range */
> >> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
> >
> > static const is usually preferred over const static, here and below.
> >
> >> + 256, 0, 358, /* ry, rcb, rcr |1.000 0.000 1.402|*/
> >> + 256, -88, -182, /* gy, gcb, gcr |1.000 -0.344 -0.714|*/
> >> + 256, 452, 0, /* by, bcb, bcr |1.000 1.772 0.000|*/
> >> + true, /* full range */
> >> +};
> >> +
> >> +/* YUV -> RGB, ITU-R BT.601, limited range */
> >> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> >> + 298, 0, 409, /* ry, rcb, rcr |1.164 0.000 1.596|*/
> >> + 298, -100, -208, /* gy, gcb, gcr |1.164 -0.392 -0.813|*/
> >> + 298, 516, 0, /* by, bcb, bcr |1.164 2.017 0.000|*/
> >> + false, /* limited range */
> >> +};
> >> +
> >> +/* YUV -> RGB, ITU-R BT.709, full range */
> >> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
> >> + 256, 0, 402, /* ry, rcb, rcr |1.000 0.000 1.570|*/
> >> + 256, -48, -120, /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
> >> + 256, 475, 0, /* by, bcb, bcr |1.000 1.856 0.000|*/
> >> + true, /* full range */
> >> +};
> >> +
> >> +/* YUV -> RGB, ITU-R BT.709, limited range */
> >> +const static struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {
> >> + 298, 0, 459, /* ry, rcb, rcr |1.164 0.000 1.793|*/
> >> + 298, -55, -136, /* gy, gcb, gcr |1.164 -0.213 -0.533|*/
> >> + 298, 541, 0, /* by, bcb, bcr |1.164 2.112 0.000|*/
> >> + false, /* limited range */
> >> +};
> >> +
> >> +/* RGB -> YUV, ITU-R BT.601, limited range */
> >> +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
> >> + 66, 129, 25, /* yr, yg, yb | 0.257 0.504 0.098|*/
> >> + -38, -74, 112, /* cbr, cbg, cbb |-0.148 -0.291 0.439|*/
> >> + 112, -94, -18, /* crr, crg, crb | 0.439 -0.368 -0.071|*/
> >> + false, /* limited range */
> >> +};
> >> +
> >> +/* RGB -> YUV, ITU-R BT.601, full range */
> >> +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_full = {
> >> + 77, 150, 29, /* yr, yg, yb | 0.299 0.587 0.114|*/
> >> + -43, -85, 128, /* cbr, cbg, cbb |-0.173 -0.339 0.511|*/
> >> + 128, -107, -21, /* crr, crg, crb | 0.511 -0.428 -0.083|*/
> >> + true, /* full range */
> >> +};
> >> +
> >> +/* RGB -> YUV, ITU-R BT.709, limited range */
> >> +const static struct csc_coef_rgb2yuv coefs_rgb2yuv_bt701_lim = {
> >
> > That should be coefs_rgb2yuv_bt709_lim
> >
> >> + 47, 157, 16, /* yr, yg, yb | 0.1826 0.6142 0.0620|*/
> >> + -26, -87, 112, /* cbr, cbg, cbb |-0.1006 -0.3386 0.4392|*/
> >> + 112, -102, -10, /* crr, crg, crb | 0.4392 -0.3989 -0.0403|*/
> >> + false, /* limited range */
> >> +};
> >
> > Why is coefs_rgb2yuv_bt709_full not supported ? Actually
> > coefs_rgb2yuv_bt601_lim and coefs_rgb2yuv_bt701_lim seem unused.
> >
>
> I must have simply forgotten. This is an old patch and I can not
> remember exactly. But I remember that I wanted to add all CSCs at one
> time so that I do not need start from the beginning when ever a new
> conversion is asked. For the moment rgb to yuv conversions are only used
> for write back and it currently only uses the default BT.601 Full range.
>
> I'll add rgb2yuv_bt709_full. Or should I remove all the unused CSCs? Or
> maybe put them behind some ifdef so that they do not bloat the kernel?
Commented-out code is frowned upon. I would just drop the unused data.
> >> +
> >> +static int dispc_ovl_set_csc(struct dispc_device *dispc,
> >> + enum omap_plane_id plane,
> >> + enum drm_color_encoding color_encoding,
> >> + enum drm_color_range color_range)
> >> {
> >> - int i;
> >> - int num_ovl = dispc_get_num_ovls(dispc);
> >> -
> >> - /* YUV -> RGB, ITU-R BT.601, limited range */
> >> - const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> >> - 298, 0, 409, /* ry, rcb, rcr */
> >> - 298, -100, -208, /* gy, gcb, gcr */
> >> - 298, 516, 0, /* by, bcb, bcr */
> >> - false, /* limited range */
> >> - };
> >> + const struct csc_coef_yuv2rgb *csc;
> >>
> >> - /* RGB -> YUV, ITU-R BT.601, limited range */
> >> - const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
> >> - 66, 129, 25, /* yr, yg, yb */
> >> - -38, -74, 112, /* cbr, cbg, cbb */
> >> - 112, -94, -18, /* crr, crg, crb */
> >> - false, /* limited range */
> >> - };
> >> + switch (color_encoding) {
> >> + case DRM_COLOR_YCBCR_BT601:
> >> + if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> >> + csc = &coefs_yuv2rgb_bt601_full;
> >> + else
> >> + csc = &coefs_yuv2rgb_bt601_lim;
> >> + break;
> >> + case DRM_COLOR_YCBCR_BT709:
> >> + if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> >> + csc = &coefs_yuv2rgb_bt709_full;
> >> + else
> >> + csc = &coefs_yuv2rgb_bt709_lim;
> >> + break;
> >> + default:
> >> + DSSERR("Unsupported CSC mode %d for plane %d\n",
> >> + color_encoding, plane);
> >> + return -EINVAL;
> >> + }
> >>
> >> - for (i = 1; i < num_ovl; i++)
> >> - dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
> >> + dispc_ovl_write_color_conv_coef(dispc, plane, csc);
> >>
> >> - if (dispc->feat->has_writeback)
> >> - dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
> >> + return 0;
> >> }
> >>
> >> static void dispc_ovl_set_ba0(struct dispc_device *dispc,
> >> @@ -2598,7 +2657,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
> >> u8 pre_mult_alpha, u8 global_alpha,
> >> enum omap_dss_rotation_type rotation_type,
> >> bool replication, const struct videomode *vm,
> >> - bool mem_to_mem)
> >> + bool mem_to_mem,
> >> + enum drm_color_encoding color_encoding,
> >> + enum drm_color_range color_range)
> >> {
> >> bool five_taps = true;
> >> bool fieldmode = false;
> >> @@ -2747,6 +2808,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
> >> fieldmode, fourcc, rotation);
> >> dispc_ovl_set_output_size(dispc, plane, out_width, out_height);
> >> dispc_ovl_set_vid_color_conv(dispc, plane, cconv);
> >> +
> >> + if (plane != OMAP_DSS_WB)
> >> + dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
> >> }
> >>
> >> dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,
> >> @@ -2783,7 +2847,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc,
> >> oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
> >> oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
> >> oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
> >> - oi->rotation_type, replication, vm, mem_to_mem);
> >> + oi->rotation_type, replication, vm, mem_to_mem,
> >> + oi->color_encoding, oi->color_range);
> >>
> >> return r;
> >> }
> >> @@ -2816,7 +2881,8 @@ static int dispc_wb_setup(struct dispc_device *dispc,
> >> wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
> >> wi->height, wi->fourcc, wi->rotation, zorder,
> >> wi->pre_mult_alpha, global_alpha, wi->rotation_type,
> >> - replication, vm, mem_to_mem);
> >> + replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
> >> + DRM_COLOR_YCBCR_LIMITED_RANGE);
> >> if (r)
> >> return r;
> >>
> >> @@ -3948,7 +4014,8 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc)
> >> dispc->feat->has_gamma_table)
> >> REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);
> >>
> >> - dispc_setup_color_conv_coef(dispc);
> >> + if (dispc->feat->has_writeback)
> >> + dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_full);
> >>
> >> dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> >> index 79f6b195c7cf..1430cab6b877 100644
> >> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> >> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> >> @@ -14,6 +14,7 @@
> >> #include <linux/platform_data/omapdss.h>
> >> #include <uapi/drm/drm_mode.h>
> >> #include <drm/drm_crtc.h>
> >> +#include <drm/drm_color_mgmt.h>
> >
> > Alphabetical order ? While at is you coulde remove uapi/
> >
> >>
> >> #define DISPC_IRQ_FRAMEDONE (1 << 0)
> >> #define DISPC_IRQ_VSYNC (1 << 1)
> >> @@ -243,6 +244,9 @@ struct omap_overlay_info {
> >> u8 global_alpha;
> >> u8 pre_mult_alpha;
> >> u8 zorder;
> >> +
> >> + enum drm_color_encoding color_encoding;
> >> + enum drm_color_range color_range;
> >> };
> >>
> >> struct omap_overlay_manager_info {
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> index 73ec99819a3d..db8e917260df 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> @@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
> >> info.pre_mult_alpha = 1;
> >> else
> >> info.pre_mult_alpha = 0;
> >> + info.color_encoding = state->color_encoding;
> >> + info.color_range = state->color_range;
> >>
> >> /* update scanout: */
> >> omap_framebuffer_update_scanout(state->fb, state, &info);
> >> @@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane)
> >> */
> >> plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
> >> ? 0 : omap_plane->id;
> >> + plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
> >> + plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;
> >> }
> >>
> >> static int omap_plane_atomic_set_property(struct drm_plane *plane,
> >> @@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = {
> >> .atomic_get_property = omap_plane_atomic_get_property,
> >> };
> >>
> >> +static bool omap_plane_supports_yuv(struct drm_plane *plane)
> >> +{
> >> + struct omap_drm_private *priv = plane->dev->dev_private;
> >> + struct omap_plane *omap_plane = to_omap_plane(plane);
> >> + const u32 *formats =
> >> + priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
> >> + int i;
> >
> > unsigned int i ?
> >
> >> +
> >> + for (i = 0; formats[i]; i++)
> >> + if (formats[i] == DRM_FORMAT_YUYV ||
> >> + formats[i] == DRM_FORMAT_UYVY ||
> >> + formats[i] == DRM_FORMAT_NV12)
> >> + return true;
> >> +
> >> + return false;
> >> +}
> >> +
> >> static const char *plane_id_to_name[] = {
> >> [OMAP_DSS_GFX] = "gfx",
> >> [OMAP_DSS_VIDEO1] = "vid1",
> >> @@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
> >> drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) |
> >> BIT(DRM_MODE_BLEND_COVERAGE));
> >>
> >> + if (omap_plane_supports_yuv(plane))
> >> + drm_plane_create_color_properties(plane,
> >> + BIT(DRM_COLOR_YCBCR_BT601) |
> >> + BIT(DRM_COLOR_YCBCR_BT709),
> >> + BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
> >> + BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> >> + DRM_COLOR_YCBCR_BT601,
> >> + DRM_COLOR_YCBCR_FULL_RANGE);
> >> +
> >> return plane;
> >>
> >> error:
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
