Move Format checks to HardwareDisplayPlane

Move Format checks to HardwareDisplayPlane

IsSupportedFormat check should be common across Atomic or Legacy path,
hence we move the logic from HardwareDisplayPlaneAtomic to
HardwareDisplayPlane.

Committed: https://crrev.com/43f471825b028d141474c9ef453c2e71591b6421
Cr-Commit-Position: refs/heads/master@{#345899}

[kalyank, 2015-08-24 16:09:40]

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

[kalyank, 2015-08-24 17:22:00]

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

https://codereview.chromium.org/1314553002/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:36: uint32_t format_type =
0;
Depends on https://codereview.chromium.org/1294113005/ and needs to rebased
after that.

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

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

https://codereview.chromium.org/1314553002/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:8: #include <gbm.h>
The bots are not going to like this include since gbm isn't available on the
non-CrOS bots. This was fine on the atomic path since it is never compiled on
the bots.

Likely need to do the GBM_BO_FORMAT_* to DRM_FORMAT_* conversion in
gbm_buffer.cc for simplicity, then just assume DRM_FORMAT_* here (would you mind
updating the interface comment and mention what the format is supposed to be?)

FYI: I'm debating whether an explicit switch-case statement should be used for
GBM_BO_FORMAT_* to DRM_FORMAT_* for clarity. On the other hand it seems like it
shouldn't matter since the gbm version is identical. You may have a better
insight on this.

https://codereview.chromium.org/1314553002/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:49: return false;
drm_buffer.cc needs to also be updated to return the correct format.
Historically we didn't care about the software path since it has a limited use
case and just 1 format.

[kalyank, 2015-08-26 08:42:42]

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

https://codereview.chromium.org/1314553002/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:8: #include <gbm.h>
On 2015/08/25 19:27:16, dnicoara wrote:
> The bots are not going to like this include since gbm isn't available on the
> non-CrOS bots. This was fine on the atomic path since it is never compiled on
> the bots.
> 
> Likely need to do the GBM_BO_FORMAT_* to DRM_FORMAT_* conversion in
> gbm_buffer.cc for simplicity, then just assume DRM_FORMAT_* here (would you
mind
> updating the interface comment and mention what the format is supposed to be?)
> 
> FYI: I'm debating whether an explicit switch-case statement should be used for
> GBM_BO_FORMAT_* to DRM_FORMAT_* for clarity. On the other hand it seems like
it
> shouldn't matter since the gbm version is identical. You may have a better
> insight on this.

Done.

https://codereview.chromium.org/1314553002/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:8: #include <gbm.h>
On 2015/08/25 19:27:16, dnicoara wrote:
> The bots are not going to like this include since gbm isn't available on the
> non-CrOS bots. This was fine on the atomic path since it is never compiled on
> the bots.
> 
> Likely need to do the GBM_BO_FORMAT_* to DRM_FORMAT_* conversion in
> gbm_buffer.cc for simplicity, then just assume DRM_FORMAT_* here (would you
mind
> updating the interface comment and mention what the format is supposed to be?)
> 
> FYI: I'm debating whether an explicit switch-case statement should be used for
> GBM_BO_FORMAT_* to DRM_FORMAT_* for clarity. On the other hand it seems like
it
> shouldn't matter since the gbm version is identical. You may have a better
> insight on this.

GBM_BO_FORMAT_* are enum values. We have GBM_FORMAT_* which should be identical
to DRM_FORMAT_*. However, we need to pass GBM_FORMAT_* as format type during
buffer creation. This will make sure that gbm_bo_get_format, returns the right
value. I have made necessary changes for this in GbmBuffer

https://codereview.chromium.org/1314553002/diff/1/ui/ozone/platform/drm/gpu/h...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:49: return false;
On 2015/08/25 19:27:16, dnicoara wrote:
> drm_buffer.cc needs to also be updated to return the correct format.
> Historically we didn't care about the software path since it has a limited use
> case and just 1 format.

Done.

[dnicoara, 2015-08-26 14:37:02]

lgtm, could you re-run the failed bot.

[kalyank, 2015-08-27 08:29:15]

On 2015/08/26 14:37:02, dnicoara wrote:
> lgtm, could you re-run the failed bot.

Have added some basic tests for checking formats. 
Can you please take another look.

[dnicoara, 2015-08-27 13:21:56]

lgtm with nits

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

https://codereview.chromium.org/1314553002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:68: if
(supported_formats_.empty())
Could you just drop this if-statement and pass in DRM_FORMAT_XRGB8888 when the
dummy plane is initialized?

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

https://codereview.chromium.org/1314553002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_unittest.cc:5: #include
<drm_fourcc.h>
empty line between C and C++ includes

[kalyank, 2015-08-27 16:07:10]

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

https://codereview.chromium.org/1314553002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane.cc:68: if
(supported_formats_.empty())
On 2015/08/27 13:21:56, dnicoara wrote:
> Could you just drop this if-statement and pass in DRM_FORMAT_XRGB8888 when the
> dummy plane is initialized?

Done.

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

https://codereview.chromium.org/1314553002/diff/140001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/gpu/hardware_display_plane_manager_unittest.cc:5: #include
<drm_fourcc.h>
On 2015/08/27 13:21:56, dnicoara wrote:
> empty line between C and C++ includes

Done.

[kalyank, 2015-08-27 16:11:30]

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

[kalyank, 2015-08-27 16:11:30]

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/1314553002/#ps160001
(title: "Review fixes")

[commit-bot: I haz the power, 2015-08-27 16:11:55]

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

[commit-bot: I haz the power, 2015-08-27 16:41:46]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-08-27 16:42:26]

Patchset 9 (id:??) landed as
https://crrev.com/43f471825b028d141474c9ef453c2e71591b6421
Cr-Commit-Position: refs/heads/master@{#345899}