[waffle] [RFC] Add display support to GBM backend

[Emil Velikov]

Hi all,

On 19/11/14 19:38, Chad Versace wrote:
> Frank sent this patch as a proof-of-concept. It's a draft, and not ready
> for committing. I'm posting to the list just to start discussion.
> 
> See [https://github.com/waffle-gl/waffle/issues/18]. I think detailed
> patch discussions like this are easier over email than Github's issue
> tracker, so let's keep the bulk of the discussion in email.
> 
> Frank, I haven't examined every detail in this patch. I focused on the
> high-level design.  I'll probably wait to examine the fine details until
> you submit a real patch for upstreaming.
> 
>> From: Frank Henigman <fjhenigman at chromium.org>
>> Date: Tue, 21 Oct 2014 22:58:45 -0400
>> Subject: [PATCH] gbm WIP: show gbm buffers on screen
>>
>> Four options based on the value of environment variable MODE:
>> COPY: copy render buffer to scanout buffer and flip to scanout buffer
>> FLIP: scan out directly from render buffer
>> ORIG: do what the code originally did, i.e. nothing
>> unset or empty: don't display, but still lock and release buffer
>>
>> TO DO
>> - make it a compile time option
>> - make it a proper run time option, via config attributes or something,
>>   instead of by env var
> 
> I prefer to make the display-mode a runtime option via attributes to
> waffle_window. Perhaps we should define a new function:
> 
>    struct waffle_window*
>    waffle_window_create_attrs(struct waffle_config *config,
>                               const intptr_t attrib_list[]);
> 
That makes perfect sense, and I think we can make use of it in WGL/GLX.
Just a quick question here:
 - Should the attrib list be platform specific, generic(for all
platforms) or both ?

"Both" might be a nice approach, this way we can have
- WAFFLE_WINDOW_FULLSCREEN
- WAFFLE_GBM_WINDOW_DISPLAY_MODE
- WAFFLE_WGL_WINDOW_HACKS
- other..


>> - leave hooks and graceful fails for (as yet) unsupported drm drivers,
>>   currently only i915 is supported
> 
> If initially the only supported driver is i915, that's ok with me. We
> have to start somewhere, and I don't like waiting for perfect completeness.
> 
buffer_copy() seems to be the only HW specific part and, as Chad, I see
no problem with having only i915 support for now. Shame gbm does not
provide an buffer_copy API for us considering it does buffer management :\

An interesting/related point here is :
How do we handle platforms where the display device is not the same as
the one doing rendering.
- In X we use xrandr to set the "coupling" policy between the render and
display device.
- Outside of X there is no standard way and every app uses its own
"hack". It would suck if we need to add another one in waffle.
At first we can push the decision to the user having via
  WAFFLE_GBM_{DISPLAY,WINDOW}_RENDER_DEVICE
  WAFFLE_GBM_{DISPLAY,WINDOW}_DISPLAY_DEVICE
and use the standard library/interface once we get one :)


[snip]
>> +struct drm_display {
>> +    int fd;
>> +    bool setcrtc_done;
>> +    struct buffer *flip_pending;
>> +    struct buffer *on_screen;
>> +
>> +    drmModeConnectorPtr conn;
>> +    drmModeModeInfoPtr mode;
>> +    drmModeCrtcPtr crtc;
>> +
>> +    // not used when scanning out from render buffers
>> +    struct buffer *front;
>> +    struct buffer *back;
>> +    void *tmp;
tmp does not seem to be used. If it's staying can we get a slightly
better name and/or comment it ?

[snip]
>> +// scanout from buffer
>> +static bool
>> +buffer_flip(struct buffer *buffer)
>> +{
>> +    struct drm_display *drm = drm_display_init(buffer->dpy);
>> +    if (!drm || !drm->mode)
>> +        return false;
>> +
>> +    if (gbm_bo_get_width(buffer->bo) < drm->mode->hdisplay ||
>> +        gbm_bo_get_height(buffer->bo) < drm->mode->vdisplay) {
>> +        wcore_errorf(WAFFLE_ERROR_UNKNOWN,
>> +                     "cannot flip buffer smaller than screen");
> 
>    This error message got me thinking... Maybe waffle_window_create()
> should
>    interpret (w,h)=(-1,-1) as a fullscreen window.
> 
Can we use WAFFLE_WINDOW_FULLSCREEN (or similar) ? Would be nice to
avoid the assumptions and keep everything explicit and clear.


[snip]
>> +bool
>> +wgbm_window_swap_buffers(struct wcore_window *wc_self)
>> +{
>> +    //XXX detect driver before invoking driver-specific code -
>> gbm_device_get_backend_name() doesn't help, it returns "drm"
>> +
>> +    if (!wegl_window_swap_buffers(wc_self))
>> +        return false;
>> +
>> +    //XXX display or window config, not env var
>> +    // O:original, possibly buggy behavior (no lock/release), F:flip,
>> C:copy
>> +    // otherwise: no display (but still lock/release)
> 
> Regarding MODE="O", Waffle now does lock/release for
> wgbm_window_swap_buffers.
> Let's make MODE="O" be equivalent to MODE="", which selects the default
> behavior:
> lock/release but don't post to display.
> 
> Of course, we need better names than "O", "F", and "C". Assuming that we
> associate these attributes with window and not the display (which might be
> a bad assumption), how do these names sound to you?
> 
>    name: WAFFLE_GBM_WINDOW_DISPLAY_MODE
>    values:
>        WAFFLE_GBM_WINDOW_DISPLAY_NONE (default)
>        WAFFLE_GBM_WINDOW_DISPLAY_FLIP
>        WAFFLE_GBM_WINDOW_DISPLAY_COPY
> 
> Question: Why do you want COPY mode? Is the purpose to exercise the
> implementation pattern of Chrome's partial display update?
> 
> Question: Do you think these display-mode attributes should be
> associated with
> the waffle_display or waffle_window? I'm leaning towards the window,
> because
> I see no reason to prevent distinct windows from having distinct
> display-modes.
> 
Keeping these attributes per window, makes more sense to me.

Should we change the name to drm, as currently we relate the platform
name with the display used :P


Cheers,
Emil