Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits

[Marijn Suijten]

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