Re: [PATCH 04/21 v2] drm/i915: skylake scaler structure definitions

[Matt Roper <matthew.d.roper@intel.com>]

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
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx