Re: [PATCH i-g-t 2/6] i-g-t: lib: Add plane pixel format support

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 09-01-18 om 13:10 schreef Maarten Lankhorst:
> Op 13-12-17 om 10:50 schreef Vidya Srinivas:
>> From: Mahesh Kumar <mahesh1.kumar@intel.com>
>>
>> This patch adds the following:
>> - Query supported pixel formats from kernel for a given
>> plane.
>> - Get number of supported pixel formats for a plane
>> - Check if format is supported by cairo
>> - Add support to get format name string
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>> ---
>>  lib/igt_fb.c  | 16 ++++++++++++++++
>>  lib/igt_kms.c | 29 +++++++++++++++++++++++++++++
>>  lib/igt_kms.h |  7 +++++++
>>  3 files changed, 52 insertions(+)
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index d4eaed7..21d666d 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -68,6 +68,22 @@ static struct format_desc_struct {
>>  	DF(XRGB2101010,	RGB30,		32, 30),
>>  	DF(ARGB8888,	ARGB32,		32, 32),
>>  };
>> +
>> +const char *igt_get_format_name(uint32_t format)
>> +{
>> +	switch(format) {
>> +		case DRM_FORMAT_RGB565:
>> +			return "RGB565";
>> +		case DRM_FORMAT_XRGB8888:
>> +			return "XRGB8888";
>> +		case DRM_FORMAT_XRGB2101010:
>> +			return "XRGB2101010";
>> +		case DRM_FORMAT_ARGB8888:
>> +			return "ARGB8888";
>> +		default:
>> +			return "Unknown";
>> +	}
>> +}
>>  #undef DF
> Right above this function, we have a struct format_desc_struct, which has a drm format parameter, and a name parameter.
> Right below this function, there's a for_each_format() defined which iterates over the formats defined in this array. :)
>
> I think igt_get_format_name can be implemented with both, removing some duplication.
Hmm, maybe get rid of igt_fb_is_cairo_supported_format and add a try_ version of igt_get_cairo_ctx/surface?

Other macros here look too specialized, nothing wrong with accessing plane->drm_plane directly for this information.
>>  #define for_each_format(f)	\
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 223dbe4..cc17f5c 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -3639,3 +3639,32 @@ uint32_t kmstest_get_vbl_flag(uint32_t pipe_id)
>>  		return pipe_flag;
>>  	}
>>  }
>> +
>> +/**
>> + * igt_is_cairo_supported_format:
>> + * @pixel_format: pixel format to be checked in supported cairo list.
>> + *
>> + * Returns true if pixel format is present in the supported cairo pixel
>> + * format list and returns false if not present/supported in cairo.
>> + */
>> +bool igt_is_cairo_supported_format(uint32_t pixel_format)
>> +{
>> +	const uint32_t *igt_formats;
>> +	int num_igt_formats;
>> +	int i;
>> +
>> +	igt_get_all_cairo_formats(&igt_formats, &num_igt_formats);
>> +	for (i = 0; i < num_igt_formats; i++) {
>> +		if (pixel_format == igt_formats[i])
>> +			return true;
>> +	}
>> +	return false;
>> +}
> I think this function belongs to igt_fb as igt_fb_is_cairo_supported_format.
>> +int igt_get_format_count_plane(igt_plane_t *plane)
>> +{
>> +	if (plane)
>> +		return plane->drm_plane->count_formats;
>> +
>> +	return -EINVAL;
>> +}
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index 2a480bf..ced7d73 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -389,6 +389,10 @@ void igt_fb_set_size(struct igt_fb *fb, igt_plane_t *plane,
>>  void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
>>  void igt_wait_for_vblank_count(int drm_fd, enum pipe pipe, int count);
>>  
>> +bool igt_is_cairo_supported_format(uint32_t pixel_format);
>> +int igt_get_format_count_plane(igt_plane_t *plane);
>> +const char *igt_get_format_name(uint32_t format);
>> +
>>  static inline bool igt_output_is_connected(igt_output_t *output)
>>  {
>>  	/* Something went wrong during probe? */
>> @@ -486,6 +490,9 @@ static inline bool igt_output_is_connected(igt_output_t *output)
>>  	for (int j__ = 0; assert(igt_can_fail()), (plane) = &(display)->pipes[(pipe)].planes[j__], \
>>  		     j__ < (display)->pipes[(pipe)].n_planes; j__++)
>>  
>> +#define for_each_format_in_plane(plane, format)	\
>> +	for (int k__ = 0; (format) = plane->drm_plane->formats[k__], k__ < plane->drm_plane->count_formats; k__++)
>> +
>>  #define IGT_FIXED(i,f)	((i) << 16 | (f))
>>  
>>  /**
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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