[Sound-open-firmware] [PATCH] [RFC]rmbox: include trace class definitions from trace header

[Yan Wang]

On 5/18/2018 6:27 PM, Liam Girdwood wrote:
> On Fri, 2018-05-18 at 16:52 +0800, yanwang wrote:
>> On Fri, 2018-05-18 at 09:06 +0100, Liam Girdwood wrote:
>>> On Fri, 2018-05-18 at 10:09 +0800, Yan Wang wrote:
>>>>
>>>>>
>>>>> So the trace class strings are deprecated, Yan was going to
>>>>> export all the
>>>>> trace
>>>>> object names + IDs via debugfs (like DAPM widgets already do).
>>>>> rmbox should
>>>>> read
>>>>> these at startup so it could then lookup comp name from the ID
>>>>> number
>>>>> embedded
>>>>> in trace data.
>>>>>
>>>>> Yan, do you have parts of this implemented ?
>>>>
>>>> In my previous RFC v2 patch set, I still keep trace class macro
>>>> definition in trace record but support switch on/off based
>>>> component id.
>>>> I am working on displaying compoent/widget name in rmbox and will
>>>> submit
>>>> in the next version patch set.
>>>> But for global trace like DMA/IPC/... or early component trace
>>>> which
>>>> locates in comp_new()/pipe_new()/buffer_new() may still need trace
>>>> class
>>>> because they haven't corresponding component/widget name?
>>>
>>> Non component trace classes can be exported as "extended boot data"
>>> i.e.
>>>
>>> 1) create a structure array that can be appended onto the boot
>>> complete message
>>> (after SRAM windows) and sent to the host.
>>>
>>> struct ipc_trace_class_elem {
>>> 	char name[SIZE];
>>> 	uint32_t id;
>>> };
>>>
>>> struct ipc_trace_class {
>>> 	int num_elems;
>>> 	struct ipc_trace_class_elem[0];
>>> };
>>>
>>> so you would have
>>>
>>> struct ipc_trace_class_elem trace_class_elems[] = {
>>> {"pipeline", PIPELINE_TRACE_ID},
>>> .....
>>> };
>>
>> Yes. I have finished this step now. And I also send name string from
>> SOF side to kernel driver.
>>
>>>
>>> 2) The driver will create a debugFS file for each trace class elem.
>>> When read
>>> (by rmbox or cat) it will contain the ID number. This way rmbox can
>>> map IDs to
>>> names for classes too.
>>
>> I think we may not need so many debugfs interfaces. We can pass one
>> parameter into one trace_level debugfs interface get all trace class
>> map. And rmbox can understand trace class after getting this map.
>>
> 
> Ok, you can do 1 file for classes, but components may come and go and may have
> different params for each so it makes sense to use a different file for each
> component.

Sure.

> 
>>>
>>> 3) rmbox reads these debugFS files at strartup to create it's ID ->
>>> name
>>> mapings.
>>>
>>> Steps 2 & 3 are very similar to the component ID name lookups.
>>>
>>> Doing this means rmbox and the driver do not need to be modified when
>>> we add new
>>> trace objects in the FW.
>>
>> Agree. We can remove all trace class macro definition.
>> both rmbox and kernel driver will be dependent on SOF. So it is
>> scalable fully.
>>
>> I attach my finished structure decalation for reviwing in the
>> following. It still include trace class. I can remove it in the next
>> version patch set.
>>
>> 922 /* DMA trace level type */
>> 923 enum sof_dma_trace_level_type {
>> 924         SOF_DMA_TRACE_LEVEL_IRQ = 0,
>> 925         SOF_DMA_TRACE_LEVEL_IPC,
>> 926         SOF_DMA_TRACE_LEVEL_DMA,
>> 927         SOF_DMA_TRACE_LEVEL_SSP,
>> 928         SOF_DMA_TRACE_LEVEL_WAIT,
>> 929         SOF_DMA_TRACE_LEVEL_LOCK,
>> 930         SOF_DMA_TRACE_LEVEL_MEM,
>> 931         SOF_DMA_TRACE_LEVEL_SA,
>> 932         SOF_DMA_TRACE_LEVEL_DMIC,
>> 933         SOF_DMA_TRACE_LEVEL_SCH,
>> 934         SOF_DMA_TRACE_LEVEL_VALUE,
>> 935         SOF_DMA_TRACE_LEVEL_END,
>> 936 };
>> 937
> 
> 
> So these are non component trace class IDs now, they dont need to be in ipc.h
> and should start at some high number so they wont conflict with component IDs
> (that start at 0).

Yes. I will keep current class << 24 for global trace. For component id, 
it will be restored in another unused uint32 bit currently. So they will 
not cause conflicts.

> 
>> 938 /* DMA for Trace level elem - SOF_IPC_DEBUG_DMA_LEVELS */
>> 939 struct sof_ipc_dma_trace_level {
>> 940         uint32_t trace_class;
>> 941         enum sof_dma_trace_level_type type;
>> 942         int32_t comp_id;
>> 943         char name[8];
>> 944         uint32_t level;
>> 945 }  __attribute__((packed));
> 
> 
> All you need here is name and ID. level, type will be set by userspace.

May we need one default level for this? I also want use comp_id to 
identify global and component trace level. If comp_id = -1, it is global 
trace level.

> 
>> 946
>> 947 /* DMA for Trace level info - SOF_IPC_DEBUG_DMA_LEVELS */
>> 948 struct sof_ipc_dma_trace_levels {
>> 949         struct sof_ipc_ext_data_hdr ext_hdr;
>> 950         uint32_t num_levels;
> 
> num_classes
> 
>> 951         struct sof_ipc_dma_trace_level level[0];
> 
> trace_class
> 
>>   
>> 952 }  __attribute__((packed));
>>
>> We can also use this structure to set and query multiply module/class
>> trace levels.
>> My current focus issue is component/widget trace level list from SOF
>> side. When FW_READY message sent, the topology should not be loadded.
> 
> Yes, topology is loaded after FW ready.

Yes.

> 
>> I may have to send component/widget trace level list later. Is it
>> right?
> 
> No, driver will create component trace debugFS objects when it loads topology.
> Driver will already have this data so no need for FW to send it.

Sure. I will keep previous solution. I will iterate widget list on 
kernel driver and get the list.

Thanks.
Yan Wang

> 
> Liam
> 
>>
>> Thanks.
>> Yan Wang
>>
>>>
>>> Liam
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>