On Tue, Nov 10, 2020 at 12:39:08PM +0200, Jani Nikula wrote:
> On Thu, 22 Oct 2020, Manasi Navare <manasi.d.navare@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
