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

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

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@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.

> +	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.

> +	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.

> +
> +	/* 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx