On Fri, Nov 11, 2016 at 09:53:35AM +0100, Peter Frühberger wrote:
> Hi,
>
> I was implementing this suggestion today and I think that will confuse
> users and also devs maintaining that code. Out of the following reason:
> compress_color_range can be true or false, it does not reference a mode,
> but an additional setting that only influences color clamping / scaling.
>
> What do we expect if someone runs in Full Range and has
> compress_color_range set to true? Also what do we expect if someone runs in
> Limited Range and additionally set this flag to true? In some cases it
> would do nothing and was not transparent.
I didn't mean you should add a new property for this. Just an internal
flag.
>
> In my original patch I implemented a new mode, which was: Video Range
> 16:235. That meant: Tell Limited info frame, don't alter any colors. It was
> a combination of two things: limited info frame + compress_color_range =
> false;
>
> The driver code mainly uses: intel_hdmi->limited_color_range to distinguish
> if colors should be clamped or not. Therefore if the above value was set,
> that just needed setting to false.
>
> I think if we decide for a compress_color_range bool, it should not work as
> an entire new mode but as an option to alter current mode, meaning
> something not set via "BroadCast RGB".
>
> Any thoughts on that matter?
>
> On Wed, Nov 9, 2016 at 9:25 PM, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx
> > wrote:
>
> > On Wed, Nov 09, 2016 at 09:11:58PM +0100, Peter Frühberger wrote:
> > > 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.
> >
> > Re-reading what was written (can't remember anymore obviously), I think
> > what both me and Daniel were after was a cleaner separation of the
> > "what should the infoframe say?" and "how should we mangle the color
> > data?".
> >
> > Daniel's "limit_color_range" is a wee bit too similar looking to
> > "limited_color_range" for my liking. So how about we go with something
> > like "bool compress_color_range". It also conveys the fact that we're
> > not just clamping things. Any objections/thoughts/better ideas?
> >
> > >
> > > Best regards
> > > Peter
> > >
> > > On Mon, Nov 30, 2015 at 3:11 PM, Ville Syrjälä <
> > > ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > > >
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
