diff mbox series

[v16,9/9] drm/i915: Compute CMRR and calculate vtotal

Message ID 20240610072203.24956-10-mitulkumar.ajitkumar.golani@intel.com (mailing list archive)
State New, archived
Headers show
Series Implement CMRR Support | expand

Commit Message

Golani, Mitulkumar Ajitkumar June 10, 2024, 7:22 a.m. UTC 
Compute Fixed Average Vtotal/CMRR with resepect to
userspace VRR enablement. Also calculate required
parameters in case of CMRR is  enabled. During
intel_vrr_compute_config, CMRR is getting enabled
based on userspace has enabled Variable refresh mode
with VRR timing generator or not. Make CMRR as small subset of
FAVT mode, when Panel is running on Fixed refresh rate
and on VRR framework then only enable CMRR to match with
actual refresh rate.

--v2:
- Update is_cmrr_frac_required function return as bool, not int. [Jani]
- Use signed int math instead of unsigned in cmrr_get_vtotal2. [Jani]
- Fix typo and usage of camel case in cmrr_get_vtotal. [Jani]
- Use do_div in cmrr_get_vtotalwhile calculating cmrr_m. [ Jani]
- Simplify cmrr and vrr compute config in intel_vrr_compute_config. [Jani]
- Correct valiable name usage in is_cmrr_frac_required. [Ville]

--v3:
- Removing RFC tag.

--v4:
- Added edp check to address edp usecase for now. (ville)
- Updated is_cmrr_fraction_required to more simplified calculation.
- on longterm goal to be worked upon uapi as suggestion from ville.

--v5:
- Correct vtotal paramas accuracy and add 2 digit precision.
- Avoid using DIV_ROUND_UP and improve scanline precision.

--v6:
- Make CMRR a small subset of FAVT mode.

--v7:
- Update commit message to avoid confusion with Legacy VRR (Ankit).
- Add cmrr.enable in last, so remove from this patch.

--v8:
- Set cmrr.enable in current patch instead of separate patch (Ankit).
- Since vrr.enable and cmrr.enable are not mutually exclusive,
handle accordingly (Ankit).
- is_edp is not required inside is_cmrr_frac_required function (Ankit).
- Add video_mode_required flag for future enhancement.
- Correct cmrr_m/cmrr_n calculation.

--v9:
- Move patch to last and set other bits before computing
cmrr.enable.(Ankit)
- Add TODO: for to address target refresh rate precision as future
enhancement.

Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  |  1 +
 .../drm/i915/display/intel_display_device.h   |  1 +
 drivers/gpu/drm/i915/display/intel_vrr.c      | 92 ++++++++++++++++---
 3 files changed, 83 insertions(+), 11 deletions(-)

Comments

Nathan Chancellor June 19, 2024, 3:42 p.m. UTC | #1 
Hi Mitul,

On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote:
...
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 4ad99a54aa83..05f67dc9d98d 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -12,6 +12,9 @@
>  #include "intel_vrr_regs.h"
>  #include "intel_dp.h"
>  
> +#define FIXED_POINT_PRECISION		100
> +#define CMRR_PRECISION_TOLERANCE	10
> +
>  bool intel_vrr_is_capable(struct intel_connector *connector)
>  {
>  	const struct drm_display_info *info = &connector->base.display_info;
> @@ -107,6 +110,52 @@ int intel_vrr_vmax_vblank_start(const struct intel_crtc_state *crtc_state)
>  	return crtc_state->vrr.vmax - intel_vrr_vblank_exit_length(crtc_state);
>  }
>  
> +static bool
> +is_cmrr_frac_required(struct intel_crtc_state *crtc_state)
> +{
> +	int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> +	if (!HAS_CMRR(i915))
> +		return false;
> +
> +	actual_refresh_k =
> +		drm_mode_vrefresh(adjusted_mode) * FIXED_POINT_PRECISION;
> +	pixel_clock_per_line =
> +		adjusted_mode->crtc_clock * 1000 / adjusted_mode->crtc_htotal;
> +	calculated_refresh_k =
> +		pixel_clock_per_line * FIXED_POINT_PRECISION / adjusted_mode->crtc_vtotal;
> +
> +	if ((actual_refresh_k - calculated_refresh_k) < CMRR_PRECISION_TOLERANCE)
> +		return false;
> +
> +	return true;
> +}
> +
> +static unsigned int
> +cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool video_mode_required)
> +{
> +	int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
> +	long long adjusted_pixel_rate;
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> +
> +	if (video_mode_required) {
> +		multiplier_m = 1001;
> +		multiplier_n = 1000;
> +	}
> +
> +	crtc_state->cmrr.cmrr_n =
> +		desired_refresh_rate * adjusted_mode->crtc_htotal * multiplier_n;
> +	vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) / crtc_state->cmrr.cmrr_n;
> +	adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 * multiplier_m;
> +	crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state->cmrr.cmrr_n);
> +
> +	return vtotal;
> +}

This change is now in -next as commit 1676ecd303ac ("drm/i915: Compute
CMRR and calculate vtotal"), where it breaks the xe build for 32-bit
platforms with:

  $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- allmodconfig drivers/gpu/drm/xe/i915-display/intel_vrr.o
  In file included from arch/arm/include/asm/div64.h:107,
                   from include/linux/math.h:6,
                   from include/linux/kernel.h:27,
                   from include/linux/cpumask.h:11,
                   from include/linux/smp.h:13,
                   from include/linux/lockdep.h:14,
                   from include/linux/spinlock.h:63,
                   from include/linux/kref.h:16,
                   from include/drm/drm_device.h:5,
                   from include/drm/drm_drv.h:35,
                   from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:13,
                   from drivers/gpu/drm/i915/display/intel_vrr.c:7:
  drivers/gpu/drm/i915/display/intel_vrr.c: In function 'cmrr_get_vtotal':
  include/asm-generic/div64.h:222:35: error: comparison of distinct pointer types lacks a cast [-Werror]
    222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
        |                                   ^~
  drivers/gpu/drm/i915/display/intel_vrr.c:155:35: note: in expansion of macro 'do_div'
    155 |         crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state->cmrr.cmrr_n);
        |                                   ^~~~~~
  cc1: all warnings being treated as errors

Also, is do_div() correct here? It is different from the other div_()
macros in that the "return value" is the remainder, not the result of
the division.

Cheers,
Nathan
Golani, Mitulkumar Ajitkumar June 19, 2024, 6:10 p.m. UTC | #2 
Hi @Nathan Chancellor

Probably fix is merged in drm-intel-next
related patch:  https://patchwork.freedesktop.org/series/134860/

Can you please check and suggest if this patch is merged ?

Thanks,
Mitul
> -----Original Message-----
> From: Nathan Chancellor <nathan@kernel.org>
> Sent: Wednesday, June 19, 2024 9:12 PM
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com>; intel-xe@lists.freedesktop.org
> Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal
> 
> Hi Mitul,
> 
> On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote:
> ...
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 4ad99a54aa83..05f67dc9d98d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -12,6 +12,9 @@
> >  #include "intel_vrr_regs.h"
> >  #include "intel_dp.h"
> >
> > +#define FIXED_POINT_PRECISION		100
> > +#define CMRR_PRECISION_TOLERANCE	10
> > +
> >  bool intel_vrr_is_capable(struct intel_connector *connector)  {
> >  	const struct drm_display_info *info = &connector->base.display_info;
> > @@ -107,6 +110,52 @@ int intel_vrr_vmax_vblank_start(const struct
> intel_crtc_state *crtc_state)
> >  	return crtc_state->vrr.vmax -
> > intel_vrr_vblank_exit_length(crtc_state);
> >  }
> >
> > +static bool
> > +is_cmrr_frac_required(struct intel_crtc_state *crtc_state) {
> > +	int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
> > +	struct drm_display_mode *adjusted_mode = &crtc_state-
> >hw.adjusted_mode;
> > +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > +
> > +	if (!HAS_CMRR(i915))
> > +		return false;
> > +
> > +	actual_refresh_k =
> > +		drm_mode_vrefresh(adjusted_mode) *
> FIXED_POINT_PRECISION;
> > +	pixel_clock_per_line =
> > +		adjusted_mode->crtc_clock * 1000 / adjusted_mode-
> >crtc_htotal;
> > +	calculated_refresh_k =
> > +		pixel_clock_per_line * FIXED_POINT_PRECISION /
> > +adjusted_mode->crtc_vtotal;
> > +
> > +	if ((actual_refresh_k - calculated_refresh_k) <
> CMRR_PRECISION_TOLERANCE)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static unsigned int
> > +cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool
> > +video_mode_required) {
> > +	int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
> > +	long long adjusted_pixel_rate;
> > +	struct drm_display_mode *adjusted_mode =
> > +&crtc_state->hw.adjusted_mode;
> > +
> > +	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> > +
> > +	if (video_mode_required) {
> > +		multiplier_m = 1001;
> > +		multiplier_n = 1000;
> > +	}
> > +
> > +	crtc_state->cmrr.cmrr_n =
> > +		desired_refresh_rate * adjusted_mode->crtc_htotal *
> multiplier_n;
> > +	vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) /
> crtc_state->cmrr.cmrr_n;
> > +	adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 *
> multiplier_m;
> > +	crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate,
> > +crtc_state->cmrr.cmrr_n);
> > +
> > +	return vtotal;
> > +}
> 
> This change is now in -next as commit 1676ecd303ac ("drm/i915: Compute
> CMRR and calculate vtotal"), where it breaks the xe build for 32-bit platforms
> with:
> 
>   $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi-
> allmodconfig drivers/gpu/drm/xe/i915-display/intel_vrr.o
>   In file included from arch/arm/include/asm/div64.h:107,
>                    from include/linux/math.h:6,
>                    from include/linux/kernel.h:27,
>                    from include/linux/cpumask.h:11,
>                    from include/linux/smp.h:13,
>                    from include/linux/lockdep.h:14,
>                    from include/linux/spinlock.h:63,
>                    from include/linux/kref.h:16,
>                    from include/drm/drm_device.h:5,
>                    from include/drm/drm_drv.h:35,
>                    from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:13,
>                    from drivers/gpu/drm/i915/display/intel_vrr.c:7:
>   drivers/gpu/drm/i915/display/intel_vrr.c: In function 'cmrr_get_vtotal':
>   include/asm-generic/div64.h:222:35: error: comparison of distinct pointer
> types lacks a cast [-Werror]
>     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
>         |                                   ^~
>   drivers/gpu/drm/i915/display/intel_vrr.c:155:35: note: in expansion of macro
> 'do_div'
>     155 |         crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state-
> >cmrr.cmrr_n);
>         |                                   ^~~~~~
>   cc1: all warnings being treated as errors
> 
> Also, is do_div() correct here? It is different from the other div_() macros in that
> the "return value" is the remainder, not the result of the division.
> 
> Cheers,
> Nathan
Nathan Chancellor June 19, 2024, 6:26 p.m. UTC | #3 
On Wed, Jun 19, 2024 at 06:10:34PM +0000, Golani, Mitulkumar Ajitkumar wrote:
> Hi @Nathan Chancellor
> 
> Probably fix is merged in drm-intel-next
> related patch:  https://patchwork.freedesktop.org/series/134860/
> 
> Can you please check and suggest if this patch is merged ?

This is still reproducible at commit 851de367dede ("drm/i915: Enable
plane/pipeDMC ATS fault interrupts on mtl") in drm-intel-next, which
includes that change as commit e2dc7cb72b25 ("drm/i915/display: Update
calculation to avoid overflow"). The issue is the dividend in do_div()
is required to be an unsigned 64-bit type but you used a signed type.
Updating adjusted_pixel_rate to be a u64 should resolve the issue and
match the return type of mul_u32_u32(). I just wasn't sure if that was
the only fix this code would need, as do_div() is not typically used
with an assignment.

Cheers,
Nathan

> > -----Original Message-----
> > From: Nathan Chancellor <nathan@kernel.org>
> > Sent: Wednesday, June 19, 2024 9:12 PM
> > To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K
> > <ankit.k.nautiyal@intel.com>; intel-xe@lists.freedesktop.org
> > Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal
> > 
> > Hi Mitul,
> > 
> > On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote:
> > ...
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > index 4ad99a54aa83..05f67dc9d98d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > @@ -12,6 +12,9 @@
> > >  #include "intel_vrr_regs.h"
> > >  #include "intel_dp.h"
> > >
> > > +#define FIXED_POINT_PRECISION		100
> > > +#define CMRR_PRECISION_TOLERANCE	10
> > > +
> > >  bool intel_vrr_is_capable(struct intel_connector *connector)  {
> > >  	const struct drm_display_info *info = &connector->base.display_info;
> > > @@ -107,6 +110,52 @@ int intel_vrr_vmax_vblank_start(const struct
> > intel_crtc_state *crtc_state)
> > >  	return crtc_state->vrr.vmax -
> > > intel_vrr_vblank_exit_length(crtc_state);
> > >  }
> > >
> > > +static bool
> > > +is_cmrr_frac_required(struct intel_crtc_state *crtc_state) {
> > > +	int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
> > > +	struct drm_display_mode *adjusted_mode = &crtc_state-
> > >hw.adjusted_mode;
> > > +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > > +
> > > +	if (!HAS_CMRR(i915))
> > > +		return false;
> > > +
> > > +	actual_refresh_k =
> > > +		drm_mode_vrefresh(adjusted_mode) *
> > FIXED_POINT_PRECISION;
> > > +	pixel_clock_per_line =
> > > +		adjusted_mode->crtc_clock * 1000 / adjusted_mode-
> > >crtc_htotal;
> > > +	calculated_refresh_k =
> > > +		pixel_clock_per_line * FIXED_POINT_PRECISION /
> > > +adjusted_mode->crtc_vtotal;
> > > +
> > > +	if ((actual_refresh_k - calculated_refresh_k) <
> > CMRR_PRECISION_TOLERANCE)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static unsigned int
> > > +cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool
> > > +video_mode_required) {
> > > +	int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
> > > +	long long adjusted_pixel_rate;
> > > +	struct drm_display_mode *adjusted_mode =
> > > +&crtc_state->hw.adjusted_mode;
> > > +
> > > +	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> > > +
> > > +	if (video_mode_required) {
> > > +		multiplier_m = 1001;
> > > +		multiplier_n = 1000;
> > > +	}
> > > +
> > > +	crtc_state->cmrr.cmrr_n =
> > > +		desired_refresh_rate * adjusted_mode->crtc_htotal *
> > multiplier_n;
> > > +	vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) /
> > crtc_state->cmrr.cmrr_n;
> > > +	adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 *
> > multiplier_m;
> > > +	crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate,
> > > +crtc_state->cmrr.cmrr_n);
> > > +
> > > +	return vtotal;
> > > +}
> > 
> > This change is now in -next as commit 1676ecd303ac ("drm/i915: Compute
> > CMRR and calculate vtotal"), where it breaks the xe build for 32-bit platforms
> > with:
> > 
> >   $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi-
> > allmodconfig drivers/gpu/drm/xe/i915-display/intel_vrr.o
> >   In file included from arch/arm/include/asm/div64.h:107,
> >                    from include/linux/math.h:6,
> >                    from include/linux/kernel.h:27,
> >                    from include/linux/cpumask.h:11,
> >                    from include/linux/smp.h:13,
> >                    from include/linux/lockdep.h:14,
> >                    from include/linux/spinlock.h:63,
> >                    from include/linux/kref.h:16,
> >                    from include/drm/drm_device.h:5,
> >                    from include/drm/drm_drv.h:35,
> >                    from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:13,
> >                    from drivers/gpu/drm/i915/display/intel_vrr.c:7:
> >   drivers/gpu/drm/i915/display/intel_vrr.c: In function 'cmrr_get_vtotal':
> >   include/asm-generic/div64.h:222:35: error: comparison of distinct pointer
> > types lacks a cast [-Werror]
> >     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
> >         |                                   ^~
> >   drivers/gpu/drm/i915/display/intel_vrr.c:155:35: note: in expansion of macro
> > 'do_div'
> >     155 |         crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state-
> > >cmrr.cmrr_n);
> >         |                                   ^~~~~~
> >   cc1: all warnings being treated as errors
> > 
> > Also, is do_div() correct here? It is different from the other div_() macros in that
> > the "return value" is the remainder, not the result of the division.
> > 
> > Cheers,
> > Nathan
Golani, Mitulkumar Ajitkumar June 20, 2024, 10:05 a.m. UTC | #4 
Hi @Nathan Chancellor,

Yes, with do_div, we are expecting the remainder value. Regarding the warning related to the adjusted_pixel_rate type cast, I haven't been able to reproduce this locally, possibly due to differences in the cross-compiler. We should consider typecasting adjusted_pixel_rate or treating it as unsigned ?

Adding @Nikula, Jani to suggest.

Regards,
Mitul
> -----Original Message-----
> From: Nathan Chancellor <nathan@kernel.org>
> Sent: Wednesday, June 19, 2024 11:56 PM
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com>; intel-xe@lists.freedesktop.org
> Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal
> 
> On Wed, Jun 19, 2024 at 06:10:34PM +0000, Golani, Mitulkumar Ajitkumar
> wrote:
> > Hi @Nathan Chancellor
> >
> > Probably fix is merged in drm-intel-next related patch:
> > https://patchwork.freedesktop.org/series/134860/
> >
> > Can you please check and suggest if this patch is merged ?
> 
> This is still reproducible at commit 851de367dede ("drm/i915: Enable
> plane/pipeDMC ATS fault interrupts on mtl") in drm-intel-next, which includes
> that change as commit e2dc7cb72b25 ("drm/i915/display: Update calculation
> to avoid overflow"). The issue is the dividend in do_div() is required to be an
> unsigned 64-bit type but you used a signed type.
> Updating adjusted_pixel_rate to be a u64 should resolve the issue and match
> the return type of mul_u32_u32(). I just wasn't sure if that was the only fix
> this code would need, as do_div() is not typically used with an assignment.
> 
> Cheers,
> Nathan
> 
> > > -----Original Message-----
> > > From: Nathan Chancellor <nathan@kernel.org>
> > > Sent: Wednesday, June 19, 2024 9:12 PM
> > > To: Golani, Mitulkumar Ajitkumar
> > > <mitulkumar.ajitkumar.golani@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K
> > > <ankit.k.nautiyal@intel.com>; intel-xe@lists.freedesktop.org
> > > Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate
> > > vtotal
> > >
> > > Hi Mitul,
> > >
> > > On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote:
> > > ...
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > > index 4ad99a54aa83..05f67dc9d98d 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > > @@ -12,6 +12,9 @@
> > > >  #include "intel_vrr_regs.h"
> > > >  #include "intel_dp.h"
> > > >
> > > > +#define FIXED_POINT_PRECISION		100
> > > > +#define CMRR_PRECISION_TOLERANCE	10
> > > > +
> > > >  bool intel_vrr_is_capable(struct intel_connector *connector)  {
> > > >  	const struct drm_display_info *info =
> > > > &connector->base.display_info; @@ -107,6 +110,52 @@ int
> > > > intel_vrr_vmax_vblank_start(const struct
> > > intel_crtc_state *crtc_state)
> > > >  	return crtc_state->vrr.vmax -
> > > > intel_vrr_vblank_exit_length(crtc_state);
> > > >  }
> > > >
> > > > +static bool
> > > > +is_cmrr_frac_required(struct intel_crtc_state *crtc_state) {
> > > > +	int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
> > > > +	struct drm_display_mode *adjusted_mode = &crtc_state-
> > > >hw.adjusted_mode;
> > > > +	struct drm_i915_private *i915 =
> > > > +to_i915(crtc_state->uapi.crtc->dev);
> > > > +
> > > > +	if (!HAS_CMRR(i915))
> > > > +		return false;
> > > > +
> > > > +	actual_refresh_k =
> > > > +		drm_mode_vrefresh(adjusted_mode) *
> > > FIXED_POINT_PRECISION;
> > > > +	pixel_clock_per_line =
> > > > +		adjusted_mode->crtc_clock * 1000 / adjusted_mode-
> > > >crtc_htotal;
> > > > +	calculated_refresh_k =
> > > > +		pixel_clock_per_line * FIXED_POINT_PRECISION /
> > > > +adjusted_mode->crtc_vtotal;
> > > > +
> > > > +	if ((actual_refresh_k - calculated_refresh_k) <
> > > CMRR_PRECISION_TOLERANCE)
> > > > +		return false;
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static unsigned int
> > > > +cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool
> > > > +video_mode_required) {
> > > > +	int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
> > > > +	long long adjusted_pixel_rate;
> > > > +	struct drm_display_mode *adjusted_mode =
> > > > +&crtc_state->hw.adjusted_mode;
> > > > +
> > > > +	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> > > > +
> > > > +	if (video_mode_required) {
> > > > +		multiplier_m = 1001;
> > > > +		multiplier_n = 1000;
> > > > +	}
> > > > +
> > > > +	crtc_state->cmrr.cmrr_n =
> > > > +		desired_refresh_rate * adjusted_mode->crtc_htotal *
> > > multiplier_n;
> > > > +	vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) /
> > > crtc_state->cmrr.cmrr_n;
> > > > +	adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 *
> > > multiplier_m;
> > > > +	crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate,
> > > > +crtc_state->cmrr.cmrr_n);
> > > > +
> > > > +	return vtotal;
> > > > +}
> > >
> > > This change is now in -next as commit 1676ecd303ac ("drm/i915:
> > > Compute CMRR and calculate vtotal"), where it breaks the xe build
> > > for 32-bit platforms
> > > with:
> > >
> > >   $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi-
> > > allmodconfig drivers/gpu/drm/xe/i915-display/intel_vrr.o
> > >   In file included from arch/arm/include/asm/div64.h:107,
> > >                    from include/linux/math.h:6,
> > >                    from include/linux/kernel.h:27,
> > >                    from include/linux/cpumask.h:11,
> > >                    from include/linux/smp.h:13,
> > >                    from include/linux/lockdep.h:14,
> > >                    from include/linux/spinlock.h:63,
> > >                    from include/linux/kref.h:16,
> > >                    from include/drm/drm_device.h:5,
> > >                    from include/drm/drm_drv.h:35,
> > >                    from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:13,
> > >                    from drivers/gpu/drm/i915/display/intel_vrr.c:7:
> > >   drivers/gpu/drm/i915/display/intel_vrr.c: In function 'cmrr_get_vtotal':
> > >   include/asm-generic/div64.h:222:35: error: comparison of distinct
> > > pointer types lacks a cast [-Werror]
> > >     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
> > >         |                                   ^~
> > >   drivers/gpu/drm/i915/display/intel_vrr.c:155:35: note: in
> > > expansion of macro 'do_div'
> > >     155 |         crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate,
> crtc_state-
> > > >cmrr.cmrr_n);
> > >         |                                   ^~~~~~
> > >   cc1: all warnings being treated as errors
> > >
> > > Also, is do_div() correct here? It is different from the other
> > > div_() macros in that the "return value" is the remainder, not the result of
> the division.
> > >
> > > Cheers,
> > > Nathan
Jani Nikula June 20, 2024, 12:59 p.m. UTC | #5 
On Thu, 20 Jun 2024, "Golani, Mitulkumar Ajitkumar" <mitulkumar.ajitkumar.golani@intel.com> wrote:
> Hi @Nathan Chancellor,
>
> Yes, with do_div, we are expecting the remainder value. Regarding the
> warning related to the adjusted_pixel_rate type cast, I haven't been
> able to reproduce this locally, possibly due to differences in the
> cross-compiler. We should consider typecasting adjusted_pixel_rate or
> treating it as unsigned ?

Please avoid top-posting on the mailing lists.

I'm guessing this will be enough.

diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 6430da25957d..5a0da64c7db3 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -137,7 +137,7 @@ static unsigned int
 cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool video_mode_required)
 {
 	int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
-	long long adjusted_pixel_rate;
+	u64 adjusted_pixel_rate;
 	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
 
 	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);


BR,
Jani.

>
> Adding @Nikula, Jani to suggest.
>
> Regards,
> Mitul
>> -----Original Message-----
>> From: Nathan Chancellor <nathan@kernel.org>
>> Sent: Wednesday, June 19, 2024 11:56 PM
>> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K
>> <ankit.k.nautiyal@intel.com>; intel-xe@lists.freedesktop.org
>> Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal
>>
>> On Wed, Jun 19, 2024 at 06:10:34PM +0000, Golani, Mitulkumar Ajitkumar
>> wrote:
>> > Hi @Nathan Chancellor
>> >
>> > Probably fix is merged in drm-intel-next related patch:
>> > https://patchwork.freedesktop.org/series/134860/
>> >
>> > Can you please check and suggest if this patch is merged ?
>>
>> This is still reproducible at commit 851de367dede ("drm/i915: Enable
>> plane/pipeDMC ATS fault interrupts on mtl") in drm-intel-next, which includes
>> that change as commit e2dc7cb72b25 ("drm/i915/display: Update calculation
>> to avoid overflow"). The issue is the dividend in do_div() is required to be an
>> unsigned 64-bit type but you used a signed type.
>> Updating adjusted_pixel_rate to be a u64 should resolve the issue and match
>> the return type of mul_u32_u32(). I just wasn't sure if that was the only fix
>> this code would need, as do_div() is not typically used with an assignment.
>>
>> Cheers,
>> Nathan
>>
>> > > -----Original Message-----
>> > > From: Nathan Chancellor <nathan@kernel.org>
>> > > Sent: Wednesday, June 19, 2024 9:12 PM
>> > > To: Golani, Mitulkumar Ajitkumar
>> > > <mitulkumar.ajitkumar.golani@intel.com>
>> > > Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K
>> > > <ankit.k.nautiyal@intel.com>; intel-xe@lists.freedesktop.org
>> > > Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate
>> > > vtotal
>> > >
>> > > Hi Mitul,
>> > >
>> > > On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote:
>> > > ...
>> > > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
>> > > > b/drivers/gpu/drm/i915/display/intel_vrr.c
>> > > > index 4ad99a54aa83..05f67dc9d98d 100644
>> > > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> > > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> > > > @@ -12,6 +12,9 @@
>> > > >  #include "intel_vrr_regs.h"
>> > > >  #include "intel_dp.h"
>> > > >
>> > > > +#define FIXED_POINT_PRECISION          100
>> > > > +#define CMRR_PRECISION_TOLERANCE       10
>> > > > +
>> > > >  bool intel_vrr_is_capable(struct intel_connector *connector)  {
>> > > >         const struct drm_display_info *info =
>> > > > &connector->base.display_info; @@ -107,6 +110,52 @@ int
>> > > > intel_vrr_vmax_vblank_start(const struct
>> > > intel_crtc_state *crtc_state)
>> > > >         return crtc_state->vrr.vmax -
>> > > > intel_vrr_vblank_exit_length(crtc_state);
>> > > >  }
>> > > >
>> > > > +static bool
>> > > > +is_cmrr_frac_required(struct intel_crtc_state *crtc_state) {
>> > > > +       int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
>> > > > +       struct drm_display_mode *adjusted_mode = &crtc_state-
>> > > >hw.adjusted_mode;
>> > > > +       struct drm_i915_private *i915 =
>> > > > +to_i915(crtc_state->uapi.crtc->dev);
>> > > > +
>> > > > +       if (!HAS_CMRR(i915))
>> > > > +               return false;
>> > > > +
>> > > > +       actual_refresh_k =
>> > > > +               drm_mode_vrefresh(adjusted_mode) *
>> > > FIXED_POINT_PRECISION;
>> > > > +       pixel_clock_per_line =
>> > > > +               adjusted_mode->crtc_clock * 1000 / adjusted_mode-
>> > > >crtc_htotal;
>> > > > +       calculated_refresh_k =
>> > > > +               pixel_clock_per_line * FIXED_POINT_PRECISION /
>> > > > +adjusted_mode->crtc_vtotal;
>> > > > +
>> > > > +       if ((actual_refresh_k - calculated_refresh_k) <
>> > > CMRR_PRECISION_TOLERANCE)
>> > > > +               return false;
>> > > > +
>> > > > +       return true;
>> > > > +}
>> > > > +
>> > > > +static unsigned int
>> > > > +cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool
>> > > > +video_mode_required) {
>> > > > +       int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
>> > > > +       long long adjusted_pixel_rate;
>> > > > +       struct drm_display_mode *adjusted_mode =
>> > > > +&crtc_state->hw.adjusted_mode;
>> > > > +
>> > > > +       desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
>> > > > +
>> > > > +       if (video_mode_required) {
>> > > > +               multiplier_m = 1001;
>> > > > +               multiplier_n = 1000;
>> > > > +       }
>> > > > +
>> > > > +       crtc_state->cmrr.cmrr_n =
>> > > > +               desired_refresh_rate * adjusted_mode->crtc_htotal *
>> > > multiplier_n;
>> > > > +       vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) /
>> > > crtc_state->cmrr.cmrr_n;
>> > > > +       adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 *
>> > > multiplier_m;
>> > > > +       crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate,
>> > > > +crtc_state->cmrr.cmrr_n);
>> > > > +
>> > > > +       return vtotal;
>> > > > +}
>> > >
>> > > This change is now in -next as commit 1676ecd303ac ("drm/i915:
>> > > Compute CMRR and calculate vtotal"), where it breaks the xe build
>> > > for 32-bit platforms
>> > > with:
>> > >
>> > >   $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi-
>> > > allmodconfig drivers/gpu/drm/xe/i915-display/intel_vrr.o
>> > >   In file included from arch/arm/include/asm/div64.h:107,
>> > >                    from include/linux/math.h:6,
>> > >                    from include/linux/kernel.h:27,
>> > >                    from include/linux/cpumask.h:11,
>> > >                    from include/linux/smp.h:13,
>> > >                    from include/linux/lockdep.h:14,
>> > >                    from include/linux/spinlock.h:63,
>> > >                    from include/linux/kref.h:16,
>> > >                    from include/drm/drm_device.h:5,
>> > >                    from include/drm/drm_drv.h:35,
>> > >                    from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:13,
>> > >                    from drivers/gpu/drm/i915/display/intel_vrr.c:7:
>> > >   drivers/gpu/drm/i915/display/intel_vrr.c: In function 'cmrr_get_vtotal':
>> > >   include/asm-generic/div64.h:222:35: error: comparison of distinct
>> > > pointer types lacks a cast [-Werror]
>> > >     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
>> > >         |                                   ^~
>> > >   drivers/gpu/drm/i915/display/intel_vrr.c:155:35: note: in
>> > > expansion of macro 'do_div'
>> > >     155 |         crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate,
>> crtc_state-
>> > > >cmrr.cmrr_n);
>> > >         |                                   ^~~~~~
>> > >   cc1: all warnings being treated as errors
>> > >
>> > > Also, is do_div() correct here? It is different from the other
>> > > div_() macros in that the "return value" is the remainder, not the result of
>> the division.
>> > >
>> > > Cheers,
>> > > Nathan
Nathan Chancellor June 20, 2024, 2:15 p.m. UTC | #6 
On Thu, Jun 20, 2024 at 03:59:03PM +0300, Jani Nikula wrote:
> On Thu, 20 Jun 2024, "Golani, Mitulkumar Ajitkumar" <mitulkumar.ajitkumar.golani@intel.com> wrote:
> > Hi @Nathan Chancellor,
> >
> > Yes, with do_div, we are expecting the remainder value. Regarding the
> > warning related to the adjusted_pixel_rate type cast, I haven't been
> > able to reproduce this locally, possibly due to differences in the
> > cross-compiler. We should consider typecasting adjusted_pixel_rate or
> > treating it as unsigned ?
> 
> Please avoid top-posting on the mailing lists.
> 
> I'm guessing this will be enough.

