[PATCH weston v2] gl-renderer: If an XRGB format is requested but unavailable, try ARGB

[Derek Foreman]

On 11/05/15 08:57 AM, Pekka Paalanen wrote:
> On Tue,  5 May 2015 16:22:22 -0500
> Derek Foreman <derekf at osg.samsung.com> wrote:
> 
>> Recent versions of Mesa have stopped exposing XRGB visuals for gl on
>> some Intel GPUs.  While this may be changed in Mesa eventually, it's
>> not impossible that some other hardware in the future won't provide
>> XRGB visuals either.
>>
>> Let's try again with an ARGB visual if XRGB is unavailable.  Since
>> we're not changing the scanout buffer format, and our current
>> rendering loop always results in saturated alpha in the frame buffer,
>> it should be Just Fine(tm) - and probably better than just exiting.
>>
>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>> ---
>>
>> Dima Ryazanov called me out for fallback_format() not working on all endians,
>> and I've reworked the loop to be a single loop instead of two, similar to
>> Daniel's suggestion (but still keeping the NULL visual_id means "give me the
>> first match" semantic of the original)
>>
>> I've also replaced calloc with aloca so I don't have to worry about freeing
>> the array on return, which simplifies things a little bit.  I didn't bother
>> zeroing out the array, since it's filled in by eglChooseConfig.
>>
>> It's still an ugly function. :)
>>
>>  src/gl-renderer.c | 57 +++++++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 43 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
>> index ae3122f..a4fd3a0 100644
>> --- a/src/gl-renderer.c
>> +++ b/src/gl-renderer.c
>> @@ -934,6 +934,14 @@ output_rotate_damage(struct weston_output *output,
>>  	go->border_damage[go->buffer_damage_index] = border_status;
>>  }
>>  
>> +/* NOTE: We now allow falling back to ARGB  gl visual ids when XRGB is
>> + * unavailable, so we're assuming the background has no transparency
>> + * and that everything with a blend, like drop shadows, will have something
>> + * opaque (like the background) drawn underneath it.
>> + *
>> + * Depending on the underlying hardware, violating that assumption could
>> + * result in seeing through to another display plane.
>> + */
>>  static void
>>  gl_renderer_repaint_output(struct weston_output *output,
>>  			      pixman_region32_t *output_damage)
>> @@ -1897,6 +1905,15 @@ log_egl_config_info(EGLDisplay egldpy, EGLConfig eglconfig)
>>  		weston_log_continue(" unknown\n");
>>  }
>>  
>> +static EGLint
>> +fallback_format(EGLint format)
>> +{
>> +	if ((format & 0xFF) != 'X')
>> +		return 0;
>> +
>> +	return (format & 0xFFFFFF00) | 'A';
>> +}
> 
> I read the function name, I read the code, and I repeat that 6 times
> before I figure out the right interpretation. That's pretty awesome. In
> a not fun way. :-P
> 
> How about calling it get_fallback_format_for()?
> 
> Are you sure this works for X11 Visuals too? Oh, but x11-backend
> doesn't pass any visual in, so that saves you.
> 
> What about GBM visuals (drmfourccs) like RA24 or BA24? Oh, but this
> actually a GBM format code, which is hand-picked and cannot just
> whatever, so this works.

Really, only the drm backend even specifies a native visual id, and it's
limited to the formats we accept in weston.ini.

In any event, it does make more sense to push that complexity into
compositor-drm.c as the gl-renderer might be used by something that uses
something else for a native visual id...

> There's so much fun going on, that I'd like to have big comment
> explaining it all, please. :-)
>
> With all these assumptions around, you might as well just hardcode a
> switch-statement and say in comments where the funny numbers come from.
> 
> Or, even better, extend the gl_renderer_create() API to have the
> fallback format passed in explicitly from the backend, which can then
> use the proper #defined names. Or go even further, if there is a list
> of acceptable visuals.
> 
> It's a question of how far you want to go...

Ok, I've implemented it with a list of formats passed in from the
back-end and only the drm backend tries to do fallback now.

I've split it into two patches, one for the gl-renderer API change, one
for drm to use a fallback fourcc.

>> +
>>  static int
>>  egl_choose_config(struct gl_renderer *gr, const EGLint *attribs,
>>  		  const EGLint *visual_id,
>> @@ -1904,41 +1921,53 @@ egl_choose_config(struct gl_renderer *gr, const EGLint *attribs,
>>  {
>>  	EGLint count = 0;
>>  	EGLint matched = 0;
>> +	EGLint fallback_id;
>>  	EGLConfig *configs;
>> +	EGLConfig fallback_config = 0;
>>  	int i;
>>  
>>  	if (!eglGetConfigs(gr->egl_display, NULL, 0, &count) || count < 1)
>>  		return -1;
>>  
>> -	configs = calloc(count, sizeof *configs);
>> +	configs = alloca(count * sizeof *configs);
> 
> The number of eglconfigs could be large, so maybe alloca isn't quite
> right here...
> 
>>  	if (!configs)
>>  		return -1;
> 
> Can't test this with alloca.

Ok, ok, I won't use alloca.  :)

>>  
>>  	if (!eglChooseConfig(gr->egl_display, attribs, configs,
>>  			      count, &matched))
>> -		goto out;
>> +		return -1;
>> +
>> +	if (!matched)
>> +		return -1;
>>  
>> +	if (!visual_id) {
>> +		*config_out = configs[0];
>> +		return 0;
>> +	}
>> +
>> +	fallback_id = fallback_format(*visual_id);
>>  	for (i = 0; i < matched; ++i) {
>>  		EGLint id;
>>  
>> -		if (visual_id) {
>> -			if (!eglGetConfigAttrib(gr->egl_display,
>> -					configs[i], EGL_NATIVE_VISUAL_ID,
>> -					&id))
>> -				continue;
>> +		if (!eglGetConfigAttrib(gr->egl_display,
>> +				configs[i], EGL_NATIVE_VISUAL_ID,
>> +				&id))
>> +			continue;
>>  
>> -			if (id != 0 && id != *visual_id)
>> -				continue;
>> +		if (id == *visual_id) {
>> +			*config_out = configs[i];
>> +			return 0;
>>  		}
>> +		if (id == fallback_id)
>> +			fallback_config = configs[i];
>> +	}
> 
> Hmm, but if we end up with the fallback_config, we will get the last
> config that matches fallback. Usually we prefer the first config that
> matches. Would it make sense to use the first fallback-matching config
> in that case?

Addressed in the follow up.

>>  
>> -		*config_out = configs[i];
>> -
>> -		free(configs);
>> +	if (fallback_config) {
>> +		weston_log("Falling back to ARGB visual\n");
>> +		*config_out = fallback_config;
>>  		return 0;
>>  	}
>>  
>> -out:
>> -	free(configs);
>>  	return -1;
>>  }
>>  
> 
> Thanks,
> pq
>