Re: [PATCH 00/43] drm/i915: Type safe register read/write and a ton of prep work

From: Chris Wilson <chris@chris-wilson.co.uk>
To: ville.syrjala@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 00/43] drm/i915: Type safe register read/write and a ton of prep work
Date: Fri, 18 Sep 2015 19:17:35 +0100	[thread overview]
Message-ID: <20150918181735.GW6442@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1442595836-23981-1-git-send-email-ville.syrjala@linux.intel.com>

On Fri, Sep 18, 2015 at 08:03:13PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Inspired by the recent misplaced parenthesis fix from Damien, I decided to try
> and see what it would take to make our register access type safe. By that mean
> you shouldn't be able to pass in anything by a proper register offset in. After
> a couple of days of hacking, this series is the result.
> 
> I managed to split out all the cleanup stuff upfront, but there is quite a bit
> of it. I did include a few random patches no strictly needed, but as long as I
> was going through the register macros I tried to fix whatever was wrong.
> 
> The actual type safe stuff is in the last patch, and I'm posting that on as an
> RFC to see what other people think about the idea. I think most of the prep
> work would be nice to have even if we decide against the type safety. Oh, turns
> out I couldn't find any new "passing crap as the register offset" type of bugs
> with this work, but this would prevent them from sneaking in. As mentioned in
> the patch, I limited this only for mmio regs for now, but we could expand it
> cover other register types.
> 
> I've only smoke tested this on IVB, HSW and BSW.
> 
> The whole thing is available in my git repo:
> git://github.com/vsyrjala/linux.git type_safe_reg_access_3
> 
> Ville Syrjälä (43):
>   drm/i915: Don't pass sdvo_reg to intel_sdvo_select_{ddc,i2c}_bus()
>   drm/i915: Parametrize LRC registers
>   drm/i915: Parametrize GEN7_GT_SCRATCH and GEN7_LRA_LIMITS
>   drm/i915: Parametrize fence registers
>   drm/i915: Parametrize FBC_TAG registers
>   drm/i915: Parametrize ILK turbo registers
>   drm/i915: Replace raw numbers with the approproate register name in
>     ILK turbo code
>   drm/i915: Parametrize TV luma/chroma filter registers
>   drm/i915: Parametrize DDI_BUF_TRANS registers
>   drm/i915: Parametrize CSR_PROGRAM registers
>   drm/i915: Parametrize UOS_RSA_SCRATCH
>   drm/i915: Add LO/HI PRIVATE_PAT registers
>   drm/i915: Always use GEN8_RING_PDP_{LDW,UDW} instead of hand rolling
>     the register offsets
>   drm/i915: Include MCHBAR_MIRROR_BASE in ILK_GDSR
>   drm/i915: Parametrize PALETTE and LGC_PALETTE
>   drm/i915: s/_CURACNTR/CURCNTR(PIPE_A)/
>   drm/i915: s/_FDI_RXA_.../FDI_RX_...(PIPE_A)/
>   drm/i915: s/_TRANSA_CHICKEN/TRANS_CHICKEN(PIPE_A)/
>   drm/i915: s/GET_CFG_CR1_REG/DPLL_CFGCR1/ etc.
>   drm/i915: Use paramtrized WRPLL_CTL()
>   drm/i915: Add VLV_HDMIB etc. which already include VLV_DISPLAY_BASE
>   drm/i915: s/DDI_BUF_CTL_A/DDI_BUF_CTL(PORT_A)/
>   drm/i915: Eliminate weird parameter inversion from BXT PPS registers
>   drm/i915: Parametrize HSW video DIP data registers
>   drm/i915: Include gpio_mmio_base in GMBUS reg defines
>   drm/i915: Protect register macro arguments
>   drm/i915: Fix a few bad hex numbers in register defines
>   drm/i915: Turn GEN5_ASSERT_IIR_IS_ZERO() into a function
>   drm/i915: s/PIPE_FRMCOUNT_GM45/PIPE_FRMCOUNT_G4X/ etc.
>   drm/i915: Parametrize and fix SWF registers
>   drm/i915: Throw out some useless variables
>   drm/i915: Clean up LVDS register handling
>   drm/i915: Remove dev_priv argument from NEEDS_FORCE_WAKE
>   drm/i915: Turn __raw_i915_read8() & co. in to inline functions
>   drm/i915: Move __raw_i915_read8() & co. into i915_drv.h
>   drm/i915: Remove the magic AUX_CTL is at DP + foo tricks
>   drm/i915: Replace the aux ddc name switch statement with a table
>   drm/i915: Parametrize AUX registes
>   drm/i915: Add dev_priv->psr_mmio_base
>   drm/i915: Store aux data reg offsets in intel_dp->aux_ch_data_reg[]
>   drm/i915: Model PSR AUX register selection more like the normal AUX
>     code
>   drm/i915: Prefix raw register defines with underscore
>   WIP: drm/i915: Type safe register read/write

I've read through the series, and it looks good. I commented in a couple
of places, and I have yet to scrutinize the parametrizations to check the
values are the same (except for where you think the original values are
bogus), but I like it and I think it is worthwhile.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx