[Intel-gfx] [PATCH 1/6] drm/i915: Colocate all GT access routines in the same file

[Paulo Zanoni]

2013/7/14 Chris Wilson <chris at chris-wilson.co.uk>:
> On Sun, Jul 14, 2013 at 12:42:49PM -0700, Ben Widawsky wrote:
>> On Fri, Jul 12, 2013 at 06:08:22PM +0100, Chris Wilson wrote:
>> > Currently, the register access code is split between i915_drv.c and
>> > intel_pm.c. It only bares a superficial resemblance to the reset of the
>> > powermanagement code, so move it all into its own file. This is to ease
>> > further patches to enforce serialised register access.
>> >
>> > v2: Scan for random abuse of I915_WRITE_NOTRACE
>> > v3: Take the opportunity to rename the GT functions as uncore. Uncore is
>> > the term used by the hardware design (and bspec) for all functions
>> > outside of the GPU (and CPU) cores in what is also known as the System
>> > Agent.
>> >
>> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>
>> git am complains about a missing newline at EOF, but I guess Daniel will
>> fix it on merge.
>>
>> Just bikesheds:
>> As before, intel_uncore_(clear|check)_errors seems silly to me.
>
> I still don't understand what you mean. They are just the code that
> existed before, so what is silly in moving them since they depend upon
> bypassing the common i915_write/read code?
>
>> Also if
>> you extracted those as a separate patch to the gt funcs, you could have
>> had just a simple file move + rename. And as you made me realize, I'm
>> not horribly thrilled with the name uncore, I liked gt. For me, the
>> distinction between _pm, and _uncore isn't really large enough, ie. many
>> things in _pm are really uncore also (anything touching the punit/rps,
>> etc). Also, since gt_funcs never really expanded, maybe just call it
>> forcewake_funcs and be done with that (since uncore forcewake sounds
>> weird to me).
>
> Looks like you made the distinction pretty clear in that paragraph
> between gt and pm. The name change is mostly a whim because we no longer
> refer to this as being the GT.
>
>> Final bikeshed, I would like to see the reset code in a separate file as
>> well (included in this could be any GEM functions we have for reset
>> only, and display as well).
>
> No. Look at the reset code, it is far too incestrous with the gt/uncore
> mechanics to be anywhere else. And it didn't seem right to separate it
> up.

While testing the Package C8+ feature I'm implementing, one of the
things I tried was "for i in $(seq 20); do glxgears & done". After I
ran this command, when I tried to drag the glxgears windows around I
got a hard machine hang. Then I applied this patch series and now the
problem is gone :)

Also, "git am" complains about white space errors on patch 1 :)

Thanks for the fix!
Paulo

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



-- 
Paulo Zanoni