On 2022-10-05 01:40:12, Dmitry Baryshkov wrote: > On 05/10/2022 01:35, Marijn Suijten wrote: > > On 2022-10-04 17:45:50, Dmitry Baryshkov wrote: > >> On Sat, 1 Oct 2022 at 22:08, Marijn Suijten > >> <marijn.suijten@xxxxxxxxxxxxxx> wrote: > >> [..] > >>> - bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8); > >>> + bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8); > >> > >> > >> bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 * 16); ? > > > > Not necessarily a fan of this, it "hides" the fact that we are dealing > > with 4 fractional bits (1/16th precision, it is correct though); but > > since this is the only use of `bpp` I can change it and document this > > fact wiht a comment on top (including referencing the validation pointed > > out in dsi_populate_dsc_params()). > > > > Alternatively we can inline the `>> 4` here? > > No, I don't think so. If we shift by 4 bits, we'd loose the fractional > part. DIV_ROUND_UP( .... , 8 * 16) ensures that we round it up rather > than just dropping it. I'd still keep the `-EINVAL` on `if (dsc->bits_per_pixel & 0xf)` to guarantee that there is no fractional part. After all, as explained in the patch description, none of this code / the DSI driver in general seems to be able to handle fractional bits per pixel. > >>> [..] > >>> - dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8; > >>> - if ((dsc->slice_width * dsc->bits_per_pixel) % 8) > >>> + dsc->slice_chunk_size = dsc->slice_width * bpp / 8; > >>> + if ((dsc->slice_width * bpp) % 8) > >> > >> One can use fixed point math here too: > >> > >> dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel + 8 * > >> 16 - 1)/ (8 * 16); > > > > Good catch, this is effectively a DIV_ROUND_UP() that we happened to > > call bytes_in_slice above... > > > > Shall I tackle this in the same patch, or insert another cleanup patch? > > It's up to you. I usually prefer separate patches, even if just to ease > bisecting between unrelated changes. Same feeling here, and have already set it up that way; added two extra patches to 1. replace this with DIV_ROUND_UP() and 2. remove the recalculation of slice_chunk_size (disguised as bytes_in_slice) above. - Marijn