Indeed, that works.

> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 6430da25957d..5a0da64c7db3 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -137,7 +137,7 @@ static unsigned int
>  cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool video_mode_required)
>  {
>  	int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
> -	long long adjusted_pixel_rate;
> +	u64 adjusted_pixel_rate;
>  	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>  
>  	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> 
> 
> BR,
> Jani.
> 
> >
> > Adding @Nikula, Jani to suggest.
> >
> > Regards,
> > Mitul
> >> -----Original Message-----
> >> From: Nathan Chancellor <nathan@kernel.org>
> >> Sent: Wednesday, June 19, 2024 11:56 PM
> >> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> >> Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K
> >> <ankit.k.nautiyal@intel.com>; intel-xe@lists.freedesktop.org
> >> Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal
> >>
> >> On Wed, Jun 19, 2024 at 06:10:34PM +0000, Golani, Mitulkumar Ajitkumar
> >> wrote:
> >> > Hi @Nathan Chancellor
> >> >
> >> > Probably fix is merged in drm-intel-next related patch:
> >> > https://patchwork.freedesktop.org/series/134860/
> >> >
> >> > Can you please check and suggest if this patch is merged ?
> >>
> >> This is still reproducible at commit 851de367dede ("drm/i915: Enable
> >> plane/pipeDMC ATS fault interrupts on mtl") in drm-intel-next, which includes
> >> that change as commit e2dc7cb72b25 ("drm/i915/display: Update calculation
> >> to avoid overflow"). The issue is the dividend in do_div() is required to be an
> >> unsigned 64-bit type but you used a signed type.
> >> Updating adjusted_pixel_rate to be a u64 should resolve the issue and match
> >> the return type of mul_u32_u32(). I just wasn't sure if that was the only fix
> >> this code would need, as do_div() is not typically used with an assignment.
> >>
> >> Cheers,
> >> Nathan
> >>
> >> > > -----Original Message-----
> >> > > From: Nathan Chancellor <nathan@kernel.org>
> >> > > Sent: Wednesday, June 19, 2024 9:12 PM
> >> > > To: Golani, Mitulkumar Ajitkumar
> >> > > <mitulkumar.ajitkumar.golani@intel.com>
> >> > > Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K
> >> > > <ankit.k.nautiyal@intel.com>; intel-xe@lists.freedesktop.org
> >> > > Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate
> >> > > vtotal
> >> > >
> >> > > Hi Mitul,
> >> > >
> >> > > On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote:
> >> > > ...
> >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> >> > > > b/drivers/gpu/drm/i915/display/intel_vrr.c
> >> > > > index 4ad99a54aa83..05f67dc9d98d 100644
> >> > > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> >> > > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> >> > > > @@ -12,6 +12,9 @@
> >> > > >  #include "intel_vrr_regs.h"
> >> > > >  #include "intel_dp.h"
> >> > > >
> >> > > > +#define FIXED_POINT_PRECISION          100
> >> > > > +#define CMRR_PRECISION_TOLERANCE       10
> >> > > > +
> >> > > >  bool intel_vrr_is_capable(struct intel_connector *connector)  {
> >> > > >         const struct drm_display_info *info =
> >> > > > &connector->base.display_info; @@ -107,6 +110,52 @@ int
> >> > > > intel_vrr_vmax_vblank_start(const struct
> >> > > intel_crtc_state *crtc_state)
> >> > > >         return crtc_state->vrr.vmax -
> >> > > > intel_vrr_vblank_exit_length(crtc_state);
> >> > > >  }
> >> > > >
> >> > > > +static bool
> >> > > > +is_cmrr_frac_required(struct intel_crtc_state *crtc_state) {
> >> > > > +       int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
> >> > > > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> >> > > >hw.adjusted_mode;
> >> > > > +       struct drm_i915_private *i915 =
> >> > > > +to_i915(crtc_state->uapi.crtc->dev);
> >> > > > +
> >> > > > +       if (!HAS_CMRR(i915))
> >> > > > +               return false;
> >> > > > +
> >> > > > +       actual_refresh_k =
> >> > > > +               drm_mode_vrefresh(adjusted_mode) *
> >> > > FIXED_POINT_PRECISION;
> >> > > > +       pixel_clock_per_line =
> >> > > > +               adjusted_mode->crtc_clock * 1000 / adjusted_mode-
> >> > > >crtc_htotal;
> >> > > > +       calculated_refresh_k =
> >> > > > +               pixel_clock_per_line * FIXED_POINT_PRECISION /
> >> > > > +adjusted_mode->crtc_vtotal;
> >> > > > +
> >> > > > +       if ((actual_refresh_k - calculated_refresh_k) <
> >> > > CMRR_PRECISION_TOLERANCE)
> >> > > > +               return false;
> >> > > > +
> >> > > > +       return true;
> >> > > > +}
> >> > > > +
> >> > > > +static unsigned int
> >> > > > +cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool
> >> > > > +video_mode_required) {
> >> > > > +       int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
> >> > > > +       long long adjusted_pixel_rate;
> >> > > > +       struct drm_display_mode *adjusted_mode =
> >> > > > +&crtc_state->hw.adjusted_mode;
> >> > > > +
> >> > > > +       desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> >> > > > +
> >> > > > +       if (video_mode_required) {
> >> > > > +               multiplier_m = 1001;
> >> > > > +               multiplier_n = 1000;
> >> > > > +       }
> >> > > > +
> >> > > > +       crtc_state->cmrr.cmrr_n =
> >> > > > +               desired_refresh_rate * adjusted_mode->crtc_htotal *
> >> > > multiplier_n;
> >> > > > +       vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) /
> >> > > crtc_state->cmrr.cmrr_n;
> >> > > > +       adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 *
> >> > > multiplier_m;
> >> > > > +       crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate,
> >> > > > +crtc_state->cmrr.cmrr_n);
> >> > > > +
> >> > > > +       return vtotal;
> >> > > > +}
> >> > >
> >> > > This change is now in -next as commit 1676ecd303ac ("drm/i915:
> >> > > Compute CMRR and calculate vtotal"), where it breaks the xe build
> >> > > for 32-bit platforms
> >> > > with:
> >> > >
> >> > >   $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi-
> >> > > allmodconfig drivers/gpu/drm/xe/i915-display/intel_vrr.o
> >> > >   In file included from arch/arm/include/asm/div64.h:107,
> >> > >                    from include/linux/math.h:6,
> >> > >                    from include/linux/kernel.h:27,
> >> > >                    from include/linux/cpumask.h:11,
> >> > >                    from include/linux/smp.h:13,
> >> > >                    from include/linux/lockdep.h:14,
> >> > >                    from include/linux/spinlock.h:63,
> >> > >                    from include/linux/kref.h:16,
> >> > >                    from include/drm/drm_device.h:5,
> >> > >                    from include/drm/drm_drv.h:35,
> >> > >                    from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:13,
> >> > >                    from drivers/gpu/drm/i915/display/intel_vrr.c:7:
> >> > >   drivers/gpu/drm/i915/display/intel_vrr.c: In function 'cmrr_get_vtotal':
> >> > >   include/asm-generic/div64.h:222:35: error: comparison of distinct
> >> > > pointer types lacks a cast [-Werror]
> >> > >     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
> >> > >         |                                   ^~
> >> > >   drivers/gpu/drm/i915/display/intel_vrr.c:155:35: note: in
> >> > > expansion of macro 'do_div'
> >> > >     155 |         crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate,
> >> crtc_state-
> >> > > >cmrr.cmrr_n);
> >> > >         |                                   ^~~~~~
> >> > >   cc1: all warnings being treated as errors
> >> > >
> >> > > Also, is do_div() correct here? It is different from the other
> >> > > div_() macros in that the "return value" is the remainder, not the result of
> >> the division.
> >> > >
> >> > > Cheers,
> >> > > Nathan
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 33f5a3ef2e94..5a91f67a8c9f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5475,6 +5475,7 @@  intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 		PIPE_CONF_CHECK_I(vrr.vsync_end);
 		PIPE_CONF_CHECK_LLI(cmrr.cmrr_m);
 		PIPE_CONF_CHECK_LLI(cmrr.cmrr_n);
+		PIPE_CONF_CHECK_BOOL(cmrr.enable);
 	}
 
 #undef PIPE_CONF_CHECK_X
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 44cda6c3e4d8..13453ea4daea 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -154,6 +154,7 @@  enum intel_display_subplatform {
 					  BIT(trans)) != 0)
 #define HAS_VRR(i915)			(DISPLAY_VER(i915) >= 11)
 #define HAS_AS_SDP(i915)		(DISPLAY_VER(i915) >= 13)
+#define HAS_CMRR(i915)			(DISPLAY_VER(i915) >= 20)
 #define INTEL_NUM_PIPES(i915)		(hweight8(DISPLAY_RUNTIME_INFO(i915)->pipe_mask))
 #define I915_HAS_HOTPLUG(i915)		(DISPLAY_INFO(i915)->has_hotplug)
 #define OVERLAY_NEEDS_PHYSICAL(i915)	(DISPLAY_INFO(i915)->overlay_needs_physical)
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 4ad99a54aa83..05f67dc9d98d 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -12,6 +12,9 @@ 
 #include "intel_vrr_regs.h"
 #include "intel_dp.h"
 
+#define FIXED_POINT_PRECISION		100
+#define CMRR_PRECISION_TOLERANCE	10
+
 bool intel_vrr_is_capable(struct intel_connector *connector)
 {
 	const struct drm_display_info *info = &connector->base.display_info;
@@ -107,6 +110,52 @@  int intel_vrr_vmax_vblank_start(const struct intel_crtc_state *crtc_state)
 	return crtc_state->vrr.vmax - intel_vrr_vblank_exit_length(crtc_state);
 }
 
+static bool
+is_cmrr_frac_required(struct intel_crtc_state *crtc_state)
+{
+	int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
+	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
+	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+
+	if (!HAS_CMRR(i915))
+		return false;
+
+	actual_refresh_k =
+		drm_mode_vrefresh(adjusted_mode) * FIXED_POINT_PRECISION;
+	pixel_clock_per_line =
+		adjusted_mode->crtc_clock * 1000 / adjusted_mode->crtc_htotal;
+	calculated_refresh_k =
+		pixel_clock_per_line * FIXED_POINT_PRECISION / adjusted_mode->crtc_vtotal;
+
+	if ((actual_refresh_k - calculated_refresh_k) < CMRR_PRECISION_TOLERANCE)
+		return false;
+
+	return true;
+}
+
+static unsigned int
+cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool video_mode_required)
+{
+	int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate;
+	long long adjusted_pixel_rate;
+	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
+
+	desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
+
+	if (video_mode_required) {
+		multiplier_m = 1001;
+		multiplier_n = 1000;
+	}
+
+	crtc_state->cmrr.cmrr_n =
+		desired_refresh_rate * adjusted_mode->crtc_htotal * multiplier_n;
+	vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) / crtc_state->cmrr.cmrr_n;
+	adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 * multiplier_m;
+	crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state->cmrr.cmrr_n);
+
+	return vtotal;
+}
+
 void
 intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
 			 struct drm_connector_state *conn_state)
@@ -116,6 +165,7 @@  intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
 	struct intel_connector *connector =
 		to_intel_connector(conn_state->connector);
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	bool is_edp = intel_dp_is_edp(intel_dp);
 	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
 	const struct drm_display_info *info = &connector->base.display_info;
 	int vmin, vmax;
@@ -160,21 +210,26 @@  intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
 	crtc_state->vrr.flipline = crtc_state->vrr.vmin + 1;
 
 	/*
-	 * For XE_LPD+, we use guardband and pipeline override
-	 * is deprecated.
+	 * When panel is VRR capable and userspace has
+	 * not enabled adaptive sync mode then Fixed Average
+	 * Vtotal mode should be enabled.
 	 */
-	if (DISPLAY_VER(i915) >= 13) {
-		crtc_state->vrr.guardband =
-			crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vblank_start;
-	} else {
-		crtc_state->vrr.pipeline_full =
-			min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start -
-			    crtc_state->framestart_delay - 1);
-	}
-
 	if (crtc_state->uapi.vrr_enabled) {
 		crtc_state->vrr.enable = true;
 		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
+	} else if (is_cmrr_frac_required(crtc_state) && is_edp) {
+		crtc_state->vrr.enable = true;
+		crtc_state->cmrr.enable = true;
+		/*
+		 * TODO: Compute precise target refresh rate to determine
+		 * if video_mode_required should be true. Currently set to
+		 * false due to uncertainty about the precise target
+		 * refresh Rate.
+		 */
+		crtc_state->vrr.vmax = cmrr_get_vtotal(crtc_state, false);
+		crtc_state->vrr.vmin = crtc_state->vrr.vmax;
+		crtc_state->vrr.flipline = crtc_state->vrr.vmin;
+		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
 	}
 
 	if (intel_dp_as_sdp_supported(intel_dp)) {
@@ -185,6 +240,19 @@  intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
 			(crtc_state->hw.adjusted_mode.crtc_vtotal -
 			 crtc_state->hw.adjusted_mode.vsync_end);
 	}
+
+	/*
+	 * For XE_LPD+, we use guardband and pipeline override
+	 * is deprecated.
+	 */
+	if (DISPLAY_VER(i915) >= 13) {
+		crtc_state->vrr.guardband =
+			crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vblank_start;
+	} else {
+		crtc_state->vrr.pipeline_full =
+			min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start -
+			    crtc_state->framestart_delay - 1);
+	}
 }
 
 static u32 trans_vrr_ctl(const struct intel_crtc_state *crtc_state)
@@ -324,6 +392,8 @@  void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
 				      TRANS_VRR_CTL(dev_priv, cpu_transcoder));
 
 	crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;
+	if (HAS_CMRR(dev_priv))
+		crtc_state->cmrr.enable = (trans_vrr_ctl & VRR_CTL_CMRR_ENABLE);
 
 	if (crtc_state->cmrr.enable) {
 		crtc_state->cmrr.cmrr_n =