Re: [PATCH 08/15] drm/i915: Move execlists defines from .c to .h

From: Dave Gordon <david.s.gordon@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org,
	"Michael H. Nguyen" <michael.h.nguyen@intel.com>
Subject: Re: [PATCH 08/15] drm/i915: Move execlists defines from .c to .h
Date: Mon, 22 Jun 2015 14:05:39 +0100	[thread overview]
Message-ID: <55880823.7080305@intel.com> (raw)
In-Reply-To: <20150617075921.GB23776@nuc-i3427.alporthouse.com>

On 17/06/15 08:59, Chris Wilson wrote:
> On Wed, Jun 17, 2015 at 08:31:59AM +0100, Dave Gordon wrote:
>> On 16/06/15 10:37, Chris Wilson wrote:
>>> On Mon, Jun 15, 2015 at 07:36:26PM +0100, Dave Gordon wrote:
>>>> From: "Michael H. Nguyen" <michael.h.nguyen@intel.com>
>>>>
>>>> Move defines from intel_lrc.c to i915_reg.h so they are accessible
>>>> to the GuC submission code; and expose a previously static function
>>>> in the execlist code which will also be required for GuC submission.
>>>
>>> What would have been better would have to been to split the lrc code
>>> from the execlists code so that the sharing is more obvious and the
>>> overloading separate from the common code.
>>> -Chris
>>
>> What would have been better is not to have put these fairly generic
>> details about the hardware into a C file in the first place. And not to
>> have split execlist and ringbuffer modes into two entirely different
>> paths. And various other historical decisions. But we can only fix the
>> code as it stands, not as it ought to have been.
>>
>> Anyway, this is just a bulk cut-n-paste, so I'm not inclined to do any
>> restructuring on it during this process. But someone working on
>> execlists could certainly tidy it up later, perhaps as part of a general
>> drive towards deduplicating the code paths and partitioning (context vs
>> ringbuffer vs engine) functionality in a more coherent way.
> 
> More to the point, you are increasing the technical debt of the code
> rather than reducing it. Code will just become less and less
> maintainable.
> -Chris

OK, I have abolished the bulk cut'n'paste :)

Turns out we really only needed a bit of it, and then I spotted a way to
reuse some of the "execlists" code (which is really LRC code) instead of
having a GuC version of same (which is what needed some of these #defines).

So in the end, this patch is replaced by simply renaming-and-exposing
the /two/ functions in intel_lrc.c that we actually need for GuC
submission. Better still, one of them may go away entirely once we eat
some of Chris' low-hanging fruit :)

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