diff mbox series

[v6,12/13] drm/i915: Set Infoframe for non modeset case for HDR

Message ID 1553078906-5991-13-git-send-email-uma.shankar@intel.com (mailing list archive)
State New, archived
Headers show
Series Add HDR Metadata Parsing and handling in DRM layer | expand

Commit Message

Shankar, Uma March 20, 2019, 10:48 a.m. UTC 
HDR metadata requires a infoframe to be set. Due to fastset,
full modeset is not performed hence adding it to update_pipe
to handle that.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Sharma, Shashank March 29, 2019, 12:14 p.m. UTC | #1 
On 3/20/2019 4:18 PM, Uma Shankar wrote:
> HDR metadata requires a infoframe to be set. Due to fastset,
> full modeset is not performed hence adding it to update_pipe
> to handle that.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 69aa0d1..a27aab9 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3566,6 +3566,10 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder,
>   				  const struct intel_crtc_state *crtc_state,
>   				  const struct drm_connector_state *conn_state)
>   {
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *intel_dig_port =
> +			enc_to_dig_port(&encoder->base);
> +
>   	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
>   		intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
>   
> @@ -3575,6 +3579,15 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder,
>   	else if (conn_state->content_protection ==
>   		 DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>   		intel_hdcp_disable(to_intel_connector(conn_state->connector));
> +
> +	/* Set the infoframe for NON modeset cases as well */
> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> +		if ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) &&
> +		    conn_state->hdr_metadata_changed)
> +			intel_dig_port->set_infoframes(encoder,
> +						       crtc_state->has_infoframe,
> +						       crtc_state, conn_state);
> +	}
>   }
>   
Looks good to me,  Please feel free to use:

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>


>   static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder,
Ville Syrjala March 29, 2019, 2:04 p.m. UTC | #2 
On Wed, Mar 20, 2019 at 04:18:25PM +0530, Uma Shankar wrote:
> HDR metadata requires a infoframe to be set. Due to fastset,
> full modeset is not performed hence adding it to update_pipe
> to handle that.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 69aa0d1..a27aab9 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3566,6 +3566,10 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder,
>  				  const struct intel_crtc_state *crtc_state,
>  				  const struct drm_connector_state *conn_state)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *intel_dig_port =
> +			enc_to_dig_port(&encoder->base);
> +
>  	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
>  		intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
>  
> @@ -3575,6 +3579,15 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder,
>  	else if (conn_state->content_protection ==
>  		 DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>  		intel_hdcp_disable(to_intel_connector(conn_state->connector));
> +
> +	/* Set the infoframe for NON modeset cases as well */
> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> +		if ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) &&
> +		    conn_state->hdr_metadata_changed)
> +			intel_dig_port->set_infoframes(encoder,
> +						       crtc_state->has_infoframe,
> +						       crtc_state, conn_state);

I don't think this should be tied to the drm infoframe. It should
be totally generic. IIRC there is also some magic dance we may have
to perform when updating infoframes with the port enabled. But the
details escape right now.

> +	}
>  }
>  
>  static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder,
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Shankar, Uma April 2, 2019, 4:27 p.m. UTC | #3 
>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
>Syrjälä
>Sent: Friday, March 29, 2019 7:35 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Liviu.Dudau@arm.com; intel-
>gfx@lists.freedesktop.org; emil.l.velikov@gmail.com; dri-
>devel@lists.freedesktop.org; Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [v6 12/13] drm/i915: Set Infoframe for non modeset case for HDR
>
>On Wed, Mar 20, 2019 at 04:18:25PM +0530, Uma Shankar wrote:
>> HDR metadata requires a infoframe to be set. Due to fastset, full
>> modeset is not performed hence adding it to update_pipe to handle
>> that.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 69aa0d1..a27aab9 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -3566,6 +3566,10 @@ static void intel_ddi_update_pipe(struct intel_encoder
>*encoder,
>>  				  const struct intel_crtc_state *crtc_state,
>>  				  const struct drm_connector_state *conn_state)  {
>> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +	struct intel_digital_port *intel_dig_port =
>> +			enc_to_dig_port(&encoder->base);
>> +
>>  	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
>>  		intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
>>
>> @@ -3575,6 +3579,15 @@ static void intel_ddi_update_pipe(struct intel_encoder
>*encoder,
>>  	else if (conn_state->content_protection ==
>>  		 DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>>  		intel_hdcp_disable(to_intel_connector(conn_state->connector));
>> +
>> +	/* Set the infoframe for NON modeset cases as well */
>> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>> +		if ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) &&
>> +		    conn_state->hdr_metadata_changed)
>> +			intel_dig_port->set_infoframes(encoder,
>> +						       crtc_state->has_infoframe,
>> +						       crtc_state, conn_state);
>
>I don't think this should be tied to the drm infoframe. It should be totally generic. IIRC
>there is also some magic dance we may have to perform when updating infoframes
>with the port enabled. But the details escape right now.

Hi Ville,
I agree, currently limiting it to DRM Infoframes so that we don't break the legacy
usecase/non HDR cases. We will have to enable this for all kind of infoframes and
validate if things are fine.

Regards,
Uma Shankar

>> +	}
>>  }
>>
>>  static void intel_ddi_set_fia_lane_count(struct intel_encoder
>> *encoder,
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Ville Syrjälä
>Intel
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 69aa0d1..a27aab9 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3566,6 +3566,10 @@  static void intel_ddi_update_pipe(struct intel_encoder *encoder,
 				  const struct intel_crtc_state *crtc_state,
 				  const struct drm_connector_state *conn_state)
 {
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *intel_dig_port =
+			enc_to_dig_port(&encoder->base);
+
 	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
 		intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);
 
@@ -3575,6 +3579,15 @@  static void intel_ddi_update_pipe(struct intel_encoder *encoder,
 	else if (conn_state->content_protection ==
 		 DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
 		intel_hdcp_disable(to_intel_connector(conn_state->connector));
+
+	/* Set the infoframe for NON modeset cases as well */
+	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
+		if ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) &&
+		    conn_state->hdr_metadata_changed)
+			intel_dig_port->set_infoframes(encoder,
+						       crtc_state->has_infoframe,
+						       crtc_state, conn_state);
+	}
 }
 
 static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder,