[Intel-gfx] [PATCH v2 18/27] drm/i915: Handle disabling planes better.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Jun 10 20:51:27 PDT 2015
Op 11-06-15 om 03:37 schreef Matt Roper:
> On Thu, Jun 04, 2015 at 02:47:48PM +0200, Maarten Lankhorst wrote:
>> Read out the initial state, and add a quirk to force disable all plane
>> that were initially active. Use this to disable planes during modeset
>> too.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_atomic.c | 7 ++
>> drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
>> drivers/gpu/drm/i915/intel_drv.h | 7 ++
>> 3 files changed, 114 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 8447a1fef332..5627df2807b0 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -96,6 +96,13 @@ int intel_atomic_check(struct drm_device *dev,
>> return -EINVAL;
>> }
>>
>> + if (crtc_state &&
>> + crtc_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE) {
>> + ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> ret = drm_atomic_helper_check_planes(dev, state);
>> if (ret)
>> return ret;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b72724121f57..3b5d23692935 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4830,11 +4830,20 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>> intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
>> }
>>
>> +static void intel_disable_planes_on_crtc(struct drm_crtc *c)
>> +{
>> + struct intel_crtc *crtc = to_intel_crtc(c);
>> + struct drm_plane *plane;
>> +
>> + drm_for_each_plane_mask(plane, crtc->base.dev,
>> + crtc->atomic.force_disabled_planes)
>> + to_intel_plane(plane)->disable_plane(plane, c);
>> +}
>> +
>> static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>> {
>> struct drm_device *dev = crtc->dev;
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> - struct intel_plane *intel_plane;
>> int pipe = intel_crtc->pipe;
>>
>> intel_crtc_wait_for_pending_flips(crtc);
>> @@ -4842,14 +4851,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>> intel_pre_disable_primary(crtc);
>>
>> intel_crtc_dpms_overlay_disable(intel_crtc);
>> - for_each_intel_plane(dev, intel_plane) {
>> - if (intel_plane->pipe == pipe) {
>> - struct drm_crtc *from = intel_plane->base.crtc;
>> -
>> - intel_plane->disable_plane(&intel_plane->base,
>> - from ?: crtc);
>> - }
>> - }
>> + intel_disable_planes_on_crtc(crtc);
>>
>> /*
>> * FIXME: Once we grow proper nuclear flip support out of this we need
>> @@ -11554,6 +11556,21 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>> if (!intel_crtc->active || mode_changed)
>> return 0;
>>
>> + if (to_intel_crtc_state(crtc_state)->quirks &
>> + PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>> + if (!plane_state->crtc) {
>> + intel_crtc->atomic.force_disabled_planes |=
>> + 1 << drm_plane_index(plane);
>> + crtc_state->plane_mask &=
>> + ~(1 << drm_plane_index(plane));
>> + }
>> +
>> + /* Initial state for sprite planes is unknown,
>> + * no need to update sprite watermarks */
>> + if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>> + mode_changed = true;
>> + }
>> +
>> was_visible = old_plane_state->visible;
>> visible = to_intel_plane_state(plane_state)->visible;
>>
>> @@ -11633,7 +11650,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>> intel_crtc->atomic.fb_bits |=
>> INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
>>
>> - if (turn_off && is_crtc_enabled) {
>> + if (turn_off && !mode_changed) {
>> intel_crtc->atomic.wait_vblank = true;
>> intel_crtc->atomic.update_sprite_watermarks |=
>> 1 << i;
>> @@ -11714,6 +11731,11 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>> "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
>> idx, crtc->state->active, intel_crtc->active);
>>
>> + /* plane mask is fixed up after all initial planes are calculated */
>> + pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>> + if (mode_changed)
>> + intel_crtc->atomic.force_disabled_planes = crtc->state->plane_mask;
>> +
>> if (mode_changed && crtc_state->enable &&
>> dev_priv->display.crtc_compute_clock &&
>> !WARN_ON(pipe_config->shared_dpll != DPLL_ID_PRIVATE)) {
>> @@ -12883,6 +12905,13 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>> return ret;
>>
>> for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> + if (to_intel_crtc_state(crtc_state)->quirks &
>> + PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>> + ret = drm_atomic_add_affected_planes(state, crtc);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> if (!needs_modeset(crtc_state))
>> continue;
>>
>> @@ -13529,13 +13558,17 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>> intel_runtime_pm_get(dev_priv);
>>
>> /* Perform vblank evasion around commit operation */
>> - if (crtc->state->active && !needs_modeset(crtc->state))
>> + if (crtc->state->active && !needs_modeset(crtc->state)) {
>> intel_crtc->atomic.evade =
>> intel_pipe_update_start(intel_crtc,
>> &intel_crtc->atomic.start_vbl_count);
>>
>> + if (intel_crtc->atomic.force_disabled_planes)
>> + intel_disable_planes_on_crtc(crtc);
>> + }
>> +
>> if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>> - skl_detach_scalers(intel_crtc);
>> + skl_detach_scalers(intel_crtc);
> Unintentional change from tabs to spaces on this line?
Oops.
>> }
>>
>> static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>> @@ -15040,14 +15073,63 @@ void i915_redisable_vga(struct drm_device *dev)
>> i915_redisable_vga_power_on(dev);
>> }
>>
>> -static bool primary_get_hw_state(struct intel_crtc *crtc)
>> +static bool intel_read_hw_plane_state(struct intel_crtc *crtc,
>> + struct intel_plane *intel_plane)
>> {
>> - struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> + struct drm_device *dev = crtc->base.dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> - if (!crtc->base.enabled)
>> - return false;
>> + switch (intel_plane->base.type) {
>> + case DRM_PLANE_TYPE_PRIMARY:
>> + return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>> +
>> + case DRM_PLANE_TYPE_CURSOR:
>> + if (IS_845G(dev) || IS_I865G(dev))
>> + return I915_READ(_CURACNTR) & CURSOR_ENABLE;
>> + else
>> + return I915_READ(CURCNTR(crtc->plane)) & CURSOR_MODE;
>> +
> If we're not trying to seamlessly inherit the cursor image, I'm not sure
> I understand what the benefit of figuring out whether the cursor is
> actually on or not is. Just assuming true, as we do for sprites, would
> be enough to make the cursor always turn off, right?
Yeah, when reworking this patch to apply after modeset revert I fixed that, but kept cursor readout since I wrote it anyway.
Also got me rid of tracking force disabled planes. :-)
>> + default:
>> + return true;
>> + }
>> +}
>> +
>> +static int readout_plane_state(struct drm_atomic_state *state,
>> + struct intel_crtc *crtc,
>> + struct intel_crtc_state *crtc_state)
>> +{
>> + struct intel_plane *p;
>> + struct drm_plane_state *drm_plane_state;
>> + bool active = crtc_state->base.active;
>> +
>> + if (active) {
>> + crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>>
>> - return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>> + /* apply to previous sw state too */
>> + to_intel_crtc_state(crtc->base.state)->quirks |=
>> + PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>> + }
>> +
>> + for_each_intel_plane(state->dev, p) {
>> + if (crtc->plane != p->plane)
>> + continue;
> Was this supposed to be comparing ->pipe rather than ->plane? For
> primary and cursor planes I believe this works out okay since ->plane
> and ->pipe are basically the same (except for pre-gen4 FBC swapping),
> but for sprite planes, the ->plane pointer indicates whether it's the
> first, second, third, etc. sprite of that specific CRTC.
>
> As written, I think you'll only be operating on the first sprite plane
> of CRTC 0, the second sprite plane of CRTC 1, etc.
Ah thanks, that would explain it. :)
~Maarten
More information about the Intel-gfx
mailing list