On 8/29/2023 5:29 PM, Bryan O'Donoghue wrote: > On 29/08/2023 09:00, Vikash Garodia wrote: >> Hi Bryan, >> >> On 8/14/2023 7:45 PM, Bryan O'Donoghue wrote: >>> On 14/08/2023 07:34, Vikash Garodia wrote: >>>>> We have two loops that check for up to 32 indexes per loop. Why not have a >>>>> capabilities index that can accommodate all 64 bits ? >>>> Max codecs supported can be 32, which is also a very high number. At max the >>>> hardware supports 5-6 codecs, including both decoder and encoder. 64 indices is >>>> would not be needed. >>>> >>> >>> But the bug you are fixing here is an overflow where we have received a full >>> range 32 bit for each decode and encode. >>> >>> How is the right fix not to extend the storage to the maximum possible 2 x 32 ? >>> Or indeed why not constrain the input data to 32/2 for each encode/decode path ? >> At this point, we agree that there is very less or no possibility to have this >> as a real usecase i.e having 64 (or more than 32) codecs supported in video >> hardware. There seem to be no value add if we are extending the cap array from >> 32 to 64, as anything beyond 32 itself indicates rogue firmware. The idea here >> is to gracefully come out of such case when firmware is responding with such >> data payload. >> Again, lets think of constraining the data to 32/2. We have 2 32 bit masks for >> decoder and encoder. Malfunctioning firmware could still send payload with all >> bits enabled in those masks. Then the driver needs to add same check to avoid >> the memcpy in such case. >> >>> The bug here is that we can copy two arrays of size X into one array of size X. >>> >>> Please consider expanding the size of the storage array to accommodate the full >>> size the protocol supports 2 x 32. >> I see this as an alternate implementation to existing handling. 64 index would >> never exist practically, so accommodating it only implies to store the data for >> invalid response and gracefully close the session. > > What's the contractual definition of "this many bits per encoder and decoder" > between firmware and APSS in that case ? > > Where do we get the idea that 32/2 per encoder/decoder is valid but 32 per > encoder decoder is invalid ? > > At this moment in time 16 encoder/decoder bits would be equally invalid. > > I suggest the right answer is to buffer the protocol data unit - PDU maximum as > an RX or constrain the maximum number of encoder/decoder bits based on HFI version. > > ie. > > - Either constrain on the PDU or > - Constrain on the known number of maximum bits per f/w version Let me simply ask this - What benefit we will be getting with above approaches over the existing handling ? Thanks, Vikash > --- > bod >
