[Intel-gfx] [PATCH] Implement Limited Video Range
Peter Frühberger
fritsch at kodi.tv
Wed Nov 9 20:11:58 UTC 2016
I am currently not sure what the way forward should be.
We are successfully implementing this patch in e.g. LibreELEC an embedded
distribution for home theater pcs. But through current bug reports - I am
not sure, if we should give such functionality to the user, that is
overwritten on a lower level, e.g. it does not what the user expects.
E.g. since some time displays are driven with 10 or 12 bit from the intel
driver, image data is dithered, scaled back and forth without being
transparent for the user. In fact we have some users with hdmi capture
cards, coming back to us if some decoded values of their video stuff does
not match.
Best regards
Peter
On Mon, Nov 30, 2015 at 3:11 PM, Ville Syrjälä <
ville.syrjala at linux.intel.com> wrote:
> On Mon, Nov 30, 2015 at 09:43:32AM +0100, Daniel Vetter wrote:
> > On Mon, Nov 30, 2015 at 08:08:48AM +0100, Peter Frühberger wrote:
> > > (Resent cause of moderation)
> > >
> > > This implements a highly needed feature in a minimal non instructive
> way.
> > > Consider a Limited Range display (like most TVs) where you want to
> watch a
> > > decoded video. The TV is set to limited range and the intel driver also
> > > uses full scaling Limited 16:235 mode, e.g. if you display the gray
> value
> > > 16 - the driver will postprocess it to something like ~ 22.
> > >
> > > The following happens currently in linux intel video world:
> > > Video is decoded with e.g. vaapi, the decoded surface is either used
> via
> > > EGL directly in Limited Range. Limited Range processing takes place
> and at
> > > the end while rendering the intel driver will scale down the limited
> range
> > > data again as it cannot distunguish of the data it gets when e.g.
> rendering
> > > with OpenGL like we succesfully do in kodi.
> > >
> > > Another use case is vaapi when using the old vaCopySurfaceGLX
> > > (vaPutSurface) which would automatically use BT601 / BT709 matrix to
> > > upscale the content without any dithering to Full Range RGB. Later on
> this
> > > is scaled down again with the Limited 16:235 setting and output.
> Quality
> > > degrades two times.
> > >
> > > Users and applications widely used want to make sure that colors
> appear on
> > > screen like they were processed after the last processing step. In
> video
> > > case two use cases make sense:
> > >
> > > a) scaling limited range to full range with dithering applied when you
> need
> > > to output fullrange
> > > b) already having limited range and outputting that without any further
> > > touching by the driver
> > >
> > > Use case a) is already possible. Usecase b) will be possible with the
> > > attached patch. It is a possibility to get Limited Range on screen in
> > > perfect quality in 2k15.
> > >
> > > For the future a userspace api to provide info frames and so on in a
> > > generic way should be considered but until we have this I bet we have 2
> > > years to go. This solution also works on X11 (without wayland) and is
> > > useful for existing applications.
> > >
> > > Thanks much for consideration.
> > >
> > > ---
> > > From deaa9d730c08aefefe3671d073d93d970bb39a31 Mon Sep 17 00:00:00 2001
> > > From: fritsch <peter.fruehberger at gmail.com>
> > > Date: Sun, 29 Nov 2015 16:38:14 +0100
> > > Subject: [PATCH] Intel: Implement Video Color Range
> > >
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > drivers/gpu/drm/i915/intel_display.c | 4 ++--
> > > drivers/gpu/drm/i915/intel_drv.h | 8 ++++++++
> > > drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++++--
> > > drivers/gpu/drm/i915/intel_modes.c | 1 +
> > > 5 files changed, 27 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 95bb27d..e453461 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3449,6 +3449,7 @@ int intel_freq_opcode(struct drm_i915_private
> > > *dev_priv, int val);
> > > #define INTEL_BROADCAST_RGB_AUTO 0
> > > #define INTEL_BROADCAST_RGB_FULL 1
> > > #define INTEL_BROADCAST_RGB_LIMITED 2
> > > +#define INTEL_BROADCAST_RGB_VIDEO 3
> > >
> > > static inline uint32_t i915_vgacntrl_reg(struct drm_device *dev)
> > > {
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 71860f8..ea40d81 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8605,7 +8605,7 @@ static void intel_set_pipe_csc(struct drm_crtc
> *crtc)
> > > * consideration.
> > > */
> > >
> > > - if (intel_crtc->config->limited_color_range)
> > > + if (intel_crtc->config->limited_color_range &&
> > > !intel_crtc->config->video_color_range)
> > > coeff = ((235 - 16) * (1 << 12) / 255) & 0xff8; /* 0.xxx... */
> > >
> > > /*
> > > @@ -8629,7 +8629,7 @@ static void intel_set_pipe_csc(struct drm_crtc
> *crtc)
> > > if (INTEL_INFO(dev)->gen > 6) {
> > > uint16_t postoff = 0;
> > >
> > > - if (intel_crtc->config->limited_color_range)
> > > + if (intel_crtc->config->limited_color_range &&
> > > !intel_crtc->config->video_color_range)
> > > postoff = (16 * (1 << 12) / 255) & 0x1fff;
> > >
> > > I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0598932..6940243 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -372,6 +372,13 @@ struct intel_crtc_state {
> > > */
> > > bool limited_color_range;
> > >
> > > + /*
> > > + *
> > > + * Use reduced/limited/broadcast rgb range without compressing.
> > > + *
> > > + */
> > > + bool video_color_range;
> >
> > I think the names and semantics are confusion, resulting in hard-to-read
> > if conditions. What about:
> >
> > bool limit_color_range; /* should we limit from full->16:235 in
> > the encoder/csc? */
>
> We might call this simply as "adjust_color_range" or something like
> that. Then it would also work for the limited->full conversion if we
> ever come up with some use case that would need such a thing. Althoguh
> it does mean we'd have to check both flags when deciding how to program
> the port/pipeconf range bit/pipe csc.
>
> >
> > bool limited_color_range_output; /* set the infoframe or not? */
> >
> > Names aren't great yet I think, coffee's not yet working ;-)
> >
> > Thanks, Daniel
> >
> > > +
> > > /* DP has a bunch of special case unfortunately, so mark the pipe
> > > * accordingly. */
> > > bool has_dp_encoder;
> > > @@ -682,6 +689,7 @@ struct intel_hdmi {
> > > int ddc_bus;
> > > bool limited_color_range;
> > > bool color_range_auto;
> > > + bool color_range_video;
> > > bool has_hdmi_sink;
> > > bool has_audio;
> > > enum hdmi_force_audio force_audio;
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 9eafa19..dc78760 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -464,7 +464,8 @@ static void intel_hdmi_set_avi_infoframe(struct
> > > drm_encoder *encoder,
> > > }
> > >
> > > if (intel_hdmi->rgb_quant_range_selectable) {
> > > - if (intel_crtc->config->limited_color_range)
> > > + if (intel_crtc->config->limited_color_range ||
> > > + intel_crtc->config->video_color_range)
> > > frame.avi.quantization_range =
> > > HDMI_QUANTIZATION_RANGE_LIMITED;
> > > else
> > > @@ -1266,6 +1267,8 @@ bool intel_hdmi_compute_config(struct
> intel_encoder
> > > *encoder,
> > > pipe_config->limited_color_range =
> > > intel_hdmi->limited_color_range;
> > > }
> > > + if (intel_hdmi->color_range_video)
> > > + pipe_config->video_color_range = true;
> > >
> > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
> > > pipe_config->pixel_multiplier = 2;
> > > @@ -1485,25 +1488,35 @@ intel_hdmi_set_property(struct drm_connector
> > > *connector,
> > > if (property == dev_priv->broadcast_rgb_property) {
> > > bool old_auto = intel_hdmi->color_range_auto;
> > > bool old_range = intel_hdmi->limited_color_range;
> > > + bool old_range_video = intel_hdmi->color_range_video;
> > >
> > > switch (val) {
> > > case INTEL_BROADCAST_RGB_AUTO:
> > > intel_hdmi->color_range_auto = true;
> > > + intel_hdmi->color_range_video = false;
> > > break;
> > > case INTEL_BROADCAST_RGB_FULL:
> > > intel_hdmi->color_range_auto = false;
> > > intel_hdmi->limited_color_range = false;
> > > + intel_hdmi->color_range_video = false;
> > > break;
> > > case INTEL_BROADCAST_RGB_LIMITED:
> > > intel_hdmi->color_range_auto = false;
> > > intel_hdmi->limited_color_range = true;
> > > + intel_hdmi->color_range_video = false;
> > > + break;
> > > + case INTEL_BROADCAST_RGB_VIDEO:
> > > + intel_hdmi->color_range_auto = false;
> > > + intel_hdmi->limited_color_range = true;
> > > + intel_hdmi->color_range_video = true;
> > > break;
> > > default:
> > > return -EINVAL;
> > > }
> > >
> > > if (old_auto == intel_hdmi->color_range_auto &&
> > > - old_range == intel_hdmi->limited_color_range)
> > > + old_range == intel_hdmi->limited_color_range &&
> > > + old_range_video == intel_hdmi->color_range_video)
> > > return 0;
> > >
> > > goto done;
> > > diff --git a/drivers/gpu/drm/i915/intel_modes.c
> > > b/drivers/gpu/drm/i915/intel_modes.c
> > > index 38a4c8c..c49681a 100644
> > > --- a/drivers/gpu/drm/i915/intel_modes.c
> > > +++ b/drivers/gpu/drm/i915/intel_modes.c
> > > @@ -103,6 +103,7 @@ static const struct drm_prop_enum_list
> > > broadcast_rgb_names[] = {
> > > { INTEL_BROADCAST_RGB_AUTO, "Automatic" },
> > > { INTEL_BROADCAST_RGB_FULL, "Full" },
> > > { INTEL_BROADCAST_RGB_LIMITED, "Limited 16:235" },
> > > + { INTEL_BROADCAST_RGB_VIDEO, "Video 16:235 pass-through" },
> > > };
> > >
> > > void
> > > --
> > > 2.5.0
> >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20161109/3d0989f5/attachment-0001.html>
More information about the Intel-gfx
mailing list