| Message ID | 1524528404-12331-2-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] drm_hwcomposer: Cleanup gl precompositor init and provide uses_GL flag | expand |
On Mon, Apr 23, 2018 at 5:06 PM, John Stultz <john.stultz@linaro.org> wrote: > If the gl precompositor isn't being used, we cannot accept > every layer as a device composited layer. > > Thus this patch adds some extra logic in the validate function > to try to map layers to available device planes, falling back > to client side compositing if we run-out of planes. > > Credit to Rob Herring, who's work this single plane patch was > originally based on. > > Feedback or alternative ideas would be greatly appreciated! > > Cc: Marissa Wall <marissaw@google.com> > Cc: Sean Paul <seanpaul@google.com> > Cc: Dmitry Shmidt <dimitrysh@google.com> > Cc: Robert Foss <robert.foss@collabora.com> > Cc: Matt Szczesiak <matt.szczesiak@arm.com> > Cc: Liviu Dudau <Liviu.Dudau@arm.com> > Cc: David Hanna <david.hanna11@gmail.com> > Cc: Rob Herring <rob.herring@linaro.org> > Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> > Cc: Alistair Strachan <astrachan@google.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: John Stultz <john.stultz@linaro.org> A patch so nice, I signed it off twice! :P Sorry, looks like I yanked and pasted one too many lines from the cc list. -john
On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote: > If the gl precompositor isn't being used, we cannot accept > every layer as a device composited layer. > > Thus this patch adds some extra logic in the validate function > to try to map layers to available device planes, falling back > to client side compositing if we run-out of planes. > > Credit to Rob Herring, who's work this single plane patch was > originally based on. > > Feedback or alternative ideas would be greatly appreciated! > > Cc: Marissa Wall <marissaw@google.com> > Cc: Sean Paul <seanpaul@google.com> > Cc: Dmitry Shmidt <dimitrysh@google.com> > Cc: Robert Foss <robert.foss@collabora.com> > Cc: Matt Szczesiak <matt.szczesiak@arm.com> > Cc: Liviu Dudau <Liviu.Dudau@arm.com> > Cc: David Hanna <david.hanna11@gmail.com> > Cc: Rob Herring <rob.herring@linaro.org> > Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> > Cc: Alistair Strachan <astrachan@google.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > drmhwctwo.cpp | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp > index dfca1a6..437a439 100644 > --- a/drmhwctwo.cpp > +++ b/drmhwctwo.cpp > @@ -686,6 +686,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, > supported(__func__); > *num_types = 0; > *num_requests = 0; > + int avail_planes = primary_planes_.size() + overlay_planes_.size(); > + > + /* > + * If more layers then planes, save one plane > + * for client composited layers > + */ > + if (avail_planes < layers_.size()) > + avail_planes--; > + > for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) { > DrmHwcTwo::HwcLayer &layer = l.second; > switch (layer.sf_type()) { > @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, > layer.set_validated_type(HWC2::Composition::Client); > ++*num_types; > break; > + case HWC2::Composition::Device: > + if (!compositor_.uses_GL() && !avail_planes) { Something is not entirely correct here, you can't assume just because you have planes available they can be used. E.g Layer has rotation, but the plane doesn't support it. This part is handled by the planning part in presentDisplay which falls back on GPU. I think the easiest and safe way is to just fallback to Composition::Client whenever the GLCompositor failed to initialize. > + layer.set_validated_type(HWC2::Composition::Client); > + ++*num_types; > + break; > + } else { > + avail_planes--; > + } > + /* fall through */ > default: > layer.set_validated_type(layer.sf_type()); > break; > -- > 2.7.4
On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> wrote: > On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote: >> If the gl precompositor isn't being used, we cannot accept >> every layer as a device composited layer. >> >> Thus this patch adds some extra logic in the validate function >> to try to map layers to available device planes, falling back >> to client side compositing if we run-out of planes. >> >> Credit to Rob Herring, who's work this single plane patch was >> originally based on. >> >> Feedback or alternative ideas would be greatly appreciated! >> >> Cc: Marissa Wall <marissaw@google.com> >> Cc: Sean Paul <seanpaul@google.com> >> Cc: Dmitry Shmidt <dimitrysh@google.com> >> Cc: Robert Foss <robert.foss@collabora.com> >> Cc: Matt Szczesiak <matt.szczesiak@arm.com> >> Cc: Liviu Dudau <Liviu.Dudau@arm.com> >> Cc: David Hanna <david.hanna11@gmail.com> >> Cc: Rob Herring <rob.herring@linaro.org> >> Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> >> Cc: Alistair Strachan <astrachan@google.com> >> Signed-off-by: John Stultz <john.stultz@linaro.org> >> Signed-off-by: John Stultz <john.stultz@linaro.org> >> --- >> drmhwctwo.cpp | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp >> index dfca1a6..437a439 100644 >> --- a/drmhwctwo.cpp >> +++ b/drmhwctwo.cpp >> @@ -686,6 +686,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, >> supported(__func__); >> *num_types = 0; >> *num_requests = 0; >> + int avail_planes = primary_planes_.size() + overlay_planes_.size(); >> + >> + /* >> + * If more layers then planes, save one plane >> + * for client composited layers >> + */ >> + if (avail_planes < layers_.size()) >> + avail_planes--; >> + >> for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) { >> DrmHwcTwo::HwcLayer &layer = l.second; >> switch (layer.sf_type()) { >> @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, >> layer.set_validated_type(HWC2::Composition::Client); >> ++*num_types; >> break; >> + case HWC2::Composition::Device: >> + if (!compositor_.uses_GL() && !avail_planes) { > > Something is not entirely correct here, you can't assume just because > you have planes available they can be used. > E.g Layer has rotation, but the plane doesn't support it. > This part is handled by the planning part in presentDisplay which > falls back on GPU. > > I think the easiest and safe way is to just fallback to > Composition::Client whenever the GLCompositor failed to initialize. > I agree, this would only work out for the single plane case where you end up using client composition for everything. In the spirit of DRM what we would probably want is to atomic test a composition, and if that fails we can still fallback to client or GL compositor depending on its availability. And then in a far far away future we might even do a few iterations simplifying our composition until it atomic tests correctly so we can still offload some "simple" parts like the cursor. Thanks, Stefan
On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <stschake@gmail.com> wrote: > On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe > <Alexandru-Cosmin.Gheorghe@arm.com> wrote: >> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote: >>> @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, >>> layer.set_validated_type(HWC2::Composition::Client); >>> ++*num_types; >>> break; >>> + case HWC2::Composition::Device: >>> + if (!compositor_.uses_GL() && !avail_planes) { >> >> Something is not entirely correct here, you can't assume just because >> you have planes available they can be used. >> E.g Layer has rotation, but the plane doesn't support it. >> This part is handled by the planning part in presentDisplay which >> falls back on GPU. Ah. Thanks for the correction here! This is part of the disconnect I have been having with the drm_hwcomposer logic. The plan/validate step doesn't really do either, instead it defers all the checking/planning to the set/present step. But unfortunately it seems if we want to use the surfaceflinger gl composer, we need to mark the layers as client rendered in the validate function. >> I think the easiest and safe way is to just fallback to >> Composition::Client whenever the GLCompositor failed to initialize. >> > > I agree, this would only work out for the single plane case where you end > up using client composition for everything. True, but maybe as a first step we simply fall back on the glcompositor, just so we have something displaying in that case (right now nothing gets displayed). > In the spirit of DRM what we would probably want is to atomic test a > composition, and if that fails we can still fallback to client or GL > compositor depending on its availability. And then in a far far away > future we might even do a few iterations simplifying our composition until > it atomic tests correctly so we can still offload some "simple" parts > like the cursor. This sounds reasonable. I suspect it will take some heavier rework of the drm_hwcomposer to move such logic to the validate step. Marissa/Sean: Any objections here? Doing more planning in the validate step sounds like it aligns closer to the HWC2 interface goals, but I don't want to have to relearn-the-hard-way any lessons that resulted in the current mode of accept it all and sort it in present. In the meantime I'll drop the flawed plane/layer allocation from this patch and resend. thanks -john
On Tue, Apr 24, 2018 at 2:38 PM John Stultz <john.stultz@linaro.org> wrote: > On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <stschake@gmail.com> wrote: > > On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe > > <Alexandru-Cosmin.Gheorghe@arm.com> wrote: > >> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote: > >>> @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, > >>> layer.set_validated_type(HWC2::Composition::Client); > >>> ++*num_types; > >>> break; > >>> + case HWC2::Composition::Device: > >>> + if (!compositor_.uses_GL() && !avail_planes) { > >> > >> Something is not entirely correct here, you can't assume just because > >> you have planes available they can be used. > >> E.g Layer has rotation, but the plane doesn't support it. > >> This part is handled by the planning part in presentDisplay which > >> falls back on GPU. > Ah. Thanks for the correction here! > This is part of the disconnect I have been having with the > drm_hwcomposer logic. The plan/validate step doesn't really do either, > instead it defers all the checking/planning to the set/present step. > But unfortunately it seems if we want to use the surfaceflinger gl > composer, we need to mark the layers as client rendered in the > validate function. > >> I think the easiest and safe way is to just fallback to > >> Composition::Client whenever the GLCompositor failed to initialize. > >> > > > > I agree, this would only work out for the single plane case where you end > > up using client composition for everything. > True, but maybe as a first step we simply fall back on the > glcompositor, just so we have something displaying in that case (right > now nothing gets displayed). > > In the spirit of DRM what we would probably want is to atomic test a > > composition, and if that fails we can still fallback to client or GL > > compositor depending on its availability. And then in a far far away > > future we might even do a few iterations simplifying our composition until > > it atomic tests correctly so we can still offload some "simple" parts > > like the cursor. > This sounds reasonable. I suspect it will take some heavier rework of > the drm_hwcomposer to move such logic to the validate step. > Marissa/Sean: Any objections here? Doing more planning in the validate > step sounds like it aligns closer to the HWC2 interface goals, but I > don't want to have to relearn-the-hard-way any lessons that resulted > in the current mode of accept it all and sort it in present. No objections, this needs to be done. As you mentioned, this is going to require a good amount of code movement. I think what we're seeing here as well, as with the writeback series, are growing pains. When this was written, it was for HWC1 and the GLCompositor was the only alternate compositor. Now that we're on HWC2 and discussing using writeback, SurfaceFlinger, and possibly GLCompositor, we need something more robust. Sean > In the meantime I'll drop the flawed plane/layer allocation from this > patch and resend. > thanks > -john
Hi John, On Tue, Apr 24, 2018 at 11:38 AM John Stultz <john.stultz@linaro.org> wrote: > On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <stschake@gmail.com> wrote: > > On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe > > <Alexandru-Cosmin.Gheorghe@arm.com> wrote: > >> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote: > >>> @@ -695,6 +704,15 @@ HWC2::Error > DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, > >>> layer.set_validated_type(HWC2::Composition::Client); > >>> ++*num_types; > >>> break; > >>> + case HWC2::Composition::Device: > >>> + if (!compositor_.uses_GL() && !avail_planes) { > >> > >> Something is not entirely correct here, you can't assume just because > >> you have planes available they can be used. > >> E.g Layer has rotation, but the plane doesn't support it. > >> This part is handled by the planning part in presentDisplay which > >> falls back on GPU. > > Ah. Thanks for the correction here! > > This is part of the disconnect I have been having with the > drm_hwcomposer logic. The plan/validate step doesn't really do either, > instead it defers all the checking/planning to the set/present step. > But unfortunately it seems if we want to use the surfaceflinger gl > composer, we need to mark the layers as client rendered in the > validate function. > > >> I think the easiest and safe way is to just fallback to > >> Composition::Client whenever the GLCompositor failed to initialize. > >> > > > > I agree, this would only work out for the single plane case where you end > > up using client composition for everything. > > True, but maybe as a first step we simply fall back on the > glcompositor, just so we have something displaying in that case (right > now nothing gets displayed). > > > In the spirit of DRM what we would probably want is to atomic test a > > composition, and if that fails we can still fallback to client or GL > > compositor depending on its availability. And then in a far far away > > future we might even do a few iterations simplifying our composition > until > > it atomic tests correctly so we can still offload some "simple" parts > > like the cursor. > > This sounds reasonable. I suspect it will take some heavier rework of > the drm_hwcomposer to move such logic to the validate step. > > Marissa/Sean: Any objections here? Doing more planning in the validate > step sounds like it aligns closer to the HWC2 interface goals, but I > don't want to have to relearn-the-hard-way any lessons that resulted > in the current mode of accept it all and sort it in present. > I agree. This was the design goal behind HWC_GEOMETRY_CHANGED / HWC2_PFN_GET_CHANGED_COMPOSITION_TYPES. The validate() function should know that the composition can be set in HWC2_PFN_PRESENT_DISPLAY so there should be no need for an internal GL fallback. This feedback to surfaceflinger also allows for small state optimizations that can't be done in the fallback. I think validating compositions w/ atomic in kernel is a valid way to do it. I would suggest that code is also refactored a little so that the validation/planning could also be done in userspace, maybe by linking a static library. This would allow drivers without a way to do it in kernel to keep working. In the meantime I'll drop the flawed plane/layer allocation from this > patch and resend. > > thanks > -john > <div dir="ltr">Hi John,<br><br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 24, 2018 at 11:38 AM John Stultz <<a href="mailto:john.stultz@linaro.org">john.stultz@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Apr 24, 2018 at 3:34 AM, Stefan Schake <<a href="mailto:stschake@gmail.com" target="_blank">stschake@gmail.com</a>> wrote:<br> > On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe<br> > <<a href="mailto:Alexandru-Cosmin.Gheorghe@arm.com" target="_blank">Alexandru-Cosmin.Gheorghe@arm.com</a>> wrote:<br> >> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote:<br> >>> @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,<br> >>> layer.set_validated_type(HWC2::Composition::Client);<br> >>> ++*num_types;<br> >>> break;<br> >>> + case HWC2::Composition::Device:<br> >>> + if (!compositor_.uses_GL() && !avail_planes) {<br> >><br> >> Something is not entirely correct here, you can't assume just because<br> >> you have planes available they can be used.<br> >> E.g Layer has rotation, but the plane doesn't support it.<br> >> This part is handled by the planning part in presentDisplay which<br> >> falls back on GPU.<br> <br> Ah. Thanks for the correction here!<br> <br> This is part of the disconnect I have been having with the<br> drm_hwcomposer logic. The plan/validate step doesn't really do either,<br> instead it defers all the checking/planning to the set/present step.<br> But unfortunately it seems if we want to use the surfaceflinger gl<br> composer, we need to mark the layers as client rendered in the<br> validate function.<br> <br> >> I think the easiest and safe way is to just fallback to<br> >> Composition::Client whenever the GLCompositor failed to initialize.<br> >><br> ><br> > I agree, this would only work out for the single plane case where you end<br> > up using client composition for everything.<br> <br> True, but maybe as a first step we simply fall back on the<br> glcompositor, just so we have something displaying in that case (right<br> now nothing gets displayed).<br> <br> > In the spirit of DRM what we would probably want is to atomic test a<br> > composition, and if that fails we can still fallback to client or GL<br> > compositor depending on its availability. And then in a far far away<br> > future we might even do a few iterations simplifying our composition until<br> > it atomic tests correctly so we can still offload some "simple" parts<br> > like the cursor.<br> <br> This sounds reasonable. I suspect it will take some heavier rework of<br> the drm_hwcomposer to move such logic to the validate step.<br> <br> Marissa/Sean: Any objections here? Doing more planning in the validate<br> step sounds like it aligns closer to the HWC2 interface goals, but I<br> don't want to have to relearn-the-hard-way any lessons that resulted<br> in the current mode of accept it all and sort it in present.<br></blockquote><div><br></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">I agree. This was the design goal behind HWC_GEOMETRY_CHANGED / HWC2_PFN_GET_CHANGED_COMPOSITION_TYPES. The validate() function should know that the composition can be set in HWC2_PFN_PRESENT_DISPLAY so there should be no need for an internal GL fallback. This feedback to surfaceflinger also allows for small state optimizations that can't be done in the fallback.</span></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">I think validating compositions w/ atomic in kernel is a valid way to do it.</span></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">I would suggest that code is also refactored a little so that the validation/planning could also be done in userspace, maybe by linking a static library. This would allow drivers without a way to do it in kernel to keep working.</span></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> In the meantime I'll drop the flawed plane/layer allocation from this<br> patch and resend.<br> <br> thanks<br> -john<br> </blockquote></div></div>
diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp index dfca1a6..437a439 100644 --- a/drmhwctwo.cpp +++ b/drmhwctwo.cpp @@ -686,6 +686,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, supported(__func__); *num_types = 0; *num_requests = 0; + int avail_planes = primary_planes_.size() + overlay_planes_.size(); + + /* + * If more layers then planes, save one plane + * for client composited layers + */ + if (avail_planes < layers_.size()) + avail_planes--; + for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) { DrmHwcTwo::HwcLayer &layer = l.second; switch (layer.sf_type()) { @@ -695,6 +704,15 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types, layer.set_validated_type(HWC2::Composition::Client); ++*num_types; break; + case HWC2::Composition::Device: + if (!compositor_.uses_GL() && !avail_planes) { + layer.set_validated_type(HWC2::Composition::Client); + ++*num_types; + break; + } else { + avail_planes--; + } + /* fall through */ default: layer.set_validated_type(layer.sf_type()); break;