On Wed, Mar 18, 2015 at 09:00:20AM +0100, Daniel Vetter wrote:
> 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.
Ok forget about all this, somehow I got really confused with the split
between intel_scaler and intel_scaler_state. Dunno why but I thought some
of the limits are in scaler_state and need to be recomputed. I'll
follow-up with new comments.
-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
