diff mbox series

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

Message ID 20201022222709.29386-3-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series VRR/Adaptive Sync enabling in i915 | expand

Commit Message

Navare, Manasi Oct. 22, 2020, 10:27 p.m. UTC 
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

Comments

Jani Nikula Nov. 10, 2020, 10:39 a.m. UTC | #1 
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.

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

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

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

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

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

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__ */
Ville Syrjälä Nov. 10, 2020, 4:06 p.m. UTC | #2 
On Thu, Oct 22, 2020 at 03:27:00PM -0700, Manasi Navare 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>
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_display_types.h"
> +#include "intel_vrr.h"
> +
> +bool intel_is_vrr_capable(struct drm_connector *connector)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
> +	const struct drm_display_info *info = &connector->display_info;
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +
> +	/*
> +	 * 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.
> +	 */
> +	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;
> +}

A whole new file for vrr seems overkill. There's probably
not going to be much to put in there.

> +
> 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;
> +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__ */
> -- 
> 2.19.1
Navare, Manasi Nov. 10, 2020, 6:48 p.m. UTC | #3 
On Tue, Nov 10, 2020 at 06:06:49PM +0200, Ville Syrjälä wrote:
> On Thu, Oct 22, 2020 at 03:27:00PM -0700, Manasi Navare 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>
> > + */
> > +
> > +#include "i915_drv.h"
> > +#include "intel_display_types.h"
> > +#include "intel_vrr.h"
> > +
> > +bool intel_is_vrr_capable(struct drm_connector *connector)
> > +{
> > +	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
> > +	const struct drm_display_info *info = &connector->display_info;
> > +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	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;
> > +}
> 
> A whole new file for vrr seems overkill. There's probably
> not going to be much to put in there.

There is actually quite a bit of functions and could bemore as we expand this functionality.
So thought it would be cleaner to it here. That way it could also be expanded to
be used on HDMI later.

Manasi

> 
> > +
> > 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;
> > +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__ */
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
Navare, Manasi Dec. 1, 2020, 10:21 p.m. UTC | #4 
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
Navare, Manasi Dec. 2, 2020, 10:40 p.m. UTC | #5 
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
Jani Nikula Dec. 3, 2020, 4:35 p.m. UTC | #6 
On Wed, 02 Dec 2020, "Navare, Manasi" <manasi.d.navare@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.
Navare, Manasi Dec. 3, 2020, 7:38 p.m. UTC | #7 
On Thu, Dec 03, 2020 at 06:35:29PM +0200, Jani Nikula wrote:
> On Wed, 02 Dec 2020, "Navare, Manasi" <manasi.d.navare@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
diff mbox series

Patch

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>
+ */
+
+#include "i915_drv.h"
+#include "intel_display_types.h"
+#include "intel_vrr.h"
+
+bool intel_is_vrr_capable(struct drm_connector *connector)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
+	const struct drm_display_info *info = &connector->display_info;
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+
+	/*
+	 * 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.
+	 */
+	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;
+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__ */