Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Maarten Lankhorst]

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 >> ---
>>>  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 = _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 
>>
>>
>>> /* 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 

Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Paulo Zanoni]

Em Qui, 2016-10-20 às 15:18 -0200, Paulo Zanoni escreveu:
> 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  > > om
> > > > 
> > > > 
> > > ---
> > >  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 = _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;
> > >  

Also obligatory bikeshed to remove the ugly blank line above :)

> > > + 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 
> > 
> > 
> > > 
> > > 
> > >   /* 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.
> > >    */
> > > - 

Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[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  > >
> > ---
> >  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 = _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 
> 
> 
> > 
> >     /* 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;
> > +   

Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Matt Roper]

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 
> ---
>  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 = _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 


>   /* 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++) {
> + unsigned rate;
>   uint16_t plane_blocks, y_plane_blocks = 0;
> - int id = skl_wm_plane_id(intel_plane);
>  
> - data_rate =