[PATCH weston v6 10/12] drm: Pass gbm_format as enum rather than string in output config

[Benoit Gschwind]

Le 18/04/2016 16:50, Pekka Paalanen a écrit :
> On Fri, 15 Apr 2016 20:28:35 -0700
> Bryce Harrington <bryce at osg.samsung.com> wrote:
> 
>> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
>> ---
>>  src/compositor-drm.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
>> index d129adc..a896785 100644
>> --- a/src/compositor-drm.c
>> +++ b/src/compositor-drm.c
>> @@ -93,13 +93,13 @@ enum weston_drm_backend_output_mode {
>>  struct weston_drm_backend_output_config {
>>  	struct weston_backend_output_config base;
>>  
>> -	/** The pixel format to be used by the output. Valid values are:
>> -	 * - NULL - The format set at backend creation time will be used
>> -	 * - "xrgb8888"
>> -	 * - "rgb565"
>> -	 * - "xrgb2101010"
>> +	/** The pixel format to be used by the output. Supported values are:
>> +	 * - 0: The format set at backend creation time will be used
>> +	 * - GBM_FORMAT_XRGB8888
>> +	 * - GBM_FORMAT_RGB565
>> +	 * - GBM_FORMAT_XRGB2101010
>>  	 */
>> -	char *gbm_format;
>> +	uint32_t gbm_format;
>>  
>>  	/** The seat to be used by the output. Set to NULL to use the
>>  	 * default seat. */
>> @@ -2336,8 +2336,12 @@ drm_configure_output(struct weston_compositor *c,
>>  			   s, name);
>>  	free(s);
>>  
>> -	weston_config_section_get_string(section,
>> -					 "gbm-format", &config->gbm_format, NULL);
>> +	weston_config_section_get_string(section, "gbm-format", &s, NULL);
>> +        if (parse_gbm_format(s, GBM_FORMAT_XRGB8888, &config->gbm_format) < 0)
>> +		weston_log("Invalid gbm-format \"%s\" for output %s\n",
>> +			   s, name);
>> +	free(s);
>> +
>>  	weston_config_section_get_string(section, "seat", &config->seat, "");
>>  	return mode;
>>  }
>> @@ -2391,8 +2395,7 @@ create_output_for_connector(struct drm_backend *b,
>>  
>>  	mode = drm_configure_output(b->compositor, b->use_current_mode,
>>  				    output->base.name, &config);
>> -	if (parse_gbm_format(config.gbm_format, b->gbm_format, &output->gbm_format) == -1)
>> -		output->gbm_format = b->gbm_format;
>> +	output->gbm_format = config.gbm_format;
>>  
>>  	setup_output_seat_constraint(b, &output->base,
>>  				     config.seat ? config.seat : "");
> 
> Hi,
> 
> we cannot do this, because the backend or anything in libweston will
> not have access to the weston_config_*() API at all.
> 
> I would not like to #include <gbm.h> in main.c either, if we just can
> reasonably avoid it.
> 
> Drawing the line here is vague, and we might improve later. OTOH, I
> certainly don't want libweston to have its own enumeration of pixel
> formats either, though time will tell if that is avoidable.
> 
> 
> Thanks,
> pq
> 

Hello Pekka,

I think what ever you try in that case, you have 2 choice when using a
library under your own one:
 1. you choose to hide anything under your library
 2. or, you leave the user to know that your are using those library and
just include this library in your own libfoo.h

IMO, it's a waste time to try to hide library dependency. In particular
in the case of libweston, that will be more or less a glue library.

In other term, I in favor to keep enums, even if they come from a
foreign library.

Best regards.

> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>