Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits
From: Marijn Suijten
Date: Tue Oct 04 2022 - 18:56:18 EST
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
> >>> [..]
> >>> - 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.