* [PATCH] Implement Limited Video Range @ 2015-11-30 7:08 Peter Frühberger 2015-11-30 8:43 ` Daniel Vetter 0 siblings, 1 reply; 8+ messages in thread From: Peter Frühberger @ 2015-11-30 7:08 UTC (permalink / raw) To: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 7176 bytes --] (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@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; + /* 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 [-- Attachment #1.2: Type: text/html, Size: 13471 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Implement Limited Video Range 2015-11-30 7:08 [PATCH] Implement Limited Video Range Peter Frühberger @ 2015-11-30 8:43 ` Daniel Vetter 2015-11-30 14:11 ` Ville Syrjälä 0 siblings, 1 reply; 8+ messages in thread From: Daniel Vetter @ 2015-11-30 8:43 UTC (permalink / raw) To: Peter Frühberger; +Cc: intel-gfx 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@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? */ 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@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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Implement Limited Video Range 2015-11-30 8:43 ` Daniel Vetter @ 2015-11-30 14:11 ` Ville Syrjälä 2016-11-09 20:11 ` Peter Frühberger 0 siblings, 1 reply; 8+ messages in thread From: Ville Syrjälä @ 2015-11-30 14:11 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx 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@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Implement Limited Video Range 2015-11-30 14:11 ` Ville Syrjälä @ 2016-11-09 20:11 ` Peter Frühberger 2016-11-09 20:25 ` Ville Syrjälä 0 siblings, 1 reply; 8+ messages in thread From: Peter Frühberger @ 2016-11-09 20:11 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 10780 bytes --] 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@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@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@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > [-- Attachment #1.2: Type: text/html, Size: 14225 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Implement Limited Video Range 2016-11-09 20:11 ` Peter Frühberger @ 2016-11-09 20:25 ` Ville Syrjälä 2016-11-11 8:53 ` Peter Frühberger 0 siblings, 1 reply; 8+ messages in thread From: Ville Syrjälä @ 2016-11-09 20:25 UTC (permalink / raw) To: Peter Frühberger; +Cc: intel-gfx 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@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@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@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@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Implement Limited Video Range 2016-11-09 20:25 ` Ville Syrjälä @ 2016-11-11 8:53 ` Peter Frühberger 2016-11-11 13:57 ` Ville Syrjälä 0 siblings, 1 reply; 8+ messages in thread From: Peter Frühberger @ 2016-11-11 8:53 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 13863 bytes --] 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. 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@linux.intel.com > 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@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@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@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@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > > -- > Ville Syrjälä > Intel OTC > [-- Attachment #1.2: Type: text/html, Size: 19547 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Implement Limited Video Range 2016-11-11 8:53 ` Peter Frühberger @ 2016-11-11 13:57 ` Ville Syrjälä 2017-09-16 11:57 ` Peter Frühberger 0 siblings, 1 reply; 8+ messages in thread From: Ville Syrjälä @ 2016-11-11 13:57 UTC (permalink / raw) To: Peter Frühberger; +Cc: intel-gfx 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@linux.intel.com > > 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@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@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@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@lists.freedesktop.org > > > > > 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Implement Limited Video Range 2016-11-11 13:57 ` Ville Syrjälä @ 2017-09-16 11:57 ` Peter Frühberger 0 siblings, 0 replies; 8+ messages in thread From: Peter Frühberger @ 2017-09-16 11:57 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 8422 bytes --] Thanks for your comments, I tried to implement a "flag driven version" below. On Fri, Nov 11, 2016 at 2:57 PM, Ville Syrjälä < ville.syrjala@linux.intel.com> wrote: > 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 opinion that makes maintainability much harder. Looking forward to your comments. >From f3bccf1611108247add0d85e8faed5430971cd71 Mon Sep 17 00:00:00 2001 From: fritsch <peter.fruehberger@gmail.com> Date: Sat, 16 Sep 2017 13:54:06 +0200 Subject: [PATCH] i915: Implement uncompressed Video Range output --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_color.c | 12 ++++++++---- drivers/gpu/drm/i915/intel_display.c | 6 ++++-- drivers/gpu/drm/i915/intel_dp.c | 3 ++- drivers/gpu/drm/i915/intel_drv.h | 11 +++++++++-- drivers/gpu/drm/i915/intel_hdmi.c | 16 +++++++++++++--- drivers/gpu/drm/i915/intel_modes.c | 1 + 7 files changed, 38 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5aecbf795b55..70bd525317c8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -4230,6 +4230,7 @@ __raw_write(64, q) #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 i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv) { diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index ff9ecd211abb..b72477a43ea4 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -149,7 +149,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) (struct drm_color_ctm *)crtc_state->ctm->data; uint64_t input[9] = { 0, }; - if (intel_crtc_state->limited_color_range) { + if (intel_crtc_state->limited_color_range && + intel_crtc_state->compress_range) { ctm_mult_by_limited(input, ctm->matrix); } else { for (i = 0; i < ARRAY_SIZE(input); i++) @@ -201,7 +202,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) * into consideration. */ for (i = 0; i < 3; i++) { - if (intel_crtc_state->limited_color_range) + if (intel_crtc_state->limited_color_range && + intel_crtc_state->compress_range) coeffs[i * 3 + i] = I9XX_CSC_COEFF_LIMITED_RANGE; else @@ -225,7 +227,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) if (INTEL_GEN(dev_priv) > 6) { uint16_t postoff = 0; - if (intel_crtc_state->limited_color_range) + if (intel_crtc_state->limited_color_range && + intel_crtc_state->compress_range) postoff = (16 * (1 << 12) / 255) & 0x1fff; I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff); @@ -236,7 +239,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) } else { uint32_t mode = CSC_MODE_YUV_TO_RGB; - if (intel_crtc_state->limited_color_range) + if (intel_crtc_state->limited_color_range && + intel_crtc_state->compress_range) mode |= CSC_BLACK_SCREEN_OFFSET; I915_WRITE(PIPE_CSC_MODE(pipe), mode); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8599e425abb1..4de43eb12f0e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7247,7 +7247,8 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc) pipeconf |= PIPECONF_PROGRESSIVE; if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && - intel_crtc->config->limited_color_range) + intel_crtc->config->limited_color_range && + intel_crtc->config->compress_range) pipeconf |= PIPECONF_COLOR_RANGE_SELECT; I915_WRITE(PIPECONF(intel_crtc->pipe), pipeconf); @@ -8177,7 +8178,8 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc) else val |= PIPECONF_PROGRESSIVE; - if (intel_crtc->config->limited_color_range) + if (intel_crtc->config->limited_color_range && + intel_crtc->config->compress_range) val |= PIPECONF_COLOR_RANGE_SELECT; I915_WRITE(PIPECONF(pipe), val); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 887953c0f495..437dd0465be3 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1926,7 +1926,8 @@ static void intel_dp_prepare(struct intel_encoder *encoder, trans_dp &= ~TRANS_DP_ENH_FRAMING; I915_WRITE(TRANS_DP_CTL(crtc->pipe), trans_dp); } else { - if (IS_G4X(dev_priv) && pipe_config->limited_color_range) + if (IS_G4X(dev_priv) && pipe_config->limited_color_range && + pipe_config->compress_range) intel_dp->DP |= DP_COLOR_RANGE_16_235; if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 307807672896..35602a1ed471 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -649,11 +649,18 @@ struct intel_crtc_state { enum transcoder cpu_transcoder; /* - * Use reduced/limited/broadcast rbg range, compressing from the full - * range fed into the crtcs. + * Use reduced/limited/broadcast rbg range. */ bool limited_color_range; + /* + * + * Compress the input range when doing limited/broadcast rgb range + * output. This is the default for Limited 16:235 Output mode. + * + */ + bool compress_range; + /* Bitmask of encoder types (enum intel_output_type) * driven by the pipe. */ diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index e6f8f30ce7bd..4963903f177e 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -880,7 +880,8 @@ static void intel_hdmi_prepare(struct intel_encoder *encoder, intel_dp_dual_mode_set_tmds_output(intel_hdmi, true); hdmi_val = SDVO_ENCODING_HDMI; - if (!HAS_PCH_SPLIT(dev_priv) && crtc_state->limited_color_range) + if (!HAS_PCH_SPLIT(dev_priv) && crtc_state->limited_color_range + && crtc_state->compress_range) hdmi_val |= HDMI_COLOR_RANGE_16_235; if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC) hdmi_val |= SDVO_VSYNC_ACTIVE_HIGH; @@ -1419,9 +1420,18 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, pipe_config->has_hdmi_sink && drm_default_rgb_quant_range(adjusted_mode) == HDMI_QUANTIZATION_RANGE_LIMITED; + pipe_config->compress_range = pipe_config->limited_color_range; + } else if (intel_conn_state->broadcast_rgb + == INTEL_BROADCAST_RGB_LIMITED) { + pipe_config->limited_color_range = true; + pipe_config->compress_range = true; + } else if (intel_conn_state->broadcast_rgb + == INTEL_BROADCAST_RGB_FULL) { + pipe_config->limited_color_range = false; + pipe_config->compress_range = false; } else { - pipe_config->limited_color_range = - intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED; + pipe_config->limited_color_range = true; + pipe_config->compress_range = false; } if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) { diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index 951e834dd274..d817558410eb 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -102,6 +102,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.11.0 [-- Attachment #1.2: Type: text/html, Size: 9417 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-16 11:57 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-30 7:08 [PATCH] Implement Limited Video Range Peter Frühberger 2015-11-30 8:43 ` Daniel Vetter 2015-11-30 14:11 ` Ville Syrjälä 2016-11-09 20:11 ` Peter Frühberger 2016-11-09 20:25 ` Ville Syrjälä 2016-11-11 8:53 ` Peter Frühberger 2016-11-11 13:57 ` Ville Syrjälä 2017-09-16 11:57 ` Peter Frühberger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.