Re: [PATCH] drm/i915: Add HAS_NATIVE_HDMI2 macro

From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Shashank Sharma <shashank.sharma@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Add HAS_NATIVE_HDMI2 macro
Date: Fri, 5 Jan 2018 09:40:56 -0800	[thread overview]
Message-ID: <20180105174055.jfxe5cqiqgi2t6o7@intel.com> (raw)
In-Reply-To: <1515148992-29989-1-git-send-email-shashank.sharma@intel.com>

On Fri, Jan 05, 2018 at 10:43:12AM +0000, Shashank Sharma wrote:
> GLK/GEN 10 and higher GEN platfomrs sport native HDMI 2.0 controller.
> This patch adds a macro HAS_NATIVE_HDMI2, and uses it to make checks
> in the code more readable.
> 
> Cc: Vivi Rodrigo <rodrigo.vivi@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   | 2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 7 +++----
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index caebd58..9a4b68b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2719,6 +2719,8 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define BSD2_RING	ENGINE_MASK(VCS2)
>  #define ALL_ENGINES	(~0)
>  
> +#define HAS_HDMI2_NATIVE(dev_priv) \
> +	(INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  #define HAS_ENGINE(dev_priv, id) \
>  	(!!((dev_priv)->info.ring_mask & ENGINE_MASK(id)))
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index bced7b9..ee4e11c 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1238,7 +1238,7 @@ static int intel_hdmi_source_max_tmds_clock(struct intel_encoder *encoder)
>  		&dev_priv->vbt.ddi_port_info[encoder->port];
>  	int max_tmds_clock;
>  
> -	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> +	if (HAS_HDMI2_NATIVE(dev_priv))
>  		max_tmds_clock = 594000;

I remember someone, probably Ville, saying that here it should be
a platform check instead of the feature because it is a decision
per platform as documented on our spec.

it is gen>=10 for now, but it can probably change in the future.

So I agree with that view and I believe we need to drop this here.

With this dropped feel free to send next v with:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  	else if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
>  		max_tmds_clock = 300000;
> @@ -1514,8 +1514,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->lane_count = 4;
>  
> -	if (scdc->scrambling.supported && (INTEL_GEN(dev_priv) >= 10 ||
> -					   IS_GEMINILAKE(dev_priv))) {
> +	if (scdc->scrambling.supported && HAS_HDMI2_NATIVE(dev_priv)) {
>  		if (scdc->scrambling.low_rates)
>  			pipe_config->hdmi_scrambling = true;
>  
> @@ -2037,7 +2036,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	connector->doublescan_allowed = 0;
>  	connector->stereo_allowed = 1;
>  
> -	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> +	if (HAS_HDMI2_NATIVE(dev_priv))
>  		connector->ycbcr_420_allowed = true;
>  
>  	intel_hdmi->ddc_bus = intel_hdmi_ddc_pin(dev_priv, port);
> -- 
> 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx