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

From: "Navare, Manasi" <manasi.d.navare@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 02/11] drm/i915/display/vrr: Create VRR file and add VRR capability check
Date: Tue, 1 Dec 2020 14:21:56 -0800	[thread overview]
Message-ID: <20201201222148.GA21862@labuser-Z97X-UD5H> (raw)
In-Reply-To: <87wnytsdgz.fsf@intel.com>

On Tue, Nov 10, 2020 at 12:39:08PM +0200, Jani Nikula wrote:
> 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.

Thanks Jani, yes will remove the author name then.

> 
> > + */
> > +
> > +#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.

Will do, and change this to intel_vrr_is_capable()

> 
> > +{
> > +	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.

Yes I agree here. I think passing intel_dp is a good idea as anyway this function
is currently only called from intel_dp specific functions and after brainstorming a bit
on how this will look for the VRR on native HDMI, it will likely require its own helpers.
So infact I was thinking of even renaming this as intel_vrr_is_capable_dp() and send intel_dp to it
and then intel_vrr_is_capable_hdmi() can be added later.
What do you think?

> 
> > +	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.

Okay

> 
> > +
> > +	/*
> > +	 * 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.

Did I miss some typos, I dont see any in the above comment?

> 
> > +	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.

Yes will add them in the next commits as I add the next functions, got it.

Regards
Manasi

> 
> 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