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

[Jani Nikula]

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. ;)


BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center