Re: [PATCH v5 03/44] drm/vkms: Add kunit tests for VKMS LUT handling

[Harry Wentland <harry.wentland@amd.com>]

On 2024-08-27 13:49, Louis Chauvet wrote:
> Le 19/08/24 - 16:56, Harry Wentland a écrit :
> 
> [...]
> 
>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>> index 3d6785d081f2..3ecda70c2b55 100644
>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>> @@ -435,3 +435,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 3d6785d081f2..3ecda70c2b55 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -435,3 +435,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>  
>  	return ret;
>  }
> +
> +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS
> +#include "tests/vkms_color_tests.c"
> +#endif>  
>>  	return ret;
>>  }
>> +
>> +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS
>> +#include "tests/vkms_color_tests.c"
>> +#endif
> 
> This is very strange to include a .c in this file, is it something done a 
> lot in the kernel? I can find only one occurence of this pattern in the 
> kernel [1], the other tests are in their own modules.
> 
> In addition it crate many warning during compilations:
> 	warning: symbol 'test_*' was not declared. Should it be static?
> 
> As other tests will be introduced (yuv [2], config [3]), it is maybe 
> interesting to introduce a new module as [2] is doing?

The VISIBLE_IF_KUNIT et al. is much nicer than including a .c file.
Thanks for pointing me to them. Will change this.

Harry

> 
> [1]: https://elixir.bootlin.com/linux/v6.11-rc5/source/fs/ext4/mballoc.c#L7047
> [2]: https://lore.kernel.org/all/20240809-yuv-v10-14-1a7c764166f7@bootlin.com/
> [3]: https://lore.kernel.org/all/20240814-google-remove-crtc-index-from-parameter-v1-15-6e179abf9fd4@bootlin.com/
>