[PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Check that the sink really declared 12bpc support before we enable it.
This should not actually never happen since it's mandatory for HDMI
sinks to support 12bpc if they support any deep color modes. But
reality disagrees with the theory and there are actually sinks in
the wild that violate the spec.

v2: Fix the output_types check
    Update commit message to state that these things are in fact real

Cc: stable@vger.kernel.org
Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a580de80d2b5..2bf5915284aa 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 
 static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc_state->base.crtc->dev;
+	struct drm_i915_private *dev_priv =
+		to_i915(crtc_state->base.crtc->dev);
+	struct drm_atomic_state *state = crtc_state->base.state;
+	struct drm_connector_state *connector_state;
+	struct drm_connector *connector;
+	int i;
 
-	if (HAS_GMCH_DISPLAY(to_i915(dev)))
+	if (HAS_GMCH_DISPLAY(dev_priv))
 		return false;
 
 	/*
 	 * HDMI 12bpc affects the clocks, so it's only possible
 	 * when not cloning with other encoder types.
 	 */
-	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
+	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
+		return false;
+
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		const struct drm_display_info *info = &connector->display_info;
+
+		if (connector_state->crtc != crtc_state->base.crtc)
+			continue;
+
+		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
+			return false;
+	}
+
+	return true;
 }
 
 bool intel_hdmi_compute_config(struct intel_encoder *encoder,
-- 
2.10.2

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Sharma, Shashank]

Regards

Shashank

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Check that the sink really declared 12bpc support before we enable it.
> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
>
> v2: Fix the output_types check
>      Update commit message to state that these things are in fact real
>
> Cc: stable@vger.kernel.org
> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index a580de80d2b5..2bf5915284aa 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>   
>   static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> -	struct drm_device *dev = crtc_state->base.crtc->dev;
> +	struct drm_i915_private *dev_priv =
> +		to_i915(crtc_state->base.crtc->dev);
> +	struct drm_atomic_state *state = crtc_state->base.state;
> +	struct drm_connector_state *connector_state;
> +	struct drm_connector *connector;
> +	int i;
>   
> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> +	if (HAS_GMCH_DISPLAY(dev_priv))
>   		return false;
>   
>   	/*
>   	 * HDMI 12bpc affects the clocks, so it's only possible
>   	 * when not cloning with other encoder types.
>   	 */
> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> +		return false;
> +
This function is called from only one place ( for now), and that already 
has a pipe_config->has_hdmi_sink check.
Does it makes sense to have only one of the checks ? I can understand 
that this might be to comple
> +	for_each_connector_in_state(state, connector, connector_state, i) {
> +		const struct drm_display_info *info = &connector->display_info;
> +
> +		if (connector_state->crtc != crtc_state->base.crtc)
> +			continue;
> +
> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> +			return false;
Do we want to have a info->bpc check too, or they are more or less same ?
- Shashank
> +	}
> +
> +	return true;
>   }
>   
>   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> --
> 2.10.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Check that the sink really declared 12bpc support before we enable it.
> > This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
> >
> > v2: Fix the output_types check
> >      Update commit message to state that these things are in fact real
> >
> > Cc: stable@vger.kernel.org
> > Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index a580de80d2b5..2bf5915284aa 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> >   
> >   static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> > -	struct drm_device *dev = crtc_state->base.crtc->dev;
> > +	struct drm_i915_private *dev_priv =
> > +		to_i915(crtc_state->base.crtc->dev);
> > +	struct drm_atomic_state *state = crtc_state->base.state;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_connector *connector;
> > +	int i;
> >   
> > -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> > +	if (HAS_GMCH_DISPLAY(dev_priv))
> >   		return false;
> >   
> >   	/*
> >   	 * HDMI 12bpc affects the clocks, so it's only possible
> >   	 * when not cloning with other encoder types.
> >   	 */
> > -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> > +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> > +		return false;
> > +
> This function is called from only one place ( for now), and that already 
> has a pipe_config->has_hdmi_sink check.
> Does it makes sense to have only one of the checks ? I can understand 
> that this might be to comple

has_hdmi_sink is not the same thing. It just says we're talking to at
least one HDMI sink and thus can send infoframes/audio. output_types on 
the other hand lists all the different port types we're cloning with.
So you can do HDMI+VGA for instance and still the HDMI goes to a real
HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
with the 1.5x clock we can't do deep color when cloning.

> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> > +		const struct drm_display_info *info = &connector->display_info;
> > +
> > +		if (connector_state->crtc != crtc_state->base.crtc)
> > +			continue;
> > +
> > +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> > +			return false;
> Do we want to have a info->bpc check too, or they are more or less same ?

That was already checked at the start of the state computation. So
state->pipe_bpp already accounts for that. However as info->bpc is just
the max bpc the sink can do it's not sufficient to guarantee it can doo
all lower bpc values.

> - Shashank
> > +	}
> > +
> > +	return true;
> >   }
> >   
> >   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > --
> > 2.10.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj�l�
Intel OTC

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Sharma, Shashank]

Regards

Shashank


