Re: [RFC 1/2] drm/i915: Add connector property to force bpc

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 19, 2017 at 03:56:37PM +0300, Jani Nikula wrote:
> On Thu, 19 Oct 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Oct 18, 2017 at 03:29:48PM -0700, Sripada, Radhakrishna wrote:
> >> Currently the user space does not have a way to force the bpc. The
> >> default bpc to be programmed is decided by the driver and is run
> >> against connector limitations. Creating a new property for the userspace
> >> to recommend a certain color depth while scanning out the pixels.
> >> 
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> Cc: Manasi Navare <manasi.d.navare@intel.com>
> >> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> >
> > Why not put this onto the pipe directly? Also, what's the userspace
> > use-case here, we need those patches too.
> 
> And rationale in the commit message, please. The "why".

One rationale is cruddy cables/dongles/level shifters/misprogrammed
buf translations/etc. which fail when we try to push 12bpc HDMI through
them. And to work around those we probably want this to be a connector
property so that xrandr will get it automagically. Won't help fbdev of
course, but for that we'd either need a modparam, or maybe some generic
way to expose properties via sysfs or something.

See eg. https://bugs.freedesktop.org/show_bug.cgi?id=91434 for likely 
12bpc issues.

As far as the implementation goes, I'd probably just make it range property
("max bpc" or something like that) with some sane range of values (8-16?,
or maybe we need to go lower?), with the default value being the max.

> 
> BR,
> Jani.
> 
> 
> 
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h     |  7 +++++++
> >>  drivers/gpu/drm/i915/intel_atomic.c |  7 +++++++
> >>  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
> >>  drivers/gpu/drm/i915/intel_hdmi.c   |  1 +
> >>  drivers/gpu/drm/i915/intel_modes.c  | 29 +++++++++++++++++++++++++++++
> >>  5 files changed, 46 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 2783f07c6fc4..e58856f7b08a 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2521,6 +2521,7 @@ struct drm_i915_private {
> >>  
> >>  	struct drm_property *broadcast_rgb_property;
> >>  	struct drm_property *force_audio_property;
> >> +	struct drm_property *force_bpc_property;
> >>  
> >>  	/* hda/i915 audio component */
> >>  	struct i915_audio_component *audio_component;
> >> @@ -2858,6 +2859,12 @@ enum hdmi_force_audio {
> >>  	HDMI_AUDIO_ON,			/* force turn on HDMI audio */
> >>  };
> >>  
> >> +enum connector_force_bpc {
> >> +	INTEL_FORCE_BPC_AUTO,
> >> +	INTEL_FORCE_BPC_8,
> >> +	INTEL_FORCE_BPC_10,
> >> +	INTEL_FORCE_BPC_12,
> >> +};
> >>  #define I915_GTT_OFFSET_NONE ((u32)-1)
> >>  
> >>  /*
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >> index 36d4e635e4ce..10a74669f49a 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -58,6 +58,8 @@ int intel_digital_connector_atomic_get_property(struct drm_connector *connector,
> >>  		*val = intel_conn_state->force_audio;
> >>  	else if (property == dev_priv->broadcast_rgb_property)
> >>  		*val = intel_conn_state->broadcast_rgb;
> >> +	else if (property == dev_priv->force_bpc_property)
> >> +		*val = intel_conn_state->force_bpc;
> >>  	else {
> >>  		DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
> >>  		return -EINVAL;
> >> @@ -95,6 +97,11 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
> >>  		return 0;
> >>  	}
> >>  
> >> +	if (property == dev_priv->force_bpc_property) {
> >> +		intel_conn_state->force_bpc = val;
> >> +		return 0;
> >> +	}
> >> +
> >>  	DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
> >>  	return -EINVAL;
> >>  }
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 77117e188b04..654e76d19514 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -341,6 +341,7 @@ struct intel_digital_connector_state {
> >>  
> >>  	enum hdmi_force_audio force_audio;
> >>  	int broadcast_rgb;
> >> +	enum connector_force_bpc force_bpc;
> >>  };
> >>  
> >>  #define to_intel_digital_connector_state(x) container_of(x, struct intel_digital_connector_state, base)
> >> @@ -1708,6 +1709,7 @@ 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_force_bpc_property(struct drm_connector *connector);
> >>  
> >>  
> >>  /* intel_overlay.c */
> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >> index 50214e342401..a2d6d97cc730 100644
> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >> @@ -1798,6 +1798,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
> >>  	intel_attach_force_audio_property(connector);
> >>  	intel_attach_broadcast_rgb_property(connector);
> >>  	intel_attach_aspect_ratio_property(connector);
> >> +	intel_attach_force_bpc_property(connector);
> >>  	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
> >> index 28a778b785ac..d7be44a951e8 100644
> >> --- a/drivers/gpu/drm/i915/intel_modes.c
> >> +++ b/drivers/gpu/drm/i915/intel_modes.c
> >> @@ -151,3 +151,32 @@ intel_attach_aspect_ratio_property(struct drm_connector *connector)
> >>  			connector->dev->mode_config.aspect_ratio_property,
> >>  			DRM_MODE_PICTURE_ASPECT_NONE);
> >>  }
> >> +
> >> +static const struct drm_prop_enum_list force_bpc_names[] = {
> >> +	{ INTEL_FORCE_BPC_AUTO, "Automatic" },
> >> +	{ INTEL_FORCE_BPC_8, "8 bpc" },
> >> +	{ INTEL_FORCE_BPC_10, "10 bpc" },
> >> +	{ INTEL_FORCE_BPC_12, "12 bpc" },
> >> +};
> >> +
> >> +void
> >> +intel_attach_force_bpc_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->force_bpc_property;
> >> +	if (prop == NULL) {
> >> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> >> +					   "bpc",
> >> +					   force_bpc_names,
> >> +					   ARRAY_SIZE(force_bpc_names));
> >> +		if (prop == NULL)
> >> +			return;
> >> +
> >> +		dev_priv->force_bpc_property = prop;
> >> +	}
> >> +
> >> +	drm_object_attach_property(&connector->base, prop, 0);
> >> +}
> >> -- 
> >> 2.9.3
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx