On Thu, May 17, 2018 at 6:25 PM, Stefan Schake <stschake@xxxxxxxxx> wrote:
> Hey Rob,
>
> On Fri, May 18, 2018 at 12:08 AM, Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