On 3/13/2017 12:53 PM, Ville Syrj�l� wrote:
> On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
>>>
>>> Check that the sink really declared 12bpc support before we enable it.
>>> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
>>>
>>> v2: Fix the output_types check
>>>       Update commit message to state that these things are in fact real
>>>
>>> Cc: stable@vger.kernel.org
>>> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
>>> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
>>>    1 file changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index a580de80d2b5..2bf5915284aa 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>>>    
>>>    static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
>>> -	struct drm_device *dev = crtc_state->base.crtc->dev;
>>> +	struct drm_i915_private *dev_priv =
>>> +		to_i915(crtc_state->base.crtc->dev);
>>> +	struct drm_atomic_state *state = crtc_state->base.state;
>>> +	struct drm_connector_state *connector_state;
>>> +	struct drm_connector *connector;
>>> +	int i;
>>>    
>>> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
>>> +	if (HAS_GMCH_DISPLAY(dev_priv))
>>>    		return false;
>>>    
>>>    	/*
>>>    	 * HDMI 12bpc affects the clocks, so it's only possible
>>>    	 * when not cloning with other encoder types.
>>>    	 */
>>> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
>>> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
>>> +		return false;
>>> +
>> This function is called from only one place ( for now), and that already
>> has a pipe_config->has_hdmi_sink check.
>> Does it makes sense to have only one of the checks ? I can understand
>> that this might be to comple
> has_hdmi_sink is not the same thing. It just says we're talking to at
> least one HDMI sink and thus can send infoframes/audio. output_types on
> the other hand lists all the different port types we're cloning with.
> So you can do HDMI+VGA for instance and still the HDMI goes to a real
> HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
> with the 1.5x clock we can't do deep color when cloning.
Makes sense :-).
>>> +	for_each_connector_in_state(state, connector, connector_state, i) {
>>> +		const struct drm_display_info *info = &connector->display_info;
>>> +
>>> +		if (connector_state->crtc != crtc_state->base.crtc)
>>> +			continue;
>>> +
>>> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
>>> +			return false;
>> Do we want to have a info->bpc check too, or they are more or less same ?
> That was already checked at the start of the state computation. So
> state->pipe_bpp already accounts for that. However as info->bpc is just
> the max bpc the sink can do it's not sufficient to guarantee it can doo
> all lower bpc values.
I guess just in this scenario, the max bpc value(12) and the value of 
our interest is same, so it was making
sense, but I agree, would not be true for all cases like max 16bpc.

Please feel free to use:
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank
>
>> - Shashank
>>> +	}
>>> +
>>> +	return true;
>>>    }
>>>    
>>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>> --
>>> 2.10.2
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 01:09:10PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/13/2017 12:53 PM, Ville Syrj�l� wrote:
> > On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >>>
> >>> Check that the sink really declared 12bpc support before we enable it.
> >>> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
> >>>
> >>> v2: Fix the output_types check
> >>>       Update commit message to state that these things are in fact real
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> >>> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
> >>>    1 file changed, 21 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> index a580de80d2b5..2bf5915284aa 100644
> >>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> >>>    
> >>>    static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> >>> -	struct drm_device *dev = crtc_state->base.crtc->dev;
> >>> +	struct drm_i915_private *dev_priv =
> >>> +		to_i915(crtc_state->base.crtc->dev);
> >>> +	struct drm_atomic_state *state = crtc_state->base.state;
> >>> +	struct drm_connector_state *connector_state;
> >>> +	struct drm_connector *connector;
> >>> +	int i;
> >>>    
> >>> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> >>> +	if (HAS_GMCH_DISPLAY(dev_priv))
> >>>    		return false;
> >>>    
> >>>    	/*
> >>>    	 * HDMI 12bpc affects the clocks, so it's only possible
> >>>    	 * when not cloning with other encoder types.
> >>>    	 */
> >>> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> >>> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> >>> +		return false;
> >>> +
> >> This function is called from only one place ( for now), and that already
> >> has a pipe_config->has_hdmi_sink check.
> >> Does it makes sense to have only one of the checks ? I can understand
> >> that this might be to comple
> > has_hdmi_sink is not the same thing. It just says we're talking to at
> > least one HDMI sink and thus can send infoframes/audio. output_types on
> > the other hand lists all the different port types we're cloning with.
> > So you can do HDMI+VGA for instance and still the HDMI goes to a real
> > HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
> > with the 1.5x clock we can't do deep color when cloning.
> Makes sense :-).
> >>> +	for_each_connector_in_state(state, connector, connector_state, i) {
> >>> +		const struct drm_display_info *info = &connector->display_info;
> >>> +
> >>> +		if (connector_state->crtc != crtc_state->base.crtc)
> >>> +			continue;
> >>> +
> >>> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> >>> +			return false;
> >> Do we want to have a info->bpc check too, or they are more or less same ?
> > That was already checked at the start of the state computation. So
> > state->pipe_bpp already accounts for that. However as info->bpc is just
> > the max bpc the sink can do it's not sufficient to guarantee it can doo
> > all lower bpc values.
> I guess just in this scenario, the max bpc value(12) and the value of 
> our interest is same, so it was making
> sense, but I agree, would not be true for all cases like max 16bpc.
> 
> Please feel free to use:
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

Thanks. Patch pushed to dinq.

> 
> - Shashank
> >
> >> - Shashank
> >>> +	}
> >>> +
> >>> +	return true;
> >>>    }
> >>>    
> >>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>> --
> >>> 2.10.2
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj�l�
Intel OTC