Re: [PATCH 13/33] iris: vidc: add helper functions for power management

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

On 7/28/2023 11:16 PM, Konrad Dybcio wrote:
> On 28.07.2023 15:23, Vikash Garodia wrote:
>> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>
>> this implements functions for calculating current load of the
>> hardware. Depending on the count of instances and
>> resolutions it selects the best clock rate for the video
>> core. Also it scales clocks, power and enable/disable dcvs.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---[...]
> 
>> +
>> +/* TODO: Move to dtsi OR use source clock instead of branch clock.*/
>> +#define MSM_VIDC_CLOCK_SOURCE_SCALING_RATIO 3
> Seems unused in this patch.
Right, moved to the resource.c file where it is used.
> 
>> +
>> +enum vidc_bus_type {
>> +	PERF,
>> +	DDR,
>> +	LLCC,
>> +};
>> +
>> +/*
>> + * Minimum dimensions for which to calculate bandwidth.
>> + * This means that anything bandwidth(0, 0) ==
>> + * bandwidth(BASELINE_DIMENSIONS.width, BASELINE_DIMENSIONS.height)
>> + */
>> +static const struct {
>> +	int height, width;
>> +} BASELINE_DIMENSIONS = {
>> +	.width = 1280,
>> +	.height = 720,
>> +};
>> +
>> +/* converts Mbps to bps (the "b" part can be bits or bytes based on context) */
> if 'b', the multiplier must be 1024 or it makes no sense
>
we are calculating the value in decimal hence multiplication factor is 1000.

>> +#define kbps(__mbps) ((__mbps) * 1000)
>> +#define bps(__mbps) (kbps(__mbps) * 1000)
>> +
> [...]
> 
>> +void __dump(struct dump dump[], int len)
>> +{
>> +	int c = 0;
>> +
>> +	for (c = 0; c < len; ++c) {
>> +		char format_line[128] = "", formatted_line[128] = "";
> That's a lot of bytes on the stack..
> 
This code will be removed in next version as part of custom debug wrapper
removal.
>> +
>> +		if (dump[c].val == DUMP_HEADER_MAGIC) {
>> +			snprintf(formatted_line, sizeof(formatted_line), "%s\n",
>> +				 dump[c].key);
>> +		} else {
>> +			snprintf(format_line, sizeof(format_line),
>> +				 "    %-35s: %s\n", dump[c].key,
>> +					 dump[c].format);
>> +			snprintf(formatted_line, sizeof(formatted_line),
>> +				 format_line, dump[c].val);
>> +		}
>> +		d_vpr_b("%s", formatted_line);
>> +	}
>> +}
>> +
>> +u64 msm_vidc_max_freq(struct msm_vidc_inst *inst)
>> +{
>> +	struct msm_vidc_core *core;
>> +	struct frequency_table *freq_tbl;
>> +	u64 freq = 0;
>> +
>> +	core = inst->core;
>> +
>> +	if (!core->resource || !core->resource->freq_set.freq_tbl ||
>> +	    !core->resource->freq_set.count) {
>> +		i_vpr_e(inst, "%s: invalid frequency table\n", __func__);
>> +		return freq;
>> +	}
>> +	freq_tbl = core->resource->freq_set.freq_tbl;
> Do we need a separate freuqency table if we have OPP?
we are using freq table at multiple places to
- setting freq to turbo
- limiting the freq to NOM
- changing the frequency values based on DCVS
hence this table is need.

will explore more to see if it can be removed.
> [...]
> 
> 
>> +	if (inst->power.fw_cf) {
>> +		cf = inst->power.fw_cf;
>> +		frame_size = (msm_vidc_get_mbs_per_frame(inst) / (32 * 8) * 3) / 2;
> too magic!
> 
this is the standard calculation for calculating the frame size, will try
to simplify this in next version.

Thanks,
Dikshita
> Konrad