Hi Bjorn,
Thanks for the comments!
On 08/23/2016 06:25 AM, Bjorn Andersson wrote:
> 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.
OK, I can give it a try.
>
>> - 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?
Of course we can, but what will be the benefit, increased readability ?
Let's keep it for now. Once the driver is merged some smarter guy could
came up with better solution ;)
>
>> +{
>> + switch (type) {
>> + case HFI_PACKETIZATION_LEGACY:
>> + return &hfi_default;
>> + case HFI_PACKETIZATION_3XX:
>> + return get_3xx_ops();
>> + }
>> +
>> + return NULL;
>> +}
>
> Regards,
> Bjorn
>
--
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
