Re: [Intel-gfx] [PATCH 02/11] drm/i915/display/vrr: Create VRR file and add VRR capability check

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 22 Oct 2020, Manasi Navare <manasi.d.navare@intel.com> wrote:
> We create a new file for all VRR related helpers.
> Also add a function to check vrr capability based on
> platform support, DPCD bits and EDID monitor range.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile            |  1 +
>  drivers/gpu/drm/i915/display/intel_vrr.c | 28 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_vrr.h | 19 ++++++++++++++++
>  3 files changed, 48 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_vrr.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_vrr.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e5574e506a5c..3beeaf517191 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -249,6 +249,7 @@ i915-y += \
>  	display/intel_sdvo.o \
>  	display/intel_tv.o \
>  	display/intel_vdsc.o \
> +	display/intel_vrr.o \
>  	display/vlv_dsi.o \
>  	display/vlv_dsi_pll.o
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> new file mode 100644
> index 000000000000..0c8a91fabb64
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2020 Intel Corporation
> + *
> + * Author: Manasi Navare <manasi.d.navare@intel.com>

I have increasingly mixed feelings about adding author lines in files in
big collaborative projects. They tend to go out of date fairly quickly,
and will cease to represent the contributions fairly. And git already
gives us this information.

> + */
> +
> +#include "i915_drv.h"
> +#include "intel_display_types.h"
> +#include "intel_vrr.h"
> +
> +bool intel_is_vrr_capable(struct drm_connector *connector)

Please prefix with intel_vrr_ consistently.

> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));

I kind of feel like either the function should a) ensure it's okay to do
intel_attached_dp() and return false if not, or b) just use struct
intel_dp as the parameter.

As it is, passing a non-dp connector to this function will fail either
subtly or spectacularly, but not graciously.

> +	const struct drm_display_info *info = &connector->display_info;
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);

Please use i915 over dev_priv in all new code.

> +
> +	/*
> +	 * DP Sink is capable of Variable refresh video timings if
> +	 * Ignore MSA bit is set in DPCD.
> +	 * EDID monitor range also should be atleast 10 for reasonable
> +	 * Adaptive sync/ VRR end user experience.
> +	 */

Please fix typos etc.

> +	return INTEL_GEN(dev_priv) >= 12 &&
> +		drm_dp_sink_can_do_video_without_timing_msa(intel_dp->dpcd) &&
> +		info->monitor_range.max_vfreq - info->monitor_range.min_vfreq > 10;
> +}
> +
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
> new file mode 100644
> index 000000000000..755746c7525c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> +*/
> +
> +#ifndef __INTEL_VRR_H__
> +#define __INTEL_VRR_H__
> +
> +#include <linux/types.h>
> +
> +struct drm_connector;

All of the below declarations are unnecessary at this commit.

BR,
Jani.

> +struct drm_i915_private;
> +struct intel_crtc_state;
> +struct intel_encoder;
> +struct intel_dp;
> +
> +bool intel_is_vrr_capable(struct drm_connector *connector);
> +
> +#endif /* __INTEL_VRR_H__ */

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