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@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..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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
