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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8ebcacd-19c7-b5e7-f372-6abde311e17d@linaro.org>
Date:   Fri, 28 Jul 2023 19:46:49 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Vikash Garodia <quic_vgarodia@...cinc.com>,
        stanimir.k.varbanov@...il.com, agross@...nel.org,
        andersson@...nel.org, mchehab@...nel.org, hans.verkuil@...co.com,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Cc:     quic_dikshita@...cinc.com
Subject: Re: [PATCH 13/33] iris: vidc: add helper functions for power
 management

On 28.07.2023 15:23, Vikash Garodia wrote:
> From: Dikshita Agarwal <quic_dikshita@...cinc.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@...cinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@...cinc.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.

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

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

> +
> +		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?
[...]


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

Konrad