Switch DRM platform to using a separate thread

Switch DRM platform to using a separate thread

All DRM operations are now performed on a separate thread. In order to
facilitate simple communication between GPU main, and GPU IO and the DRM
thread, proxy helpers were introduced. To minimize the surface between
the threads the proxy was introduced at the DrmWindow and
DrmGpuPlatformSupport level.

For IPCs all messages arriving to the GPU IO are directly routed to the
DRM thread via the DrmGpuPlatformSupportProxy. One benefit is that there
is no need for special cases for the cursors since it is now set on the
same thread as all other DRM operations.

Since Ozone does not retain ownership of the SurfaceOzoneEGL &
SurfaceOzoneCanvs, these objects are kept on the GPU main/IO for
simplicity and to minimize the number of proxy objects. DrmWindow is
then the entry point into the DRM thread. Thus DrmWindowProxy is a
wrapper that abstracts sending of messages between threads.

BUG=none

Committed: https://crrev.com/14630e92ee1cba7bc76643434944b99272a1deff
Cr-Commit-Position: refs/heads/master@{#353819}

[dnicoara, 2015-09-09 16:21:39]

dnicoara@chromium.org changed reviewers:
+ spang@chromium.org

[dnicoara, 2015-09-09 16:21:40]

This is the change to move the DRM calls onto a separate thread. I still have to
go over it to make sure I didn't miss anything, but it worked fine for me.

I'd love any suggestions on breaking this in smaller bits. I couldn't see an
easy way to do it without checking in unused code.

[spang, 2015-09-14 18:41:43]

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_surface_factory.h (right):

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_surface_factory.h:22: public
base::SupportsWeakPtr<DrmSurfaceFactory> {
use WeakPtrFactory instead

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_window_proxy.cc (right):

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_window_proxy.cc:15:
DrmWindowProxy::DrmWindowProxy(
Can we merge this with the surface class?

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_window_proxy.cc:39: PostSyncTask(
What stops these sync queries from turning into races?

I think maybe we should do something like

initially, gpu main thread calls GetSurfaceConfiguration(window_id), and gets
back a struct

struct SurfaceConfiguration {
  scoped_refptr<DrmDevice> drm_device;
  bool requires_gl_finish; // is UDL
  gfx::Size size;
};

inside swap buffers, gpu main thread calls SchedulePageFlip(queued_planes,
surface_configuration);

On the DRM thread you can check that the configuration is unchanged and if not,
return NAK_RECREATE_BUFFERS.

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_window_proxy.cc:46: gfx::Rect
DrmWindowProxy::GetBounds() const {
I don't think this function should exist here.

[dnicoara, 2015-09-17 21:58:21]

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_surface_factory.h (right):

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_surface_factory.h:22: public
base::SupportsWeakPtr<DrmSurfaceFactory> {
On 2015/09/14 18:41:43, spang wrote:
> use WeakPtrFactory instead

No longer relevant.

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
File ui/ozone/platform/drm/gpu/drm_window_proxy.cc (right):

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_window_proxy.cc:15:
DrmWindowProxy::DrmWindowProxy(
On 2015/09/14 18:41:43, spang wrote:
> Can we merge this with the surface class?

Done.

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_window_proxy.cc:39: PostSyncTask(
On 2015/09/14 18:41:43, spang wrote:
> What stops these sync queries from turning into races?
> 
> I think maybe we should do something like
> 
> initially, gpu main thread calls GetSurfaceConfiguration(window_id), and gets
> back a struct
> 
> struct SurfaceConfiguration {
>   scoped_refptr<DrmDevice> drm_device;
>   bool requires_gl_finish; // is UDL
>   gfx::Size size;
> };
> 
> inside swap buffers, gpu main thread calls SchedulePageFlip(queued_planes,
> surface_configuration);
> 
> On the DRM thread you can check that the configuration is unchanged and if
not,
> return NAK_RECREATE_BUFFERS.

I've re-done this function to no longer require a sync post.

As for your other concerns:
1) If surface resizes, this is already handled since we ignore requests when the
primary buffer is of the wrong size. We can also get this signal from the
buffers send with the ShedulePageFlip() call if we need to do anything fancy.

2) If the surface changes devices, we can also tell from the buffers since we
can look-up the device they were allocated on.

https://codereview.chromium.org/1311043016/diff/1/ui/ozone/platform/drm/gpu/d...
ui/ozone/platform/drm/gpu/drm_window_proxy.cc:46: gfx::Rect
DrmWindowProxy::GetBounds() const {
On 2015/09/14 18:41:43, spang wrote:
> I don't think this function should exist here.

Removed, no longer necessary.

[spang, 2015-09-18 00:46:25]

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_platform_support.h (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_platform_support.h:43: : public
base::SupportsWeakPtr<DrmGpuPlatformSupport> {
Please only use WeakPtrFactory for weak pointers.

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_platform_support_proxy.cc (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_platform_support_proxy.cc:66: bool
MessageFilterProxy::OnMessageReceived(const IPC::Message& message) {
I would strongly prefer to just unpickle the messages here. Something like

IPC_MESSAGE_HANDLER(OzoneGpuMsg_CreateWindow, OnCreateWindow)
// etc

void DrmMessageFilter(gfx::AcceleratedWidget widget) {
  drm_thread_proxy_->CreateWindow(widget);
}

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:151: cached_window_ =
drm_thread_->GetWindow(cached_widget_);
How about:

(1) store window_id -> Surface map in GbmSurfaceFactory
(2) queue overlays on GbmSurfaceless
(3) send the whole stack in OnSwapBuffersAsync

It could be done as a preliminary step.

[dnicoara, 2015-09-18 17:43:08]

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_platform_support.h (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_platform_support.h:43: : public
base::SupportsWeakPtr<DrmGpuPlatformSupport> {
On 2015/09/18 00:46:25, spang wrote:
> Please only use WeakPtrFactory for weak pointers.
> 
> 

Why? SupportsWeakPtr is meant to be used when you want external objects to take
weak pointers to this. Avoids boiler plate code.

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_platform_support_proxy.cc (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_platform_support_proxy.cc:66: bool
MessageFilterProxy::OnMessageReceived(const IPC::Message& message) {
On 2015/09/18 00:46:25, spang wrote:
> I would strongly prefer to just unpickle the messages here. Something like
> 
> IPC_MESSAGE_HANDLER(OzoneGpuMsg_CreateWindow, OnCreateWindow)
> // etc
> 
> void DrmMessageFilter(gfx::AcceleratedWidget widget) {
>   drm_thread_proxy_->CreateWindow(widget);
> }
> 

Why unpickle here? I don't see the benefit. It is cleaner to just pass the
message to the DrmThread and it would be handled internally to the DrmThread. It
also reduces the need for boilerplate code.

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:151: cached_window_ =
drm_thread_->GetWindow(cached_widget_);
On 2015/09/18 00:46:25, spang wrote:
> How about:
> 
> (1) store window_id -> Surface map in GbmSurfaceFactory
> (2) queue overlays on GbmSurfaceless
> (3) send the whole stack in OnSwapBuffersAsync
> 
> It could be done as a preliminary step.

Acknowledged.

[spang, 2015-09-18 20:49:55]

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_platform_support.h (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_platform_support.h:43: : public
base::SupportsWeakPtr<DrmGpuPlatformSupport> {
On 2015/09/18 17:43:07, dnicoara wrote:
> On 2015/09/18 00:46:25, spang wrote:
> > Please only use WeakPtrFactory for weak pointers.
> > 
> > 
> 
> Why? SupportsWeakPtr is meant to be used when you want external objects to
take
> weak pointers to this. Avoids boiler plate code.

SupportsWeakPtr causes subtle bugs and is deprecated. Please just implement the
boilerplate.

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_platform_support_proxy.cc (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_platform_support_proxy.cc:66: bool
MessageFilterProxy::OnMessageReceived(const IPC::Message& message) {
On 2015/09/18 17:43:08, dnicoara wrote:
> On 2015/09/18 00:46:25, spang wrote:
> > I would strongly prefer to just unpickle the messages here. Something like
> > 
> > IPC_MESSAGE_HANDLER(OzoneGpuMsg_CreateWindow, OnCreateWindow)
> > // etc
> > 
> > void DrmMessageFilter(gfx::AcceleratedWidget widget) {
> >   drm_thread_proxy_->CreateWindow(widget);
> > }
> > 
> 
> Why unpickle here? I don't see the benefit. It is cleaner to just pass the
> message to the DrmThread and it would be handled internally to the DrmThread.
It
> also reduces the need for boilerplate code.

You've already lost on the boilerplate code by having to switch on the message
type.

I think you are worrying too much about adding boilerplate that is actually
trivial and not enough about the fact that you needed different, more
subtle/error-prone boilerplate instead. Like the whole RegisterSender() thing.
Let's keep IPC centralized so it's easy to audit.

The DRM service thread should have an API that takes regular method calls, which
we call API from here and from GPU main. What you're calling MessageFilterProxy
is a the right place to terminate the IPC connection, type check the messages,
and forward along only well-formed commands.

[dnicoara, 2015-09-29 17:57:05]

dnicoara@chromium.org changed reviewers:
+ alexst@chromium.org

[dnicoara, 2015-09-29 17:57:05]

+ alexst@ so we can discuss buffer creation on GPU IO. (This is the create the
buffer via a sync posted message to the DRM thread vs the current approach with
relying on a lock in DrmDeviceManager)

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_platform_support.h (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_platform_support.h:43: : public
base::SupportsWeakPtr<DrmGpuPlatformSupport> {
On 2015/09/18 20:49:55, spang wrote:
> On 2015/09/18 17:43:07, dnicoara wrote:
> > On 2015/09/18 00:46:25, spang wrote:
> > > Please only use WeakPtrFactory for weak pointers.
> > > 
> > > 
> > 
> > Why? SupportsWeakPtr is meant to be used when you want external objects to
> take
> > weak pointers to this. Avoids boiler plate code.
> 
> SupportsWeakPtr causes subtle bugs and is deprecated. Please just implement
the
> boilerplate.

Removed entirely since it's not needed anymore.

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_gpu_platform_support_proxy.cc (right):

https://codereview.chromium.org/1311043016/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_gpu_platform_support_proxy.cc:66: bool
MessageFilterProxy::OnMessageReceived(const IPC::Message& message) {
On 2015/09/18 20:49:55, spang wrote:
> On 2015/09/18 17:43:08, dnicoara wrote:
> > On 2015/09/18 00:46:25, spang wrote:
> > > I would strongly prefer to just unpickle the messages here. Something like
> > > 
> > > IPC_MESSAGE_HANDLER(OzoneGpuMsg_CreateWindow, OnCreateWindow)
> > > // etc
> > > 
> > > void DrmMessageFilter(gfx::AcceleratedWidget widget) {
> > >   drm_thread_proxy_->CreateWindow(widget);
> > > }
> > > 
> > 
> > Why unpickle here? I don't see the benefit. It is cleaner to just pass the
> > message to the DrmThread and it would be handled internally to the
DrmThread.
> It
> > also reduces the need for boilerplate code.
> 
> You've already lost on the boilerplate code by having to switch on the message
> type.
> 
> I think you are worrying too much about adding boilerplate that is actually
> trivial and not enough about the fact that you needed different, more
> subtle/error-prone boilerplate instead. Like the whole RegisterSender() thing.
> Let's keep IPC centralized so it's easy to audit.
> 
> The DRM service thread should have an API that takes regular method calls,
which
> we call API from here and from GPU main. What you're calling
MessageFilterProxy
> is a the right place to terminate the IPC connection, type check the messages,
> and forward along only well-formed commands.

Done. I've stopped somewhere in between what we've last spoken of since I felt
like duplicating these calls didn't add any extra clarity.

[spang, 2015-09-29 22:55:37]

I'm happy without the DrmThreadProxy if you want to delete it. I do think it
it'd give a very strong separation between the service thread & everyone else,
but you're definitely correct that it's more verbose. So either way is fine with
me.

https://codereview.chromium.org/1311043016/diff/120001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread.cc (right):

https://codereview.chromium.org/1311043016/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.cc:93: scoped_ptr<DrmWindowProxy>
DrmThread::CreateWindowProxy(
I think it is inconsistent to put it here. In every other case, you're posting
to DrmThread from the caller. This is the only place that DrmThread does its own
post.

If you move it to GbmSurfaceFactory then it won't an exception anymore, and you
can all the non-lifecycle methods aren't thread safe (well, excpet
task_runner()).

WDYT?

https://codereview.chromium.org/1311043016/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.cc:123: void
DrmThread::OnCreateWindow(gfx::AcceleratedWidget widget) {
We should use the imperative

CreateWindow()

OnCreateWindow says "I'm responding to CreateWindow" which I think is fairly
misleading here. There's two events:

(1) delivering the CreateWindow IPC message
(2) actually creating the window

It's much clearer to refer to (2) here, and ignore (1) since it's not as
relevant. The only reason the older code said "On" was because it was directly
responding to (1).

Same goes for the rest of the commands below.

[dnicoara, 2015-09-30 15:17:57]

https://codereview.chromium.org/1311043016/diff/120001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread.cc (right):

https://codereview.chromium.org/1311043016/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.cc:93: scoped_ptr<DrmWindowProxy>
DrmThread::CreateWindowProxy(
On 2015/09/29 22:55:37, spang wrote:
> I think it is inconsistent to put it here. In every other case, you're posting
> to DrmThread from the caller. This is the only place that DrmThread does its
own
> post.
> 
> If you move it to GbmSurfaceFactory then it won't an exception anymore, and
you
> can all the non-lifecycle methods aren't thread safe (well, excpet
> task_runner()).
> 
> WDYT?

Yeah, that was bugging me a bit too. The DrmWindowProxy is still bugging me a
bit though. I find it bad to have the proxy creation and the window operation
calls all on DrmThread. The alternatives I could think of are:
1) Have the call to DrmThread::CreateWindow be a sync call, though the downside
is that it would block GPU IO.
2) Have something like DrmThread::Flush and wait on that in
GbmSurfaceFactory::CreateSurfacelessEGLSurfaceForWidget(). Then the whole
DrmWindowProxy can be deleted. The downside is that this is feels like a big
hammer.

What's your take on this?

https://codereview.chromium.org/1311043016/diff/120001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.cc:123: void
DrmThread::OnCreateWindow(gfx::AcceleratedWidget widget) {
On 2015/09/29 22:55:37, spang wrote:
> We should use the imperative
> 
> CreateWindow()
> 
> OnCreateWindow says "I'm responding to CreateWindow" which I think is fairly
> misleading here. There's two events:
> 
> (1) delivering the CreateWindow IPC message
> (2) actually creating the window
> 
> It's much clearer to refer to (2) here, and ignore (1) since it's not as
> relevant. The only reason the older code said "On" was because it was directly
> responding to (1).
> 
> Same goes for the rest of the commands below.

Done. Sounds reasonable.

[dnicoara, 2015-09-30 15:31:10]

On 2015/09/29 22:55:37, spang wrote:
> I'm happy without the DrmThreadProxy if you want to delete it. I do think it
> it'd give a very strong separation between the service thread & everyone else,
> but you're definitely correct that it's more verbose. So either way is fine
with
> me.
> 

To me it feels like the MessageFilter is really the proxy to the DrmThread.
Maybe something like:

class DrmThreadProxy {
 public:
  MessageFilterProxy* GetMessageFilterProxy();  // This would be the current
MessageFilter.
  DrmWindowProxy* GetWindowProxy();
};

[spang, 2015-09-30 15:43:44]

On 2015/09/30 15:31:10, dnicoara wrote:
> On 2015/09/29 22:55:37, spang wrote:
> > I'm happy without the DrmThreadProxy if you want to delete it. I do think it
> > it'd give a very strong separation between the service thread & everyone
else,
> > but you're definitely correct that it's more verbose. So either way is fine
> with
> > me.
> > 
> 
> To me it feels like the MessageFilter is really the proxy to the DrmThread.
> Maybe something like:
> 
> class DrmThreadProxy {
>  public:
>   MessageFilterProxy* GetMessageFilterProxy();  // This would be the current
> MessageFilter.
>   DrmWindowProxy* GetWindowProxy();
> };

I get what you're saying. I think these would be Create() and return scopers
though.

[dnicoara, 2015-09-30 18:06:41]

On 2015/09/30 15:43:44, spang wrote:
> On 2015/09/30 15:31:10, dnicoara wrote:
> > On 2015/09/29 22:55:37, spang wrote:
> > > I'm happy without the DrmThreadProxy if you want to delete it. I do think
it
> > > it'd give a very strong separation between the service thread & everyone
> else,
> > > but you're definitely correct that it's more verbose. So either way is
fine
> > with
> > > me.
> > > 
> > 
> > To me it feels like the MessageFilter is really the proxy to the DrmThread.
> > Maybe something like:
> > 
> > class DrmThreadProxy {
> >  public:
> >   MessageFilterProxy* GetMessageFilterProxy();  // This would be the current
> > MessageFilter.
> >   DrmWindowProxy* GetWindowProxy();
> > };
> 
> I get what you're saying. I think these would be Create() and return scopers
> though.

Done.

I need to think more about how DrmDeviceManager is being used. I think there are
corner cases that aren't handled correctly.

[spang, 2015-09-30 18:17:35]

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread_proxy.h (right):

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread_proxy.h:30:
scoped_refptr<MessageFilterProxy> CreateMessageFilterProxy();
Do we need to refcount?

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/message_filter_proxy.h (right):

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/message_filter_proxy.h:31: class MessageFilterProxy :
public IPC::MessageFilter {
I think DrmThread(Proxy?)MessageFilter.

Otherwise I parse this as "proxy for the message filter", but as there isn't a
message filter inside the thread to proxy it doesn't make much sense, while
"message filter for the drm thread" does.

[spang, 2015-09-30 18:47:31]

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread_proxy.cc (right):

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread_proxy.cc:29:
PostSyncTask(drm_thread_.task_runner(), base::Bind(&base::DoNothing));
Can you do the sync post in CreateNativePixmap instead and make this one async.

I think now that we have a thread we should be able to just get rid of
DrmDeviceManager and just use DrmWindow directly.

[spang, 2015-09-30 19:24:43]

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/gbm_surfaceless.cc (right):

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/gbm_surfaceless.cc:60:
device_manager_->GetDrmDevice(window_->widget());
For this part, let's add a

bool GbmBuffer::ShouldFinishBeforeSwap()  [name open for bikeshedding]

and set it when creating the buffer in the UDL case.

We can then track the glFinish requirement in GbmSurfaceless as buffers are
scheduled, and expose it as SurfaceOzoneEGL::ShouldFinishBeforeSwap() (which
gets called from GLSurfaceOzoneSurfaceless::SwapBuffers).

alexst: Please sanity check this.

[dnicoara, 2015-10-01 14:36:15]

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread_proxy.cc (right):

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread_proxy.cc:29:
PostSyncTask(drm_thread_.task_runner(), base::Bind(&base::DoNothing));
On 2015/09/30 18:47:31, spang wrote:
> Can you do the sync post in CreateNativePixmap instead and make this one
async.
> 
> I think now that we have a thread we should be able to just get rid of
> DrmDeviceManager and just use DrmWindow directly.

I was talking to alexst@ yesterday and I'll rework the DrmDeviceManager so we
don't need to access it directly (We'll be posting a sync task when allocating
buffers). That'll solve this issue.

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread_proxy.h (right):

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread_proxy.h:30:
scoped_refptr<MessageFilterProxy> CreateMessageFilterProxy();
On 2015/09/30 18:17:35, spang wrote:
> Do we need to refcount?

Unfortunately yes, because the base class, IPC::MessageFilter, is ref-counted.

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/gbm_surfaceless.cc (right):

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/gbm_surfaceless.cc:60:
device_manager_->GetDrmDevice(window_->widget());
On 2015/09/30 19:24:43, spang wrote:
> For this part, let's add a
> 
> bool GbmBuffer::ShouldFinishBeforeSwap()  [name open for bikeshedding]

I don't have a strong opinion on naming.
> 
> and set it when creating the buffer in the UDL case.
> 
> We can then track the glFinish requirement in GbmSurfaceless as buffers are
> scheduled, and expose it as SurfaceOzoneEGL::ShouldFinishBeforeSwap() (which
> gets called from GLSurfaceOzoneSurfaceless::SwapBuffers).
> 
> alexst: Please sanity check this.

Sure, I think that can work.

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/message_filter_proxy.h (right):

https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/message_filter_proxy.h:31: class MessageFilterProxy :
public IPC::MessageFilter {
On 2015/09/30 18:17:35, spang wrote:
> I think DrmThread(Proxy?)MessageFilter.
> 
> Otherwise I parse this as "proxy for the message filter", but as there isn't a
> message filter inside the thread to proxy it doesn't make much sense, while
> "message filter for the drm thread" does.

Hmm, I see what you mean. I was going for a uniform naming convention. The
"Filter" term doesn't really apply on the DRM thread side. How about
DrmThreadMessageProxy?

[spang, 2015-10-01 17:33:52]

On 2015/10/01 14:36:15, dnicoara wrote:
>
https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
> File ui/ozone/platform/drm/gpu/drm_thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
> ui/ozone/platform/drm/gpu/drm_thread_proxy.cc:29:
> PostSyncTask(drm_thread_.task_runner(), base::Bind(&base::DoNothing));
> On 2015/09/30 18:47:31, spang wrote:
> > Can you do the sync post in CreateNativePixmap instead and make this one
> async.
> > 
> > I think now that we have a thread we should be able to just get rid of
> > DrmDeviceManager and just use DrmWindow directly.
> 
> I was talking to alexst@ yesterday and I'll rework the DrmDeviceManager so we
> don't need to access it directly (We'll be posting a sync task when allocating
> buffers). That'll solve this issue.
> 
>
https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
> File ui/ozone/platform/drm/gpu/drm_thread_proxy.h (right):
> 
>
https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
> ui/ozone/platform/drm/gpu/drm_thread_proxy.h:30:
> scoped_refptr<MessageFilterProxy> CreateMessageFilterProxy();
> On 2015/09/30 18:17:35, spang wrote:
> > Do we need to refcount?
> 
> Unfortunately yes, because the base class, IPC::MessageFilter, is ref-counted.
> 
>
https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
> File ui/ozone/platform/drm/gpu/gbm_surfaceless.cc (right):
> 
>
https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
> ui/ozone/platform/drm/gpu/gbm_surfaceless.cc:60:
> device_manager_->GetDrmDevice(window_->widget());
> On 2015/09/30 19:24:43, spang wrote:
> > For this part, let's add a
> > 
> > bool GbmBuffer::ShouldFinishBeforeSwap()  [name open for bikeshedding]
> 
> I don't have a strong opinion on naming.
> > 
> > and set it when creating the buffer in the UDL case.
> > 
> > We can then track the glFinish requirement in GbmSurfaceless as buffers are
> > scheduled, and expose it as SurfaceOzoneEGL::ShouldFinishBeforeSwap() (which
> > gets called from GLSurfaceOzoneSurfaceless::SwapBuffers).
> > 
> > alexst: Please sanity check this.
> 
> Sure, I think that can work.
> 
>
https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
> File ui/ozone/platform/drm/gpu/message_filter_proxy.h (right):
> 
>
https://codereview.chromium.org/1311043016/diff/160001/ui/ozone/platform/drm/...
> ui/ozone/platform/drm/gpu/message_filter_proxy.h:31: class MessageFilterProxy
:
> public IPC::MessageFilter {
> On 2015/09/30 18:17:35, spang wrote:
> > I think DrmThread(Proxy?)MessageFilter.
> > 
> > Otherwise I parse this as "proxy for the message filter", but as there isn't
a
> > message filter inside the thread to proxy it doesn't make much sense, while
> > "message filter for the drm thread" does.
> 
> Hmm, I see what you mean. I was going for a uniform naming convention. The
> "Filter" term doesn't really apply on the DRM thread side. How about
> DrmThreadMessageProxy?

Shrug, ok.

[spang, 2015-10-05 18:08:26]

lgtm

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread.cc (right):

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.cc:111: window->SchedulePageFlip(planes,
callback);
Should this call the callback if there's no window?

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.cc:119:
window->GetVSyncParameters(callback);
Should this call the callback if there's no window?

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.h:39: // the exception of buffer
allocations which happen on the threads requiring the
need update

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.h:70: void
WindowBoundsChanged(gfx::AcceleratedWidget widget,
SetWindowBounds

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.h:72: void CursorSet(gfx::AcceleratedWidget
widget,
SetCursor for consistency

.. unless you want to change everything else (NativeDisplayConfigure, etc).

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.h:76: void
CursorMove(gfx::AcceleratedWidget widget, const gfx::Point& location);
MoveCursor

[dnicoara, 2015-10-05 18:57:12]

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread.cc (right):

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.cc:111: window->SchedulePageFlip(planes,
callback);
On 2015/10/05 18:08:26, spang wrote:
> Should this call the callback if there's no window?

Hmm, isn't required since the surface is going to be destroyed shortly. But I
can see why this is a bad idea. Added the else branch to call the callback.

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.cc:119:
window->GetVSyncParameters(callback);
On 2015/10/05 18:08:26, spang wrote:
> Should this call the callback if there's no window?

Not needed since the vsync provider doesn't mandate that the callback is ever
called. I've added a comment to explain this.

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.h:39: // the exception of buffer
allocations which happen on the threads requiring the
On 2015/10/05 18:08:26, spang wrote:
> need update

Done.

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.h:70: void
WindowBoundsChanged(gfx::AcceleratedWidget widget,
On 2015/10/05 18:08:26, spang wrote:
> SetWindowBounds

Done.

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.h:72: void CursorSet(gfx::AcceleratedWidget
widget,
On 2015/10/05 18:08:26, spang wrote:
> SetCursor for consistency
> 
> .. unless you want to change everything else (NativeDisplayConfigure, etc).

Done.

https://codereview.chromium.org/1311043016/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/drm_thread.h:76: void
CursorMove(gfx::AcceleratedWidget widget, const gfx::Point& location);
On 2015/10/05 18:08:26, spang wrote:
> MoveCursor

Done.

[dnicoara, 2015-10-09 15:32:04]

The CQ bit was checked by dnicoara@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-09 15:32:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1311043016/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1311043016/240001

[commit-bot: I haz the power, 2015-10-09 15:53:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-09 15:53:45]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[dnicoara, 2015-10-09 16:03:14]

The CQ bit was checked by dnicoara@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-09 16:07:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1311043016/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1311043016/260001

[commit-bot: I haz the power, 2015-10-09 16:44:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-09 16:44:31]

Dry run: This issue passed the CQ dry run.

[dnicoara, 2015-10-13 18:38:42]

The CQ bit was checked by dnicoara@chromium.org

[dnicoara, 2015-10-13 18:38:42]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1311043016/#ps260001
(title: "update & fix clang")

[commit-bot: I haz the power, 2015-10-13 18:42:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1311043016/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1311043016/260001

[commit-bot: I haz the power, 2015-10-13 19:19:38]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-10-13 19:20:27]

Patchset 14 (id:??) landed as
https://crrev.com/14630e92ee1cba7bc76643434944b99272a1deff
Cr-Commit-Position: refs/heads/master@{#353819}