| Message ID | 1426896282-23215-5-git-send-email-chandra.konduru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 20, 2015 at 05:04:25PM -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. > > v2: > -made single copy of min/max values for scalers (Matt) > > Signed-off-by: Chandra Konduru <chandra.konduru@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..1da5087 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; If I'm reading later patches correctly, this looks like this is always PS_SCALER_MODE_HQ if one scaler is needed, or PS_SCALER_MODE_DYN if multiple scalers are needed. So the values for each of a CRTC's scalers doesn't actually vary; should this just be a single value in intel_crtc_scalar_state rather than being duplicated for each scaler? > + uint32_t filter; Is filter a constant? Unless I missed something in later patches, it looks like it's set to PS_FILTER_MEDIUM and then never changed. Can we drop the field and just use the constant itself where appropriate? > +}; > + > +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 stick this in intel_crtc since it never changes (i.e., distinguish runtime-changeable state from immutable hardware traits)? > + 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; > + > + /* scaler used by crtc for panel fitting purpose */ > + int scaler_id; Calling this something like 'pfit_scaler_id' might make it a little more intuitive what this is for when it's used in the code. > + > + /* > + * 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. > + */ > + uint32_t min_hsr; > + uint32_t max_hsr; > + uint32_t min_vsr; > + uint32_t max_vsr; > + uint32_t min_hvsr; > + uint32_t max_hvsr; I'm still not sure I understand why we need these in the state structure. The max_* fields here are set once, never changed, and never even read back, so I think they're completely droppable. The min_vsr and min_hvsr fields are updated later, but never actually read back, so I think they can go too. The only value we actually make use of here is min_hsr; I notice that it can get adjusted upward, but never downward, so I'm not sure if the logic there (patch #7) is quite right, but we may be able to just replace it with a direct use of crtc_clock instead? > + > + 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; I think these are set once and never changed, so a simple #define might be easier. > +}; > + > 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; If we can kill off a bunch of the fields above, then we may be able to put the remaining few fields directly in intel_crtc_state and eliminate a level of structure nesting, which might make things a bit simpler. Matt > }; > > struct intel_pipe_wm { > -- > 1.7.9.5 >
On Tue, Mar 24, 2015 at 10:13:54PM -0700, Matt Roper wrote: > On Fri, Mar 20, 2015 at 05:04:25PM -0700, Chandra Konduru wrote: > > +struct intel_scaler { > > + int id; > > + int in_use; > > + uint32_t mode; > > If I'm reading later patches correctly, this looks like this is always > PS_SCALER_MODE_HQ if one scaler is needed, or PS_SCALER_MODE_DYN if > multiple scalers are needed. So the values for each of a CRTC's scalers > doesn't actually vary; should this just be a single value in > intel_crtc_scalar_state rather than being duplicated for each scaler? Or we can just compute it at runtime with hweight of the inuse scaler bitmask. > > + uint32_t filter; > > Is filter a constant? Unless I missed something in later patches, it > looks like it's set to PS_FILTER_MEDIUM and then never changed. Can we > drop the field and just use the constant itself where appropriate? Concured. I prefer that we add struct members only when there's a need - all that indirection just makes it harder to follow the code logic. I also agree with all of the other comments from Matt cut out here. [snip] > > + struct intel_crtc_scaler_state scaler_state; > > If we can kill off a bunch of the fields above, then we may be able to > put the remaining few fields directly in intel_crtc_state and eliminate > a level of structure nesting, which might make things a bit simpler. Imo substructures for separate things make sense - otherwise we'll just have a pile of members with the same prefix, which semantically is exactly the same thing. But I agree that reducing dynamic state to only the bits that are indeed dynamic (and not trivially derived from existing dynamic state) is a good goal. -Daniel
> -----Original Message----- > From: Roper, Matthew D > Sent: Tuesday, March 24, 2015 10:14 PM > To: Konduru, Chandra > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander > Subject: Re: [PATCH 04/21 v2] drm/i915: skylake scaler structure definitions > > On Fri, Mar 20, 2015 at 05:04:25PM -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. > > > > v2: > > -made single copy of min/max values for scalers (Matt) > > > > Signed-off-by: Chandra Konduru <chandra.konduru@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..1da5087 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; > > If I'm reading later patches correctly, this looks like this is always > PS_SCALER_MODE_HQ if one scaler is needed, or PS_SCALER_MODE_DYN if > multiple scalers are needed. So the values for each of a CRTC's scalers doesn't > actually vary; should this just be a single value in intel_crtc_scalar_state rather > than being duplicated for each scaler? There is another mode required for nv12 which is coming up in next series. So it needs per scaler mode. > > > + uint32_t filter; > > Is filter a constant? Unless I missed something in later patches, it looks like it's > set to PS_FILTER_MEDIUM and then never changed. Can we drop the field and > just use the constant itself where appropriate? Hardware has additional filter types. Initial implementation is using only one type. So kept it as a parameter in scaler than hardcoding. > > > +}; > > + > > +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 stick this in intel_crtc since it never changes (i.e., distinguish runtime- > changeable state from immutable hardware traits)? The approach I followed is to keep all scaler related in one place. Certainly this can go to crtc, but don't see any issue by keeping it here. > > > + 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; > > + > > + /* scaler used by crtc for panel fitting purpose */ > > + int scaler_id; > > Calling this something like 'pfit_scaler_id' might make it a little more intuitive > what this is for when it's used in the code. Sure, but it is obvious that scaler_id here is for crtc, as it is hanging inside crtc_state. > > > + > > + /* > > + * 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. > > + */ > > + uint32_t min_hsr; > > + uint32_t max_hsr; > > + uint32_t min_vsr; > > + uint32_t max_vsr; > > + uint32_t min_hvsr; > > + uint32_t max_hvsr; > > I'm still not sure I understand why we need these in the state structure. The > max_* fields here are set once, never changed, and never even read back, so I > think they're completely droppable. The min_vsr and min_hvsr fields are > updated later, but never actually read back, so I think they can go too. The only > value we actually make use of here is min_hsr; I notice that it can get adjusted > upward, but never downward, so I'm not sure if the logic there (patch #7) is > quite right, but we may be able to just replace it with a direct use of crtc_clock > instead? > The approach I took is have state maintain values not only for this platform but for upcoming platform. This way, check function can be used not only for skl but also for upcoming platforms by initializing them appropriately in init function. Max fields can be dropped for skl, but kept it for above reason. Also currently there aren't properties to expose scaler limits/ranges to userland. One another reason for keeping them is to report as part of properties once they gets added in drm core. Even if we don't add for any reason, there is no harm in having all scaler limits in one place. > > > + > > + 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; > > I think these are set once and never changed, so a simple #define might be > easier. Again these were also kept here to make functions that use them work for upcoming platforms. > > > +}; > > + > > 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; > > If we can kill off a bunch of the fields above, then we may be able to put the > remaining few fields directly in intel_crtc_state and eliminate a level of structure > nesting, which might make things a bit simpler. It is bit tricky than that. Initially I started that way, but in crtc_state computations, current scaler state needs to be copied over to newer state to make it work correctly in legacy paths. Instead of copying individual members one by one, put them in a structure to save some typing and make code cleaner. > > > Matt > > > }; > > > > struct intel_pipe_wm { > > -- > > 1.7.9.5 > > > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3f7d05e..1da5087 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; +}; + +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; + 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; + + /* scaler used by crtc for panel fitting purpose */ + int scaler_id; + + /* + * 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. + */ + 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_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 {
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. v2: -made single copy of min/max values for scalers (Matt) Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 99 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+)