Re: [PATCH v2 07/20] drm/i915: Rework primary plane stuff slightly.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 08-07-15 om 11:27 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 04:02:32PM +0200, Maarten Lankhorst wrote:
>> Op 07-07-15 om 13:16 schreef Daniel Vetter:
>>> On Tue, Jul 07, 2015 at 09:08:18AM +0200, Maarten Lankhorst wrote:
>>>> Make sure the primary plane is set up correctly. This is done by
>>>> setting plane_state->src and plane_state->crtc.
>>>>
>>>> All non-primary planes get disabled.
>>> Too terse commit message, fails to mention that this is about hw
>>> readout completely. Also should mention that this removes the
>>> initial_planes hack.
>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic.c  |   7 --
>>>>  drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++----------------------
>>>>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
>>>>  3 files changed, 60 insertions(+), 116 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>>>> index 429677a111d5..b593612a917d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>> @@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
>>>>  		return -EINVAL;
>>>>  	}
>>>>  
>>>> -	if (crtc_state &&
>>>> -	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>>>> -		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 eb7c2e2819b7..fa1102392eca 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -109,6 +109,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
>>>>  	struct intel_crtc_state *crtc_state);
>>>>  static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
>>>>  			   int num_connectors);
>>>> +static void intel_pre_disable_primary(struct drm_crtc *crtc);
>>>>  
>>>>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>>>>  {
>>>> @@ -2582,11 +2583,12 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>>>>  {
>>>>  	struct drm_device *dev = intel_crtc->base.dev;
>>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> -	struct drm_crtc *c;
>>>> -	struct intel_crtc *i;
>>>>  	struct drm_i915_gem_object *obj;
>>>> -	struct drm_plane *primary = intel_crtc->base.primary;
>>>>  	struct drm_framebuffer *fb;
>>>> +	struct drm_plane *primary = intel_crtc->base.primary;
>>>> +	struct intel_plane *p;
>>>> +	struct intel_plane_state *plane_state =
>>>> +		to_intel_plane_state(primary->state);
>>>>  
>>>>  	if (!plane_config->fb)
>>>>  		return;
>>>> @@ -2602,16 +2604,11 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>>>>  	 * Failed to alloc the obj, check to see if we should share
>>>>  	 * an fb with another CRTC instead
>>>>  	 */
>>>> -	for_each_crtc(dev, c) {
>>>> -		i = to_intel_crtc(c);
>>>> -
>>>> -		if (c == &intel_crtc->base)
>>>> -			continue;
>>>> -
>>>> -		if (!i->active)
>>>> +	for_each_intel_plane(dev, p) {
>>>> +		if (p->base.type != DRM_PLANE_TYPE_PRIMARY)
>>>>  			continue;
>>>>  
>>>> -		fb = c->primary->fb;
>>>> +		fb = p->base.state->fb;
>>> This seems to break the sharing logic completely: We want to check primary
>>> planes of all other crtcs to see whether we could merged the fb together.
>>> We don't even read out plane state for non-primary planes, so the below fb
>>> check will never be non-NULL.
>> I thought this was about multiple planes sharing the same fb with same offset.
>> And as such checking for the crtc is unnecessary, for the current crtc it will be be NULL here.
>>
>> This only reads out the current fb, not different ones.
>>
>> And sharing the same fb with other crtc's is done in intel_fbdev_init_bios.
> This is about sharing the same fb but across different crtcs - bios never
> enables more than the primary plane anyway. And you can't rely upon
> fbdev_init_bios since that's not run at all when I915_FBDEV=n.
>
> So yes with current code this loop here reconstruct the shared between
> primary planes on different crtcs (if the stolen allocator tells us that
> our range is occupied already). fbdev_init_bios just tries to create a
> config matching the one the bios has set up (and then pick a suitable fb
> for fbcon from the ones already allocated).
>
> Maybe we should extract this as try_to_find_shared_fb or similar to make
> the code self-explanatory?
>
>>>>  		if (!fb)
>>>>  			continue;
>>>>  
>>>> @@ -2622,18 +2619,28 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>>>>  		}
>>>>  	}
>>>>  
>>>> +	intel_pre_disable_primary(&intel_crtc->base);
>>>> +	to_intel_plane(primary)->disable_plane(primary, &intel_crtc->base);
>>>> +
>>>>  	return;
>>>>  
>>>>  valid_fb:
>>>> +	drm_framebuffer_reference(fb);
>>>> +	primary->fb = plane_state->base.fb = fb;
>>>> +	plane_state->base.crtc = primary->crtc = &intel_crtc->base;
>>>> +
>>>> +	plane_state->base.src_x = plane_state->base.src_y = 0;
>>>> +	plane_state->base.src_w = fb->width << 16;
>>>> +	plane_state->base.src_h = fb->height << 16;
>>>> +
>>>> +	plane_state->base.crtc_x = plane_state->base.src_y = 0;
>>>> +	plane_state->base.crtc_w = fb->width;
>>>> +	plane_state->base.crtc_h = fb->height;
>>>> +
>>>> +	plane_state->visible = true;
>>>>  	obj = intel_fb_obj(fb);
>>>>  	if (obj->tiling_mode != I915_TILING_NONE)
>>>>  		dev_priv->preserve_bios_swizzle = true;
>>>> -
>>>> -	primary->fb = fb;
>>>> -	primary->crtc = primary->state->crtc = &intel_crtc->base;
>>>> -	update_state_fb(primary);
>>> Do we still have other users of update_state_fb left?
>> Just the page flip handler.
>>>> -	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
>>>> -	obj->frontbuffer_bits |= to_intel_plane(primary)->frontbuffer_bit;
>>>>  }
>>>>  
>>>>  static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>>>> @@ -11761,34 +11768,6 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
>>>>  	return true;
>>>>  }
>>>>  
>>>> -static void intel_crtc_check_initial_planes(struct drm_crtc *crtc,
>>>> -					    struct drm_crtc_state *crtc_state)
>>>> -{
>>>> -	struct intel_crtc_state *pipe_config =
>>>> -		to_intel_crtc_state(crtc_state);
>>>> -	struct drm_plane *p;
>>>> -	unsigned visible_mask = 0;
>>>> -
>>>> -	drm_for_each_plane_mask(p, crtc->dev, crtc_state->plane_mask) {
>>>> -		struct drm_plane_state *plane_state =
>>>> -			drm_atomic_get_existing_plane_state(crtc_state->state, p);
>>>> -
>>>> -		if (WARN_ON(!plane_state))
>>>> -			continue;
>>>> -
>>>> -		if (!plane_state->fb)
>>>> -			crtc_state->plane_mask &=
>>>> -				~(1 << drm_plane_index(p));
>>>> -		else if (to_intel_plane_state(plane_state)->visible)
>>>> -			visible_mask |= 1 << drm_plane_index(p);
>>>> -	}
>>>> -
>>>> -	if (!visible_mask)
>>>> -		return;
>>>> -
>>>> -	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>>>> -}
>>>> -
>>>>  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>>>>  				   struct drm_crtc_state *crtc_state)
>>>>  {
>>>> @@ -11810,10 +11789,6 @@ 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 */
>>>> -	if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
>>>> -		intel_crtc_check_initial_planes(crtc, crtc_state);
>>>> -
>>>>  	if (mode_changed && !crtc_state->active)
>>>>  		intel_crtc->atomic.update_wm_post = true;
>>>>  
>>>> @@ -13155,19 +13130,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>>>>  			continue;
>>>>  		}
>>>>  
>>>> -		if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>>>> -			ret = drm_atomic_add_affected_planes(state, crtc);
>>>> -			if (ret)
>>>> -				return ret;
>>>> -
>>>> -			/*
>>>> -			 * We ought to handle i915.fastboot here.
>>>> -			 * If no modeset is required and the primary plane has
>>>> -			 * a fb, update the members of crtc_state as needed,
>>>> -			 * and run the necessary updates during vblank evasion.
>>>> -			 */
>>>> -		}
>>>> -
>>>>  		if (!needs_modeset(crtc_state)) {
>>>>  			if (!(pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE))
>>>>  				continue;
>>>> @@ -15149,25 +15111,30 @@ void intel_modeset_init(struct drm_device *dev)
>>>>  	drm_modeset_unlock_all(dev);
>>>>  
>>>>  	for_each_intel_crtc(dev, crtc) {
>>>> -		if (!crtc->active)
>>>> +		struct intel_initial_plane_config plane_config;
>>>> +		struct drm_plane *plane;
>>>> +
>>>> +		if (!crtc->base.state->active)
>>>>  			continue;
>>>>  
>>>> +		/* disable all non-primary planes */
>>>> +		drm_for_each_plane_mask(plane, dev,
>>>> +					crtc->base.state->plane_mask)
>>>> +			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>>>> +				to_intel_plane(plane)->disable_plane(plane, &crtc->base);
>>>> +
>>>> +		crtc->base.state->plane_mask &=
>>>> +			1 << drm_plane_index(crtc->base.primary);
>>>> +
>>>> +		plane_config.fb = NULL;
>>>> +		dev_priv->display.get_initial_plane_config(crtc,
>>>> +							   &plane_config);
>>>> +
>>>>  		/*
>>>> -		 * Note that reserving the BIOS fb up front prevents us
>>>> -		 * from stuffing other stolen allocations like the ring
>>>> -		 * on top.  This prevents some ugliness at boot time, and
>>>> -		 * can even allow for smooth boot transitions if the BIOS
>>>> -		 * fb is large enough for the active pipe configuration.
>>>> +		 * If the fb is shared between multiple heads, we'll
>>>> +		 * just get the first one.
>>>>  		 */
>>>> -		if (dev_priv->display.get_initial_plane_config) {
>>>> -			dev_priv->display.get_initial_plane_config(crtc,
>>>> -							   &crtc->plane_config);
>>>> -			/*
>>>> -			 * If the fb is shared between multiple heads, we'll
>>>> -			 * just get the first one.
>>>> -			 */
>>>> -			intel_find_initial_plane_obj(crtc, &crtc->plane_config);
>>>> -		}
>>>> +		intel_find_initial_plane_obj(crtc, &plane_config);
>>>>  	}
>>>>  }
>>> Ok I looked at this and the readout_plane_state function and I think we
>>> have a bit a confusion about responsibilities here. The big thing is that
>>> only driver load cares about reconstructing plane state accurately, for
>>> resume and lid notifier we just want to make sure that we update the
>>> planes. And that could be achieved by unconditionally setting
>>> crtc_state->planes_changed. We already have all the plane states when
>>> restoring state anyway.
>> On resume I force a modeset for this reason, and set the plane_mask.
>> This makes sure any plane gets disabled.
>>
>> The modeset will also add all planes, which sets planes_changed if needed.
> Yeah that's what I had in mind for resume: We need to grab all plane
> states anyway (to be able to restore the old config), so at most we need
> to set a planes_changed to make sure the update happens.
>
>> A commit on its own doesn't do this, when a plane doesn't have a fb
>> it won't disable it on its own because the plane's assumed to be disabled
>> when old_plane_state->fb == NULL and new_plane_state->fb == NULL.
> Not a problem since too many planes doesn't seem to happen in reality. At
> least we didn't have code to force-disable planes on resume/lid_notify
> before, which means we don't suddenly need it now. sanitize_* should only
> fix up stuff that's broken for i915 on real-world machines, not what all
> could be possible.
>
>> I don't know if I can call disable_plane in this loop either, because that will
>> update the watermarks on skylake with the call to intel_update_sprite_watermarks
>> calling skl_update_sprite_wm calling skl_update_wm.
> Hm that sounds like a bug still. Maybe just forget about disabling
> non-primary planes for now on driver load? We don't seem to have any need
> currently either. And for primary planes maybe we can hard-code something
> which just clears the PLANE_ENABLE bit and does nothing else?  Kind of a
> super-low-level plane force disable. Similar to how we have a special
> function to force-disable the vga plane.
>
>> We don't even have any watermarks calculated yet, so that will break.
>>> That means we should move readout_plane_state into the above loop. Which
>>> then gets a bit too big, so better extract this into a
>>> intel_reconstruct_plane_state or similar. This function should then do all
>>> the plane state reconstruction.
>>>
>>> That also means we don't have to play tricks with plane_mask like you do.
>>> We simply reconstruct the primary plane (if possible) and force-disable
>>> all the others. Since this only happens at driver load there's no need to
>>> clear out any state for sprite/cursors since it's already fully cleared.
>>>
>>> I think this way we should be able to have everything in one place, and
>>> that should allow us to simplify things a lot.
>> I do this trick for atomic resume, we can't allocate the original fb in that case
>> but I still want to sanitize everything. This either happens because
>> a new primary fb gets committed or the primary fb gets disabled.
> Well on resume we don't care at all about the original fb for two reasons:
> - Anything remotely modern resumes with everything off.
> - We only recover the initial plane from the bios to ensure boot-up is
>   completely flicker-free. If we don't reserve that we'll allocate rings
>   and other gpu objects in there which isn't pretty.
>
> I don't think we need to sanitize planes either as long as we force a full
> plane update on resume (by forcing planes_changed or similar).
This doesn't work.

If a plane has no fb in software you won't update it with commit_planes_on_crtc,
because old_state->crtc and plane_state->crtc are both NULL.
> And for the lid_restore case the problem hasn't been that the bios changed
> the plane state really, but that it just went ahead and disabled
> everything on its own. That will result in a crtc_enable which will
> restore everything correctly.
>
> Imo we don't need to have a perfect sanitize for everything, this code
> really just fixes up what's actually been broken in real-world machines
> out there. That kind of real-world testing is also why I want to change as
> little as possible in the sanitize_* functions.
>
>>>> @@ -15404,47 +15371,30 @@ static void readout_plane_state(struct intel_crtc *crtc,
>>>>  	struct intel_plane *p;
>>>>  	struct drm_plane_state *drm_plane_state;
>>>>  	bool active = crtc_state->base.active;
>>>> +	bool wrong_plane;
>>>>  
>>>> -	if (active) {
>>>> -		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>>>> -
>>>> -		/* apply to previous sw state too */
>>>> -		to_intel_crtc_state(crtc->base.state)->quirks |=
>>>> -			PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>>>> -	}
>>>> +	wrong_plane = active && !intel_check_plane_mapping(crtc);
>>>> +	if (wrong_plane)
>>>> +		crtc->pipe = !crtc->pipe;
>>>>  
>>>>  	for_each_intel_plane(crtc->base.dev, p) {
>>>> -		bool visible = active;
>>>> -
>>>>  		if (crtc->pipe != p->pipe)
>>>>  			continue;
>>>>  
>>>>  		drm_plane_state = p->base.state;
>>>>  
>>>> -		/* Plane scaler state is not touched here. The first atomic
>>>> -		 * commit will restore all plane scalers to its old state.
>>>> -		 */
>>>> -
>>>> -		if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) {
>>>> -			visible = primary_get_hw_state(crtc);
>>>> -			to_intel_plane_state(drm_plane_state)->visible = visible;
>>>> -		} else {
>>>> -			/*
>>>> -			 * unknown state, assume it's off to force a transition
>>>> -			 * to on when calculating state changes.
>>>> -			 */
>>>> +		if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
>>>> +			to_intel_plane_state(drm_plane_state)->visible =
>>>> +				primary_get_hw_state(crtc);
>>>> +		else
>>>>  			to_intel_plane_state(drm_plane_state)->visible = false;
>>>> -		}
>>>>  
>>>> -		if (visible) {
>>>> -			crtc_state->base.plane_mask |=
>>>> -				1 << drm_plane_index(&p->base);
>>>> -		} else if (crtc_state->base.state) {
>>>> -			/* Make this unconditional for atomic hw readout. */
>>>> -			crtc_state->base.plane_mask &=
>>>> -				~(1 << drm_plane_index(&p->base));
>>>> -		}
>>>> +		crtc_state->base.plane_mask |=
>>>> +			1 << drm_plane_index(&p->base);
>>>>  	}
>>>> +
>>>> +	if (wrong_plane)
>>>> +		crtc->pipe = !crtc->pipe;
>>>>  }
>>>>  
>>>>  static void intel_modeset_readout_hw_state(struct drm_device *dev)
>>>> @@ -15664,6 +15614,9 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>>>  			update_state_fb(c->primary);
>>>>  			c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
>>>>  		}
>>>> +		else
>>>> +			obj->frontbuffer_bits |=
>>>> +				to_intel_plane(c->primary)->frontbuffer_bit;
>>> This is part of plane state reconstruction, no need to move it. The only
>>> reason we have to pin late is that gem isn't initialized yet fully when we
>>> want to reconstruct the modeset state. And that pinning shouldn't ever
>>> fail, it just increments pin_count and writes the ptes (again).
>> Why bother handling the return value at all then?
> To print the warning. I think even the cleanup is pointless since at worst
> we'll underrun the pin count, and we have protection for that already
> (again with a loud warning). If you want you can remove that code.
Ok.

~Maarten

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux