[hwc] drm_hwcomposer: Support assigning planes in ValidateDisplay

Message ID 20180517220816.31504-1-robh@kernel.org
State New
Headers show
Series
  • [hwc] drm_hwcomposer: Support assigning planes in ValidateDisplay
Related show

Commit Message

Rob Herring May 17, 2018, 10:08 p.m.
In order to assign planes to layers in ValidateDisplay, testing
compositing with a DRM atomic modeset test is needed as PresentDisplay
is too late. This means most of PresentDisplay needs to be run from
ValidateDisplay, so refactor PresentDisplay and plumb a test flag down
the stack.

Signed-off-by: Rob Herring <robh@kernel.org>
---
Based on my GL comp removal work. Adding atomic test path turned out to 
be easier than using the Planner directly.

 drmdisplaycompositor.cpp |  4 +-
 drmdisplaycompositor.h   |  2 +-
 drmhwctwo.cpp            | 79 ++++++++++++++++++++++++++++++++++------
 drmhwctwo.h              |  1 +
 4 files changed, 73 insertions(+), 13 deletions(-)

Comments

Stefan Schake May 17, 2018, 11:25 p.m. | #1
Hey Rob,

On Fri, May 18, 2018 at 12:08 AM, Rob Herring <robh@kernel.org> wrote:
> In order to assign planes to layers in ValidateDisplay, testing
> compositing with a DRM atomic modeset test is needed as PresentDisplay
> is too late. This means most of PresentDisplay needs to be run from
> ValidateDisplay, so refactor PresentDisplay and plumb a test flag down
> the stack.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Based on my GL comp removal work. Adding atomic test path turned out to
> be easier than using the Planner directly.
>
>  drmdisplaycompositor.cpp |  4 +-
>  drmdisplaycompositor.h   |  2 +-
>  drmhwctwo.cpp            | 79 ++++++++++++++++++++++++++++++++++------
>  drmhwctwo.h              |  1 +
>  4 files changed, 73 insertions(+), 13 deletions(-)
>
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index 2f9f6c6772da..ceee6b1a4bd6 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -469,7 +469,7 @@ void DrmDisplayCompositor::ApplyFrame(
>  }
>
>  int DrmDisplayCompositor::ApplyComposition(
> -    std::unique_ptr<DrmDisplayComposition> composition) {
> +    std::unique_ptr<DrmDisplayComposition> composition, bool test) {

Since ApplyComposition just calls CommitFrame which already has test
support, can we reuse that for a TestComposition function?

>    int ret = 0;
>    switch (composition->type()) {
>      case DRM_COMPOSITION_TYPE_FRAME:
> @@ -481,6 +481,8 @@ int DrmDisplayCompositor::ApplyComposition(
>            ALOGE("Commit test failed for display %d, FIXME", display_);
>            return ret;
>          }
> +        if (test)
> +          return 0;

Then we could also drop this hunk.

>        }
>
>        ApplyFrame(std::move(composition), ret);
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index ed46873c3409..7c5ebb005dc4 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -43,7 +43,7 @@ class DrmDisplayCompositor {
>    int Init(DrmResources *drm, int display);
>
>    std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
> -  int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
> +  int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition, bool test);

And this one.

>    int Composite();
>    void Dump(std::ostringstream *out) const;
>
> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> index deaf14363e8c..19f73e721154 100644
> --- a/drmhwctwo.cpp
> +++ b/drmhwctwo.cpp
> @@ -480,8 +480,7 @@ void DrmHwcTwo::HwcDisplay::AddFenceToRetireFence(int fd) {
>    }
>  }
>
> -HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
> -  supported(__func__);
> +HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) {

This is called CreateComposition, but it also tests and even commits.
If this only sets up the DrmDisplayComposition, we could just have the
test and commit in Validate/Present respectively and drop the flag.
Unfortunately it also uses and mutates the layer state, and moving that
into Present/Validate from here looks more difficult.

>    std::vector<DrmCompositionDisplayLayersMap> layers_map;
>    layers_map.emplace_back();
>    DrmCompositionDisplayLayersMap &map = layers_map.back();
> @@ -494,7 +493,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>    uint32_t client_z_order = 0;
>    std::map<uint32_t, DrmHwcTwo::HwcLayer *> z_map;
>    for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
> -    switch (l.second.validated_type()) {
> +    HWC2::Composition comp_type;
> +    if (test)
> +      comp_type = l.second.sf_type();
> +    else
> +      comp_type = l.second.validated_type();
> +
> +    switch (comp_type) {
>        case HWC2::Composition::Device:
>          z_map.emplace(std::make_pair(l.second.z_order(), &l.second));
>          break;
> @@ -510,6 +515,10 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>    if (use_client_layer)
>      z_map.emplace(std::make_pair(client_z_order, &client_layer_));
>
> +  if (z_map.empty())
> +    return HWC2::Error::BadLayer;
> +
> +

Extra newline.

>    // now that they're ordered by z, add them to the composition
>    for (std::pair<const uint32_t, DrmHwcTwo::HwcLayer *> &l : z_map) {
>      DrmHwcLayer layer;
> @@ -521,10 +530,6 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>      }
>      map.layers.emplace_back(std::move(layer));
>    }
> -  if (map.layers.empty()) {
> -    *retire_fence = -1;
> -    return HWC2::Error::None;
> -  }
>
>    std::unique_ptr<DrmDisplayComposition> composition =
>        compositor_.CreateComposition();
> @@ -555,13 +560,29 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>      i = overlay_planes.erase(i);
>    }
>
> -  AddFenceToRetireFence(composition->take_out_fence());
> +  if (!test)
> +    AddFenceToRetireFence(composition->take_out_fence());
>
> -  ret = compositor_.ApplyComposition(std::move(composition));
> +  ret = compositor_.ApplyComposition(std::move(composition), test);
>    if (ret) {
>      ALOGE("Failed to apply the frame composition ret=%d", ret);
>      return HWC2::Error::BadParameter;
>    }
> +  return HWC2::Error::None;
> +}
> +
> +HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
> +  supported(__func__);
> +  HWC2::Error ret;
> +
> +  ret = CreateComposition(false);
> +  if (ret == HWC2::Error::BadLayer) {
> +      // Can we really have no client or device layers?
> +      *retire_fence = -1;
> +      return HWC2::Error::None;
> +  }
> +  if (ret != HWC2::Error::None)
> +    return ret;
>
>    // The retire fence returned here is for the last frame, so return it and
>    // promote the next retire fence
> @@ -586,7 +607,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetActiveConfig(hwc2_config_t config) {
>        compositor_.CreateComposition();
>    composition->Init(drm_, crtc_, importer_.get(), planner_.get(), frame_no_);
>    int ret = composition->SetDisplayMode(*mode);
> -  ret = compositor_.ApplyComposition(std::move(composition));
> +  ret = compositor_.ApplyComposition(std::move(composition), false);
>    if (ret) {
>      ALOGE("Failed to queue dpms composition on %d", ret);
>      return HWC2::Error::BadConfig;
> @@ -666,7 +687,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetPowerMode(int32_t mode_in) {
>        compositor_.CreateComposition();
>    composition->Init(drm_, crtc_, importer_.get(), planner_.get(), frame_no_);
>    composition->SetDpmsMode(dpms_value);
> -  int ret = compositor_.ApplyComposition(std::move(composition));
> +  int ret = compositor_.ApplyComposition(std::move(composition), false);

With a TestComposition we can drop the two hunks here.

>    if (ret) {
>      ALOGE("Failed to apply the dpms composition ret=%d", ret);
>      return HWC2::Error::BadParameter;
> @@ -683,11 +704,47 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetVsyncEnabled(int32_t enabled) {
>  HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>                                                     uint32_t *num_requests) {
>    supported(__func__);
> +  size_t plane_count = 0;
>    *num_types = 0;
>    *num_requests = 0;
> +  size_t avail_planes = primary_planes_.size() + overlay_planes_.size();
> +
> +  HWC2::Error ret;
> +
> +  for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_)
> +    l.second.set_validated_type(HWC2::Composition::Invalid);
> +
> +  ret = CreateComposition(true);
> +  if (ret != HWC2::Error::None)
> +    // Assume the test failed due to overlay planes
> +    avail_planes = 1;
> +
> +  std::map<uint32_t, DrmHwcTwo::HwcLayer *> z_map;
> +  for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
> +    if (l.second.sf_type() == HWC2::Composition::Device)
> +        z_map.emplace(std::make_pair(l.second.z_order(), &l.second));
> +  }
> +
> +  /*
> +   * If more layers then planes, save one plane
> +   * for client composited layers
> +   */
> +  if (avail_planes < layers_.size())
> +       avail_planes--;
> +
> +  for (std::pair<const uint32_t, DrmHwcTwo::HwcLayer *> &l : z_map) {
> +    if (!avail_planes--)
> +        break;
> +    l.second->set_validated_type(HWC2::Composition::Device);
> +  }
> +
>    for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
>      DrmHwcTwo::HwcLayer &layer = l.second;
>      switch (layer.sf_type()) {
> +      case HWC2::Composition::Device:
> +        if (layer.validated_type() == HWC2::Composition::Device)
> +          break;
> +        // fall thru
>        case HWC2::Composition::SolidColor:
>        case HWC2::Composition::Cursor:
>        case HWC2::Composition::Sideband:
> diff --git a/drmhwctwo.h b/drmhwctwo.h
> index 0490e2ac7f85..bbf55d9a08bc 100644
> --- a/drmhwctwo.h
> +++ b/drmhwctwo.h
> @@ -171,6 +171,7 @@ class DrmHwcTwo : public hwc2_device_t {
>                                     float *min_luminance);
>      HWC2::Error GetReleaseFences(uint32_t *num_elements, hwc2_layer_t *layers,
>                                   int32_t *fences);
> +    HWC2::Error CreateComposition(bool test);

This is more of a private function than a HWC2 hook.

>      HWC2::Error PresentDisplay(int32_t *retire_fence);
>      HWC2::Error SetActiveConfig(hwc2_config_t config);
>      HWC2::Error SetClientTarget(buffer_handle_t target, int32_t acquire_fence,
> --
> 2.17.0
>

Thanks for working on this! Fixup for the nits above:

https://gitlab.freedesktop.org/stschake/drm-hwcomposer/commit/3aa0eb88

Took a short look at isolating the composition, too, but it's pretty
interwoven with the layer state.

Regards,
Stefan
Rob Herring May 18, 2018, 1:21 p.m. | #2
On Thu, May 17, 2018 at 6:25 PM, Stefan Schake <stschake@gmail.com> wrote:
> Hey Rob,
>
> On Fri, May 18, 2018 at 12:08 AM, Rob Herring <robh@kernel.org> wrote:
>> In order to assign planes to layers in ValidateDisplay, testing
>> compositing with a DRM atomic modeset test is needed as PresentDisplay
>> is too late. This means most of PresentDisplay needs to be run from
>> ValidateDisplay, so refactor PresentDisplay and plumb a test flag down
>> the stack.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> Based on my GL comp removal work. Adding atomic test path turned out to
>> be easier than using the Planner directly.
>>
>>  drmdisplaycompositor.cpp |  4 +-
>>  drmdisplaycompositor.h   |  2 +-
>>  drmhwctwo.cpp            | 79 ++++++++++++++++++++++++++++++++++------
>>  drmhwctwo.h              |  1 +
>>  4 files changed, 73 insertions(+), 13 deletions(-)
>>
>> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
>> index 2f9f6c6772da..ceee6b1a4bd6 100644
>> --- a/drmdisplaycompositor.cpp
>> +++ b/drmdisplaycompositor.cpp
>> @@ -469,7 +469,7 @@ void DrmDisplayCompositor::ApplyFrame(
>>  }
>>
>>  int DrmDisplayCompositor::ApplyComposition(
>> -    std::unique_ptr<DrmDisplayComposition> composition) {
>> +    std::unique_ptr<DrmDisplayComposition> composition, bool test) {
>
> Since ApplyComposition just calls CommitFrame which already has test
> support, can we reuse that for a TestComposition function?
>
>>    int ret = 0;
>>    switch (composition->type()) {
>>      case DRM_COMPOSITION_TYPE_FRAME:
>> @@ -481,6 +481,8 @@ int DrmDisplayCompositor::ApplyComposition(
>>            ALOGE("Commit test failed for display %d, FIXME", display_);
>>            return ret;
>>          }
>> +        if (test)
>> +          return 0;
>
> Then we could also drop this hunk.
>
>>        }
>>
>>        ApplyFrame(std::move(composition), ret);
>> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
>> index ed46873c3409..7c5ebb005dc4 100644
>> --- a/drmdisplaycompositor.h
>> +++ b/drmdisplaycompositor.h
>> @@ -43,7 +43,7 @@ class DrmDisplayCompositor {
>>    int Init(DrmResources *drm, int display);
>>
>>    std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
>> -  int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
>> +  int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition, bool test);
>
> And this one.
>
>>    int Composite();
>>    void Dump(std::ostringstream *out) const;
>>
>> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
>> index deaf14363e8c..19f73e721154 100644
>> --- a/drmhwctwo.cpp
>> +++ b/drmhwctwo.cpp
>> @@ -480,8 +480,7 @@ void DrmHwcTwo::HwcDisplay::AddFenceToRetireFence(int fd) {
>>    }
>>  }
>>
>> -HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>> -  supported(__func__);
>> +HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) {
>
> This is called CreateComposition, but it also tests and even commits.
> If this only sets up the DrmDisplayComposition, we could just have the
> test and commit in Validate/Present respectively and drop the flag.
> Unfortunately it also uses and mutates the layer state, and moving that
> into Present/Validate from here looks more difficult.
>
>>    std::vector<DrmCompositionDisplayLayersMap> layers_map;
>>    layers_map.emplace_back();
>>    DrmCompositionDisplayLayersMap &map = layers_map.back();
>> @@ -494,7 +493,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>>    uint32_t client_z_order = 0;
>>    std::map<uint32_t, DrmHwcTwo::HwcLayer *> z_map;
>>    for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
>> -    switch (l.second.validated_type()) {
>> +    HWC2::Composition comp_type;
>> +    if (test)
>> +      comp_type = l.second.sf_type();
>> +    else
>> +      comp_type = l.second.validated_type();
>> +
>> +    switch (comp_type) {
>>        case HWC2::Composition::Device:
>>          z_map.emplace(std::make_pair(l.second.z_order(), &l.second));
>>          break;
>> @@ -510,6 +515,10 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>>    if (use_client_layer)
>>      z_map.emplace(std::make_pair(client_z_order, &client_layer_));
>>
>> +  if (z_map.empty())
>> +    return HWC2::Error::BadLayer;
>> +
>> +
>
> Extra newline.
>
>>    // now that they're ordered by z, add them to the composition
>>    for (std::pair<const uint32_t, DrmHwcTwo::HwcLayer *> &l : z_map) {
>>      DrmHwcLayer layer;
>> @@ -521,10 +530,6 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>>      }
>>      map.layers.emplace_back(std::move(layer));
>>    }
>> -  if (map.layers.empty()) {
>> -    *retire_fence = -1;
>> -    return HWC2::Error::None;
>> -  }
>>
>>    std::unique_ptr<DrmDisplayComposition> composition =
>>        compositor_.CreateComposition();
>> @@ -555,13 +560,29 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>>      i = overlay_planes.erase(i);
>>    }
>>
>> -  AddFenceToRetireFence(composition->take_out_fence());
>> +  if (!test)
>> +    AddFenceToRetireFence(composition->take_out_fence());
>>
>> -  ret = compositor_.ApplyComposition(std::move(composition));
>> +  ret = compositor_.ApplyComposition(std::move(composition), test);
>>    if (ret) {
>>      ALOGE("Failed to apply the frame composition ret=%d", ret);
>>      return HWC2::Error::BadParameter;
>>    }
>> +  return HWC2::Error::None;
>> +}
>> +
>> +HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>> +  supported(__func__);
>> +  HWC2::Error ret;
>> +
>> +  ret = CreateComposition(false);
>> +  if (ret == HWC2::Error::BadLayer) {
>> +      // Can we really have no client or device layers?
>> +      *retire_fence = -1;
>> +      return HWC2::Error::None;
>> +  }
>> +  if (ret != HWC2::Error::None)
>> +    return ret;
>>
>>    // The retire fence returned here is for the last frame, so return it and
>>    // promote the next retire fence
>> @@ -586,7 +607,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetActiveConfig(hwc2_config_t config) {
>>        compositor_.CreateComposition();
>>    composition->Init(drm_, crtc_, importer_.get(), planner_.get(), frame_no_);
>>    int ret = composition->SetDisplayMode(*mode);
>> -  ret = compositor_.ApplyComposition(std::move(composition));
>> +  ret = compositor_.ApplyComposition(std::move(composition), false);
>>    if (ret) {
>>      ALOGE("Failed to queue dpms composition on %d", ret);
>>      return HWC2::Error::BadConfig;
>> @@ -666,7 +687,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetPowerMode(int32_t mode_in) {
>>        compositor_.CreateComposition();
>>    composition->Init(drm_, crtc_, importer_.get(), planner_.get(), frame_no_);
>>    composition->SetDpmsMode(dpms_value);
>> -  int ret = compositor_.ApplyComposition(std::move(composition));
>> +  int ret = compositor_.ApplyComposition(std::move(composition), false);
>
> With a TestComposition we can drop the two hunks here.
>
>>    if (ret) {
>>      ALOGE("Failed to apply the dpms composition ret=%d", ret);
>>      return HWC2::Error::BadParameter;
>> @@ -683,11 +704,47 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetVsyncEnabled(int32_t enabled) {
>>  HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>>                                                     uint32_t *num_requests) {
>>    supported(__func__);
>> +  size_t plane_count = 0;
>>    *num_types = 0;
>>    *num_requests = 0;
>> +  size_t avail_planes = primary_planes_.size() + overlay_planes_.size();
>> +
>> +  HWC2::Error ret;
>> +
>> +  for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_)
>> +    l.second.set_validated_type(HWC2::Composition::Invalid);
>> +
>> +  ret = CreateComposition(true);
>> +  if (ret != HWC2::Error::None)
>> +    // Assume the test failed due to overlay planes
>> +    avail_planes = 1;
>> +
>> +  std::map<uint32_t, DrmHwcTwo::HwcLayer *> z_map;
>> +  for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
>> +    if (l.second.sf_type() == HWC2::Composition::Device)
>> +        z_map.emplace(std::make_pair(l.second.z_order(), &l.second));
>> +  }
>> +
>> +  /*
>> +   * If more layers then planes, save one plane
>> +   * for client composited layers
>> +   */
>> +  if (avail_planes < layers_.size())
>> +       avail_planes--;
>> +
>> +  for (std::pair<const uint32_t, DrmHwcTwo::HwcLayer *> &l : z_map) {
>> +    if (!avail_planes--)
>> +        break;
>> +    l.second->set_validated_type(HWC2::Composition::Device);
>> +  }
>> +
>>    for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
>>      DrmHwcTwo::HwcLayer &layer = l.second;
>>      switch (layer.sf_type()) {
>> +      case HWC2::Composition::Device:
>> +        if (layer.validated_type() == HWC2::Composition::Device)
>> +          break;
>> +        // fall thru
>>        case HWC2::Composition::SolidColor:
>>        case HWC2::Composition::Cursor:
>>        case HWC2::Composition::Sideband:
>> diff --git a/drmhwctwo.h b/drmhwctwo.h
>> index 0490e2ac7f85..bbf55d9a08bc 100644
>> --- a/drmhwctwo.h
>> +++ b/drmhwctwo.h
>> @@ -171,6 +171,7 @@ class DrmHwcTwo : public hwc2_device_t {
>>                                     float *min_luminance);
>>      HWC2::Error GetReleaseFences(uint32_t *num_elements, hwc2_layer_t *layers,
>>                                   int32_t *fences);
>> +    HWC2::Error CreateComposition(bool test);
>
> This is more of a private function than a HWC2 hook.
>
>>      HWC2::Error PresentDisplay(int32_t *retire_fence);
>>      HWC2::Error SetActiveConfig(hwc2_config_t config);
>>      HWC2::Error SetClientTarget(buffer_handle_t target, int32_t acquire_fence,
>> --
>> 2.17.0
>>
>
> Thanks for working on this! Fixup for the nits above:
>
> https://gitlab.freedesktop.org/stschake/drm-hwcomposer/commit/3aa0eb88

Thanks. That's all much better. I've folded that in and updated the
merge request.

Rob

Patch

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index 2f9f6c6772da..ceee6b1a4bd6 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -469,7 +469,7 @@  void DrmDisplayCompositor::ApplyFrame(
 }
 
 int DrmDisplayCompositor::ApplyComposition(
-    std::unique_ptr<DrmDisplayComposition> composition) {
+    std::unique_ptr<DrmDisplayComposition> composition, bool test) {
   int ret = 0;
   switch (composition->type()) {
     case DRM_COMPOSITION_TYPE_FRAME:
@@ -481,6 +481,8 @@  int DrmDisplayCompositor::ApplyComposition(
           ALOGE("Commit test failed for display %d, FIXME", display_);
           return ret;
         }
+        if (test)
+          return 0;
       }
 
       ApplyFrame(std::move(composition), ret);
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index ed46873c3409..7c5ebb005dc4 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -43,7 +43,7 @@  class DrmDisplayCompositor {
   int Init(DrmResources *drm, int display);
 
   std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
-  int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
+  int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition, bool test);
   int Composite();
   void Dump(std::ostringstream *out) const;
 
diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
index deaf14363e8c..19f73e721154 100644
--- a/drmhwctwo.cpp
+++ b/drmhwctwo.cpp
@@ -480,8 +480,7 @@  void DrmHwcTwo::HwcDisplay::AddFenceToRetireFence(int fd) {
   }
 }
 
-HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
-  supported(__func__);
+HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) {
   std::vector<DrmCompositionDisplayLayersMap> layers_map;
   layers_map.emplace_back();
   DrmCompositionDisplayLayersMap &map = layers_map.back();
@@ -494,7 +493,13 @@  HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
   uint32_t client_z_order = 0;
   std::map<uint32_t, DrmHwcTwo::HwcLayer *> z_map;
   for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
-    switch (l.second.validated_type()) {
+    HWC2::Composition comp_type;
+    if (test)
+      comp_type = l.second.sf_type();
+    else
+      comp_type = l.second.validated_type();
+
+    switch (comp_type) {
       case HWC2::Composition::Device:
         z_map.emplace(std::make_pair(l.second.z_order(), &l.second));
         break;
@@ -510,6 +515,10 @@  HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
   if (use_client_layer)
     z_map.emplace(std::make_pair(client_z_order, &client_layer_));
 
+  if (z_map.empty())
+    return HWC2::Error::BadLayer;
+
+
   // now that they're ordered by z, add them to the composition
   for (std::pair<const uint32_t, DrmHwcTwo::HwcLayer *> &l : z_map) {
     DrmHwcLayer layer;
@@ -521,10 +530,6 @@  HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
     }
     map.layers.emplace_back(std::move(layer));
   }
-  if (map.layers.empty()) {
-    *retire_fence = -1;
-    return HWC2::Error::None;
-  }
 
   std::unique_ptr<DrmDisplayComposition> composition =
       compositor_.CreateComposition();
@@ -555,13 +560,29 @@  HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
     i = overlay_planes.erase(i);
   }
 
-  AddFenceToRetireFence(composition->take_out_fence());
+  if (!test)
+    AddFenceToRetireFence(composition->take_out_fence());
 
-  ret = compositor_.ApplyComposition(std::move(composition));
+  ret = compositor_.ApplyComposition(std::move(composition), test);
   if (ret) {
     ALOGE("Failed to apply the frame composition ret=%d", ret);
     return HWC2::Error::BadParameter;
   }
+  return HWC2::Error::None;
+}
+
+HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
+  supported(__func__);
+  HWC2::Error ret;
+
+  ret = CreateComposition(false);
+  if (ret == HWC2::Error::BadLayer) {
+      // Can we really have no client or device layers?
+      *retire_fence = -1;
+      return HWC2::Error::None;
+  }
+  if (ret != HWC2::Error::None)
+    return ret;
 
   // The retire fence returned here is for the last frame, so return it and
   // promote the next retire fence
@@ -586,7 +607,7 @@  HWC2::Error DrmHwcTwo::HwcDisplay::SetActiveConfig(hwc2_config_t config) {
       compositor_.CreateComposition();
   composition->Init(drm_, crtc_, importer_.get(), planner_.get(), frame_no_);
   int ret = composition->SetDisplayMode(*mode);
-  ret = compositor_.ApplyComposition(std::move(composition));
+  ret = compositor_.ApplyComposition(std::move(composition), false);
   if (ret) {
     ALOGE("Failed to queue dpms composition on %d", ret);
     return HWC2::Error::BadConfig;
@@ -666,7 +687,7 @@  HWC2::Error DrmHwcTwo::HwcDisplay::SetPowerMode(int32_t mode_in) {
       compositor_.CreateComposition();
   composition->Init(drm_, crtc_, importer_.get(), planner_.get(), frame_no_);
   composition->SetDpmsMode(dpms_value);
-  int ret = compositor_.ApplyComposition(std::move(composition));
+  int ret = compositor_.ApplyComposition(std::move(composition), false);
   if (ret) {
     ALOGE("Failed to apply the dpms composition ret=%d", ret);
     return HWC2::Error::BadParameter;
@@ -683,11 +704,47 @@  HWC2::Error DrmHwcTwo::HwcDisplay::SetVsyncEnabled(int32_t enabled) {
 HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
                                                    uint32_t *num_requests) {
   supported(__func__);
+  size_t plane_count = 0;
   *num_types = 0;
   *num_requests = 0;
+  size_t avail_planes = primary_planes_.size() + overlay_planes_.size();
+
+  HWC2::Error ret;
+
+  for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_)
+    l.second.set_validated_type(HWC2::Composition::Invalid);
+
+  ret = CreateComposition(true);
+  if (ret != HWC2::Error::None)
+    // Assume the test failed due to overlay planes
+    avail_planes = 1;
+
+  std::map<uint32_t, DrmHwcTwo::HwcLayer *> z_map;
+  for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
+    if (l.second.sf_type() == HWC2::Composition::Device)
+        z_map.emplace(std::make_pair(l.second.z_order(), &l.second));
+  }
+
+  /*
+   * If more layers then planes, save one plane
+   * for client composited layers
+   */
+  if (avail_planes < layers_.size())
+       avail_planes--;
+
+  for (std::pair<const uint32_t, DrmHwcTwo::HwcLayer *> &l : z_map) {
+    if (!avail_planes--)
+        break;
+    l.second->set_validated_type(HWC2::Composition::Device);
+  }
+
   for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
     DrmHwcTwo::HwcLayer &layer = l.second;
     switch (layer.sf_type()) {
+      case HWC2::Composition::Device:
+        if (layer.validated_type() == HWC2::Composition::Device)
+          break;
+        // fall thru
       case HWC2::Composition::SolidColor:
       case HWC2::Composition::Cursor:
       case HWC2::Composition::Sideband:
diff --git a/drmhwctwo.h b/drmhwctwo.h
index 0490e2ac7f85..bbf55d9a08bc 100644
--- a/drmhwctwo.h
+++ b/drmhwctwo.h
@@ -171,6 +171,7 @@  class DrmHwcTwo : public hwc2_device_t {
                                    float *min_luminance);
     HWC2::Error GetReleaseFences(uint32_t *num_elements, hwc2_layer_t *layers,
                                  int32_t *fences);
+    HWC2::Error CreateComposition(bool test);
     HWC2::Error PresentDisplay(int32_t *retire_fence);
     HWC2::Error SetActiveConfig(hwc2_config_t config);
     HWC2::Error SetClientTarget(buffer_handle_t target, int32_t acquire_fence,