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

Chris Wilson
2015-06-17T07:59:30Z

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

--
Chris Wilson, Intel Open Source Technology Centre