[PATCH 0/5] Improving the situation regarding RGB quantization ranges

[PATCH 0/5] Improving the situation regarding RGB quantization ranges

[Yussuf Khalil]

The CTA-861 specification differentiates between IT and CE modes. For the
latter, it requires sources to send "limited RGB quantization range", i.e.,
allowed RGB values are constrained to 16 - 235 (in the 8 bit case). A sink may
indicate support for full range RGB (0 - 255) in CE modes through its EDID,
allowing the source to override the recommendations set by CTA-861.

For computer screens it is usually desirable to have full range RGB output. In
reality, though, many displays set a CE mode as preferred in their EDID and
leave the "Quantization Range Selectable" bit unset despite having no issues
with full range output. Therefore, it makes sense to provide the user with
a method of overriding the standards-compliant selection.

The current situation in DRM regarding the RGB quantization range is somewhat
messy. We have drivers that
 - behave standards-compliant and provide a custom override property (e.g.,
   i915, gma500)
 - behave standards-compliant and provide no override (e.g., vc4)
 - behave standards-compliant and select full range for CE modes if the screen
   indicates support (e.g., tda998x)
 - probably don't care at all

This series is an effort to fix the situation. It introduces a property for
overriding the RGB quantization range that is defined in the DRM core and can
be attached to connectors by all drivers, providing a driver-independent way of
overriding the defaults to userspace. So far, I've wired up the new property in
i915 only.

Yussuf Khalil (5):
  drm/modes: Indicate CEA-861 CE modes to user-space
  drm: Add "RGB quantization range" connector property
  drm: Add drm_connector_state_select_rgb_quantization_range() helper
  drm/atomic-helper: Consider RGB quantization changes to be mode
    changes
  drm/i915: Replace "Broadcast RGB" with "RGB quantization range"
    property

 drivers/gpu/drm/drm_atomic_helper.c           |  6 ++
 drivers/gpu/drm/drm_atomic_uapi.c             |  4 ++
 drivers/gpu/drm/drm_connector.c               | 66 +++++++++++++++++++
 drivers/gpu/drm/drm_modes.c                   | 14 ++++
 drivers/gpu/drm/i915/display/intel_atomic.c   |  8 ---
 .../gpu/drm/i915/display/intel_connector.c    | 39 ++++-------
 .../gpu/drm/i915/display/intel_connector.h    |  2 +-
 .../drm/i915/display/intel_display_types.h    |  8 ---
 drivers/gpu/drm/i915/display/intel_dp.c       | 24 ++-----
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  2 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     | 19 ++----
 drivers/gpu/drm/i915/display/intel_sdvo.c     | 18 ++---
 drivers/gpu/drm/i915/i915_drv.h               |  1 -
 include/drm/drm_connector.h                   | 27 ++++++++
 include/drm/drm_mode_config.h                 |  7 ++
 include/uapi/drm/drm_mode.h                   |  2 +
 16 files changed, 160 insertions(+), 87 deletions(-)

-- 
2.26.0

[PATCH 0/5] Improving the situation regarding RGB quantization ranges

[Yussuf Khalil]

The CTA-861 specification differentiates between IT and CE modes. For the
latter, it requires sources to send "limited RGB quantization range", i.e.,
allowed RGB values are constrained to 16 - 235 (in the 8 bit case). A sink may
indicate support for full range RGB (0 - 255) in CE modes through its EDID,
allowing the source to override the recommendations set by CTA-861.

For computer screens it is usually desirable to have full range RGB output. In
reality, though, many displays set a CE mode as preferred in their EDID and
leave the "Quantization Range Selectable" bit unset despite having no issues
with full range output. Therefore, it makes sense to provide the user with
a method of overriding the standards-compliant selection.

The current situation in DRM regarding the RGB quantization range is somewhat
messy. We have drivers that
 - behave standards-compliant and provide a custom override property (e.g.,
   i915, gma500)
 - behave standards-compliant and provide no override (e.g., vc4)
 - behave standards-compliant and select full range for CE modes if the screen
   indicates support (e.g., tda998x)
 - probably don't care at all

This series is an effort to fix the situation. It introduces a property for
overriding the RGB quantization range that is defined in the DRM core and can
be attached to connectors by all drivers, providing a driver-independent way of
overriding the defaults to userspace. So far, I've wired up the new property in
i915 only.

Yussuf Khalil (5):
  drm/modes: Indicate CEA-861 CE modes to user-space
  drm: Add "RGB quantization range" connector property
  drm: Add drm_connector_state_select_rgb_quantization_range() helper
  drm/atomic-helper: Consider RGB quantization changes to be mode
    changes
  drm/i915: Replace "Broadcast RGB" with "RGB quantization range"
    property

 drivers/gpu/drm/drm_atomic_helper.c           |  6 ++
 drivers/gpu/drm/drm_atomic_uapi.c             |  4 ++
 drivers/gpu/drm/drm_connector.c               | 66 +++++++++++++++++++
 drivers/gpu/drm/drm_modes.c                   | 14 ++++
 drivers/gpu/drm/i915/display/intel_atomic.c   |  8 ---
 .../gpu/drm/i915/display/intel_connector.c    | 39 ++++-------
 .../gpu/drm/i915/display/intel_connector.h    |  2 +-
 .../drm/i915/display/intel_display_types.h    |  8 ---
 drivers/gpu/drm/i915/display/intel_dp.c       | 24 ++-----
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  2 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     | 19 ++----
 drivers/gpu/drm/i915/display/intel_sdvo.c     | 18 ++---
 drivers/gpu/drm/i915/i915_drv.h               |  1 -
 include/drm/drm_connector.h                   | 27 ++++++++
 include/drm/drm_mode_config.h                 |  7 ++
 include/uapi/drm/drm_mode.h                   |  2 +
 16 files changed, 160 insertions(+), 87 deletions(-)

-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm/modes: Indicate CEA-861 CE modes to user-space

[Yussuf Khalil]

Add a new flag to mark modes that are considered a CE mode according to the
CEA-861 specification. Modes without this flag are implicitly considered to
be IT modes.

User-space applications may use this flag to determine possible
implications of using a CE mode (e.g., limited RGB range).

There is no use for this flag inside the kernel, so we set it only when
communicating a mode to user-space.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/drm_modes.c | 14 ++++++++++++++
 include/uapi/drm/drm_mode.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index d4d64518e11b..0d8a032f437d 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1973,6 +1973,14 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 		break;
 	}
 
