Atomic: Don’t keep track of Cursor planes

Atomic: Don’t keep track of Cursor planes

CRTC controller always assume they have a cursor plane and it is updated
via cursor specific DRM API. In our side, cursor plane is part of the
planes list which are free to be used. Now, we dont keep track of these
planes as we shouldn’t try to use them for any other purpose.

Committed: https://crrev.com/2238227bae73cc03d5dc388f50255dac52951c2e
Cr-Commit-Position: refs/heads/master@{#345398}

[kalyank, 2015-08-19 22:18:37]

kalyan.kondapally@intel.com changed reviewers:
+ achaulk@chromium.org, alexst@chromium.org, dnicoara@chromium.org

[dnicoara, 2015-08-20 14:22:46]

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/c...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:152: if
(plane->CanUseForCrtc(crtc_) && !plane->owning_crtc() &&
I think that the plane allocation logic will serve cursor planes with the rest
of the planes during HDCM::AssignOverlayPlanes() which will cause this to return
no cursor planes. We don't want to end up in a situation where we're asked to
show a cursor but fail because the cursor plane is displaying some different
overlay.

This should take the simpler approach of always assuming the cursor plane is
allocated to the CRTC.

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/h...
File ui/ozone/platform/drm/gpu/hardware_display_plane.h (right):

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane.h:25: enum Type { Primary,
Overlay, Cursor, Dummy };
As per the coding guide, please use MACRO_STYLE for enum values.

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/h...
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc (right):

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:134: switch (value) {
Could you please split this in a helper function? Then just have something like
"type_ = GetPlaneType(plane_props->prop_values[i]);"

[kalyank, 2015-08-20 16:36:18]

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/c...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:152: if
(plane->CanUseForCrtc(crtc_) && !plane->owning_crtc() &&
On 2015/08/20 14:22:45, dnicoara wrote:
> I think that the plane allocation logic will serve cursor planes with the rest
> of the planes during HDCM::AssignOverlayPlanes() which will cause this to
return
> no cursor planes. We don't want to end up in a situation where we're asked to
> show a cursor but fail because the cursor plane is displaying some different
> overlay.
> 
> This should take the simpler approach of always assuming the cursor plane is
> allocated to the CRTC.

Done.

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/c...
ui/ozone/platform/drm/gpu/crtc_controller.cc:152: if
(plane->CanUseForCrtc(crtc_) && !plane->owning_crtc() &&
On 2015/08/20 14:22:45, dnicoara wrote:
> I think that the plane allocation logic will serve cursor planes with the rest
> of the planes during HDCM::AssignOverlayPlanes() which will cause this to
return
> no cursor planes. We don't want to end up in a situation where we're asked to
> show a cursor but fail because the cursor plane is displaying some different
> overlay.
> 
> This should take the simpler approach of always assuming the cursor plane is
> allocated to the CRTC.
Removed the extra check related to owned crtc.

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/h...
File ui/ozone/platform/drm/gpu/hardware_display_plane.h (right):

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane.h:25: enum Type { Primary,
Overlay, Cursor, Dummy };
On 2015/08/20 14:22:45, dnicoara wrote:
> As per the coding guide, please use MACRO_STYLE for enum values.

Done.

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/h...
File ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc (right):

https://codereview.chromium.org/1294113005/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane_atomic.cc:134: switch (value) {
On 2015/08/20 14:22:46, dnicoara wrote:
> Could you please split this in a helper function? Then just have something
like
> "type_ = GetPlaneType(plane_props->prop_values[i]);"

Done.

[dnicoara, 2015-08-21 21:38:38]

https://codereview.chromium.org/1294113005/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1294113005/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/crtc_controller.cc:148: if (!cursor_plane_) {
I was thinking that HardwareDisplayPlaneManager should mark them as always in
use. That would simplify the remainder of the logic since the CRTC controllers
always assume they have a cursor plane and the cursor plane is updated via a
cursor specific DRM API.

[kalyank, 2015-08-21 22:05:59]

https://codereview.chromium.org/1294113005/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):

https://codereview.chromium.org/1294113005/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/crtc_controller.cc:148: if (!cursor_plane_) {
On 2015/08/21 21:38:38, dnicoara wrote:
> I was thinking that HardwareDisplayPlaneManager should mark them as always in
> use. That would simplify the remainder of the logic since the CRTC controllers
> always assume they have a cursor plane and the cursor plane is updated via a
> cursor specific DRM API.

K, I have changed so that we dont even keep track of Cursor planes in
HardwareDisplayManager (As we dont do anything with them later).

[kalyank, 2015-08-21 22:06:23]

On 2015/08/21 22:05:59, kalyank wrote:
>
https://codereview.chromium.org/1294113005/diff/20001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/crtc_controller.cc (right):
> 
>
https://codereview.chromium.org/1294113005/diff/20001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/crtc_controller.cc:148: if (!cursor_plane_) {
> On 2015/08/21 21:38:38, dnicoara wrote:
> > I was thinking that HardwareDisplayPlaneManager should mark them as always
in
> > use. That would simplify the remainder of the logic since the CRTC
controllers
> > always assume they have a cursor plane and the cursor plane is updated via a
> > cursor specific DRM API.
> 
> K, I have changed so that we dont even keep track of Cursor planes in
> HardwareDisplayManager (As we dont do anything with them later).


Also, I have also reworked so that the logic to query plane type is not only
Atomic specific, as I think
it would be nice if we can avoid deviating too much between both paths (Atomic &
Legacy).

[kalyank, 2015-08-22 01:54:38]

https://codereview.chromium.org/1294113005/diff/80001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/80001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:59:
ScopedDrmObjectPropertyPtr plane_props(drmModeObjectGetProperties(
Sorry, the previous reply got badly formatted.
I reworked the changes in HardwareDisplayPlaneAtomic and moved them to
HardwareDisplayPlane. Now, legacy and Atomic paths will exercise these changes.
It would be nice if we can keep the code common for both paths.

[dnicoara, 2015-08-24 13:39:50]

Thanks, lgtm.

https://codereview.chromium.org/1294113005/diff/80001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_plane.h (right):

https://codereview.chromium.org/1294113005/diff/80001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.h:52: const
ScopedDrmObjectPropertyPtr& plane_props);
nit: add empty line after.

[kalyank, 2015-08-24 16:26:10]

https://codereview.chromium.org/1294113005/diff/80001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/hardware_display_plane.h (right):

https://codereview.chromium.org/1294113005/diff/80001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/hardware_display_plane.h:52: const
ScopedDrmObjectPropertyPtr& plane_props);
On 2015/08/24 13:39:50, dnicoara wrote:
> nit: add empty line after.

Done.

[kalyank, 2015-08-24 19:59:22]

On 2015/08/24 13:39:50, dnicoara wrote:
> Thanks, lgtm.
> 
>
https://codereview.chromium.org/1294113005/diff/80001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/hardware_display_plane.h (right):
> 
>
https://codereview.chromium.org/1294113005/diff/80001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/hardware_display_plane.h:52: const
> ScopedDrmObjectPropertyPtr& plane_props);
> nit: add empty line after.

can you please take another look as I needed to add things in 
HardwareDisplayplane(Adding missing defines and removing fourcc header) 
to fix cast_shell linux build

[dnicoara, 2015-08-25 16:27:13]

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:17: #define FOURCC(a, b, c,
d)                                    \
#defines shouldn't be in a namespace

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:17: #define FOURCC(a, b, c,
d)                                    \
What was the issue with including drm_fourcc.h?

[kalyank, 2015-08-25 16:38:09]

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:17: #define FOURCC(a, b, c,
d)                                    \
On 2015/08/25 16:27:13, dnicoara wrote:
> #defines shouldn't be in a namespace

Acknowledged.

[kalyank, 2015-08-25 16:39:47]

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:17: #define FOURCC(a, b, c,
d)                                    \
On 2015/08/25 16:27:13, dnicoara wrote:
> What was the issue with including drm_fourcc.h?

chromeos_daisy_chromium_compile_only_ng, cast_shelllinux failed with the
following error:
"/daisy+7391.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/libdrm
-march=armv7-a -mtune=generic-armv7-a -mfpu=neon -mfloat-abi=hard -mthumb
--sysroot=/b/build/slave/chromeos_daisy/build/.cros_cache/chrome-sdk/tarballs/daisy+7391.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz
-O2 -fno-ident -fdata-sections -ffunction-sections -funwind-tables -pipe
-march=armv7-a -mtune=cortex-a15 -mfpu=neon -mfloat-abi=hard
-D__google_stl_debug_vector=1 -fno-exceptions -fno-rtti -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wsign-compare -Wno-abi -std=gnu++11 -Wno-narrowing
-Wno-literal-suffix  -c
../../ui/ozone/platform/drm/gpu/hardware_display_plane.cc -o
obj/ui/ozone/platform/drm/gpu/ozone_platform_drm.hardware_display_plane.o
../../ui/ozone/platform/drm/gpu/hardware_display_plane.cc:8:28: fatal error:
drm/drm_fourcc.h: No such file or directory
 #include <drm/drm_fourcc.h>
                            ^
compilation terminated.
ninja: build stopped: subcommand failed."

[kalyank, 2015-08-25 16:54:37]

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:17: #define FOURCC(a, b, c,
d)                                    \
On 2015/08/25 16:27:13, dnicoara wrote:
> #defines shouldn't be in a namespace

