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

[Navare, Manasi]

On Thu, Dec 03, 2020 at 06:35:29PM +0200, Jani Nikula wrote:
> On Wed, 02 Dec 2020, "Navare, Manasi" <manasi.d.navare at intel.com> wrote:
> > 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:
> >> > > +{
> >> > > +	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.
> 
> It's fine, as long as we don't cast to intel_dp unless we know it's
> intel_dp.
> 
> >> > > +
> >> > > +	/*
> >> > > +	 * 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?
> 
> Odd capitalization, "atleast", "sync/ VRR", maybe also reflow the
> paragraph.
> 
> I know it's nitpicking, but other people will read the comment many,
> many more times than you write it. ;)

Okay yes will remove the odd capitalizations.

Manasi

> 
> 
> BR,
> Jani.
> 
> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center