On Tue, Mar 17, 2015 at 09:20:11PM +0000, Konduru, Chandra wrote:
>
>
> > -----Original Message-----
> > From: Roper, Matthew D
> > Sent: Tuesday, March 17, 2015 9:13 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel; Conselvan De Oliveira, Ander
> > Subject: Re: [PATCH 04/21] drm/i915: skylake scaler structure definitions
> >
> > On Sat, Mar 14, 2015 at 10:55:29PM -0700, Chandra Konduru wrote:
> > > skylake scaler structure definitions. scalers live in crtc_state as
> > > they are pipe resources. They can be used either as plane scaler or
> > > panel fitter.
> > >
> > > scaler assigned to either plane (for plane scaling) or crtc (for panel
> > > fitting) is saved in scaler_id in plane_state or crtc_state respectively.
> > >
> > > scaler_id is used instead of scaler pointer in plane or crtc state to
> > > avoid updating scaler pointer everytime a new crtc_state is created.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_drv.h | 99
> > ++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 99 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 3f7d05e..d9a3b64 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -256,6 +256,35 @@ struct intel_plane_state {
> > > * enable/disable the primary plane
> > > */
> > > bool hides_primary;
> > > +
> > > + /*
> > > + * scaler_id
> > > + * = -1 : not using a scaler
> > > + * >= 0 : using a scalers
> > > + *
> > > + * plane requiring a scaler:
> > > + * - During check_plane, its bit is set in
> > > + * crtc_state->scaler_state.scaler_users by calling helper function
> > > + * update_scaler_users.
> > > + * - scaler_id indicates the scaler it got assigned.
> > > + *
> > > + * plane doesn't require a scaler:
> > > + * - this can happen when scaling is no more required or plane simply
> > > + * got disabled.
> > > + * - During check_plane, corresponding bit is reset in
> > > + * crtc_state->scaler_state.scaler_users by calling helper function
> > > + * update_scaler_users.
> > > + *
> > > + * There are two scenarios:
> > > + * 1. the freed up scaler is assigned to crtc or some other plane
> > > + * In this case, as part of plane programming scaler_id will be set
> > > + * to -1 using helper function detach_scalers
> > > + * 2. the freed up scaler is not assigned to anyone
> > > + * In this case, as part of plane programming scaler registers will
> > > + * be reset and scaler_id will also be reset to -1 using the same
> > > + * helper function detach_scalers
> > > + */
> > > + int scaler_id;
> > > };
> > >
> > > struct intel_initial_plane_config {
> > > @@ -265,6 +294,74 @@ struct intel_initial_plane_config {
> > > u32 base;
> > > };
> > >
> > > +struct intel_scaler {
> > > + int id;
> > > + int in_use;
> > > + uint32_t mode;
> > > + uint32_t filter;
> > > +
> > > + /*
> > > + * Supported scaling ratio is represented as a range in [min max]
> > > + * variables. This range covers both up and downscaling
> > > + * where scaling ratio = (dst * 100)/src.
> > > + * In above range any value:
> > > + * < 100 represents downscaling coverage
> > > + * > 100 represents upscaling coverage
> > > + * = 100 represents no-scaling (i.e., 1:1)
> > > + * e.g., a min value = 50 means -> supports upto 50% of original image
> > > + * a max value = 200 means -> supports upto 200% of original image
> > > + *
> > > + * if incoming flip requires scaling in the supported [min max] range
> > > + * then requested scaling will be performed.
> > > + */
> >
> > I've only skimmed a little ahead in the series, so I might have missed something,
> > but do we really need to track these on a per-scaler basis?
> > When you use the values here, I think you're always pulling the values from
> > scaler[0] unconditionally from what I saw so duplicating the values for each
> > scaler doesn't really help us.
> >
> > Is it possible to keep just one copy of these min/max values? On SKL all of our
> > scalers are homogeneous, so it doesn't feel like there's too much value to
> > duplicating these values. If we have a future platform with heterogeneous
> > scalers, it seems like we can figure out how to handle that appropriately if/when
> > we get there.
>
> I put them per scaler basis for future scalability, but can make them as one copy.
> It has some cascading effect on other patches so send out this one and other
> impacted ones.
In my experience adding flexibility in the design for which we don't yet
have a clear use-case established is a tricky design mistake: It always
sounds good and very often turns out to be a costly endeavor. I much
prefer to be as lazy as possible in code design and if there's no need yet
to not write code.
Also, can't we recompute these limits at runtime? I can't quickly check
since the code is in some other patches, but iirc that would resolve some
of the issues I've pointed out later on with keeping these in sync.
I think as a design rule atomic state structures should be constrained to
the input values (we need them by design of the atomic interfaces) and the
computed values we want to put into the hw. Intermediate results,
especially for constraint checking just complicate all the state handling.
There are some exceptions (e.g. pll computation where we have intermediate
dotclock values), but adding state to the permanent state structures has
costs. Imo better to avoid it.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
