Op 20-10-16 om 19:18 schreef Paulo Zanoni:
> Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu:
>> On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote:
>>> It's only used in one function, and can be calculated without
>>> caching it
>>> in the global struct by using
>>> drm_atomic_crtc_state_for_each_plane_state.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx
>>> ---
>>> drivers/gpu/drm/i915/intel_drv.h | 4 ----
>>> drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++---------
>>> ------------
>>> 2 files changed, 21 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index bb468c974e14..888054518f3c 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
>>> struct skl_pipe_wm optimal;
>>> struct skl_ddb_entry ddb;
>>>
>>> - /* cached plane data rate */
>>> - unsigned plane_data_rate[I915_MAX_PLANES];
>>> - unsigned
>>> plane_y_data_rate[I915_MAX_PLANES];
>>> -
>>> /* minimum block allocation */
>>> uint16_t minimum_blocks[I915_MAX_PLANES];
>>> uint16_t
>>> minimum_y_blocks[I915_MAX_PLANES];
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>> b/drivers/gpu/drm/i915/intel_pm.c
>>> index b96a899c899d..97b6202c4097 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct
>>> intel_crtc_state *cstate,
>>> * 3 * 4096 * 8192 * 4 < 2^32
>>> */
>>> static unsigned int
>>> -skl_get_total_relative_data_rate(struct intel_crtc_state
>>> *intel_cstate)
>>> +skl_get_total_relative_data_rate(struct intel_crtc_state
>>> *intel_cstate,
>>> + unsigned *plane_data_rate,
>>> + unsigned *plane_y_data_rate)
>>> {
>>> struct drm_crtc_state *cstate = &intel_cstate->base;
>>> struct drm_atomic_state *state = cstate->state;
>>> struct drm_crtc *crtc = cstate->crtc;
>>> - struct drm_device *dev = crtc->dev;
>>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>> struct drm_plane *plane;
>>> const struct intel_plane *intel_plane;
>>> @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct
>>> intel_crtc_state *intel_cstate)
>>> /* packed/uv */
>>> rate = skl_plane_relative_data_rate(intel_cstate,
>>> pstate, 0);
>>> - intel_cstate->wm.skl.plane_data_rate[id] = rate;
>>> + plane_data_rate[id] = rate;
>>> +
>>> + total_data_rate += rate;
>>>
>>> /* y-plane */
>>> rate = skl_plane_relative_data_rate(intel_cstate,
>>> pstate, 1);
>>> - intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
>>> - }
>>> -
>>> - /* Calculate CRTC's total data rate from cached values */
>>> - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
>>> {
>>> - int id = skl_wm_plane_id(intel_plane);
>>> + plane_y_data_rate[id] = rate;
>>>
>>> - /* packed/uv */
>>> - total_data_rate += intel_cstate-
>>>> wm.skl.plane_data_rate[id];
>>> - total_data_rate += intel_cstate-
>>>> wm.skl.plane_y_data_rate[id];
>>> + total_data_rate += rate;
>>> }
>>>
>>> return total_data_rate;
>>> @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
>>> *cstate,
>>> int num_active;
>>> int id, i;
>>>
>>> + unsigned data_rate[I915_MAX_PLANES] = {};
>>> + unsigned y_data_rate[I915_MAX_PLANES] = {};
>>> +
>> Minor nitpick; if you picked a different names here (e.g.,
>> plane_data_rate[]) then you could leave the local variables farther
>> down
>> named 'data_rate' and 'y_data_rate' which would reduce the diff
>> changes
>> and result in a slightly smaller patch.
>>
>> Whether or not you feel like making that change, killing the caching
>> is
>> good so,
>>
>> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>>
>>
>>> /* Clear the partitioning for disabled planes. */
>>> memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>>> memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>>> @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct
>>> intel_crtc_state *cstate,
>>> *
>>> * FIXME: we may not allocate every single block here.
>>> */
>>> - total_data_rate =
>>> skl_get_total_relative_data_rate(cstate);
>>> + total_data_rate = skl_get_total_relative_data_rate(cstate,
>>> data_rate, y_data_rate);
>>> if (total_data_rate == 0)
>>> return 0;
>>>
>>> start = alloc->start;
>>> - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
>>> {
>>> - unsigned int data_rate, y_data_rate;
>>> + for (id = 0; id < I915_MAX_PLANES; id++) {
> Can we please use a different kind of iteration? Although this is
> correct today, history shows that the number of planes increases over
> time and the code may suddenly break when if we ever introduce PLANE_D.
>
> Perhaps:
> for_each_intel_plane_on_crtc(...) {
> id = skl_wm_plane_id(intel_plane);
> ...
> }
>
> With that fixed (and, in case you want, Matt's suggestion):
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
Why would that break with PLANE_D? in that case rate = 0 and nothing happens. The hooks for it will never be called anyway, since it only updates up to max_planes.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
