Re: [PACTH i-g-t v4 00/13] Remove compile time depencencies on libdrm_intel.

[Robert Foss <robert.foss@collabora.com>]

On 2016-06-29 06:42 AM, Emil Velikov wrote:
> On 23 June 2016 at 10:34,  <robert.foss@collabora.com> wrote:
>> From: Robert Foss <robert.foss@collabora.com>
>>
>>
>> Hey,
>>
>> I've been looking at the possibilty of removing the compile time depency on
>> libdrm_intel. There are two technical solutions to this problem as far as
>> I can see; stubs and conditional compilation.
>>
>> This series uses the stubbing approach.
>>
>> Changes since v1:
>> - Replaced the automake flags HAVE_VC4/NOUVEAU/INTEL with HAVE_LIBDRM_XXX.
>> - Move conditionals from Makefile.sources to Arduino.mk/Makefile.am.
>> - Removed duplicated i915_drm.h symbols from intel_drm_stubs.h.
>> - Replaced igt_require with igt_require_f to communicate stubs being the cause
>>    of failure.
>> - Rename intel_drm_stubs to intel_bufmgr.
>> - Moved intel_bufmgr to lib/stubs/drm.
>> - Remove header inclusion changes in favor for inclusion of stubs in
>>    lib/stubs/drm using build scripts.
>> - Rebased on trunk.
>>
>> Changes since v2:
>> - Removed conditional compilation from intel_bufmgr.h.
>> - Enable HAVE_LIBDRM_INTEL on android platforms.
>> - Remove unnecessary whitespace.
>> - Remove unnecessary inclusion of C files.
>> - De-duplicated intel_bufmgr.c error string.
>> - Changed Makefile.sources variable names to be non-automake specific
>>
>> Changes since v3:
>> - Added signoff to two commits.
>> - Changed automake if not statement.
>> - Removed accidental space character.
>> - Copied in new copy of intel_bufmgr.h
>> - Improved wording of lib/stubs/drm/README.
>>
> Just an idea/suggestion for the future, not sure if it's normal approach in IGT:
> Adding the detailed change log to the respective patch (be that before
> or after the --- line) as opposed to here is better IMHO. It provides
> provides direct, quick and easy feedback to the reviewer.

Making a per commit changelog is probably the way to go, I hope you'll 
forgive me if don't create a retroactive one for this series though.
I'll make it habit for upcoming series.

>
> Something that just hit me - 1/13 should only do "check for
> libdrm_intel and error otherwise. set the AC_CONDITIONAL()" with the
> actual "allow libdrm_intel less systems to build IGT" patch coming
> after the stubs were introduced.

If someone feels strongly about the issue I'd happily make that change 
and submit a v++ patch series.

>
> As-is building w/o libdrm_intel will be allowed, yet broken through
> the series, which shouldn't be an issue here. Just something worth
> mentioning for future work.
>
> As-is the series is
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>
> -Emil
>

Thanks for having a look Emil and all the feedback, it's much appreciated.

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