Re: [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.
Date: Thu, 20 Oct 2016 08:05:27 +0200	[thread overview]
Message-ID: <8f7cbd15-b316-5db1-e5f0-e1542a9409c0@linux.intel.com> (raw)
In-Reply-To: <20161019231502.GA19156@intel.com>

Hey,

Op 20-10-16 om 01:15 schreef Matt Roper:
> On Wed, Oct 12, 2016 at 03:28:18PM +0200, Maarten Lankhorst wrote:
>> Allow the driver to write watermarks during atomic evasion.
>> This will make it possible to write the watermarks in a cleaner
>> way on gen9+.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
>>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++----------
>>  drivers/gpu/drm/i915/intel_pm.c      | 19 +++++++++++++++++--
>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index f65ccf9b0bea..09588c58148f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -484,6 +484,7 @@ struct sdvo_device_mapping {
>>  
>>  struct intel_connector;
>>  struct intel_encoder;
>> +struct intel_atomic_state;
>>  struct intel_crtc_state;
>>  struct intel_initial_plane_config;
>>  struct intel_crtc;
>> @@ -497,8 +498,9 @@ struct drm_i915_display_funcs {
>>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>>  				       struct intel_crtc *intel_crtc,
>>  				       struct intel_crtc_state *newstate);
>> -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
>> -	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
>> +	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
>> +	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
>> +	void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> initial_watermarks() and optimize_watermarks() are currently only used
> on ILK (and possibly by in-development VLV/CHV patches that Ville is
> working on?).  As far as I can see, the top-level state that we add as a
> parameter here doesn't actually get used in the implementations.  Are
> you adding it to just make them more similar to the signature of the new
> atomic_evade_watermarks vfunc or did you have something else in mind?
>
> I'd also suggest adding a brief comment to your new skl_evade_crtc_wm()
> function that indicates that nearly all of the gen9 watermark values are
> per-plane values that get written as part of the general plane update in
> skylake_update_primary_plane and/or skl_update_plane.  Given that those
> two functions are located in other files that may help clarify to future
> developers why this function appears so trivial.

We don't completely use intel_atomic_state here yet, patch 7 uses it for the
ddb allocation and because initial_watermarks ends up calling
.atomic_evade_watermarks().

I added intel_atomic_state to all callbacks to keep the function signature
identical. It looked better to me to put the function signature in a small
change, and then the behavioral change in patch 7 separately, for easier
bisection.

~Maarten

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