+	if (drm_match_cea_mode(in) > 1) {
+		/*
+		 * All modes in CTA-861-G Table 1 are CE modes, except 640x480p
+		 * (VIC 1).
+		 */
+		out->flags |= DRM_MODE_FLAG_CEA_861_CE_MODE;
+	}
+
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 }
@@ -2045,6 +2053,12 @@ int drm_mode_convert_umode(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	/*
+	 * The CEA-861 CE mode flag is purely informational and intended for
+	 * userspace only.
+	 */
+	out->flags &= ~DRM_MODE_FLAG_CEA_861_CE_MODE;
+
 	out->status = drm_mode_validate_driver(dev, out);
 	if (out->status != MODE_OK)
 		return -EINVAL;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 735c8cfdaaa1..5e78b350b2e2 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -124,6 +124,8 @@ extern "C" {
 #define  DRM_MODE_FLAG_PIC_AR_256_135 \
 			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
 
+#define DRM_MODE_FLAG_CEA_861_CE_MODE (1<<23)
+
 #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
 				 DRM_MODE_FLAG_NHSYNC |		\
 				 DRM_MODE_FLAG_PVSYNC |		\
-- 
2.26.0

[PATCH 1/5] drm/modes: Indicate CEA-861 CE modes to user-space

[Yussuf Khalil]

Add a new flag to mark modes that are considered a CE mode according to the
CEA-861 specification. Modes without this flag are implicitly considered to
be IT modes.

User-space applications may use this flag to determine possible
implications of using a CE mode (e.g., limited RGB range).

There is no use for this flag inside the kernel, so we set it only when
communicating a mode to user-space.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/drm_modes.c | 14 ++++++++++++++
 include/uapi/drm/drm_mode.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index d4d64518e11b..0d8a032f437d 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1973,6 +1973,14 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 		break;
 	}
 
+	if (drm_match_cea_mode(in) > 1) {
+		/*
+		 * All modes in CTA-861-G Table 1 are CE modes, except 640x480p
+		 * (VIC 1).
+		 */
+		out->flags |= DRM_MODE_FLAG_CEA_861_CE_MODE;
+	}
+
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 }
@@ -2045,6 +2053,12 @@ int drm_mode_convert_umode(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	/*
+	 * The CEA-861 CE mode flag is purely informational and intended for
+	 * userspace only.
+	 */
+	out->flags &= ~DRM_MODE_FLAG_CEA_861_CE_MODE;
+
 	out->status = drm_mode_validate_driver(dev, out);
 	if (out->status != MODE_OK)
 		return -EINVAL;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 735c8cfdaaa1..5e78b350b2e2 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -124,6 +124,8 @@ extern "C" {
 #define  DRM_MODE_FLAG_PIC_AR_256_135 \
 			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
 
+#define DRM_MODE_FLAG_CEA_861_CE_MODE (1<<23)
+
 #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
 				 DRM_MODE_FLAG_NHSYNC |		\
 				 DRM_MODE_FLAG_PVSYNC |		\
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/modes: Indicate CEA-861 CE modes to user-space

[Daniel Vetter]

On Mon, Apr 13, 2020 at 11:40:22PM +0200, Yussuf Khalil wrote:
> Add a new flag to mark modes that are considered a CE mode according to the
> CEA-861 specification. Modes without this flag are implicitly considered to
> be IT modes.
> 
> User-space applications may use this flag to determine possible
> implications of using a CE mode (e.g., limited RGB range).
> 
> There is no use for this flag inside the kernel, so we set it only when
> communicating a mode to user-space.
> 
> Signed-off-by: Yussuf Khalil <dev@pp3345.net>

Do we have userspace for this?

If we go with the existing quant range property you don't need new
userspace for the property itself. But this flag here is new uapi, so
needs userspace per

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

Also since this standardizes kms uapi, we need testcases per

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-requirements-for-userspace-api

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_modes.c | 14 ++++++++++++++
>  include/uapi/drm/drm_mode.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index d4d64518e11b..0d8a032f437d 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1973,6 +1973,14 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  		break;
>  	}
>  
> +	if (drm_match_cea_mode(in) > 1) {
> +		/*
> +		 * All modes in CTA-861-G Table 1 are CE modes, except 640x480p
> +		 * (VIC 1).
> +		 */
> +		out->flags |= DRM_MODE_FLAG_CEA_861_CE_MODE;
> +	}
> +
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  }
> @@ -2045,6 +2053,12 @@ int drm_mode_convert_umode(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * The CEA-861 CE mode flag is purely informational and intended for
> +	 * userspace only.
> +	 */
> +	out->flags &= ~DRM_MODE_FLAG_CEA_861_CE_MODE;
> +
>  	out->status = drm_mode_validate_driver(dev, out);
>  	if (out->status != MODE_OK)
>  		return -EINVAL;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 735c8cfdaaa1..5e78b350b2e2 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -124,6 +124,8 @@ extern "C" {
>  #define  DRM_MODE_FLAG_PIC_AR_256_135 \
>  			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
>  
> +#define DRM_MODE_FLAG_CEA_861_CE_MODE (1<<23)
> +
>  #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
>  				 DRM_MODE_FLAG_NHSYNC |		\
>  				 DRM_MODE_FLAG_PVSYNC |		\
> -- 
> 2.26.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/5] drm/modes: Indicate CEA-861 CE modes to user-space

[Daniel Vetter]

On Mon, Apr 13, 2020 at 11:40:22PM +0200, Yussuf Khalil wrote:
> Add a new flag to mark modes that are considered a CE mode according to the
> CEA-861 specification. Modes without this flag are implicitly considered to
> be IT modes.
> 
> User-space applications may use this flag to determine possible
> implications of using a CE mode (e.g., limited RGB range).
> 
> There is no use for this flag inside the kernel, so we set it only when
> communicating a mode to user-space.
> 
> Signed-off-by: Yussuf Khalil <dev@pp3345.net>

Do we have userspace for this?

If we go with the existing quant range property you don't need new
userspace for the property itself. But this flag here is new uapi, so
needs userspace per

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

Also since this standardizes kms uapi, we need testcases per

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-requirements-for-userspace-api

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_modes.c | 14 ++++++++++++++
>  include/uapi/drm/drm_mode.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index d4d64518e11b..0d8a032f437d 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1973,6 +1973,14 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  		break;
>  	}
>  
> +	if (drm_match_cea_mode(in) > 1) {
> +		/*
> +		 * All modes in CTA-861-G Table 1 are CE modes, except 640x480p
> +		 * (VIC 1).
> +		 */
> +		out->flags |= DRM_MODE_FLAG_CEA_861_CE_MODE;
> +	}
> +
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  }
> @@ -2045,6 +2053,12 @@ int drm_mode_convert_umode(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * The CEA-861 CE mode flag is purely informational and intended for
> +	 * userspace only.
> +	 */
> +	out->flags &= ~DRM_MODE_FLAG_CEA_861_CE_MODE;
> +
>  	out->status = drm_mode_validate_driver(dev, out);
>  	if (out->status != MODE_OK)
>  		return -EINVAL;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 735c8cfdaaa1..5e78b350b2e2 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -124,6 +124,8 @@ extern "C" {
>  #define  DRM_MODE_FLAG_PIC_AR_256_135 \
>  			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
>  
> +#define DRM_MODE_FLAG_CEA_861_CE_MODE (1<<23)
> +
>  #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
>  				 DRM_MODE_FLAG_NHSYNC |		\
>  				 DRM_MODE_FLAG_PVSYNC |		\
> -- 
> 2.26.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/modes: Indicate CEA-861 CE modes to user-space

[Yussuf Khalil]

On Tue, 2020-04-14 at 14:41 +0200, Daniel Vetter wrote:
> On Mon, Apr 13, 2020 at 11:40:22PM +0200, Yussuf Khalil wrote:
> > Add a new flag to mark modes that are considered a CE mode
> > according to the
> > CEA-861 specification. Modes without this flag are implicitly
> > considered to
> > be IT modes.
> > 
> > User-space applications may use this flag to determine possible
> > implications of using a CE mode (e.g., limited RGB range).
> > 
> > There is no use for this flag inside the kernel, so we set it only
> > when
> > communicating a mode to user-space.
> > 
> > Signed-off-by: Yussuf Khalil <dev@pp3345.net>
> 
> Do we have userspace for this?
> 
> If we go with the existing quant range property you don't need new
> userspace for the property itself. But this flag here is new uapi, so
> needs userspace per
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> 
> Also since this standardizes kms uapi, we need testcases per
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-requirements-for-userspace-api
> 
> Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_modes.c | 14 ++++++++++++++
> >  include/uapi/drm/drm_mode.h |  2 ++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c
> > b/drivers/gpu/drm/drm_modes.c
> > index d4d64518e11b..0d8a032f437d 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1973,6 +1973,14 @@ void drm_mode_convert_to_umode(struct
> > drm_mode_modeinfo *out,
> >  		break;
> >  	}
> >  
> > +	if (drm_match_cea_mode(in) > 1) {
> > +		/*
> > +		 * All modes in CTA-861-G Table 1 are CE modes, except
> > 640x480p
> > +		 * (VIC 1).
> > +		 */
> > +		out->flags |= DRM_MODE_FLAG_CEA_861_CE_MODE;
> > +	}
> > +
> >  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >  }
> > @@ -2045,6 +2053,12 @@ int drm_mode_convert_umode(struct drm_device
> > *dev,
> >  		return -EINVAL;
> >  	}
> >  
> > +	/*
> > +	 * The CEA-861 CE mode flag is purely informational and
> > intended for
> > +	 * userspace only.
> > +	 */
> > +	out->flags &= ~DRM_MODE_FLAG_CEA_861_CE_MODE;
> > +
> >  	out->status = drm_mode_validate_driver(dev, out);
> >  	if (out->status != MODE_OK)
> >  		return -EINVAL;
> > diff --git a/include/uapi/drm/drm_mode.h
> > b/include/uapi/drm/drm_mode.h
> > index 735c8cfdaaa1..5e78b350b2e2 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -124,6 +124,8 @@ extern "C" {
> >  #define  DRM_MODE_FLAG_PIC_AR_256_135 \
> >  			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> >  
> > +#define DRM_MODE_FLAG_CEA_861_CE_MODE (1<<23)
> > +
> >  #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
> >  				 DRM_MODE_FLAG_NHSYNC |		\
> >  				 DRM_MODE_FLAG_PVSYNC |		\
> > -- 
> > 2.26.0
> > 

Sorry, I wasn't aware DRM had these additional requirements. I do have a user-
space implementation in mutter and gnome-control-center that makes use of the
new property and this flag on my local machine. I'll try to propose the branch
upstream before sending in the next revision of this patchset.

Do I understand it correctly that this will require test cases for both the
property itself and the new flag? I'll write a patch for IGT then.

Regards
Yussuf

Re: [PATCH 1/5] drm/modes: Indicate CEA-861 CE modes to user-space

[Yussuf Khalil]

On Tue, 2020-04-14 at 14:41 +0200, Daniel Vetter wrote:
> On Mon, Apr 13, 2020 at 11:40:22PM +0200, Yussuf Khalil wrote:
> > Add a new flag to mark modes that are considered a CE mode
> > according to the
> > CEA-861 specification. Modes without this flag are implicitly
> > considered to
> > be IT modes.
> > 
> > User-space applications may use this flag to determine possible
> > implications of using a CE mode (e.g., limited RGB range).
> > 
> > There is no use for this flag inside the kernel, so we set it only
> > when
> > communicating a mode to user-space.
> > 
> > Signed-off-by: Yussuf Khalil <dev@pp3345.net>
> 
> Do we have userspace for this?
> 
> If we go with the existing quant range property you don't need new
> userspace for the property itself. But this flag here is new uapi, so
> needs userspace per
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> 
> Also since this standardizes kms uapi, we need testcases per
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-requirements-for-userspace-api
> 
> Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_modes.c | 14 ++++++++++++++
> >  include/uapi/drm/drm_mode.h |  2 ++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c
> > b/drivers/gpu/drm/drm_modes.c
> > index d4d64518e11b..0d8a032f437d 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1973,6 +1973,14 @@ void drm_mode_convert_to_umode(struct
> > drm_mode_modeinfo *out,
> >  		break;
> >  	}
> >  
> > +	if (drm_match_cea_mode(in) > 1) {
> > +		/*
> > +		 * All modes in CTA-861-G Table 1 are CE modes, except
> > 640x480p
> > +		 * (VIC 1).
> > +		 */
> > +		out->flags |= DRM_MODE_FLAG_CEA_861_CE_MODE;
> > +	}
> > +
> >  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >  }
> > @@ -2045,6 +2053,12 @@ int drm_mode_convert_umode(struct drm_device
> > *dev,
> >  		return -EINVAL;
> >  	}
> >  
> > +	/*
> > +	 * The CEA-861 CE mode flag is purely informational and
> > intended for
> > +	 * userspace only.
> > +	 */
> > +	out->flags &= ~DRM_MODE_FLAG_CEA_861_CE_MODE;
> > +
> >  	out->status = drm_mode_validate_driver(dev, out);
> >  	if (out->status != MODE_OK)
> >  		return -EINVAL;
> > diff --git a/include/uapi/drm/drm_mode.h
> > b/include/uapi/drm/drm_mode.h
> > index 735c8cfdaaa1..5e78b350b2e2 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -124,6 +124,8 @@ extern "C" {
> >  #define  DRM_MODE_FLAG_PIC_AR_256_135 \
> >  			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> >  
> > +#define DRM_MODE_FLAG_CEA_861_CE_MODE (1<<23)
> > +
> >  #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
> >  				 DRM_MODE_FLAG_NHSYNC |		\
> >  				 DRM_MODE_FLAG_PVSYNC |		\
> > -- 
> > 2.26.0
> > 

Sorry, I wasn't aware DRM had these additional requirements. I do have a user-
space implementation in mutter and gnome-control-center that makes use of the
new property and this flag on my local machine. I'll try to propose the branch
upstream before sending in the next revision of this patchset.

Do I understand it correctly that this will require test cases for both the
property itself and the new flag? I'll write a patch for IGT then.

Regards
Yussuf

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/modes: Indicate CEA-861 CE modes to user-space

[Daniel Vetter]

On Thu, Apr 16, 2020 at 03:51:36PM +0200, Yussuf Khalil wrote:
> On Tue, 2020-04-14 at 14:41 +0200, Daniel Vetter wrote:
> > On Mon, Apr 13, 2020 at 11:40:22PM +0200, Yussuf Khalil wrote:
> > > Add a new flag to mark modes that are considered a CE mode
> > > according to the
> > > CEA-861 specification. Modes without this flag are implicitly
> > > considered to
> > > be IT modes.
> > > 
> > > User-space applications may use this flag to determine possible
> > > implications of using a CE mode (e.g., limited RGB range).
> > > 
> > > There is no use for this flag inside the kernel, so we set it only
> > > when
> > > communicating a mode to user-space.
> > > 
> > > Signed-off-by: Yussuf Khalil <dev@pp3345.net>
> > 
> > Do we have userspace for this?
> > 
> > If we go with the existing quant range property you don't need new
> > userspace for the property itself. But this flag here is new uapi, so
> > needs userspace per
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> > 
> > Also since this standardizes kms uapi, we need testcases per
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-requirements-for-userspace-api
> > 
> > Cheers, Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_modes.c | 14 ++++++++++++++
> > >  include/uapi/drm/drm_mode.h |  2 ++
> > >  2 files changed, 16 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_modes.c
> > > b/drivers/gpu/drm/drm_modes.c
> > > index d4d64518e11b..0d8a032f437d 100644
> > > --- a/drivers/gpu/drm/drm_modes.c
> > > +++ b/drivers/gpu/drm/drm_modes.c
> > > @@ -1973,6 +1973,14 @@ void drm_mode_convert_to_umode(struct
> > > drm_mode_modeinfo *out,
> > >  		break;
> > >  	}
> > >  
> > > +	if (drm_match_cea_mode(in) > 1) {
> > > +		/*
> > > +		 * All modes in CTA-861-G Table 1 are CE modes, except
> > > 640x480p
> > > +		 * (VIC 1).
> > > +		 */
> > > +		out->flags |= DRM_MODE_FLAG_CEA_861_CE_MODE;
> > > +	}
> > > +
> > >  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> > >  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> > >  }
> > > @@ -2045,6 +2053,12 @@ int drm_mode_convert_umode(struct drm_device
> > > *dev,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	/*
> > > +	 * The CEA-861 CE mode flag is purely informational and
> > > intended for
> > > +	 * userspace only.
> > > +	 */
> > > +	out->flags &= ~DRM_MODE_FLAG_CEA_861_CE_MODE;
> > > +
> > >  	out->status = drm_mode_validate_driver(dev, out);
> > >  	if (out->status != MODE_OK)
> > >  		return -EINVAL;
> > > diff --git a/include/uapi/drm/drm_mode.h
> > > b/include/uapi/drm/drm_mode.h
> > > index 735c8cfdaaa1..5e78b350b2e2 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -124,6 +124,8 @@ extern "C" {
> > >  #define  DRM_MODE_FLAG_PIC_AR_256_135 \
> > >  			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> > >  
> > > +#define DRM_MODE_FLAG_CEA_861_CE_MODE (1<<23)
> > > +
> > >  #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
> > >  				 DRM_MODE_FLAG_NHSYNC |		\
> > >  				 DRM_MODE_FLAG_PVSYNC |		\
> > > -- 
> > > 2.26.0
> > > 
> 
> Sorry, I wasn't aware DRM had these additional requirements. I do have a user-
> space implementation in mutter and gnome-control-center that makes use of the
> new property and this flag on my local machine. I'll try to propose the branch
> upstream before sending in the next revision of this patchset.
> 
> Do I understand it correctly that this will require test cases for both the
> property itself and the new flag? I'll write a patch for IGT then.

Yup. We even have some edid injection stuff so you can (for some value of
"can") test this on systems without such a monitor. That would obviously
be the gold standard for this, so that CI systems can make sure we don't
break any of this in the driver side.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/5] drm/modes: Indicate CEA-861 CE modes to user-space

[Daniel Vetter]

On Thu, Apr 16, 2020 at 03:51:36PM +0200, Yussuf Khalil wrote:
> On Tue, 2020-04-14 at 14:41 +0200, Daniel Vetter wrote:
> > On Mon, Apr 13, 2020 at 11:40:22PM +0200, Yussuf Khalil wrote:
> > > Add a new flag to mark modes that are considered a CE mode
> > > according to the
> > > CEA-861 specification. Modes without this flag are implicitly
> > > considered to
> > > be IT modes.
> > > 
> > > User-space applications may use this flag to determine possible
> > > implications of using a CE mode (e.g., limited RGB range).
> > > 
> > > There is no use for this flag inside the kernel, so we set it only
> > > when
> > > communicating a mode to user-space.
> > > 
> > > Signed-off-by: Yussuf Khalil <dev@pp3345.net>
> > 
> > Do we have userspace for this?
> > 
> > If we go with the existing quant range property you don't need new
> > userspace for the property itself. But this flag here is new uapi, so
> > needs userspace per
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> > 
> > Also since this standardizes kms uapi, we need testcases per
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#testing-requirements-for-userspace-api
> > 
> > Cheers, Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_modes.c | 14 ++++++++++++++
> > >  include/uapi/drm/drm_mode.h |  2 ++
> > >  2 files changed, 16 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_modes.c
> > > b/drivers/gpu/drm/drm_modes.c
> > > index d4d64518e11b..0d8a032f437d 100644
> > > --- a/drivers/gpu/drm/drm_modes.c
> > > +++ b/drivers/gpu/drm/drm_modes.c
> > > @@ -1973,6 +1973,14 @@ void drm_mode_convert_to_umode(struct
> > > drm_mode_modeinfo *out,
> > >  		break;
> > >  	}
> > >  
> > > +	if (drm_match_cea_mode(in) > 1) {
> > > +		/*
> > > +		 * All modes in CTA-861-G Table 1 are CE modes, except
> > > 640x480p
> > > +		 * (VIC 1).
> > > +		 */
> > > +		out->flags |= DRM_MODE_FLAG_CEA_861_CE_MODE;
> > > +	}
> > > +
> > >  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> > >  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> > >  }
> > > @@ -2045,6 +2053,12 @@ int drm_mode_convert_umode(struct drm_device
> > > *dev,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	/*
> > > +	 * The CEA-861 CE mode flag is purely informational and
> > > intended for
> > > +	 * userspace only.
> > > +	 */
> > > +	out->flags &= ~DRM_MODE_FLAG_CEA_861_CE_MODE;
> > > +
> > >  	out->status = drm_mode_validate_driver(dev, out);
> > >  	if (out->status != MODE_OK)
> > >  		return -EINVAL;
> > > diff --git a/include/uapi/drm/drm_mode.h
> > > b/include/uapi/drm/drm_mode.h
> > > index 735c8cfdaaa1..5e78b350b2e2 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -124,6 +124,8 @@ extern "C" {
> > >  #define  DRM_MODE_FLAG_PIC_AR_256_135 \
> > >  			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> > >  
> > > +#define DRM_MODE_FLAG_CEA_861_CE_MODE (1<<23)
> > > +
> > >  #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
> > >  				 DRM_MODE_FLAG_NHSYNC |		\
> > >  				 DRM_MODE_FLAG_PVSYNC |		\
> > > -- 
> > > 2.26.0
> > > 
> 
> Sorry, I wasn't aware DRM had these additional requirements. I do have a user-
> space implementation in mutter and gnome-control-center that makes use of the
> new property and this flag on my local machine. I'll try to propose the branch
> upstream before sending in the next revision of this patchset.
> 
> Do I understand it correctly that this will require test cases for both the
> property itself and the new flag? I'll write a patch for IGT then.

Yup. We even have some edid injection stuff so you can (for some value of
"can") test this on systems without such a monitor. That would obviously
be the gold standard for this, so that CI systems can make sure we don't
break any of this in the driver side.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm: Add "RGB quantization range" connector property

[Yussuf Khalil]

Add a new "RGB quantization range" property with three possible values:
Automatic, Limited, and Full. User-space may use this property to override
the automatic selection of the RGB range as specified by CTA-861. Drivers
should attach this property to all CTA-861 sinks.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
 drivers/gpu/drm/drm_connector.c   | 35 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 23 ++++++++++++++++++++
 include/drm/drm_mode_config.h     |  7 +++++++
 4 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a1e5e262bae2..f12b3a40c6cf 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -766,6 +766,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 						   fence_ptr);
 	} else if (property == connector->max_bpc_property) {
 		state->max_requested_bpc = val;
+	} else if (property == config->rgb_quantization_range_property) {
+		state->rgb_quantization_range = val;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -842,6 +844,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = 0;
 	} else if (property == connector->max_bpc_property) {
 		*val = state->max_requested_bpc;
+	} else if (property == config->rgb_quantization_range_property) {
+		*val = state->rgb_quantization_range;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 644f0ad10671..e60d3b6e5e56 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -929,6 +929,12 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
 	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
 };
 
+static const struct drm_prop_enum_list rgb_quantization_range_options[] = {
+	{ DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC, "Automatic" },
+	{ DRM_MODE_RGB_QUANTIZATION_RANGE_LIMITED, "Limited" },
+	{ DRM_MODE_RGB_QUANTIZATION_RANGE_FULL, "Full" },
+};
+
 /**
  * DOC: standard connector properties
  *
@@ -1804,6 +1810,35 @@ int drm_mode_create_dp_colorspace_property(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_mode_create_dp_colorspace_property);
 
+/**
+ * drm_mode_create_rgb_quantization_range_property - create RGB quantization
+ * range property
+ * @dev: device to create the RGB quantization range property on.
+ *
+ * Called by a driver the first time it's needed, must be attached to connectors
+ * of CEA-861-compliant sinks.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_create_rgb_quantization_range_property(struct drm_device *dev)
+{
+	if (dev->mode_config.rgb_quantization_range_property)
+		return 0;
+
+	dev->mode_config.rgb_quantization_range_property =
+		drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+					 "RGB quantization range",
+					 rgb_quantization_range_options,
+					 ARRAY_SIZE(rgb_quantization_range_options));
+
+	if (!dev->mode_config.rgb_quantization_range_property)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_rgb_quantization_range_property);
+
 /**
  * drm_mode_create_content_type_property - create content type property
  * @dev: DRM device
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 19ae6bb5c85b..f605e0fbcc0e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -303,6 +303,22 @@ struct drm_monitor_range_info {
 #define DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT		14
 #define DRM_MODE_COLORIMETRY_BT601_YCC			15
 
+/**
+ * enum drm_rgb_quantization_range - rgb_quantization_range property value
+ *
+ * This enum lists the possible options for the rgb_quantization_range property.
+ *
+ * @DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC:	Let the driver select the
+ * 						appropriate quantization range.
+ * @DRM_MODE_RGB_QUANTIZATION_RANGE_LIMITED:	Force limited range RGB.
+ * @DRM_MODE_RGB_QUANTIZATION_RANGE_FULL:	Force full range RGB.
+ */
+enum drm_rgb_quantization_range {
+	DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC = 0,
+	DRM_MODE_RGB_QUANTIZATION_RANGE_LIMITED = 1,
+	DRM_MODE_RGB_QUANTIZATION_RANGE_FULL = 2,
+};
+
 /**
  * enum drm_bus_flags - bus_flags info for &drm_display_info
  *
@@ -661,6 +677,12 @@ struct drm_connector_state {
 	 */
 	u32 colorspace;
 
+	/**
+	 * @rgb_quantization_range: Connector property to control the selected
+	 * RGB quantization range for CEA-861 outputs.
+	 */
+	u32 rgb_quantization_range;
+
 	/**
 	 * @writeback_job: Writeback job for writeback connectors
 	 *
@@ -1575,6 +1597,7 @@ int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
 int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector);
 int drm_mode_create_dp_colorspace_property(struct drm_connector *connector);
 int drm_mode_create_content_type_property(struct drm_device *dev);
+int drm_mode_create_rgb_quantization_range_property(struct drm_device *dev);
 void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
 					 const struct drm_connector_state *conn_state);
 
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 3bcbe30339f0..745587f6b83c 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -855,6 +855,13 @@ struct drm_mode_config {
 	 */
 	struct drm_property *hdcp_content_type_property;
 
+	/**
+	 * @rgb_quantization_range_property: Connector property to allow
+	 * user-space to override the RGB quantization range for CEA-861
+	 * outputs.
+	 */
+	struct drm_property *rgb_quantization_range_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
-- 
2.26.0

[PATCH 2/5] drm: Add "RGB quantization range" connector property

[Yussuf Khalil]

Add a new "RGB quantization range" property with three possible values:
Automatic, Limited, and Full. User-space may use this property to override
the automatic selection of the RGB range as specified by CTA-861. Drivers
should attach this property to all CTA-861 sinks.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
 drivers/gpu/drm/drm_connector.c   | 35 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       | 23 ++++++++++++++++++++
 include/drm/drm_mode_config.h     |  7 +++++++
 4 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a1e5e262bae2..f12b3a40c6cf 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -766,6 +766,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 						   fence_ptr);
 	} else if (property == connector->max_bpc_property) {
 		state->max_requested_bpc = val;
+	} else if (property == config->rgb_quantization_range_property) {
+		state->rgb_quantization_range = val;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -842,6 +844,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = 0;
 	} else if (property == connector->max_bpc_property) {
 		*val = state->max_requested_bpc;
+	} else if (property == config->rgb_quantization_range_property) {
+		*val = state->rgb_quantization_range;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 644f0ad10671..e60d3b6e5e56 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -929,6 +929,12 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
 	{ DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
 };
 
+static const struct drm_prop_enum_list rgb_quantization_range_options[] = {
+	{ DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC, "Automatic" },
+	{ DRM_MODE_RGB_QUANTIZATION_RANGE_LIMITED, "Limited" },
+	{ DRM_MODE_RGB_QUANTIZATION_RANGE_FULL, "Full" },
+};
+
 /**
  * DOC: standard connector properties
  *
@@ -1804,6 +1810,35 @@ int drm_mode_create_dp_colorspace_property(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_mode_create_dp_colorspace_property);
 
+/**
+ * drm_mode_create_rgb_quantization_range_property - create RGB quantization
+ * range property
+ * @dev: device to create the RGB quantization range property on.
+ *
+ * Called by a driver the first time it's needed, must be attached to connectors
+ * of CEA-861-compliant sinks.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_create_rgb_quantization_range_property(struct drm_device *dev)
+{
+	if (dev->mode_config.rgb_quantization_range_property)
+		return 0;
+
+	dev->mode_config.rgb_quantization_range_property =
+		drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+					 "RGB quantization range",
+					 rgb_quantization_range_options,
+					 ARRAY_SIZE(rgb_quantization_range_options));
+
+	if (!dev->mode_config.rgb_quantization_range_property)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_rgb_quantization_range_property);
+
 /**
  * drm_mode_create_content_type_property - create content type property
  * @dev: DRM device
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 19ae6bb5c85b..f605e0fbcc0e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -303,6 +303,22 @@ struct drm_monitor_range_info {
 #define DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT		14
 #define DRM_MODE_COLORIMETRY_BT601_YCC			15
 
+/**
+ * enum drm_rgb_quantization_range - rgb_quantization_range property value
+ *
+ * This enum lists the possible options for the rgb_quantization_range property.
+ *
+ * @DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC:	Let the driver select the
+ * 						appropriate quantization range.
+ * @DRM_MODE_RGB_QUANTIZATION_RANGE_LIMITED:	Force limited range RGB.
+ * @DRM_MODE_RGB_QUANTIZATION_RANGE_FULL:	Force full range RGB.
+ */
+enum drm_rgb_quantization_range {
+	DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC = 0,
+	DRM_MODE_RGB_QUANTIZATION_RANGE_LIMITED = 1,
+	DRM_MODE_RGB_QUANTIZATION_RANGE_FULL = 2,
+};
+
 /**
  * enum drm_bus_flags - bus_flags info for &drm_display_info
  *
@@ -661,6 +677,12 @@ struct drm_connector_state {
 	 */
 	u32 colorspace;
 
+	/**
+	 * @rgb_quantization_range: Connector property to control the selected
+	 * RGB quantization range for CEA-861 outputs.
+	 */
+	u32 rgb_quantization_range;
+
 	/**
 	 * @writeback_job: Writeback job for writeback connectors
 	 *
@@ -1575,6 +1597,7 @@ int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
 int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connector);
 int drm_mode_create_dp_colorspace_property(struct drm_connector *connector);
 int drm_mode_create_content_type_property(struct drm_device *dev);
+int drm_mode_create_rgb_quantization_range_property(struct drm_device *dev);
 void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
 					 const struct drm_connector_state *conn_state);
 
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 3bcbe30339f0..745587f6b83c 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -855,6 +855,13 @@ struct drm_mode_config {
 	 */
 	struct drm_property *hdcp_content_type_property;
 
+	/**
+	 * @rgb_quantization_range_property: Connector property to allow
+	 * user-space to override the RGB quantization range for CEA-861
+	 * outputs.
+	 */
+	struct drm_property *rgb_quantization_range_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: Add "RGB quantization range" connector property

[Simon Ser]

Can you also add docs for this property in "DOC: standard connector
properties" in drm_connector.c?

Re: [PATCH 2/5] drm: Add "RGB quantization range" connector property

[Simon Ser]

Can you also add docs for this property in "DOC: standard connector
properties" in drm_connector.c?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm: Add drm_connector_state_select_rgb_quantization_range() helper

[Yussuf Khalil]

This helper can be used to determine the appropriate RGB quantization range
based on a connector's "RGB quantization range" property and a mode.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/drm_connector.c | 31 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     |  4 ++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index e60d3b6e5e56..d5a46bbf284e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2187,6 +2187,37 @@ int drm_connector_set_panel_orientation_with_quirk(
 }
 EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
 
+/**
+ * drm_connector_state_select_rgb_quantization_range - find RGB quantization
+ * range appropriate for a connector's state and mode
+ *
+ * @state: state of the connector for which to determine the range
+ * @mode: mode for which to determine the range
+ *
+ * For a given connector state (i.e., RGB quantization range property) and a
+ * given mode, determine which RGB quantization range should be used.
+ *
+ * Returns:
+ * A constant from the HDMI quantization range enum.
+ */
+enum hdmi_quantization_range drm_connector_state_select_rgb_quantization_range(
+	const struct drm_connector_state *state,
+        const struct drm_display_mode *mode)
+{
+	switch (state->rgb_quantization_range) {
+	default:
+		WARN_ON(1);
+		/* fallthrough */
+	case DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC:
+		return drm_default_rgb_quant_range(mode);
+	case DRM_MODE_RGB_QUANTIZATION_RANGE_FULL:
+		return HDMI_QUANTIZATION_RANGE_FULL;
+	case DRM_MODE_RGB_QUANTIZATION_RANGE_LIMITED:
+		return HDMI_QUANTIZATION_RANGE_LIMITED;
+	}
+}
+EXPORT_SYMBOL(drm_connector_state_select_rgb_quantization_range);
+
 int drm_connector_set_obj_prop(struct drm_mode_object *obj,
 				    struct drm_property *property,
 				    uint64_t value)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index f605e0fbcc0e..43ce305d882f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -42,6 +42,7 @@ struct drm_property_blob;
 struct drm_printer;
 struct edid;
 struct i2c_adapter;
+struct drm_display_mode;
 
 enum drm_connector_force {
 	DRM_FORCE_UNSPECIFIED,
@@ -1621,6 +1622,9 @@ int drm_connector_set_panel_orientation_with_quirk(
 	int width, int height);
 int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+enum hdmi_quantization_range drm_connector_state_select_rgb_quantization_range(
+	const struct drm_connector_state *state,
+	const struct drm_display_mode *mode);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.26.0

[PATCH 3/5] drm: Add drm_connector_state_select_rgb_quantization_range() helper

[Yussuf Khalil]

This helper can be used to determine the appropriate RGB quantization range
based on a connector's "RGB quantization range" property and a mode.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/drm_connector.c | 31 +++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     |  4 ++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index e60d3b6e5e56..d5a46bbf284e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2187,6 +2187,37 @@ int drm_connector_set_panel_orientation_with_quirk(
 }
 EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
 
+/**
+ * drm_connector_state_select_rgb_quantization_range - find RGB quantization
+ * range appropriate for a connector's state and mode
+ *
+ * @state: state of the connector for which to determine the range
+ * @mode: mode for which to determine the range
+ *
+ * For a given connector state (i.e., RGB quantization range property) and a
+ * given mode, determine which RGB quantization range should be used.
+ *
+ * Returns:
+ * A constant from the HDMI quantization range enum.
+ */
+enum hdmi_quantization_range drm_connector_state_select_rgb_quantization_range(
+	const struct drm_connector_state *state,
+        const struct drm_display_mode *mode)
+{
+	switch (state->rgb_quantization_range) {
+	default:
+		WARN_ON(1);
+		/* fallthrough */
+	case DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC:
+		return drm_default_rgb_quant_range(mode);
+	case DRM_MODE_RGB_QUANTIZATION_RANGE_FULL:
+		return HDMI_QUANTIZATION_RANGE_FULL;
+	case DRM_MODE_RGB_QUANTIZATION_RANGE_LIMITED:
+		return HDMI_QUANTIZATION_RANGE_LIMITED;
+	}
+}
+EXPORT_SYMBOL(drm_connector_state_select_rgb_quantization_range);
+
 int drm_connector_set_obj_prop(struct drm_mode_object *obj,
 				    struct drm_property *property,
 				    uint64_t value)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index f605e0fbcc0e..43ce305d882f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -42,6 +42,7 @@ struct drm_property_blob;
 struct drm_printer;
 struct edid;
 struct i2c_adapter;
+struct drm_display_mode;
 
 enum drm_connector_force {
 	DRM_FORCE_UNSPECIFIED,
@@ -1621,6 +1622,9 @@ int drm_connector_set_panel_orientation_with_quirk(
 	int width, int height);
 int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+enum hdmi_quantization_range drm_connector_state_select_rgb_quantization_range(
+	const struct drm_connector_state *state,
+	const struct drm_display_mode *mode);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/atomic-helper: Consider RGB quantization changes to be mode changes

[Yussuf Khalil]

Ensure RGB quantization range changes are applied immediately.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 85d163f16801..b74e90a2b214 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -686,6 +686,12 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			if (old_connector_state->max_requested_bpc !=
 			    new_connector_state->max_requested_bpc)
 				new_crtc_state->connectors_changed = true;
+
+			if (drm_connector_state_select_rgb_quantization_range(
+				old_connector_state, &new_crtc_state->mode) !=
+			    drm_connector_state_select_rgb_quantization_range(
+				new_connector_state, &new_crtc_state->mode))
+				new_crtc_state->mode_changed = true;
 		}
 
 		if (funcs->atomic_check)
-- 
2.26.0

[PATCH 4/5] drm/atomic-helper: Consider RGB quantization changes to be mode changes

[Yussuf Khalil]

Ensure RGB quantization range changes are applied immediately.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 85d163f16801..b74e90a2b214 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -686,6 +686,12 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			if (old_connector_state->max_requested_bpc !=
 			    new_connector_state->max_requested_bpc)
 				new_crtc_state->connectors_changed = true;
+
+			if (drm_connector_state_select_rgb_quantization_range(
+				old_connector_state, &new_crtc_state->mode) !=
+			    drm_connector_state_select_rgb_quantization_range(
+				new_connector_state, &new_crtc_state->mode))
+				new_crtc_state->mode_changed = true;
 		}
 
 		if (funcs->atomic_check)
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Yussuf Khalil]

DRM now has a globally available "RGB quantization range" connector
property. i915's "Broadcast RGB" that fulfils the same purpose is now
considered deprecated, so drop it in favor of the DRM property.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/i915/display/intel_atomic.c   |  8 ----
 .../gpu/drm/i915/display/intel_connector.c    | 39 ++++++-------------
 .../gpu/drm/i915/display/intel_connector.h    |  2 +-
 .../drm/i915/display/intel_display_types.h    |  8 ----
 drivers/gpu/drm/i915/display/intel_dp.c       | 24 ++++--------
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  2 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     | 19 ++++-----
 drivers/gpu/drm/i915/display/intel_sdvo.c     | 18 ++++-----
 drivers/gpu/drm/i915/i915_drv.h               |  1 -
 9 files changed, 34 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index d043057d2fa0..5dbbd8e8aa5d 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -63,8 +63,6 @@ int intel_digital_connector_atomic_get_property(struct drm_connector *connector,
 
 	if (property == dev_priv->force_audio_property)
 		*val = intel_conn_state->force_audio;
-	else if (property == dev_priv->broadcast_rgb_property)
-		*val = intel_conn_state->broadcast_rgb;
 	else {
 		drm_dbg_atomic(&dev_priv->drm,
 			       "Unknown property [PROP:%d:%s]\n",
@@ -99,11 +97,6 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
 		return 0;
 	}
 
-	if (property == dev_priv->broadcast_rgb_property) {
-		intel_conn_state->broadcast_rgb = val;
-		return 0;
-	}
-
 	drm_dbg_atomic(&dev_priv->drm, "Unknown property [PROP:%d:%s]\n",
 		       property->base.id, property->name);
 	return -EINVAL;
@@ -145,7 +138,6 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
 	 * up in a modeset.
 	 */
 	if (new_conn_state->force_audio != old_conn_state->force_audio ||
-	    new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb ||
 	    new_conn_state->base.colorspace != old_conn_state->base.colorspace ||
 	    new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
 	    new_conn_state->base.content_type != old_conn_state->base.content_type ||
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 903e49659f56..1b6439e3ccaf 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -238,34 +238,6 @@ intel_attach_force_audio_property(struct drm_connector *connector)
 	drm_object_attach_property(&connector->base, prop, 0);
 }
 
-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" },
-};
-
-void
-intel_attach_broadcast_rgb_property(struct drm_connector *connector)
-{
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct drm_property *prop;
-
-	prop = dev_priv->broadcast_rgb_property;
-	if (prop == NULL) {
-		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
-					   "Broadcast RGB",
-					   broadcast_rgb_names,
-					   ARRAY_SIZE(broadcast_rgb_names));
-		if (prop == NULL)
-			return;
-
-		dev_priv->broadcast_rgb_property = prop;
-	}
-
-	drm_object_attach_property(&connector->base, prop, 0);
-}
-
 void
 intel_attach_aspect_ratio_property(struct drm_connector *connector)
 {
@@ -297,3 +269,14 @@ intel_attach_colorspace_property(struct drm_connector *connector)
 	drm_object_attach_property(&connector->base,
 				   connector->colorspace_property, 0);
 }
+
+void
+intel_attach_rgb_quantization_range_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+
+	if (!drm_mode_create_rgb_quantization_range_property(dev))
+		drm_object_attach_property(&connector->base,
+			dev->mode_config.rgb_quantization_range_property,
+			DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC);
+}
\ No newline at end of file
diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h
index 93a7375c8196..ece946915407 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.h
+++ b/drivers/gpu/drm/i915/display/intel_connector.h
@@ -28,8 +28,8 @@ int intel_connector_update_modes(struct drm_connector *connector,
 				 struct edid *edid);
 int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 void intel_attach_force_audio_property(struct drm_connector *connector);
-void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
 void intel_attach_aspect_ratio_property(struct drm_connector *connector);
 void intel_attach_colorspace_property(struct drm_connector *connector);
+void intel_attach_rgb_quantization_range_property(struct drm_connector *connector);
 
 #endif /* __INTEL_CONNECTOR_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 5e00e611f077..acd547ee292f 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -76,13 +76,6 @@ enum hdmi_force_audio {
 	HDMI_AUDIO_ON,			/* force turn on HDMI audio */
 };
 
-/* "Broadcast RGB" property */
-enum intel_broadcast_rgb {
-	INTEL_BROADCAST_RGB_AUTO,
-	INTEL_BROADCAST_RGB_FULL,
-	INTEL_BROADCAST_RGB_LIMITED,
-};
-
 struct intel_framebuffer {
 	struct drm_framebuffer base;
 	struct intel_frontbuffer *frontbuffer;
@@ -443,7 +436,6 @@ struct intel_digital_connector_state {
 	struct drm_connector_state base;
 
 	enum hdmi_force_audio force_audio;
-	int broadcast_rgb;
 };
 
 #define to_intel_digital_connector_state(x) container_of(x, struct intel_digital_connector_state, base)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 804b1d966f66..eb975eed6c72 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2343,10 +2343,9 @@ intel_dp_ycbcr420_config(struct intel_dp *intel_dp,
 bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state,
 				  const struct drm_connector_state *conn_state)
 {
-	const struct intel_digital_connector_state *intel_conn_state =
-		to_intel_digital_connector_state(conn_state);
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
+	enum hdmi_quantization_range range;
 
 	/*
 	 * Our YCbCr output is always limited range.
@@ -2355,22 +2354,13 @@ bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state,
 	 * some conflicting bits in PIPECONF which will mess up
 	 * the colors on the monitor.
 	 */
-	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
+	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+	    crtc_state->pipe_bpp == 18)
 		return false;
 
-	if (intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
-		/*
-		 * See:
-		 * CEA-861-E - 5.1 Default Encoding Parameters
-		 * VESA DisplayPort Ver.1.2a - 5.1.1.1 Video Colorimetry
-		 */
-		return crtc_state->pipe_bpp != 18 &&
-			drm_default_rgb_quant_range(adjusted_mode) ==
-			HDMI_QUANTIZATION_RANGE_LIMITED;
-	} else {
-		return intel_conn_state->broadcast_rgb ==
-			INTEL_BROADCAST_RGB_LIMITED;
-	}
+	range = drm_connector_state_select_rgb_quantization_range(conn_state,
+								  adjusted_mode);
+	return range == HDMI_QUANTIZATION_RANGE_LIMITED;
 }
 
 static bool intel_dp_port_has_audio(struct drm_i915_private *dev_priv,
@@ -6844,7 +6834,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	if (!IS_G4X(dev_priv) && port != PORT_A)
 		intel_attach_force_audio_property(connector);
 
-	intel_attach_broadcast_rgb_property(connector);
+	intel_attach_rgb_quantization_range_property(connector);
 	if (HAS_GMCH(dev_priv))
 		drm_connector_attach_max_bpc_property(connector, 6, 10);
 	else if (INTEL_GEN(dev_priv) >= 5)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 44f3fd251ca1..41f54f2bb7ec 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -730,7 +730,7 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 		goto err;
 
 	intel_attach_force_audio_property(connector);
-	intel_attach_broadcast_rgb_property(connector);
+	intel_attach_rgb_quantization_range_property(connector);
 
 	/*
 	 * Reuse the prop from the SST connector because we're
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 39930232b253..028b959a8192 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2381,10 +2381,9 @@ static int intel_hdmi_compute_clock(struct intel_encoder *encoder,
 static bool intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_state,
 					   const struct drm_connector_state *conn_state)
 {
-	const struct intel_digital_connector_state *intel_conn_state =
-		to_intel_digital_connector_state(conn_state);
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
+	enum hdmi_quantization_range range;
 
 	/*
 	 * Our YCbCr output is always limited range.
@@ -2393,17 +2392,13 @@ static bool intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_s
 	 * some conflicting bits in PIPECONF which will mess up
 	 * the colors on the monitor.
 	 */
-	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
+	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+	    !crtc_state->has_hdmi_sink)
 		return false;
 