Done.

[dnicoara, 2015-08-25 17:35:32]

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:17: #define FOURCC(a, b, c,
d)                                    \
On 2015/08/25 16:39:47, kalyank wrote:
> On 2015/08/25 16:27:13, dnicoara wrote:
> > What was the issue with including drm_fourcc.h?
> 
> chromeos_daisy_chromium_compile_only_ng, cast_shelllinux failed with the
> following error:
>
"/daisy+7391.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/libdrm
> -march=armv7-a -mtune=generic-armv7-a -mfpu=neon -mfloat-abi=hard -mthumb
>
--sysroot=/b/build/slave/chromeos_daisy/build/.cros_cache/chrome-sdk/tarballs/daisy+7391.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz
> -O2 -fno-ident -fdata-sections -ffunction-sections -funwind-tables -pipe
> -march=armv7-a -mtune=cortex-a15 -mfpu=neon -mfloat-abi=hard
> -D__google_stl_debug_vector=1 -fno-exceptions -fno-rtti
-fno-threadsafe-statics
> -fvisibility-inlines-hidden -Wsign-compare -Wno-abi -std=gnu++11
-Wno-narrowing
> -Wno-literal-suffix  -c
> ../../ui/ozone/platform/drm/gpu/hardware_display_plane.cc -o
> obj/ui/ozone/platform/drm/gpu/ozone_platform_drm.hardware_display_plane.o
> ../../ui/ozone/platform/drm/gpu/hardware_display_plane.cc:8:28: fatal error:
> drm/drm_fourcc.h: No such file or directory
>  #include <drm/drm_fourcc.h>
>                             ^
> compilation terminated.
> ninja: build stopped: subcommand failed."

Silly me, I was looking at the wrong version.

I found this odd and looked at the compiler output. It looks like the includes
contain the "<sysroot>/usr/include/libdrm" path and drm_fourcc.h is there. Could
you just drop the "drm" folder in the path? That seemed to have worked for me
locally.

[kalyank, 2015-08-25 17:56:18]

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:17: #define FOURCC(a, b, c,
d)                                    \
On 2015/08/25 17:35:32, dnicoara wrote:
> On 2015/08/25 16:39:47, kalyank wrote:
> > On 2015/08/25 16:27:13, dnicoara wrote:
> > > What was the issue with including drm_fourcc.h?
> > 
> > chromeos_daisy_chromium_compile_only_ng, cast_shelllinux failed with the
> > following error:
> >
>
"/daisy+7391.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/libdrm
> > -march=armv7-a -mtune=generic-armv7-a -mfpu=neon -mfloat-abi=hard -mthumb
> >
>
--sysroot=/b/build/slave/chromeos_daisy/build/.cros_cache/chrome-sdk/tarballs/daisy+7391.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz
> > -O2 -fno-ident -fdata-sections -ffunction-sections -funwind-tables -pipe
> > -march=armv7-a -mtune=cortex-a15 -mfpu=neon -mfloat-abi=hard
> > -D__google_stl_debug_vector=1 -fno-exceptions -fno-rtti
> -fno-threadsafe-statics
> > -fvisibility-inlines-hidden -Wsign-compare -Wno-abi -std=gnu++11
> -Wno-narrowing
> > -Wno-literal-suffix  -c
> > ../../ui/ozone/platform/drm/gpu/hardware_display_plane.cc -o
> > obj/ui/ozone/platform/drm/gpu/ozone_platform_drm.hardware_display_plane.o
> > ../../ui/ozone/platform/drm/gpu/hardware_display_plane.cc:8:28: fatal error:
> > drm/drm_fourcc.h: No such file or directory
> >  #include <drm/drm_fourcc.h>
> >                             ^
> > compilation terminated.
> > ninja: build stopped: subcommand failed."
> 
> Silly me, I was looking at the wrong version.
> 
> I found this odd and looked at the compiler output. It looks like the includes
> contain the "<sysroot>/usr/include/libdrm" path and drm_fourcc.h is there.
Could
> you just drop the "drm" folder in the path? That seemed to have worked for me
> locally.

Thanks, should have checked that. Waiting for trybot results now.

[kalyank, 2015-08-25 18:12:49]

On 2015/08/25 17:56:18, kalyank wrote:
>
https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
> File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):
> 
>
https://codereview.chromium.org/1294113005/diff/140001/ui/ozone/platform/drm/...
> ui/ozone/platform/drm/gpu/hardware_display_plane.cc:17: #define FOURCC(a, b,
c,
> d)                                    \
> On 2015/08/25 17:35:32, dnicoara wrote:
> > On 2015/08/25 16:39:47, kalyank wrote:
> > > On 2015/08/25 16:27:13, dnicoara wrote:
> > you just drop the "drm" folder in the path? That seemed to have worked for
me
> > locally.
> 
> Thanks, should have checked that. Waiting for trybot results now.

That fixed things, all bots seem to be happy now.

[dnicoara, 2015-08-25 18:19:13]

lgtm with change required by the style guide

https://codereview.chromium.org/1294113005/diff/180001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:7: #include <drm_fourcc.h>
Empty line between different include types (system "C", system "C++", local
chrome)

[kalyank, 2015-08-25 18:31:55]

https://codereview.chromium.org/1294113005/diff/180001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/gpu/hardware_display_plane.cc (right):

https://codereview.chromium.org/1294113005/diff/180001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:7: #include <drm_fourcc.h>
On 2015/08/25 18:19:13, dnicoara wrote:
> Empty line between different include types (system "C", system "C++", local
> chrome)

Done.

[kalyank, 2015-08-25 18:33:04]

The CQ bit was checked by kalyan.kondapally@intel.com

[kalyank, 2015-08-25 18:33:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dnicoara@chromium.org
Link to the patchset: https://codereview.chromium.org/1294113005/#ps200001
(title: "Style fix")

[commit-bot: I haz the power, 2015-08-25 18:34:04]

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

[commit-bot: I haz the power, 2015-08-25 19:05:18]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-25 19:06:00]

Patchset 11 (id:??) landed as
https://crrev.com/2238227bae73cc03d5dc388f50255dac52951c2e
Cr-Commit-Position: refs/heads/master@{#345398}