Re: [PATCH 00/31] IPS/DRRS/PSR rework with PSR enabled by default

[Daniel Stone <daniel@fooishbar.org>]

Hi Rodrigo,

On 10 November 2015 at 15:57, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote:
> On Mon, 2015-11-09 at 11:47 +0000, Daniel Stone wrote:
>> I've been looking at pulling this on top of Maarten's tree, and
>
> I'm afraid I didn't followed completely your idea and maybe because I
> don't know the code you are referring to. What tree specifically are
> you talking about?

Ah, it's at git://people.freedesktop.org/~mlankhorst/linux#rework-page-flip.
The code itself isn't that important though - it makes for a very
messy rebase but has little functional impact - it's more about the
style overall. I'll try to explain a bit better. :\

>> currently my overriding wish is that, rather than the checks
>> sprinkled
>> all over various state-change functions,
>
> Ok, so you are suggesting we totally remove the pipe config variables
> right? I haven't considered this possibility since I believed some
> people would prefer to let the variables there. And also for PSRxDRRS
> conflict this would be safier since DRRS cannot be enabled if PSR is
> going to be enabled. With your solution we need to pay attention and
> never let someone invert PSR and DRRS orders.

Hm, not so much removing as making more use of them!

At the moment, the IPS/PSR logic is quite distributed. Taking this as
an example, from patch #6:

@@ -11562,12 +11554,8 @@ int intel_plane_atomic_calc_changes(struct
drm_crtc_state *crtc_state,

                if (turn_off) {
                        /*
-                        * FIXME: Actually if we will still have any other
-                        * plane enabled on the pipe we could let IPS enabled
-                        * still, but for now lets consider that when we make
-                        * primary invisible by setting DSPCNTR to 0 on
-                        * update_primary_plane function IPS needs to be
-                        * disable.
+                        * IPS disable_if_alone function will be called
+                        * in order to decide if IPS disable is really needed.
                         */
                        intel_crtc->atomic.disable_ips = true;

This gets called when the primary plane is being disabled. So the logic is:
  - if IPS was enabled
  - and the primary plane was on
  - and the primary plane was no longer on
  - maybe disable the plane

The implementation is buggy (see reply to patch coming shortly), but
more importantly, I think the state-machine concept - where you react
to very specific changes (e.g. 'disable a plane') is quite fragile. As
an example, there is nothing I can see in your patch that handles the
following case:
  - IPS is enabled with one primary and one overlay plane
  - disable the primary plane: intel_ips_disable_if_alone() is called,
and does not disable IPS, since the overlay plane is still active
  - disable the overlay plane, but IPS remains enabled even though no
planes are active

For me, a better way of approaching this - very much in line with the
atomic conversion work that has been ongoing ever since
intel_crtc_state was created - would be to examine the actual state as
a whole. So, rather than performing specific checks in response to one
specific action (disabling the primary plane), you would have one
function in intel_ips.c, which examines the entire CRTC state, and
returns a boolean answer: should IPS be enabled or disabled?

This keeps the logic in one place, and means that rather than
sprinkling magic enable/disable calls in response to specific
_actions_ - which gets difficult to do anyway with atomic changes -
you only ever perform enable/disable on the result of one function
which inspects the overall state ('are there any planes enabled in
this target state?') and comes up with an answer. Thus the interaction
with the core code is limited to:
    bool target_ips = intel_ips_target_state(intel_crtc_state);

Does that make sense? It's just about deriving the conditions from an
overall target state, rather than making them reactive to specific
actions.

Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx