[Intel-gfx] [PATCH 04/21] drm/i915: skylake scaler structure definitions
Konduru, Chandra
chandra.konduru at intel.com
Tue Mar 17 14:20:11 PDT 2015
> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, March 17, 2015 9:13 AM
> To: Konduru, Chandra
> Cc: intel-gfx at lists.freedesktop.org; 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 at intel.com>
> > ---
> > 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.
>
> > + uint32_t min_hsr;
> > + uint32_t max_hsr;
> > + uint32_t min_vsr;
> > + uint32_t max_vsr;
> > + uint32_t min_hvsr;
> > + uint32_t max_hvsr;
> > +
> > + uint32_t min_src_w;
> > + uint32_t max_src_w;
> > + uint32_t min_src_h;
> > + uint32_t max_src_h;
> > + uint32_t min_dst_w;
> > + uint32_t max_dst_w;
> > + uint32_t min_dst_h;
> > + uint32_t max_dst_h;
> > +};
> > +
> > +struct intel_crtc_scaler_state {
> > +#define INTEL_MAX_SCALERS 2
> > +#define SKL_NUM_SCALERS INTEL_MAX_SCALERS
> > + /* scalers available on this crtc */
> > + int num_scalers;
>
> Maybe add .num_scalers to the device_info struct? I know it doesn't make
> much of a difference, but it feels cleaner to have immutable traits of the
> hardware in device_info or even the base intel_crtc structure and leave the state
> variable for tracking things that can change at runtime.
num_scalers isn't symmetric across pipes, so we need maintain it per crtc.
>
> > + struct intel_scaler scalers[INTEL_MAX_SCALERS];
> > +
> > + /*
> > + * scaler_users: keeps track of users requesting scalers on this crtc.
> > + *
> > + * If a bit is set, a user is using a scaler.
> > + * Here user can be a plane or crtc as defined below:
> > + * bits 0-30 - plane (bit position is index from drm_plane_index)
> > + * bit 31 - crtc
> > + *
> > + * Instead of creating a new index to cover planes and crtc, using
> > + * existing drm_plane_index for planes which is well less than 31
> > + * planes and bit 31 for crtc. This should be fine to cover all
> > + * our platforms.
> > + *
> > + * intel_atomic_setup_scalers will setup available scalers to users
> > + * requesting scalers. It will gracefully fail if request exceeds
> > + * avilability.
> > + */
> > +#define SKL_CRTC_INDEX 31
> > + unsigned scaler_users;
>
> Might be slightly preferable to use a type with a specific size when creating a
> bitmask? I.e., uint32_t or uint64_t, just to be explicit.
I looked at other variables carrying bits, for example, disable_pipes,
prepare_pipes, modeset_pipes, disabled_planes etc. and all are
using just like above. And followed the same.
>
> > +
> > + /* scaler used by crtc for panel fitting purpose */
> > + int scaler_id;
> > +};
> > +
> > struct intel_crtc_state {
> > struct drm_crtc_state base;
> >
> > @@ -391,6 +488,8 @@ struct intel_crtc_state {
> >
> > bool dp_encoder_is_mst;
> > int pbn;
> > +
> > + struct intel_crtc_scaler_state scaler_state;
> > };
> >
> > struct intel_pipe_wm {
> > --
> > 1.7.9.5
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
More information about the Intel-gfx
mailing list