[PATCH 12/13] drm/i915: Use a plain old int for the cdclk/mdclk ratio
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Apr 2 14:49:25 UTC 2024
On Fri, Mar 29, 2024 at 03:23:34PM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjala (2024-03-27 14:45:43-03:00)
> >From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> >No point in throwing around u8 when we're dealing with
> >just an integer. Use a plain old boring 'int'.
>
> Learned and noted :-)
>
> Thanks for fixing that.
>
> Should we also modify the member mdclk_cdclk_ratio of intel_dbuf_state?
My rule of thumb is to prefer the smallest type when embedded
in structures. Whether that's a good idea I'm not really sure.
I suppose there is always some risk of forgeting to bump up the
type size when needed.
On the other hand we do have some absolutely massive structs
(looking at you intel_crtc_state!) where trying to keep things
small and optimally packed seems like a good idea. For the
dbuf state it probably doesn't make a lick of difference.
The other case is eg. big arrays/lists of structs where I
think generally it's a good idea to minimize the size as there
the overhead is potentially multiplied by the number of elements
Otherwise these just waste unswappable kernel memory.
I suppose one argument for using small types in all structures
is consistency. People tend to cargo cult what they see so
if existing code does it then new code should end up with the
same approach. Though I suppose it might also work against us
and trick people into also using the smaller types for function
arguments and on stack variables as well.
Apart from running out of bits, the main danger with the
smaller types is C's integer promotion and arithmetic
conversion rules. I feel most people don't generally know
how those work. Eg. people see a u8 and assume everything
to do with it is unsigned, and then they might get confused
whether negative values are possible or not as a result of
some artihmetic expression. So the safest type is pretty
much the plain old 'int', and that is in fact what u8 & co.
end up being due to the aforementioned language rules.
tldr I don't really have a great answer here :/
>
> In any case,
>
> Reviewed-by: Gustavo Sousa <gustavo.sousa at intel.com>
Thanks.
>
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 +++---
> > drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++--
> > drivers/gpu/drm/i915/display/skl_watermark.c | 6 ++++--
> > drivers/gpu/drm/i915/display/skl_watermark.h | 6 ++++--
> > 4 files changed, 13 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >index 66c161d7b485..5cba0d08189b 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >@@ -1893,8 +1893,8 @@ static u32 xe2lpd_mdclk_source_sel(struct drm_i915_private *i915)
> > return MDCLK_SOURCE_SEL_CD2XCLK;
> > }
> >
> >-u8 intel_mdclk_cdclk_ratio(struct drm_i915_private *i915,
> >- const struct intel_cdclk_config *cdclk_config)
> >+int intel_mdclk_cdclk_ratio(struct drm_i915_private *i915,
> >+ const struct intel_cdclk_config *cdclk_config)
> > {
> > if (mdclk_source_is_cdclk_pll(i915))
> > return DIV_ROUND_UP(cdclk_config->vco, cdclk_config->cdclk);
> >@@ -3321,7 +3321,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> >
> > if (intel_mdclk_cdclk_ratio(dev_priv, &old_cdclk_state->actual) !=
> > intel_mdclk_cdclk_ratio(dev_priv, &new_cdclk_state->actual)) {
> >- u8 ratio = intel_mdclk_cdclk_ratio(dev_priv, &new_cdclk_state->actual);
> >+ int ratio = intel_mdclk_cdclk_ratio(dev_priv, &new_cdclk_state->actual);
> >
> > ret = intel_dbuf_state_set_mdclk_cdclk_ratio(state, ratio);
> > if (ret)
> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> >index 5d4faf401774..cfdcdec07a4d 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> >@@ -67,8 +67,8 @@ void intel_update_cdclk(struct drm_i915_private *dev_priv);
> > u32 intel_read_rawclk(struct drm_i915_private *dev_priv);
> > bool intel_cdclk_clock_changed(const struct intel_cdclk_config *a,
> > const struct intel_cdclk_config *b);
> >-u8 intel_mdclk_cdclk_ratio(struct drm_i915_private *i915,
> >- const struct intel_cdclk_config *cdclk_config);
> >+int intel_mdclk_cdclk_ratio(struct drm_i915_private *i915,
> >+ const struct intel_cdclk_config *cdclk_config);
> > bool intel_cdclk_is_decreasing_later(struct intel_atomic_state *state);
> > void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
> > void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
> >diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> >index ca0f1f89e6d9..1b48009efe2b 100644
> >--- a/drivers/gpu/drm/i915/display/skl_watermark.c
> >+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> >@@ -3616,7 +3616,8 @@ static void intel_mbus_dbox_update(struct intel_atomic_state *state)
> > }
> > }
> >
> >-int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state, u8 ratio)
> >+int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state,
> >+ int ratio)
> > {
> > struct intel_dbuf_state *dbuf_state;
> >
> >@@ -3629,7 +3630,8 @@ int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state, u8
> > return intel_atomic_lock_global_state(&dbuf_state->base);
> > }
> >
> >-void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio, bool joined_mbus)
> >+void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915,
> >+ int ratio, bool joined_mbus)
> > {
> > enum dbuf_slice slice;
> >
> >diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
> >index 3323a1d973f9..ef1a008466be 100644
> >--- a/drivers/gpu/drm/i915/display/skl_watermark.h
> >+++ b/drivers/gpu/drm/i915/display/skl_watermark.h
> >@@ -74,11 +74,13 @@ intel_atomic_get_dbuf_state(struct intel_atomic_state *state);
> > to_intel_dbuf_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->display.dbuf.obj))
> >
> > int intel_dbuf_init(struct drm_i915_private *i915);
> >-int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state, u8 ratio);
> >+int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state,
> >+ int ratio);
> >
> > void intel_dbuf_pre_plane_update(struct intel_atomic_state *state);
> > void intel_dbuf_post_plane_update(struct intel_atomic_state *state);
> >-void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio, bool joined_mbus);
> >+void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915,
> >+ int ratio, bool joined_mbus);
> > void intel_dbuf_mbus_pre_ddb_update(struct intel_atomic_state *state);
> > void intel_dbuf_mbus_post_ddb_update(struct intel_atomic_state *state);
> >
> >--
> >2.43.2
> >
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list