* Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal [not found] ` <20240610072203.24956-10-mitulkumar.ajitkumar.golani@intel.com> @ 2024-06-19 15:42 ` Nathan Chancellor 2024-06-19 18:10 ` Golani, Mitulkumar Ajitkumar 0 siblings, 1 reply; 6+ messages in thread From: Nathan Chancellor @ 2024-06-19 15:42 UTC (permalink / raw) To: Mitul Golani; +Cc: intel-gfx, ankit.k.nautiyal, intel-xe 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal 2024-06-19 15:42 ` [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal Nathan Chancellor @ 2024-06-19 18:10 ` Golani, Mitulkumar Ajitkumar 2024-06-19 18:26 ` Nathan Chancellor 0 siblings, 1 reply; 6+ messages in thread From: Golani, Mitulkumar Ajitkumar @ 2024-06-19 18:10 UTC (permalink / raw) To: Nathan Chancellor Cc: intel-gfx@lists.freedesktop.org, Nautiyal, Ankit K, intel-xe@lists.freedesktop.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal 2024-06-19 18:10 ` Golani, Mitulkumar Ajitkumar @ 2024-06-19 18:26 ` Nathan Chancellor 2024-06-20 10:05 ` Golani, Mitulkumar Ajitkumar 0 siblings, 1 reply; 6+ messages in thread From: Nathan Chancellor @ 2024-06-19 18:26 UTC (permalink / raw) To: Golani, Mitulkumar Ajitkumar Cc: intel-gfx@lists.freedesktop.org, Nautiyal, Ankit K, intel-xe@lists.freedesktop.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal 2024-06-19 18:26 ` Nathan Chancellor @ 2024-06-20 10:05 ` Golani, Mitulkumar Ajitkumar 2024-06-20 12:59 ` Jani Nikula 0 siblings, 1 reply; 6+ messages in thread From: Golani, Mitulkumar Ajitkumar @ 2024-06-20 10:05 UTC (permalink / raw) To: Nathan Chancellor Cc: intel-gfx@lists.freedesktop.org, Nautiyal, Ankit K, intel-xe@lists.freedesktop.org, Kurmi, Suresh Kumar, Shankar, Uma, Nikula, Jani 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal 2024-06-20 10:05 ` Golani, Mitulkumar Ajitkumar @ 2024-06-20 12:59 ` Jani Nikula 2024-06-20 14:15 ` Nathan Chancellor 0 siblings, 1 reply; 6+ messages in thread From: Jani Nikula @ 2024-06-20 12:59 UTC (permalink / raw) To: Golani, Mitulkumar Ajitkumar, Nathan Chancellor Cc: intel-gfx@lists.freedesktop.org, Nautiyal, Ankit K, intel-xe@lists.freedesktop.org, Kurmi, Suresh Kumar, Shankar, Uma 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 -- Jani Nikula, Intel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal 2024-06-20 12:59 ` Jani Nikula @ 2024-06-20 14:15 ` Nathan Chancellor 0 siblings, 0 replies; 6+ messages in thread From: Nathan Chancellor @ 2024-06-20 14:15 UTC (permalink / raw) To: Jani Nikula Cc: Golani, Mitulkumar Ajitkumar, intel-gfx@lists.freedesktop.org, Nautiyal, Ankit K, intel-xe@lists.freedesktop.org, Kurmi, Suresh Kumar, Shankar, Uma 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-20 14:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240610072203.24956-1-mitulkumar.ajitkumar.golani@intel.com>
[not found] ` <20240610072203.24956-10-mitulkumar.ajitkumar.golani@intel.com>
2024-06-19 15:42 ` [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal Nathan Chancellor
2024-06-19 18:10 ` Golani, Mitulkumar Ajitkumar
2024-06-19 18:26 ` Nathan Chancellor
2024-06-20 10:05 ` Golani, Mitulkumar Ajitkumar
2024-06-20 12:59 ` Jani Nikula
2024-06-20 14:15 ` Nathan Chancellor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox