Re: [PATCH 5/8] media: vidc: add Host Firmware Interface (HFI)

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 22 Aug 06:13 PDT 2016, Stanimir Varbanov wrote:

> This is the implementation of HFI. It is loaded with the
> responsibility to comunicate with the firmware through an
> interface commands and messages.
> 
>  - hfi.c has interface functions used by the core, decoder
> and encoder parts to comunicate with the firmware. For example
> there are functions for session and core initialisation.
> 

I can't help feeling that the split between core.c and hfi.c is a
remnant of a vidc driver supporting both HFI and pre-HFI with the same
v4l code.

What do you think about merging vidc_core with hfi_core and vidc_inst
with hfi_inst? Both seems to be in a 1:1 relationship.

>  - hfi_cmds has packetization operations which preparing
> packets to be send from host to firmware.
> 
>  - hfi_msgs takes care of messages sent from firmware to the
> host.
> 
[..]
> diff --git a/drivers/media/platform/qcom/vidc/hfi_cmds.c b/drivers/media/platform/qcom/vidc/hfi_cmds.c
[..]
> +
> +static const struct hfi_packetization_ops hfi_default = {
> +	.sys_init = pkt_sys_init,
> +	.sys_pc_prep = pkt_sys_pc_prep,
> +	.sys_idle_indicator = pkt_sys_idle_indicator,
> +	.sys_power_control = pkt_sys_power_control,
> +	.sys_set_resource = pkt_sys_set_resource,
> +	.sys_release_resource = pkt_sys_unset_resource,
> +	.sys_debug_config = pkt_sys_debug_config,
> +	.sys_coverage_config = pkt_sys_coverage_config,
> +	.sys_ping = pkt_sys_ping,
> +	.sys_image_version = pkt_sys_image_version,
> +	.ssr_cmd = pkt_ssr_cmd,
> +	.session_init = pkt_session_init,
> +	.session_cmd = pkt_session_cmd,
> +	.session_set_buffers = pkt_session_set_buffers,
> +	.session_release_buffers = pkt_session_release_buffers,
> +	.session_etb_decoder = pkt_session_etb_decoder,
> +	.session_etb_encoder = pkt_session_etb_encoder,
> +	.session_ftb = pkt_session_ftb,
> +	.session_parse_seq_header = pkt_session_parse_seq_header,
> +	.session_get_seq_hdr = pkt_session_get_seq_hdr,
> +	.session_flush = pkt_session_flush,
> +	.session_get_property = pkt_session_get_property,
> +	.session_set_property = pkt_session_set_property,
> +};
> +
> +static const struct hfi_packetization_ops *get_3xx_ops(void)
> +{
> +	static struct hfi_packetization_ops hfi_3xx;
> +
> +	hfi_3xx = hfi_default;
> +	hfi_3xx.session_set_property = pkt_session_set_property_3xx;
> +
> +	return &hfi_3xx;
> +}
> +
> +const struct hfi_packetization_ops *
> +hfi_get_pkt_ops(enum hfi_packetization_type type)

The only reasonable argument I can come up with for not just exposing
these as global functions would be that there are 23 of them... Can we
skip the jump table?

> +{
> +	switch (type) {
> +	case HFI_PACKETIZATION_LEGACY:
> +		return &hfi_default;
> +	case HFI_PACKETIZATION_3XX:
> +		return get_3xx_ops();
> +	}
> +
> +	return NULL;
> +}

Regards,
Bjorn