Re: [PATCH 01/10] drm: i915: Defining Compression Capabilities

["Singh, Gaurav K" <gaurav.k.singh@intel.com>]

On 2/24/2018 5:24 AM, Manasi Navare wrote:
> On Fri, Feb 23, 2018 at 09:25:44PM +0530, Gaurav K Singh wrote:
>> For Vesa Display Stream compression, defining structures for
>> compression capabilities to be stored in encoder.
>>
>> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  | 125 +++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h |  62 +++++++++++++++++++
>>   include/drm/drm_dp_helper.h      |   1 +
>>   3 files changed, 188 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 0d8cb74e7d02..4b1c323c0925 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -780,6 +780,131 @@ struct i915_psr {
>>   	void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
>>   };
>>   
>> +/* DSC Configuration structure */
>> +#define NUM_BUF_RANGES		15
>> +
>> +/* Configuration for a single Rate Control model range */
>> +struct rc_range_parameters {
>> +	/* Min Quantization Parameters allowed for this range */
>> +	unsigned long range_min_qp;
> Its only a 5 bit value, so uint_8 should be good, why have a unsigned long.
> Same for max_qp which is 5 bits and bpg_offset which is 6 bits.
> Please consider these for the parameters below.
My bad, will take care in next patchset.
>
>> +	/* Max Quantization Parameters allowed for this range */
>> +	unsigned long range_max_qp;
>> +	/* Bits/group offset to apply to target for this group */
>> +	unsigned long range_bpg_offset;
>> +};
>> +
>> +struct vdsc_config {
>> +	/* Bits / component for previous reconstructed line buffer */
>> +	unsigned long line_buf_depth;
>> +	/*
>> +	 * Rate control buffer size (in bits); not in PPS,
>> +	 * used only in C model for checking overflow
>> +	 */
>> +	unsigned long rc_bits;
>> +	/* Bits per component to code (must be 8, 10, or 12) */
>> +	unsigned long bits_per_component;
>> +	/*
>> +	 * Flag indicating to do RGB - YCoCg conversion
>> +	 * and back (should be 1 for RGB input)
>> +	 */
>> +	bool convert_rgb;
>> +	unsigned long slice_count;
> For eDP, it can be max 4, why unsigned long?
Will take care.
>
>> +	/* Slice Width */
>> +	unsigned long slice_width;
>> +	/* Slice Height */
>> +	unsigned long slice_height;
>> +	/*
>> +	 * 4:2:2 enable mode (from PPS, 4:2:2 conversion happens
>> +	 * outside of DSC encode/decode algorithm)
>> +	 */
>> +	bool enable422;
>> +	/* Picture Width */
>> +	unsigned long pic_width;
>> +	/* Picture Height */
>> +	unsigned long pic_height;
>> +	/* Offset to bits/group used by RC to determine QP adjustment */
>> +	unsigned long rc_tgt_offset_high;
>> +	/* Offset to bits/group used by RC to determine QP adjustment */
>> +	unsigned long rc_tgt_offset_low;
>> +	/* Bits/pixel target << 4 (ie., 4 fractional bits) */
>> +	unsigned long bits_per_pixel;
>> +	/*
>> +	 * Factor to determine if an edge is present based
>> +	 * on the bits produced
>> +	 */
>> +	unsigned long rc_edge_factor;
>> +	/* Slow down incrementing once the range reaches this value */
>> +	unsigned long rc_quant_incr_limit1;
>> +	/* Slow down incrementing once the range reaches this value */
>> +	unsigned long rc_quant_incr_limit0;
>> +	/* Number of pixels to delay the initial transmission */
>> +	unsigned long initial_xmit_delay;
>> +	/* Number of pixels to delay the VLD on the decoder,not including SSM */
>> +	unsigned long  initial_dec_delay;
>> +	/* Block prediction range (in pixels) */
>> +	bool block_pred_enable;
>> +	/* Bits/group offset to use for first line of the slice */
>> +	unsigned long first_line_bpg_Ofs;
>> +	/* Value to use for RC model offset at slice start */
>> +	unsigned long initial_offset;
>> +	/* X position in the picture of top-left corner of slice */
>> +	unsigned long x_start;
>> +	/* Y position in the picture of top-left corner of slice */
>> +	unsigned long y_start;
>> +	/* Thresholds defining each of the buffer ranges */
>> +	unsigned long rc_buf_thresh[NUM_BUF_RANGES - 1];
>> +	/* Parameters for each of the RC ranges */
>> +	struct rc_range_parameters rc_range_params[NUM_BUF_RANGES];
>> +	/* Total size of RC model */
>> +	unsigned long rc_model_size;
>> +	/* Minimum QP where flatness information is sent */
>> +	unsigned long flatness_minQp;
>> +	/* Maximum QP where flatness information is sent */
>> +	unsigned long flatness_maxQp;
>> +	/*
>> +	 * MAX-MIN for all components is required to
>> +	 * be <= this value for flatness to be used
>> +	 */
>> +	unsigned long flatness_det_thresh;
>> +	/* Initial value for scale factor */
>> +	unsigned long initial_scale_value;
>> +	/* Decrement scale factor every scale_decrement_interval groups */
>> +	unsigned long scale_decrement_interval;
>> +	/* Increment scale factor every scale_increment_interval groups */
>> +	unsigned long scale_increment_interval;
>> +	/* Non-first line BPG offset to use */
>> +	unsigned long nfl_bpg_offset;
>> +	/* BPG offset used to enforce slice bit */
>> +	unsigned long slice_bpg_offset;
>> +	/* Final RC linear transformation offset value */
>> +	unsigned long final_offset;
>> +	/* Enable on-off VBR (ie., disable stuffing bits) */
>> +	bool vbr_enable;
>> +	/* Mux word size (in bits) for SSM mode */
>> +	unsigned long mux_word_size;
>> +	/*
>> +	 * The (max) size in bytes of the "chunks" that are
>> +	 * used in slice multiplexing
>> +	 */
>> +	unsigned long chunk_size;
>> +	/* Placeholder for PPS identifier */
>> +	unsigned long pps_identifier;
>> +	/* DSC Minor Version */
>> +	unsigned long dsc_version_minor;
>> +	/* DSC Major version */
>> +	unsigned long dsc_version_major;
>> +	/* Number of VDSC engines */
>> +	unsigned long num_vdsc_instances;
>> +};
>> +
>> +/* Compression caps stored in encoder */
>> +struct i915_compression_params {
>> +	bool compression_support;
>> +	unsigned long compression_bpp;
> Does this indicate the 16 bit value with 4 fractional bits? May be add a comment for that.
Yes, it indicates that only. Will add a comment for that.
>
>> +	struct vdsc_config dsc_cfg;
>> +	unsigned char slice_count;
>> +};
> There is probably no need to have these convoluted structs, the compressed bpp and slice count
> are obtained directly from DPCDs which can be populated where needed with the helpers instead
> of populating them into dp_dsc_sink_cap and then populating i915_compression_params.slice_count/
> compressed_bpp.
>
> Again I have helpers for those already currently on the internal M-L.
> intel_dp can then only have compression_support and dsc_cfg.
My idea was to use a local struct variable named dp_sink_dsc_caps and 
then get the required values from DPCDs and do the required bitwise 
operations if needed .Using this local struct to populate dsc_cfg.
But i do agree and will populate dsc_cfg directly from DPCDs in the next 
patchset without using the local struct variable.
>> +
>>   enum intel_pch {
>>   	PCH_NONE = 0,	/* No PCH present */
>>   	PCH_IBX,	/* Ibexpeak PCH */
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 5853d92a6512..6e1b907990bf 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -954,6 +954,63 @@ struct intel_dp_compliance {
>>   	u8 test_lane_count;
>>   };
>>   
>> +/* Vesa Display Stream Capability of DP Sink */
>> +struct dp_sink_dsc_caps {
>> +	/* Display Stream Compression Support */
>> +	bool is_dsc_supported;
>> +	u8 dsc_major_ver;
>> +	u8 dsc_minor_ver;
>> +	u16 rcbuffer_blocksize;
>> +	/* n+1 value */
>> +	u16 rcbuffer_size_in_blocks;
>> +	unsigned long rcbuffer_size;
>> +
>> +	union {
>> +		u8 slice_caps;
>> +		struct {
>> +			u8 one_slice_per_line_support : 1;
>> +			u8 two_slice_per_line_support : 1;
>> +			u8 slice_caps_reserved1 : 1;
>> +			u8 four_slice_per_line_support : 1;
>> +			u8 slice_caps_reserved2 : 4;
>> +		};
>> +	};
>> +	/* Decode line buffer bits of precision */
>> +	unsigned long line_buffer_bit_depth;
>> +	bool is_block_pred_supported;
>> +	unsigned long sink_support_max_bpp;
>> +
>> +	union {
>> +		u8 color_format_caps;
>> +		struct {
>> +			u8 RGB_support : 1;
>> +			u8 YCbCr444_support : 1;
>> +			u8 YCbCr422_support : 1;
>> +			u8 color_format_caps_reserved : 5;
>> +		};
>> +	};
>> +
>> +	union {
>> +		u8 color_depth_caps;
>> +		struct {
>> +			u8 color_depth_caps_reserved1 : 1;
>> +			u8 support_8bpc : 1;
>> +			u8 support_10bpc : 1;
>> +			u8 support_12bpc : 1;
>> +			u8 color_depth_caps_reserved2 : 4;
>> +		};
>> +	};
>> +
>> +	u16 slice_height;
>> +	u16 slice_width;
>> +	/* Y Resolution */
>> +	u16 pic_height;
>> +	/* X Resolution */
>> +	u16 pic_width;
>> +};
> Since we will cache the entire DPCD register set for DSC, we really dont need to have structure
> for saving those values, they can be computed whenever needed using helpers/macros that read the cached
> DPCD values. That is the convention followed for values obtained from DP DPCD in rest of the driver.
> I have the patches for helpers that give these values but currently on internal M-L.
>
> Manasi
>
>> +
>> +
>> +
>>   struct intel_dp {
>>   	i915_reg_t output_reg;
>>   	i915_reg_t aux_ch_ctl_reg;
>> @@ -971,6 +1028,8 @@ struct intel_dp {
>>   	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
>>   	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>>   	uint8_t edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
>> +	uint8_t dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE];
>> +	uint8_t fec_dpcd;
>>   	/* source rates */
>>   	int num_source_rates;
>>   	const int *source_rates;
>> @@ -1046,6 +1105,9 @@ struct intel_dp {
>>   
>>   	/* Displayport compliance testing */
>>   	struct intel_dp_compliance compliance;
>> +
>> +	/* For Vesa Display Stream Compression Support */
>> +	struct i915_compression_params compr_params;
>>   };
>>   
>>   struct intel_lspcon {
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index da58a428c8d7..05f811c50d28 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -896,6 +896,7 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
>>   #define DP_RECEIVER_CAP_SIZE		0xf
>>   #define EDP_PSR_RECEIVER_CAP_SIZE	2
>>   #define EDP_DISPLAY_CTL_CAP_SIZE	3
>> +#define DP_DSC_RECEIVER_CAP_SIZE	0xb
>>   
>>   void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
>>   void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx