Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Pierre-Louis Bossart]

On 7/30/24 11:19, Krzysztof Kozlowski wrote:
> On 30/07/2024 10:59, Pierre-Louis Bossart wrote:
>>>>>
>>>>> 	/* Read dpn properties for source port(s) */
>>>>> 	sdw_slave_read_dpn(slave, prop->src_dpn_prop, nval,
>>>>> 			   prop->source_ports, "source");
>>>>>
>>>>> IOW, this is a valid change, but it's an optimization, not a fix in the
>>>>> usual sense of 'kernel oops otherwise'.
>>>>>
>>>>> Am I missing something?
>>>>>
>>>>> BTW, the notion of DPn is that n > 0. DP0 is a special case with
>>>>> different properties, BIT(0) cannot be set for either of the sink/source
>>>>> port bitmask.
>>>>
>>>> I think we speak about two different things. port num > 1, that's
>>>> correct. But index for src_dpn_prop array is something different. Look
>>>> at mipi-disco sdw_slave_read_dpn():
>>>>
>>>> 173         u32 bit, i = 0;
>>>> ...
>>>> 178         addr = ports;
>>>> 179         /* valid ports are 1 to 14 so apply mask */
>>>> 180         addr &= GENMASK(14, 1);
>>>> 181
>>>> 182         for_each_set_bit(bit, &addr, 32) {
>>>> ...
>>>> 186                 dpn[i].num = bit;
>>>>
>>>>
>>>> so dpn[0..i] = 1..n
>>>> where i is also the bit in the mask.
>>
>> yes, agreed on the indexing.
>>
>> But are we in agreement that the case of non-contiguous ports would not
>> create any issues? the existing code is not efficient but it wouldn't
>> crash, would it?
>>
>> There are multiple cases of non-contiguous ports, I am not aware of any
>> issues...
>>
>> rt700-sdw.c:    prop->source_ports = 0x14; /* BITMAP: 00010100 */
>> rt711-sdca-sdw.c:       prop->source_ports = 0x14; /* BITMAP: 00010100
>> rt712-sdca-sdw.c:       prop->source_ports = BIT(8) | BIT(4);
>> rt715-sdca-sdw.c:       prop->source_ports = 0x50;/* BITMAP: 01010000 */
>> rt722-sdca-sdw.c:       prop->source_ports = BIT(6) | BIT(2); /* BITMAP:
>> 01000100 */
>>
>> same for sinks:
>>
>> rt712-sdca-sdw.c:       prop->sink_ports = BIT(3) | BIT(1); /* BITMAP:
>> 00001010 */
>> rt722-sdca-sdw.c:       prop->sink_ports = BIT(3) | BIT(1); /* BITMAP:
>> 00001010 */
> 
> All these work because they have separate source and sink dpn_prop
> arrays. Separate arrays, separate number of ports, separate masks - all
> this is good. Now going to my code...
> 
>>
>>>> Similar implementation was done in Qualcomm wsa and wcd codecs like:
>>>> array indexed from 0:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n51
>>>>
>>>> genmask from 0, with a mistake:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n1255
>>>>
>>>> The mistake I corrected here:
>>>> https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d7a8b56f05@xxxxxxxxxx/
>>>>
>>>> To summarize, the mask does not denote port numbers (1...14) but indices
>>>> of the dpn array which are from 0..whatever (usually -1 from port number).
>>>>
>>>
>>> Let me also complete this with a real life example of my work in
>>> progress. I want to use same dpn_prop array for sink and source ports
>>> and use different masks. The code in progress is:
>>>
>>> https://git.codelinaro.org/krzysztof.kozlowski/linux/-/commit/ef709a0e8ab2498751305367e945df18d7a05c78#6f965d7b74e712a5cfcbc1cca407b85443a66bac_2147_2157
>>>
>>> Without this patch, I get -EINVAL from sdw_get_slave_dpn_prop():
>>>   soundwire sdw-master-1-0: Program transport params failed: -2
>>
>> Not following, sorry. The sink and source masks are separate on purpose,
>> to allow for bi-directional ports. The SoundWire spec allows a port to
>> be configured at run-time either as source or sink. In practice I've
>> never seen this happen, all existing hardware relies on ports where the
>> direction is hard-coded/fixed, but still we want to follow the spec.
> 
> The ports are indeed hard-coded/fixed.
> 
>>
>> So if ports can be either source or sink, I am not sure how the
>> properties could be shared with a single array?
> 
> Because I could, just easier to code. :) Are you saying the code is not
> correct? If I understand the concept of source/sink dpn port mask, it
> should be correct. I have some array with source and sink ports. I pass
> it to Soundwire with a mask saying which ports are source and which are
> sink.
> 
>>
>> Those two lines aren't clear to me at all:
>>
>> 	pdev->prop.sink_dpn_prop = wsa884x_sink_dpn_prop;
>> 	pdev->prop.src_dpn_prop = wsa884x_sink_dpn_prop;
> 
> I could do: s/wsa884x_sink_dpn_prop/wsa884x_dpn_prop/ and expect the
> code to be correct.

Ah I think I see what you are trying to do, you have a single dpn_prop
array but each entry is valid for either sink or source depending on the
sink / source_mask which don't overlap.

Did I get this right?