Op 12-02-16 om 14:56 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> Factor out intel_fbc_supports_rotation
> ^ not anymore
>
>
>> and use it in
>> pre_plane_update as well. This leaves intel_crtc->atomic
>> empty, so remove it too.
>>
>> Changes since v1:
>> - Add a intel_fbc_supports_rotation helper.
> Changes since v2:
> - No more need for rotation special-casing.
>
> (I suppose you also have to edit the commit title to be v3)
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-------------
>> ----------
>> drivers/gpu/drm/i915/intel_drv.h | 15 ----------
>> 2 files changed, 20 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 54be8a255f1f..00cb261c6787 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>> {
>> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
>>> base.crtc);
>> struct drm_atomic_state *old_state = old_crtc_state-
>>> base.state;
>> - struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>> struct intel_crtc_state *pipe_config =
>> to_intel_crtc_state(crtc->base.state);
>> struct drm_device *dev = crtc->base.dev;
>> - struct drm_i915_private *dev_priv = dev->dev_private;
>> struct drm_plane *primary = crtc->base.primary;
>> struct drm_plane_state *old_pri_state =
>> drm_atomic_get_existing_plane_state(old_state,
>> primary);
>> @@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>> if (pipe_config->wm_changed && pipe_config->base.active)
>> intel_update_watermarks(&crtc->base);
>>
>> - if (atomic->update_fbc)
>> - intel_fbc_post_update(crtc);
>> -
>> if (old_pri_state) {
> For a code reader that is not very familiar with all the atomic history
> and its details, it's not trivial to conclude that "if
> (drm_atomic_get_existing_plane_state(primary))", then we're updating
> the primary plane on this atomic commit. And before this patch, it's
> much much easier to conclude that update_fbc will be true when an
> atomic update is touching the primary plane because that's explicitly
> stated by the cod.
>
> So although "let's kill this redundant struct" sounds good, it seems to
> me that code clarity is going away with this patch, so I wonder if the
> benefits of the patch outweigh the downsides. Isn't there something
> else we could do about this, such as some renaming, or adding some
> function aliases or just extra commenting?
>
>> struct intel_plane_state *primary_state =
>> to_intel_plane_state(primary->state);
>> struct intel_plane_state *old_primary_state =
>> to_intel_plane_state(old_pri_state);
>>
>> + intel_fbc_post_update(crtc);
>> +
>> if (primary_state->visible &&
>> (needs_modeset(&pipe_config->base) ||
>> !old_primary_state->visible))
>> intel_post_enable_primary(&crtc->base);
>> }
>> -
>> - memset(atomic, 0, sizeof(*atomic));
>> }
>>
>> static void intel_pre_plane_update(struct intel_crtc_state
>> *old_crtc_state)
>> @@ -4821,7 +4816,6 @@ static void intel_pre_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
>>> base.crtc);
>> struct drm_device *dev = crtc->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> - struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>> struct intel_crtc_state *pipe_config =
>> to_intel_crtc_state(crtc->base.state);
>> struct drm_atomic_state *old_state = old_crtc_state-
>>> base.state;
>> @@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>> drm_atomic_get_existing_plane_state(old_state,
>> primary);
>> bool modeset = needs_modeset(&pipe_config->base);
>>
>> - if (atomic->update_fbc)
>> - intel_fbc_pre_update(crtc);
>> -
>> if (old_pri_state) {
>> struct intel_plane_state *primary_state =
>> to_intel_plane_state(primary->state);
>> struct intel_plane_state *old_primary_state =
>> to_intel_plane_state(old_pri_state);
>> + bool turn_off = old_primary_state->visible &&
>> + (modeset || !primary_state->visible);
> Not really related to the patch, but ok to keep since it's trivial...
>
>> +
>> + intel_fbc_pre_update(crtc);
>>
>> - if (old_primary_state->visible &&
>> - (modeset || !primary_state->visible))
>> + if (turn_off)
>> intel_pre_disable_primary(&crtc->base);
>> }
>>
>> @@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>> if (visible || was_visible)
>> pipe_config->fb_bits |= to_intel_plane(plane)-
>>> frontbuffer_bit;
>>
>> - switch (plane->type) {
>> - case DRM_PLANE_TYPE_PRIMARY:
>> - intel_crtc->atomic.update_fbc = true;
>> -
>> - break;
>> - case DRM_PLANE_TYPE_CURSOR:
>> - break;
>> - case DRM_PLANE_TYPE_OVERLAY:
>> - /*
>> - * WaCxSRDisabledForSpriteScaling:ivb
>> - *
>> - * cstate->update_wm was already set above, so this
>> flag will
>> - * take effect when we commit and program
>> watermarks.
>> - */
>> - if (IS_IVYBRIDGE(dev) &&
>> - needs_scaling(to_intel_plane_state(plane_state))
>> &&
>> - !needs_scaling(old_plane_state))
>> - pipe_config->disable_lp_wm = true;
>> + /*
>> + * WaCxSRDisabledForSpriteScaling:ivb
>> + *
>> + * cstate->update_wm was already set above, so this flag
>> will
>> + * take effect when we commit and program watermarks.
>> + */
>> + if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
>> IS_IVYBRIDGE(dev) &&
>> + needs_scaling(to_intel_plane_state(plane_state)) &&
>> + !needs_scaling(old_plane_state))
>> + pipe_config->disable_lp_wm = true;
>>
>> - break;
>> - }
>> return 0;
>> }
>>
>> @@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
>> drm_device *dev,
>> struct intel_crtc_state *pipe_config =
>> to_intel_crtc_state(crtc_state);
>>
>> - memset(&to_intel_crtc(crtc)->atomic, 0,
>> - sizeof(struct intel_crtc_atomic_commit));
>> -
>> /* Catch I915_MODE_FLAG_INHERITED */
>> if (crtc_state->mode.private_flags != crtc->state-
>>> mode.private_flags)
>> crtc_state->mode_changed = true;
>> @@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
>> drm_device *dev,
>> if (!modeset)
>> intel_pre_plane_update(to_intel_crtc_state(c
>> rtc_state));
>>
>> - if (crtc->state->active && intel_crtc-
>>> atomic.update_fbc)
>> + if ((modeset || update_pipe) && crtc->state->active)
> Same here: not easy for me to verify things are the same now. Even
> worse: the check for "is this atomic commit touching the primary
> plane?" is now written in a completely different way than the one
> introduced above. Maybe there's something we could do to make the code
> easier to read.
No, this is called now every time when the crtc has a modeset or update_pipe, even with no primary plane.
This is because atomic may set a mode without having a primary plane, and in that case fbc_enable may never be called.
Seemed like a bug in the original code..
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
