On Mon, Mar 04, 2019 at 06:32:25PM +0200, Jani Nikula wrote:
> On Mon, 25 Feb 2019, Ville Syrjala <ville.syrj...@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >
> > Make the code less repetitive by extracting a few small helpers.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 68 +++++++++++++++++++++------------
> > 1 file changed, 43 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 48c6bc44072d..b94bf475b04c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1068,16 +1068,42 @@ static void intel_sanitize_options(struct
> > drm_i915_private *dev_priv)
> > intel_gvt_sanitize_options(dev_priv);
> > }
> >
> > -static int skl_get_dimm_ranks(u8 size, u32 rank)
> > +static int skl_get_dimm_size(u16 val)
> > {
> > - if (size == 0)
> > + return val & SKL_DRAM_SIZE_MASK;
> > +}
> > +
> > +static int skl_get_dimm_width(u16 val)
> > +{
> > + if (skl_get_dimm_size(val) == 0)
> > return 0;
> > - if (rank == SKL_DRAM_RANK_SINGLE)
> > - return 1;
> > - else if (rank == SKL_DRAM_RANK_DUAL)
> > - return 2;
> >
> > - return 0;
> > + switch (val & SKL_DRAM_WIDTH_MASK) {
> > + case SKL_DRAM_WIDTH_X8:
> > + case SKL_DRAM_WIDTH_X16:
> > + case SKL_DRAM_WIDTH_X32:
> > + val = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> > + return 8 << val;
> > + default:
> > + MISSING_CASE(val);
> > + return 0;
> > + }
> > +}
> > +
> > +static int skl_get_dimm_ranks(u16 val)
> > +{
> > + if (skl_get_dimm_size(val) == 0)
> > + return 0;
> > +
> > + switch (val & SKL_DRAM_RANK_MASK) {
> > + case SKL_DRAM_RANK_SINGLE:
> > + case SKL_DRAM_RANK_DUAL:
> > + val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> > + return val + 1;
> > + default:
> > + MISSING_CASE(val);
> > + return 0;
> > + }
>
> I don't much care for this dual use of both the macro and then the
> calculation. I'd either just calculate, or return pre-calculated values
> from the cases, not both. The missing cases can also never happen.
I generally lean toward the arithmetic option myself, and I did
consider it here as well. I suppose I ended up being swayed
slightly towards the other end of the spectrum by the potential
documentation value of the case labels. And so I ended up
somewhere in the middle.
>
> But it all checks out, so
>
> Reviewed-by: Jani Nikula <jani.nik...@intel.com>
>
>
> > }
> >
> > static bool
> > @@ -1098,30 +1124,22 @@ skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
> > static int
> > skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
> > {
> > - u32 tmp_l, tmp_s;
> > - u32 s_val = val >> SKL_DRAM_S_SHIFT;
> > + u16 tmp_l, tmp_s;
> >
> > - if (!val)
> > - return -EINVAL;
> > + tmp_l = val & 0xffff;
> > + tmp_s = val >> 16;
> >
> > - tmp_l = val & SKL_DRAM_SIZE_MASK;
> > - tmp_s = s_val & SKL_DRAM_SIZE_MASK;
> > + ch->l_info.size = skl_get_dimm_size(tmp_l);
> > + ch->s_info.size = skl_get_dimm_size(tmp_s);
> >
> > - if (tmp_l == 0 && tmp_s == 0)
> > + if (ch->l_info.size == 0 && ch->s_info.size == 0)
> > return -EINVAL;
> >
> > - ch->l_info.size = tmp_l;
> > - ch->s_info.size = tmp_s;
> > -
> > - tmp_l = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> > - tmp_s = (s_val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> > - ch->l_info.width = (1 << tmp_l) * 8;
> > - ch->s_info.width = (1 << tmp_s) * 8;
> > + ch->l_info.width = skl_get_dimm_width(tmp_l);
> > + ch->s_info.width = skl_get_dimm_width(tmp_s);
> >
> > - tmp_l = val & SKL_DRAM_RANK_MASK;
> > - tmp_s = s_val & SKL_DRAM_RANK_MASK;
> > - ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
> > - ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
> > + ch->l_info.ranks = skl_get_dimm_ranks(tmp_l);
> > + ch->s_info.ranks = skl_get_dimm_ranks(tmp_s);
> >
> > if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
> > ch->ranks = 2;
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
