Hi Emil, again thanks for the review. 2016-08-30 15:15 GMT+02:00 Emil Velikov <emil.l.velikov@xxxxxxxxx>: > On 30 August 2016 at 10:08, Daniel Vetter <daniel@xxxxxxxx> wrote: >> On Tue, Aug 30, 2016 at 09:14:51AM +0200, Christian Gmeiner wrote: >>> From: The etnaviv authors <dri-devel@xxxxxxxxxxxxxxxxxxxxx> >>> >>> This adds the following basic unit tests: >>> >>> - etnaviv_2d_test >>> Let the 2D core render a defined pattern into a bo >>> and store it as bmp. >>> >>> - etnaviv_bo_cache_test >>> Basic tests to validate the bo-cache behavior. >>> >>> - etnaviv_cmd_stream_test >>> Tests for the etna_cmd_stream API. >> >> igt (now at a new location at >> https://cgit.freedesktop.org/drm/igt-gpu-tools/) would be a nice place for >> these I think, if you want to participate there. vc4 has tests in there, >> and collabora is porting a lot of the kms tests to be generic (so that >> they can be run on any kms driver). Then you could just run those tests on >> any driver and have a reasonable assurance you didn't break the world. >> > Having things in IGT might be better indeed. For the moment it might > be better to land this as-is and move things in due time while add new > tests directly in IGT ? > > Other than that - a couple of trivial questions/suggestions. Similar > to the patch 1/2 ones - can be done as follow-up if needed. > Will send a v2 soon. >>> --- /dev/null >>> +++ b/tests/etnaviv/cmdstream.xml.h > >>> +The rules-ng-ng source files this header was generated from are: >>> +- /home/orion/projects/etna_viv/rnndb/cmdstream.xml ( 12589 bytes, from 2013-09-01 10:53:22) >>> +- /home/orion/projects/etna_viv/rnndb/common.xml ( 18379 bytes, from 2014-01-27 15:58:05) > Always wondered about this - can we fix rnn to not print the full path > but just files or rnndb/filename.xml ? > >>> +Copyright (C) 2013 > There is something funny here - currently we're not 2013 and the > copyright holders are missing. Guess something went bonkers in rnn ? > Good catch - fixed in the source repo and will be corrected in v2. >>> +int main(int argc, char *argv[]) >>> +{ > >>> +fail: >>> + if (stream) >>> + etna_cmd_stream_del(stream); >>> + >>> + if (pipe) >>> + etna_pipe_del(pipe); >>> + >>> + if (gpu) >>> + etna_gpu_del(gpu); >>> + >>> + if (dev) >>> + etna_device_del(dev); >>> + >>> + close(fd); >>> + > Feel free to ignore: you can use separate labels and drop all the if > checks. Also s/fail/out/ since this is not an error path. > Same comment goes the whole patch. > Sure why not. thanks -- Christian Gmeiner, MSc https://soundcloud.com/christian-gmeiner _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