-	if (intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
-		/* See CEA-861-E - 5.1 Default Encoding Parameters */
-		return crtc_state->has_hdmi_sink &&
-			drm_default_rgb_quant_range(adjusted_mode) ==
-			HDMI_QUANTIZATION_RANGE_LIMITED;
-	} else {
-		return intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
-	}
+	range = drm_connector_state_select_rgb_quantization_range(conn_state,
+								  adjusted_mode);
+	return range == HDMI_QUANTIZATION_RANGE_LIMITED;
 }
 
 int intel_hdmi_compute_config(struct intel_encoder *encoder,
@@ -2867,7 +2862,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 				hdmi_to_dig_port(intel_hdmi);
 
 	intel_attach_force_audio_property(connector);
-	intel_attach_broadcast_rgb_property(connector);
+	intel_attach_rgb_quantization_range_property(connector);
 	intel_attach_aspect_ratio_property(connector);
 
 	/*
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 637d8fe2f8c2..fda6daae4f97 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -1328,26 +1328,22 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
 	pipe_config->has_hdmi_sink = intel_has_hdmi_sink(intel_sdvo, conn_state);
 
 	if (pipe_config->has_hdmi_sink) {
+		enum hdmi_quantization_range range;
+
 		if (intel_sdvo_state->base.force_audio == HDMI_AUDIO_AUTO)
 			pipe_config->has_audio = intel_sdvo->has_hdmi_audio;
 		else
 			pipe_config->has_audio =
 				intel_sdvo_state->base.force_audio == HDMI_AUDIO_ON;
-	}
 
-	if (intel_sdvo_state->base.broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
+		range = drm_connector_state_select_rgb_quantization_range(
+				conn_state, adjusted_mode);
+
 		/*
-		 * See CEA-861-E - 5.1 Default Encoding Parameters
-		 *
 		 * FIXME: This bit is only valid when using TMDS encoding and 8
 		 * bit per color mode.
 		 */
-		if (pipe_config->has_hdmi_sink &&
-		    drm_match_cea_mode(adjusted_mode) > 1)
-			pipe_config->limited_color_range = true;
-	} else {
-		if (pipe_config->has_hdmi_sink &&
-		    intel_sdvo_state->base.broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED)
+		if (range == HDMI_QUANTIZATION_RANGE_LIMITED)
 			pipe_config->limited_color_range = true;
 	}
 
@@ -2662,7 +2658,7 @@ intel_sdvo_add_hdmi_properties(struct intel_sdvo *intel_sdvo,
 
 	intel_attach_force_audio_property(&connector->base.base);
 	if (INTEL_GEN(dev_priv) >= 4 && IS_MOBILE(dev_priv)) {
-		intel_attach_broadcast_rgb_property(&connector->base.base);
+		intel_attach_rgb_quantization_range_property(&connector->base.base);
 	}
 	intel_attach_aspect_ratio_property(&connector->base.base);
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1f5b9a584f71..3105f582019c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1042,7 +1042,6 @@ struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 	struct work_struct fbdev_suspend_work;
 
-	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
 	/* hda/i915 audio component */
-- 
2.26.0

[PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Yussuf Khalil]

DRM now has a globally available "RGB quantization range" connector
property. i915's "Broadcast RGB" that fulfils the same purpose is now
considered deprecated, so drop it in favor of the DRM property.

Signed-off-by: Yussuf Khalil <dev@pp3345.net>
---
 drivers/gpu/drm/i915/display/intel_atomic.c   |  8 ----
 .../gpu/drm/i915/display/intel_connector.c    | 39 ++++++-------------
 .../gpu/drm/i915/display/intel_connector.h    |  2 +-
 .../drm/i915/display/intel_display_types.h    |  8 ----
 drivers/gpu/drm/i915/display/intel_dp.c       | 24 ++++--------
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  2 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     | 19 ++++-----
 drivers/gpu/drm/i915/display/intel_sdvo.c     | 18 ++++-----
 drivers/gpu/drm/i915/i915_drv.h               |  1 -
 9 files changed, 34 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index d043057d2fa0..5dbbd8e8aa5d 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -63,8 +63,6 @@ int intel_digital_connector_atomic_get_property(struct drm_connector *connector,
 
 	if (property == dev_priv->force_audio_property)
 		*val = intel_conn_state->force_audio;
-	else if (property == dev_priv->broadcast_rgb_property)
-		*val = intel_conn_state->broadcast_rgb;
 	else {
 		drm_dbg_atomic(&dev_priv->drm,
 			       "Unknown property [PROP:%d:%s]\n",
@@ -99,11 +97,6 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
 		return 0;
 	}
 
-	if (property == dev_priv->broadcast_rgb_property) {
-		intel_conn_state->broadcast_rgb = val;
-		return 0;
-	}
-
 	drm_dbg_atomic(&dev_priv->drm, "Unknown property [PROP:%d:%s]\n",
 		       property->base.id, property->name);
 	return -EINVAL;
@@ -145,7 +138,6 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
 	 * up in a modeset.
 	 */
 	if (new_conn_state->force_audio != old_conn_state->force_audio ||
-	    new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb ||
 	    new_conn_state->base.colorspace != old_conn_state->base.colorspace ||
 	    new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
 	    new_conn_state->base.content_type != old_conn_state->base.content_type ||
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 903e49659f56..1b6439e3ccaf 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -238,34 +238,6 @@ intel_attach_force_audio_property(struct drm_connector *connector)
 	drm_object_attach_property(&connector->base, prop, 0);
 }
 
-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" },
-};
-
-void
-intel_attach_broadcast_rgb_property(struct drm_connector *connector)
-{
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct drm_property *prop;
-
-	prop = dev_priv->broadcast_rgb_property;
-	if (prop == NULL) {
-		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
-					   "Broadcast RGB",
-					   broadcast_rgb_names,
-					   ARRAY_SIZE(broadcast_rgb_names));
-		if (prop == NULL)
-			return;
-
-		dev_priv->broadcast_rgb_property = prop;
-	}
-
-	drm_object_attach_property(&connector->base, prop, 0);
-}
-
 void
 intel_attach_aspect_ratio_property(struct drm_connector *connector)
 {
@@ -297,3 +269,14 @@ intel_attach_colorspace_property(struct drm_connector *connector)
 	drm_object_attach_property(&connector->base,
 				   connector->colorspace_property, 0);
 }
+
+void
+intel_attach_rgb_quantization_range_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+
+	if (!drm_mode_create_rgb_quantization_range_property(dev))
+		drm_object_attach_property(&connector->base,
+			dev->mode_config.rgb_quantization_range_property,
+			DRM_MODE_RGB_QUANTIZATION_RANGE_AUTOMATIC);
+}
\ No newline at end of file
diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h
index 93a7375c8196..ece946915407 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.h
+++ b/drivers/gpu/drm/i915/display/intel_connector.h
@@ -28,8 +28,8 @@ int intel_connector_update_modes(struct drm_connector *connector,
 				 struct edid *edid);
 int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 void intel_attach_force_audio_property(struct drm_connector *connector);
-void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
 void intel_attach_aspect_ratio_property(struct drm_connector *connector);
 void intel_attach_colorspace_property(struct drm_connector *connector);
+void intel_attach_rgb_quantization_range_property(struct drm_connector *connector);
 
 #endif /* __INTEL_CONNECTOR_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 5e00e611f077..acd547ee292f 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -76,13 +76,6 @@ enum hdmi_force_audio {
 	HDMI_AUDIO_ON,			/* force turn on HDMI audio */
 };
 
-/* "Broadcast RGB" property */
-enum intel_broadcast_rgb {
-	INTEL_BROADCAST_RGB_AUTO,
-	INTEL_BROADCAST_RGB_FULL,
-	INTEL_BROADCAST_RGB_LIMITED,
-};
-
 struct intel_framebuffer {
 	struct drm_framebuffer base;
 	struct intel_frontbuffer *frontbuffer;
@@ -443,7 +436,6 @@ struct intel_digital_connector_state {
 	struct drm_connector_state base;
 
 	enum hdmi_force_audio force_audio;
-	int broadcast_rgb;
 };
 
 #define to_intel_digital_connector_state(x) container_of(x, struct intel_digital_connector_state, base)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 804b1d966f66..eb975eed6c72 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2343,10 +2343,9 @@ intel_dp_ycbcr420_config(struct intel_dp *intel_dp,
 bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state,
 				  const struct drm_connector_state *conn_state)
 {
-	const struct intel_digital_connector_state *intel_conn_state =
-		to_intel_digital_connector_state(conn_state);
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
+	enum hdmi_quantization_range range;
 
 	/*
 	 * Our YCbCr output is always limited range.
@@ -2355,22 +2354,13 @@ bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state,
 	 * some conflicting bits in PIPECONF which will mess up
 	 * the colors on the monitor.
 	 */
-	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
+	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+	    crtc_state->pipe_bpp == 18)
 		return false;
 
-	if (intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
-		/*
-		 * See:
-		 * CEA-861-E - 5.1 Default Encoding Parameters
-		 * VESA DisplayPort Ver.1.2a - 5.1.1.1 Video Colorimetry
-		 */
-		return crtc_state->pipe_bpp != 18 &&
-			drm_default_rgb_quant_range(adjusted_mode) ==
-			HDMI_QUANTIZATION_RANGE_LIMITED;
-	} else {
-		return intel_conn_state->broadcast_rgb ==
-			INTEL_BROADCAST_RGB_LIMITED;
-	}
+	range = drm_connector_state_select_rgb_quantization_range(conn_state,
+								  adjusted_mode);
+	return range == HDMI_QUANTIZATION_RANGE_LIMITED;
 }
 
 static bool intel_dp_port_has_audio(struct drm_i915_private *dev_priv,
@@ -6844,7 +6834,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	if (!IS_G4X(dev_priv) && port != PORT_A)
 		intel_attach_force_audio_property(connector);
 
-	intel_attach_broadcast_rgb_property(connector);
+	intel_attach_rgb_quantization_range_property(connector);
 	if (HAS_GMCH(dev_priv))
 		drm_connector_attach_max_bpc_property(connector, 6, 10);
 	else if (INTEL_GEN(dev_priv) >= 5)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 44f3fd251ca1..41f54f2bb7ec 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -730,7 +730,7 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 		goto err;
 
 	intel_attach_force_audio_property(connector);
-	intel_attach_broadcast_rgb_property(connector);
+	intel_attach_rgb_quantization_range_property(connector);
 
 	/*
 	 * Reuse the prop from the SST connector because we're
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 39930232b253..028b959a8192 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2381,10 +2381,9 @@ static int intel_hdmi_compute_clock(struct intel_encoder *encoder,
 static bool intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_state,
 					   const struct drm_connector_state *conn_state)
 {
-	const struct intel_digital_connector_state *intel_conn_state =
-		to_intel_digital_connector_state(conn_state);
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
+	enum hdmi_quantization_range range;
 
 	/*
 	 * Our YCbCr output is always limited range.
@@ -2393,17 +2392,13 @@ static bool intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_s
 	 * some conflicting bits in PIPECONF which will mess up
 	 * the colors on the monitor.
 	 */
-	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
+	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+	    !crtc_state->has_hdmi_sink)
 		return false;
 
-	if (intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
-		/* See CEA-861-E - 5.1 Default Encoding Parameters */
-		return crtc_state->has_hdmi_sink &&
-			drm_default_rgb_quant_range(adjusted_mode) ==
-			HDMI_QUANTIZATION_RANGE_LIMITED;
-	} else {
-		return intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
-	}
+	range = drm_connector_state_select_rgb_quantization_range(conn_state,
+								  adjusted_mode);
+	return range == HDMI_QUANTIZATION_RANGE_LIMITED;
 }
 
 int intel_hdmi_compute_config(struct intel_encoder *encoder,
@@ -2867,7 +2862,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 				hdmi_to_dig_port(intel_hdmi);
 
 	intel_attach_force_audio_property(connector);
-	intel_attach_broadcast_rgb_property(connector);
+	intel_attach_rgb_quantization_range_property(connector);
 	intel_attach_aspect_ratio_property(connector);
 
 	/*
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 637d8fe2f8c2..fda6daae4f97 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -1328,26 +1328,22 @@ static int intel_sdvo_compute_config(struct intel_encoder *encoder,
 	pipe_config->has_hdmi_sink = intel_has_hdmi_sink(intel_sdvo, conn_state);
 
 	if (pipe_config->has_hdmi_sink) {
+		enum hdmi_quantization_range range;
+
 		if (intel_sdvo_state->base.force_audio == HDMI_AUDIO_AUTO)
 			pipe_config->has_audio = intel_sdvo->has_hdmi_audio;
 		else
 			pipe_config->has_audio =
 				intel_sdvo_state->base.force_audio == HDMI_AUDIO_ON;
-	}
 
-	if (intel_sdvo_state->base.broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
+		range = drm_connector_state_select_rgb_quantization_range(
+				conn_state, adjusted_mode);
+
 		/*
-		 * See CEA-861-E - 5.1 Default Encoding Parameters
-		 *
 		 * FIXME: This bit is only valid when using TMDS encoding and 8
 		 * bit per color mode.
 		 */
-		if (pipe_config->has_hdmi_sink &&
-		    drm_match_cea_mode(adjusted_mode) > 1)
-			pipe_config->limited_color_range = true;
-	} else {
-		if (pipe_config->has_hdmi_sink &&
-		    intel_sdvo_state->base.broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED)
+		if (range == HDMI_QUANTIZATION_RANGE_LIMITED)
 			pipe_config->limited_color_range = true;
 	}
 
@@ -2662,7 +2658,7 @@ intel_sdvo_add_hdmi_properties(struct intel_sdvo *intel_sdvo,
 
 	intel_attach_force_audio_property(&connector->base.base);
 	if (INTEL_GEN(dev_priv) >= 4 && IS_MOBILE(dev_priv)) {
-		intel_attach_broadcast_rgb_property(&connector->base.base);
+		intel_attach_rgb_quantization_range_property(&connector->base.base);
 	}
 	intel_attach_aspect_ratio_property(&connector->base.base);
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1f5b9a584f71..3105f582019c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1042,7 +1042,6 @@ struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 	struct work_struct fbdev_suspend_work;
 
-	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
 	/* hda/i915 audio component */
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Simon Ser]

On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <dev@pp3345.net> wrote:

> DRM now has a globally available "RGB quantization range" connector
> property. i915's "Broadcast RGB" that fulfils the same purpose is now
> considered deprecated, so drop it in favor of the DRM property.

For a UAPI point-of-view, I'm not sure this is fine. Some user-space
might depend on this property, dropping it would break such user-space.

Can we make this property deprecated but still keep it for backwards
compatibility?

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Simon Ser]

On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <dev@pp3345.net> wrote:

> DRM now has a globally available "RGB quantization range" connector
> property. i915's "Broadcast RGB" that fulfils the same purpose is now
> considered deprecated, so drop it in favor of the DRM property.

For a UAPI point-of-view, I'm not sure this is fine. Some user-space
might depend on this property, dropping it would break such user-space.

Can we make this property deprecated but still keep it for backwards
compatibility?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Jani Nikula]

On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <dev@pp3345.net> wrote:
>
>> DRM now has a globally available "RGB quantization range" connector
>> property. i915's "Broadcast RGB" that fulfils the same purpose is now
>> considered deprecated, so drop it in favor of the DRM property.
>
> For a UAPI point-of-view, I'm not sure this is fine. Some user-space
> might depend on this property, dropping it would break such user-space.

Agreed.

> Can we make this property deprecated but still keep it for backwards
> compatibility?

Would be nice to make the i915 specific property an "alias" for the new
property, however I'm not sure how you'd make that happen. Otherwise
juggling between the two properties is going to be a nightmare.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Jani Nikula]

On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <dev@pp3345.net> wrote:
>
>> DRM now has a globally available "RGB quantization range" connector
>> property. i915's "Broadcast RGB" that fulfils the same purpose is now
>> considered deprecated, so drop it in favor of the DRM property.
>
> For a UAPI point-of-view, I'm not sure this is fine. Some user-space
> might depend on this property, dropping it would break such user-space.

Agreed.

> Can we make this property deprecated but still keep it for backwards
> compatibility?

Would be nice to make the i915 specific property an "alias" for the new
property, however I'm not sure how you'd make that happen. Otherwise
juggling between the two properties is going to be a nightmare.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Jani Nikula]

On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
>> On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <dev@pp3345.net> wrote:
>>
>>> DRM now has a globally available "RGB quantization range" connector
>>> property. i915's "Broadcast RGB" that fulfils the same purpose is now
>>> considered deprecated, so drop it in favor of the DRM property.
>>
>> For a UAPI point-of-view, I'm not sure this is fine. Some user-space
>> might depend on this property, dropping it would break such user-space.
>
> Agreed.
>
>> Can we make this property deprecated but still keep it for backwards
>> compatibility?
>
> Would be nice to make the i915 specific property an "alias" for the new
> property, however I'm not sure how you'd make that happen. Otherwise
> juggling between the two properties is going to be a nightmare.

Ah, the obvious easy choice is to use the property and enum names
already being used by i915 and gma500, and you have no problem. Perhaps
they're not the names you'd like, but then looking at the total lack of
consistency across property naming makes them fit right in. ;)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Jani Nikula]

On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
>> On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <dev@pp3345.net> wrote:
>>
>>> DRM now has a globally available "RGB quantization range" connector
>>> property. i915's "Broadcast RGB" that fulfils the same purpose is now
>>> considered deprecated, so drop it in favor of the DRM property.
>>
>> For a UAPI point-of-view, I'm not sure this is fine. Some user-space
>> might depend on this property, dropping it would break such user-space.
>
> Agreed.
>
>> Can we make this property deprecated but still keep it for backwards
>> compatibility?
>
> Would be nice to make the i915 specific property an "alias" for the new
> property, however I'm not sure how you'd make that happen. Otherwise
> juggling between the two properties is going to be a nightmare.

Ah, the obvious easy choice is to use the property and enum names
already being used by i915 and gma500, and you have no problem. Perhaps
they're not the names you'd like, but then looking at the total lack of
consistency across property naming makes them fit right in. ;)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Daniel Vetter]

On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> >> On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <dev@pp3345.net> wrote:
> >>
> >>> DRM now has a globally available "RGB quantization range" connector
> >>> property. i915's "Broadcast RGB" that fulfils the same purpose is now
> >>> considered deprecated, so drop it in favor of the DRM property.
> >>
> >> For a UAPI point-of-view, I'm not sure this is fine. Some user-space
> >> might depend on this property, dropping it would break such user-space.
> >
> > Agreed.
> >
> >> Can we make this property deprecated but still keep it for backwards
> >> compatibility?
> >
> > Would be nice to make the i915 specific property an "alias" for the new
> > property, however I'm not sure how you'd make that happen. Otherwise
> > juggling between the two properties is going to be a nightmare.
> 
> Ah, the obvious easy choice is to use the property and enum names
> already being used by i915 and gma500, and you have no problem. Perhaps
> they're not the names you'd like, but then looking at the total lack of
> consistency across property naming makes them fit right in. ;)

Yeah if we don't have contradictory usage across drivers when modernizing
these properties, then let's just stick with the names already there. It's
not pretty, but works better since more userspace/internet howtos know how
to use this stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Daniel Vetter]

On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> >> On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <dev@pp3345.net> wrote:
> >>
> >>> DRM now has a globally available "RGB quantization range" connector
> >>> property. i915's "Broadcast RGB" that fulfils the same purpose is now
> >>> considered deprecated, so drop it in favor of the DRM property.
> >>
> >> For a UAPI point-of-view, I'm not sure this is fine. Some user-space
> >> might depend on this property, dropping it would break such user-space.
> >
> > Agreed.
> >
> >> Can we make this property deprecated but still keep it for backwards
> >> compatibility?
> >
> > Would be nice to make the i915 specific property an "alias" for the new
> > property, however I'm not sure how you'd make that happen. Otherwise
> > juggling between the two properties is going to be a nightmare.
> 
> Ah, the obvious easy choice is to use the property and enum names
> already being used by i915 and gma500, and you have no problem. Perhaps
> they're not the names you'd like, but then looking at the total lack of
> consistency across property naming makes them fit right in. ;)

Yeah if we don't have contradictory usage across drivers when modernizing
these properties, then let's just stick with the names already there. It's
not pretty, but works better since more userspace/internet howtos know how
to use this stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Yussuf Khalil]

On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com>
> > wrote:
> > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > dev@pp3345.net> wrote:
> > > > 
> > > > > DRM now has a globally available "RGB quantization range"
> > > > > connector
> > > > > property. i915's "Broadcast RGB" that fulfils the same
> > > > > purpose is now
> > > > > considered deprecated, so drop it in favor of the DRM
> > > > > property.
> > > > 
> > > > For a UAPI point-of-view, I'm not sure this is fine. Some user-
> > > > space
> > > > might depend on this property, dropping it would break such
> > > > user-space.
> > > 
> > > Agreed.
> > > 
> > > > Can we make this property deprecated but still keep it for
> > > > backwards
> > > > compatibility?
> > > 
> > > Would be nice to make the i915 specific property an "alias" for
> > > the new
> > > property, however I'm not sure how you'd make that happen.
> > > Otherwise
> > > juggling between the two properties is going to be a nightmare.
> > 
> > Ah, the obvious easy choice is to use the property and enum names
> > already being used by i915 and gma500, and you have no problem.
> > Perhaps
> > they're not the names you'd like, but then looking at the total
> > lack of
> > consistency across property naming makes them fit right in. ;)
> 
> Yeah if we don't have contradictory usage across drivers when
> modernizing
> these properties, then let's just stick with the names already there.
> It's
> not pretty, but works better since more userspace/internet howtos
> know how
> to use this stuff.
> -Daniel

Note that i915's "Broadcast RGB" isn't the same as gma500's: i915 has an
"Automatic" option, whereas gma500 does not. Also, radeon has a property called
"output_csc" that fulfills a similar purpose. Looking at the code, though, it
seems that radeon does not adhere to the standard correctly (or I am missing
something).

An alternative would be to leave the existing driver-specific properties and
change them to be pseudo-aliases for the "RGB quantization range" property.
This can be done by letting the drivers read from and write to the new property
when user-space tries to read or modify the driver's property. This way we could
retain full backwards compatibility for all drivers equally.

What do you think?

Regards
Yussuf

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Yussuf Khalil]

On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com>
> > wrote:
> > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > dev@pp3345.net> wrote:
> > > > 
> > > > > DRM now has a globally available "RGB quantization range"
> > > > > connector
> > > > > property. i915's "Broadcast RGB" that fulfils the same
> > > > > purpose is now
> > > > > considered deprecated, so drop it in favor of the DRM
> > > > > property.
> > > > 
> > > > For a UAPI point-of-view, I'm not sure this is fine. Some user-
> > > > space
> > > > might depend on this property, dropping it would break such
> > > > user-space.
> > > 
> > > Agreed.
> > > 
> > > > Can we make this property deprecated but still keep it for
> > > > backwards
> > > > compatibility?
> > > 
> > > Would be nice to make the i915 specific property an "alias" for
> > > the new
> > > property, however I'm not sure how you'd make that happen.
> > > Otherwise
> > > juggling between the two properties is going to be a nightmare.
> > 
> > Ah, the obvious easy choice is to use the property and enum names
> > already being used by i915 and gma500, and you have no problem.
> > Perhaps
> > they're not the names you'd like, but then looking at the total
> > lack of
> > consistency across property naming makes them fit right in. ;)
> 
> Yeah if we don't have contradictory usage across drivers when
> modernizing
> these properties, then let's just stick with the names already there.
> It's
> not pretty, but works better since more userspace/internet howtos
> know how
> to use this stuff.
> -Daniel

Note that i915's "Broadcast RGB" isn't the same as gma500's: i915 has an
"Automatic" option, whereas gma500 does not. Also, radeon has a property called
"output_csc" that fulfills a similar purpose. Looking at the code, though, it
seems that radeon does not adhere to the standard correctly (or I am missing
something).

An alternative would be to leave the existing driver-specific properties and
change them to be pseudo-aliases for the "RGB quantization range" property.
This can be done by letting the drivers read from and write to the new property
when user-space tries to read or modify the driver's property. This way we could
retain full backwards compatibility for all drivers equally.

What do you think?

Regards
Yussuf

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Jani Nikula]

On Tue, 14 Apr 2020, Yussuf Khalil <dev@pp3345.net> wrote:
> On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
>> On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
>> > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com>
>> > wrote:
>> > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
>> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
>> > > > dev@pp3345.net> wrote:
>> > > > 
>> > > > > DRM now has a globally available "RGB quantization range"
>> > > > > connector
>> > > > > property. i915's "Broadcast RGB" that fulfils the same
>> > > > > purpose is now
>> > > > > considered deprecated, so drop it in favor of the DRM
>> > > > > property.
>> > > > 
>> > > > For a UAPI point-of-view, I'm not sure this is fine. Some user-
>> > > > space
>> > > > might depend on this property, dropping it would break such
>> > > > user-space.
>> > > 
>> > > Agreed.
>> > > 
>> > > > Can we make this property deprecated but still keep it for
>> > > > backwards
>> > > > compatibility?
>> > > 
>> > > Would be nice to make the i915 specific property an "alias" for
>> > > the new
>> > > property, however I'm not sure how you'd make that happen.
>> > > Otherwise
>> > > juggling between the two properties is going to be a nightmare.
>> > 
>> > Ah, the obvious easy choice is to use the property and enum names
>> > already being used by i915 and gma500, and you have no problem.
>> > Perhaps
>> > they're not the names you'd like, but then looking at the total
>> > lack of
>> > consistency across property naming makes them fit right in. ;)
>> 
>> Yeah if we don't have contradictory usage across drivers when
>> modernizing
>> these properties, then let's just stick with the names already there.
>> It's
>> not pretty, but works better since more userspace/internet howtos
>> know how
>> to use this stuff.
>> -Daniel
>
> Note that i915's "Broadcast RGB" isn't the same as gma500's: i915 has an
> "Automatic" option, whereas gma500 does not.

Adding "Automatic" option that just defaults to "Full" in gma500 does
not break existing userspace, but allows for extending it to do more
clever things later.

> Also, radeon has a property called
> "output_csc" that fulfills a similar purpose. Looking at the code, though, it
> seems that radeon does not adhere to the standard correctly (or I am missing
> something).
>
> An alternative would be to leave the existing driver-specific properties and
> change them to be pseudo-aliases for the "RGB quantization range" property.
> This can be done by letting the drivers read from and write to the new property
> when user-space tries to read or modify the driver's property. This way we could
> retain full backwards compatibility for all drivers equally.
>
> What do you think?

I'm obviously biased and predisposed to avoid adding extra complexity to
i915 when it's not necessary. We'd have *two* connector properties for
the same thing until the end of time, even if one is an alias for the
other.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Jani Nikula]

On Tue, 14 Apr 2020, Yussuf Khalil <dev@pp3345.net> wrote:
> On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
>> On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
>> > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com>
>> > wrote:
>> > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
>> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
>> > > > dev@pp3345.net> wrote:
>> > > > 
>> > > > > DRM now has a globally available "RGB quantization range"
>> > > > > connector
>> > > > > property. i915's "Broadcast RGB" that fulfils the same
>> > > > > purpose is now
>> > > > > considered deprecated, so drop it in favor of the DRM
>> > > > > property.
>> > > > 
>> > > > For a UAPI point-of-view, I'm not sure this is fine. Some user-
>> > > > space
>> > > > might depend on this property, dropping it would break such
>> > > > user-space.
>> > > 
>> > > Agreed.
>> > > 
>> > > > Can we make this property deprecated but still keep it for
>> > > > backwards
>> > > > compatibility?
>> > > 
>> > > Would be nice to make the i915 specific property an "alias" for
>> > > the new
>> > > property, however I'm not sure how you'd make that happen.
>> > > Otherwise
>> > > juggling between the two properties is going to be a nightmare.
>> > 
>> > Ah, the obvious easy choice is to use the property and enum names
>> > already being used by i915 and gma500, and you have no problem.
>> > Perhaps
>> > they're not the names you'd like, but then looking at the total
>> > lack of
>> > consistency across property naming makes them fit right in. ;)
>> 
>> Yeah if we don't have contradictory usage across drivers when
>> modernizing
>> these properties, then let's just stick with the names already there.
>> It's
>> not pretty, but works better since more userspace/internet howtos
>> know how
>> to use this stuff.
>> -Daniel
>
> Note that i915's "Broadcast RGB" isn't the same as gma500's: i915 has an
> "Automatic" option, whereas gma500 does not.

Adding "Automatic" option that just defaults to "Full" in gma500 does
not break existing userspace, but allows for extending it to do more
clever things later.

> Also, radeon has a property called
> "output_csc" that fulfills a similar purpose. Looking at the code, though, it
> seems that radeon does not adhere to the standard correctly (or I am missing
> something).
>
> An alternative would be to leave the existing driver-specific properties and
> change them to be pseudo-aliases for the "RGB quantization range" property.
> This can be done by letting the drivers read from and write to the new property
> when user-space tries to read or modify the driver's property. This way we could
> retain full backwards compatibility for all drivers equally.
>
> What do you think?

I'm obviously biased and predisposed to avoid adding extra complexity to
i915 when it's not necessary. We'd have *two* connector properties for
the same thing until the end of time, even if one is an alias for the
other.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Daniel Vetter]

On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> On Tue, 14 Apr 2020, Yussuf Khalil <dev@pp3345.net> wrote:
> > On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> >> On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> >> > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com>
> >> > wrote:
> >> > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> >> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> >> > > > dev@pp3345.net> wrote:
> >> > > > 
> >> > > > > DRM now has a globally available "RGB quantization range"
> >> > > > > connector
> >> > > > > property. i915's "Broadcast RGB" that fulfils the same
> >> > > > > purpose is now
> >> > > > > considered deprecated, so drop it in favor of the DRM
> >> > > > > property.
> >> > > > 
> >> > > > For a UAPI point-of-view, I'm not sure this is fine. Some user-
> >> > > > space
> >> > > > might depend on this property, dropping it would break such
> >> > > > user-space.
> >> > > 
> >> > > Agreed.
> >> > > 
> >> > > > Can we make this property deprecated but still keep it for
> >> > > > backwards
> >> > > > compatibility?
> >> > > 
> >> > > Would be nice to make the i915 specific property an "alias" for
> >> > > the new
> >> > > property, however I'm not sure how you'd make that happen.
> >> > > Otherwise
> >> > > juggling between the two properties is going to be a nightmare.
> >> > 
> >> > Ah, the obvious easy choice is to use the property and enum names
> >> > already being used by i915 and gma500, and you have no problem.
> >> > Perhaps
> >> > they're not the names you'd like, but then looking at the total
> >> > lack of
> >> > consistency across property naming makes them fit right in. ;)
> >> 
> >> Yeah if we don't have contradictory usage across drivers when
> >> modernizing
> >> these properties, then let's just stick with the names already there.
> >> It's
> >> not pretty, but works better since more userspace/internet howtos
> >> know how
> >> to use this stuff.
> >> -Daniel
> >
> > Note that i915's "Broadcast RGB" isn't the same as gma500's: i915 has an
> > "Automatic" option, whereas gma500 does not.
> 
> Adding "Automatic" option that just defaults to "Full" in gma500 does
> not break existing userspace, but allows for extending it to do more
> clever things later.

gma500 could keep it's own property, without the "Automatic" value. We've
done tricks like this for other properties too.

> > Also, radeon has a property called
> > "output_csc" that fulfills a similar purpose. Looking at the code, though, it
> > seems that radeon does not adhere to the standard correctly (or I am missing
> > something).
> >
> > An alternative would be to leave the existing driver-specific properties and
> > change them to be pseudo-aliases for the "RGB quantization range" property.
> > This can be done by letting the drivers read from and write to the new property
> > when user-space tries to read or modify the driver's property. This way we could
> > retain full backwards compatibility for all drivers equally.
> >
> > What do you think?
> 
> I'm obviously biased and predisposed to avoid adding extra complexity to
> i915 when it's not necessary. We'd have *two* connector properties for
> the same thing until the end of time, even if one is an alias for the
> other.

Yeah just pick one, and implement the others as aliases. Drivers can do
the aliases in their atomic_get/set_property functions pretty easily,
atomic properties aren't stored anywhere else than decoded (which was done
partially to make stuff like this work).

Coming up a new property name just so that everyone suffers equally feels
silly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Daniel Vetter]

On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> On Tue, 14 Apr 2020, Yussuf Khalil <dev@pp3345.net> wrote:
> > On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> >> On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> >> > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com>
> >> > wrote:
> >> > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> >> > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> >> > > > dev@pp3345.net> wrote:
> >> > > > 
> >> > > > > DRM now has a globally available "RGB quantization range"
> >> > > > > connector
> >> > > > > property. i915's "Broadcast RGB" that fulfils the same
> >> > > > > purpose is now
> >> > > > > considered deprecated, so drop it in favor of the DRM
> >> > > > > property.
> >> > > > 
> >> > > > For a UAPI point-of-view, I'm not sure this is fine. Some user-
> >> > > > space
> >> > > > might depend on this property, dropping it would break such
> >> > > > user-space.
> >> > > 
> >> > > Agreed.
> >> > > 
> >> > > > Can we make this property deprecated but still keep it for
> >> > > > backwards
> >> > > > compatibility?
> >> > > 
> >> > > Would be nice to make the i915 specific property an "alias" for
> >> > > the new
> >> > > property, however I'm not sure how you'd make that happen.
> >> > > Otherwise
> >> > > juggling between the two properties is going to be a nightmare.
> >> > 
> >> > Ah, the obvious easy choice is to use the property and enum names
> >> > already being used by i915 and gma500, and you have no problem.
> >> > Perhaps
> >> > they're not the names you'd like, but then looking at the total
> >> > lack of
> >> > consistency across property naming makes them fit right in. ;)
> >> 
> >> Yeah if we don't have contradictory usage across drivers when
> >> modernizing
> >> these properties, then let's just stick with the names already there.
> >> It's
> >> not pretty, but works better since more userspace/internet howtos
> >> know how
> >> to use this stuff.
> >> -Daniel
> >
> > Note that i915's "Broadcast RGB" isn't the same as gma500's: i915 has an
> > "Automatic" option, whereas gma500 does not.
> 
> Adding "Automatic" option that just defaults to "Full" in gma500 does
> not break existing userspace, but allows for extending it to do more
> clever things later.

gma500 could keep it's own property, without the "Automatic" value. We've
done tricks like this for other properties too.

> > Also, radeon has a property called
> > "output_csc" that fulfills a similar purpose. Looking at the code, though, it
> > seems that radeon does not adhere to the standard correctly (or I am missing
> > something).
> >
> > An alternative would be to leave the existing driver-specific properties and
> > change them to be pseudo-aliases for the "RGB quantization range" property.
> > This can be done by letting the drivers read from and write to the new property
> > when user-space tries to read or modify the driver's property. This way we could
> > retain full backwards compatibility for all drivers equally.
> >
> > What do you think?
> 
> I'm obviously biased and predisposed to avoid adding extra complexity to
> i915 when it's not necessary. We'd have *two* connector properties for
> the same thing until the end of time, even if one is an alias for the
> other.

Yeah just pick one, and implement the others as aliases. Drivers can do
the aliases in their atomic_get/set_property functions pretty easily,
atomic properties aren't stored anywhere else than decoded (which was done
partially to make stuff like this work).

Coming up a new property name just so that everyone suffers equally feels
silly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Yussuf Khalil]

On Wed, 2020-04-15 at 13:13 +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> > On Tue, 14 Apr 2020, Yussuf Khalil <dev@pp3345.net> wrote:
> > > On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> > > > On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> > > > > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com
> > > > > >
> > > > > wrote:
> > > > > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> > > > > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > > > > dev@pp3345.net> wrote:
> > > > > > > 
> > > > > > > > DRM now has a globally available "RGB quantization
> > > > > > > > range"
> > > > > > > > connector
> > > > > > > > property. i915's "Broadcast RGB" that fulfils the same
> > > > > > > > purpose is now
> > > > > > > > considered deprecated, so drop it in favor of the DRM
> > > > > > > > property.
> > > > > > > 
> > > > > > > For a UAPI point-of-view, I'm not sure this is fine. Some
> > > > > > > user-
> > > > > > > space
> > > > > > > might depend on this property, dropping it would break
> > > > > > > such
> > > > > > > user-space.
> > > > > > 
> > > > > > Agreed.
> > > > > > 
> > > > > > > Can we make this property deprecated but still keep it
> > > > > > > for
> > > > > > > backwards
> > > > > > > compatibility?
> > > > > > 
> > > > > > Would be nice to make the i915 specific property an "alias"
> > > > > > for
> > > > > > the new
> > > > > > property, however I'm not sure how you'd make that happen.
> > > > > > Otherwise
> > > > > > juggling between the two properties is going to be a
> > > > > > nightmare.
> > > > > 
> > > > > Ah, the obvious easy choice is to use the property and enum
> > > > > names
> > > > > already being used by i915 and gma500, and you have no
> > > > > problem.
> > > > > Perhaps
> > > > > they're not the names you'd like, but then looking at the
> > > > > total
> > > > > lack of
> > > > > consistency across property naming makes them fit right in.
> > > > > ;)
> > > > 
> > > > Yeah if we don't have contradictory usage across drivers when
> > > > modernizing
> > > > these properties, then let's just stick with the names already
> > > > there.
> > > > It's
> > > > not pretty, but works better since more userspace/internet
> > > > howtos
> > > > know how
> > > > to use this stuff.
> > > > -Daniel
> > > 
> > > Note that i915's "Broadcast RGB" isn't the same as gma500's: i915
> > > has an
> > > "Automatic" option, whereas gma500 does not.
> > 
> > Adding "Automatic" option that just defaults to "Full" in gma500
> > does
> > not break existing userspace, but allows for extending it to do
> > more
> > clever things later.
> 
> gma500 could keep it's own property, without the "Automatic" value.
> We've
> done tricks like this for other properties too.
> 
> > > Also, radeon has a property called
> > > "output_csc" that fulfills a similar purpose. Looking at the
> > > code, though, it
> > > seems that radeon does not adhere to the standard correctly (or I
> > > am missing
> > > something).
> > > 
> > > An alternative would be to leave the existing driver-specific
> > > properties and
> > > change them to be pseudo-aliases for the "RGB quantization range"
> > > property.
> > > This can be done by letting the drivers read from and write to
> > > the new property
> > > when user-space tries to read or modify the driver's property.
> > > This way we could
> > > retain full backwards compatibility for all drivers equally.
> > > 
> > > What do you think?
> > 
> > I'm obviously biased and predisposed to avoid adding extra
> > complexity to
> > i915 when it's not necessary. We'd have *two* connector properties
> > for
> > the same thing until the end of time, even if one is an alias for
> > the
> > other.
> 
> Yeah just pick one, and implement the others as aliases. Drivers can
> do
> the aliases in their atomic_get/set_property functions pretty easily,
> atomic properties aren't stored anywhere else than decoded (which was
> done
> partially to make stuff like this work).
> 
> Coming up a new property name just so that everyone suffers equally
> feels
> silly.
> -Daniel

Okay, I understand your point. Leaving gma500 without a proper implementation of
the "Automatic" value isn't an option though as that would break the whole
purpose of this patchset: having a property that works precisely the same way
across all drivers. I'll build a patch that implements standards-compliant
behavior for gma500 then, so that it can use the same property as the others.

Regards
Yussuf

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Yussuf Khalil]

