Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Dave Gordon <david.s.gordon@intel.com>]

On 10/06/15 09:09, Jani Nikula wrote:
> On Tue, 09 Jun 2015, Dave Gordon <david.s.gordon@intel.com> wrote:
>> Regardless of whether it's used, we have an inconsistency between the
>> definitions of PORT_DFT_I9XX and PORT_DFT2_G4X -- one includes the
>> mmio_offset and the other doesn't.
> 
> It's not inconsistent, it's consistent on another level:
> 
> We've settled on including the mmio_offset only for macros that need
> it. If a macro is relevant only on platforms that all have the same mmio
> offset, the offset is included statically (currently 0 or
> VLV_DISPLAY_BASE). dev_priv->info.display_mmio_offset is only used when
> the macro is relevant on platforms with different mmio offsets. This we
> try to follow consistently.

So you only have a problem when a previously-statically located block
turns into something that can be (and is) moved around on different
platforms ... I suppose that shouldn't happen too often.

>> Personally I think the #define with an implicit dependency on an
>> object called "dev_priv" is really ugly and we should move away from
>> that style rather than adding more of them, but that's a lot of work.
> 
> Agreed in principle, but I think we've lost the battle. It's way too
> much churn, and makes a lot of code really ugly. Compare these:
> 
> 	I915_WRITE(FOOBAR, I915_READ(FOOBAR) | FOOBAR_ENABLE);
> 
> 	I915_WRITE(dev_priv, FOOBAR(dev_priv),
> 		   I915_READ(dev_priv, FOOBAR(dev_priv)) | FOOBAR_ENABLE);

I'd rather write

	uint32_t foobar = I915_DISP_REG_READ(dev_priv, FOOBAR);
	I915_DISP_REG_WRITE(dev_priv, foobar | FOOBAR_ENABLE);

And if there were very many places where that read-modify-write was
used, we could define that as

#define	I915_DISP_REG_BITSET(dev_priv, reg, bits) ...

Of course it would require people to use the right macro according to
whether the register was part of the display block, the GT block, or
however many other areas there are that might be independently relocated
... but it might be more future proof :)

> We'll try to focus on only requiring "dev_priv" implicitly and nothing
> else, and where a parameter does get passed, we'll try to make it
> "dev_priv" as that pretty much has to be around anyway.
> 
> BR,
> Jani.

Well I guess we're stuck with 'dev_priv' for the foreseeable future :(

Hmmm ... we have quite a bit of code that passes 'dev' around rather
than dev_priv, where the (usually static) called function immediately
uses it to derive dev_priv, and where the only other uses in the called
function are in calls to INTEL_INFO(dev) and its derivatives. For example:

static int get_context_size(struct drm_device *dev)
{
        struct drm_i915_private *dev_priv = dev->dev_private;
        int ret;
        u32 reg;

        switch (INTEL_INFO(dev)->gen) {
        case 6:
                reg = I915_READ(CXT_SIZE);
                ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
                break;
        case 7:
                reg = I915_READ(GEN7_CXT_SIZE);
                if (IS_HASWELL(dev))
                        ret = HSW_CXT_TOTAL_SIZE;
                else
                        ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
                break;
        case 8:
                ret = GEN8_CXT_TOTAL_SIZE;
                break;
        default:
                BUG();
        }

        return ret;
}

But since INTEL_INFO() can take 'dev_priv' as a parameter (as an
alternative to 'dev'), all the uses of 'dev' here could be replaced by
'dev_priv' and then the parameter changed to pass that directly, thus
potentially eliminating quite a few extra memory references. Applied
driver-wide, that micro-optimisation might add up to something useful :)

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx