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

["Navare, Manasi" <manasi.d.navare@intel.com>]

On Tue, Dec 01, 2020 at 02:21:56PM -0800, Navare, Manasi wrote:
> 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.

Actually after doing some code rewriting, I think it is best to still pass
the drm connector but I am adding the check now for connector type
Only if its eDP or DP now I get the intel_dp.
Future we can add HDMI there as well.

Manasi

> 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx