Re: [PATCH 10/13] drm/i915: Calculate watermark configuration during atomic check

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 28-08-15 om 15:42 schreef Ander Conselvan De Oliveira:
> On Thu, 2015-08-20 at 18:12 -0700, Matt Roper wrote:
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      | 10 ++++++
>>  drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  drivers/gpu/drm/i915/intel_pm.c      | 66 +++++++-----------------------------
>>  4 files changed, 71 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c91bab9..ac13cd7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1686,6 +1686,13 @@ struct i915_execbuffer_params {
>>  	struct drm_i915_gem_request     *request;
>>  };
>>  
>> +/* used in computing the new watermarks state */
>> +struct intel_wm_config {
>> +	unsigned int num_pipes_active;
>> +	bool sprites_enabled;
>> +	bool sprites_scaled;
>> +};
>> +
>>  struct drm_i915_private {
>>  	struct drm_device *dev;
>>  	struct kmem_cache *objects;
>> @@ -1903,6 +1910,9 @@ struct drm_i915_private {
>>  		 */
>>  		uint16_t skl_latency[8];
>>  
>> +		/* Committed wm config */
>> +		struct intel_wm_config config;
>> +
>>  		/*
>>  		 * The skl_wm_values structure is a bit too big for stack
>>  		 * allocation, so we keep the staging struct where we store
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index c40f025..8e9d87a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13005,6 +13005,44 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Handle calculation of various watermark data at the end of the atomic check
>> + * phase.  The code here should be run after the per-crtc and per-plane 'check'
>> + * handlers to ensure that all derived state has been updated.
>> + */
>> +static void calc_watermark_data(struct drm_atomic_state *state)
>> +{
>> +	struct drm_device *dev = state->dev;
>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *cstate;
>> +	struct drm_plane *plane;
>> +	struct drm_plane_state *pstate;
>> +
>> +	/*
>> +	 * Calculate watermark configuration details now that derived
>> +	 * plane/crtc state is all properly updated.
>> +	 */
>> +	drm_for_each_crtc(crtc, dev) {
>> +		cstate = drm_atomic_get_existing_crtc_state(state, crtc) ?:
>> +			crtc->state;
>> +
> Did you intend to check crtc->active here?
>
>> +		intel_state->wm_config.num_pipes_active++;
>> +	}
>> +	drm_for_each_legacy_plane(plane, dev) {
>> +		pstate = drm_atomic_get_existing_plane_state(state, plane) ?:
>> +			plane->state;
>> +
>> +		if (!to_intel_plane_state(pstate)->visible)
>> +			continue;
> If I understand correctly, it is possible for a plane on an inactive crtc to have visible = true. In
> that case, the result here would be different than the function this replaces, which counts only
> planes on active crtcs.
>
Now that i915's atomic visibility is updated correctly in almost all cases except initial readout perhaps. :-)

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