On Wed, 2020-04-15 at 13:13 +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> > On Tue, 14 Apr 2020, Yussuf Khalil <dev@pp3345.net> wrote:
> > > On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> > > > On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> > > > > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com
> > > > > >
> > > > > wrote:
> > > > > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> > > > > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > > > > dev@pp3345.net> wrote:
> > > > > > > 
> > > > > > > > DRM now has a globally available "RGB quantization
> > > > > > > > range"
> > > > > > > > connector
> > > > > > > > property. i915's "Broadcast RGB" that fulfils the same
> > > > > > > > purpose is now
> > > > > > > > considered deprecated, so drop it in favor of the DRM
> > > > > > > > property.
> > > > > > > 
> > > > > > > For a UAPI point-of-view, I'm not sure this is fine. Some
> > > > > > > user-
> > > > > > > space
> > > > > > > might depend on this property, dropping it would break
> > > > > > > such
> > > > > > > user-space.
> > > > > > 
> > > > > > Agreed.
> > > > > > 
> > > > > > > Can we make this property deprecated but still keep it
> > > > > > > for
> > > > > > > backwards
> > > > > > > compatibility?
> > > > > > 
> > > > > > Would be nice to make the i915 specific property an "alias"
> > > > > > for
> > > > > > the new
> > > > > > property, however I'm not sure how you'd make that happen.
> > > > > > Otherwise
> > > > > > juggling between the two properties is going to be a
> > > > > > nightmare.
> > > > > 
> > > > > Ah, the obvious easy choice is to use the property and enum
> > > > > names
> > > > > already being used by i915 and gma500, and you have no
> > > > > problem.
> > > > > Perhaps
> > > > > they're not the names you'd like, but then looking at the
> > > > > total
> > > > > lack of
> > > > > consistency across property naming makes them fit right in.
> > > > > ;)
> > > > 
> > > > Yeah if we don't have contradictory usage across drivers when
> > > > modernizing
> > > > these properties, then let's just stick with the names already
> > > > there.
> > > > It's
> > > > not pretty, but works better since more userspace/internet
> > > > howtos
> > > > know how
> > > > to use this stuff.
> > > > -Daniel
> > > 
> > > Note that i915's "Broadcast RGB" isn't the same as gma500's: i915
> > > has an
> > > "Automatic" option, whereas gma500 does not.
> > 
> > Adding "Automatic" option that just defaults to "Full" in gma500
> > does
> > not break existing userspace, but allows for extending it to do
> > more
> > clever things later.
> 
> gma500 could keep it's own property, without the "Automatic" value.
> We've
> done tricks like this for other properties too.
> 
> > > Also, radeon has a property called
> > > "output_csc" that fulfills a similar purpose. Looking at the
> > > code, though, it
> > > seems that radeon does not adhere to the standard correctly (or I
> > > am missing
> > > something).
> > > 
> > > An alternative would be to leave the existing driver-specific
> > > properties and
> > > change them to be pseudo-aliases for the "RGB quantization range"
> > > property.
> > > This can be done by letting the drivers read from and write to
> > > the new property
> > > when user-space tries to read or modify the driver's property.
> > > This way we could
> > > retain full backwards compatibility for all drivers equally.
> > > 
> > > What do you think?
> > 
> > I'm obviously biased and predisposed to avoid adding extra
> > complexity to
> > i915 when it's not necessary. We'd have *two* connector properties
> > for
> > the same thing until the end of time, even if one is an alias for
> > the
> > other.
> 
> Yeah just pick one, and implement the others as aliases. Drivers can
> do
> the aliases in their atomic_get/set_property functions pretty easily,
> atomic properties aren't stored anywhere else than decoded (which was
> done
> partially to make stuff like this work).
> 
> Coming up a new property name just so that everyone suffers equally
> feels
> silly.
> -Daniel

Okay, I understand your point. Leaving gma500 without a proper implementation of
the "Automatic" value isn't an option though as that would break the whole
purpose of this patchset: having a property that works precisely the same way
across all drivers. I'll build a patch that implements standards-compliant
behavior for gma500 then, so that it can use the same property as the others.

Regards
Yussuf

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Daniel Vetter]

On Thu, Apr 16, 2020 at 03:44:53PM +0200, Yussuf Khalil wrote:
> On Wed, 2020-04-15 at 13:13 +0200, Daniel Vetter wrote:
> > On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> > > On Tue, 14 Apr 2020, Yussuf Khalil <dev@pp3345.net> wrote:
> > > > On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> > > > > On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> > > > > > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com
> > > > > > >
> > > > > > wrote:
> > > > > > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> > > > > > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > > > > > dev@pp3345.net> wrote:
> > > > > > > > 
> > > > > > > > > DRM now has a globally available "RGB quantization
> > > > > > > > > range"
> > > > > > > > > connector
> > > > > > > > > property. i915's "Broadcast RGB" that fulfils the same
> > > > > > > > > purpose is now
> > > > > > > > > considered deprecated, so drop it in favor of the DRM
> > > > > > > > > property.
> > > > > > > > 
> > > > > > > > For a UAPI point-of-view, I'm not sure this is fine. Some
> > > > > > > > user-
> > > > > > > > space
> > > > > > > > might depend on this property, dropping it would break
> > > > > > > > such
> > > > > > > > user-space.
> > > > > > > 
> > > > > > > Agreed.
> > > > > > > 
> > > > > > > > Can we make this property deprecated but still keep it
> > > > > > > > for
> > > > > > > > backwards
> > > > > > > > compatibility?
> > > > > > > 
> > > > > > > Would be nice to make the i915 specific property an "alias"
> > > > > > > for
> > > > > > > the new
> > > > > > > property, however I'm not sure how you'd make that happen.
> > > > > > > Otherwise
> > > > > > > juggling between the two properties is going to be a
> > > > > > > nightmare.
> > > > > > 
> > > > > > Ah, the obvious easy choice is to use the property and enum
> > > > > > names
> > > > > > already being used by i915 and gma500, and you have no
> > > > > > problem.
> > > > > > Perhaps
> > > > > > they're not the names you'd like, but then looking at the
> > > > > > total
> > > > > > lack of
> > > > > > consistency across property naming makes them fit right in.
> > > > > > ;)
> > > > > 
> > > > > Yeah if we don't have contradictory usage across drivers when
> > > > > modernizing
> > > > > these properties, then let's just stick with the names already
> > > > > there.
> > > > > It's
> > > > > not pretty, but works better since more userspace/internet
> > > > > howtos
> > > > > know how
> > > > > to use this stuff.
> > > > > -Daniel
> > > > 
> > > > Note that i915's "Broadcast RGB" isn't the same as gma500's: i915
> > > > has an
> > > > "Automatic" option, whereas gma500 does not.
> > > 
> > > Adding "Automatic" option that just defaults to "Full" in gma500
> > > does
> > > not break existing userspace, but allows for extending it to do
> > > more
> > > clever things later.
> > 
> > gma500 could keep it's own property, without the "Automatic" value.
> > We've
> > done tricks like this for other properties too.
> > 
> > > > Also, radeon has a property called
> > > > "output_csc" that fulfills a similar purpose. Looking at the
> > > > code, though, it
> > > > seems that radeon does not adhere to the standard correctly (or I
> > > > am missing
> > > > something).
> > > > 
> > > > An alternative would be to leave the existing driver-specific
> > > > properties and
> > > > change them to be pseudo-aliases for the "RGB quantization range"
> > > > property.
> > > > This can be done by letting the drivers read from and write to
> > > > the new property
> > > > when user-space tries to read or modify the driver's property.
> > > > This way we could
> > > > retain full backwards compatibility for all drivers equally.
> > > > 
> > > > What do you think?
> > > 
> > > I'm obviously biased and predisposed to avoid adding extra
> > > complexity to
> > > i915 when it's not necessary. We'd have *two* connector properties
> > > for
> > > the same thing until the end of time, even if one is an alias for
> > > the
> > > other.
> > 
> > Yeah just pick one, and implement the others as aliases. Drivers can
> > do
> > the aliases in their atomic_get/set_property functions pretty easily,
> > atomic properties aren't stored anywhere else than decoded (which was
> > done
> > partially to make stuff like this work).
> > 
> > Coming up a new property name just so that everyone suffers equally
> > feels
> > silly.
> > -Daniel
> 
> Okay, I understand your point. Leaving gma500 without a proper implementation of
> the "Automatic" value isn't an option though as that would break the whole
> purpose of this patchset: having a property that works precisely the same way
> across all drivers. I'll build a patch that implements standards-compliant
> behavior for gma500 then, so that it can use the same property as the others.

Sounds best. I just generally refrain from volunteering people for work on
very old and abandoned drivers. But if you're willing to type the code,
I'm happy to take it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: Replace "Broadcast RGB" with "RGB quantization range" property

[Daniel Vetter]

On Thu, Apr 16, 2020 at 03:44:53PM +0200, Yussuf Khalil wrote:
> On Wed, 2020-04-15 at 13:13 +0200, Daniel Vetter wrote:
> > On Wed, Apr 15, 2020 at 10:33:25AM +0300, Jani Nikula wrote:
> > > On Tue, 14 Apr 2020, Yussuf Khalil <dev@pp3345.net> wrote:
> > > > On Tue, 2020-04-14 at 14:34 +0200, Daniel Vetter wrote:
> > > > > On Tue, Apr 14, 2020 at 02:21:06PM +0300, Jani Nikula wrote:
> > > > > > On Tue, 14 Apr 2020, Jani Nikula <jani.nikula@linux.intel.com
> > > > > > >
> > > > > > wrote:
> > > > > > > On Mon, 13 Apr 2020, Simon Ser <contact@emersion.fr> wrote:
> > > > > > > > On Monday, April 13, 2020 11:40 PM, Yussuf Khalil <
> > > > > > > > dev@pp3345.net> wrote:
> > > > > > > > 
> > > > > > > > > DRM now has a globally available "RGB quantization
> > > > > > > > > range"
> > > > > > > > > connector
> > > > > > > > > property. i915's "Broadcast RGB" that fulfils the same
> > > > > > > > > purpose is now
> > > > > > > > > considered deprecated, so drop it in favor of the DRM
> > > > > > > > > property.
> > > > > > > > 
> > > > > > > > For a UAPI point-of-view, I'm not sure this is fine. Some
> > > > > > > > user-
> > > > > > > > space
> > > > > > > > might depend on this property, dropping it would break
> > > > > > > > such
> > > > > > > > user-space.
> > > > > > > 
> > > > > > > Agreed.
> > > > > > > 
> > > > > > > > Can we make this property deprecated but still keep it
> > > > > > > > for
> > > > > > > > backwards
> > > > > > > > compatibility?
> > > > > > > 
> > > > > > > Would be nice to make the i915 specific property an "alias"
> > > > > > > for
> > > > > > > the new
> > > > > > > property, however I'm not sure how you'd make that happen.
> > > > > > > Otherwise
> > > > > > > juggling between the two properties is going to be a
> > > > > > > nightmare.
> > > > > > 
> > > > > > Ah, the obvious easy choice is to use the property and enum
> > > > > > names
> > > > > > already being used by i915 and gma500, and you have no
> > > > > > problem.
> > > > > > Perhaps
> > > > > > they're not the names you'd like, but then looking at the
> > > > > > total
> > > > > > lack of
> > > > > > consistency across property naming makes them fit right in.
> > > > > > ;)
> > > > > 
> > > > > Yeah if we don't have contradictory usage across drivers when
> > > > > modernizing
> > > > > these properties, then let's just stick with the names already
> > > > > there.
> > > > > It's
> > > > > not pretty, but works better since more userspace/internet
> > > > > howtos
> > > > > know how
> > > > > to use this stuff.
> > > > > -Daniel
> > > > 
> > > > Note that i915's "Broadcast RGB" isn't the same as gma500's: i915
> > > > has an
> > > > "Automatic" option, whereas gma500 does not.
> > > 
> > > Adding "Automatic" option that just defaults to "Full" in gma500
> > > does
> > > not break existing userspace, but allows for extending it to do
> > > more
> > > clever things later.
> > 
> > gma500 could keep it's own property, without the "Automatic" value.
> > We've
> > done tricks like this for other properties too.
> > 
> > > > Also, radeon has a property called
> > > > "output_csc" that fulfills a similar purpose. Looking at the
> > > > code, though, it
> > > > seems that radeon does not adhere to the standard correctly (or I
> > > > am missing
> > > > something).
> > > > 
> > > > An alternative would be to leave the existing driver-specific
> > > > properties and
> > > > change them to be pseudo-aliases for the "RGB quantization range"
> > > > property.
> > > > This can be done by letting the drivers read from and write to
> > > > the new property
> > > > when user-space tries to read or modify the driver's property.
> > > > This way we could
> > > > retain full backwards compatibility for all drivers equally.
> > > > 
> > > > What do you think?
> > > 
> > > I'm obviously biased and predisposed to avoid adding extra
> > > complexity to
> > > i915 when it's not necessary. We'd have *two* connector properties
> > > for
> > > the same thing until the end of time, even if one is an alias for
> > > the
> > > other.
> > 
> > Yeah just pick one, and implement the others as aliases. Drivers can
> > do
> > the aliases in their atomic_get/set_property functions pretty easily,
> > atomic properties aren't stored anywhere else than decoded (which was
> > done
> > partially to make stuff like this work).
> > 
> > Coming up a new property name just so that everyone suffers equally
> > feels
> > silly.
> > -Daniel
> 
> Okay, I understand your point. Leaving gma500 without a proper implementation of
> the "Automatic" value isn't an option though as that would break the whole
> purpose of this patchset: having a property that works precisely the same way
> across all drivers. I'll build a patch that implements standards-compliant
> behavior for gma500 then, so that it can use the same property as the others.

Sounds best. I just generally refrain from volunteering people for work on
very old and abandoned drivers. But if you're willing to type the code,
I'm happy to take it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel