| Message ID | 20230329-rfc-msm-dsc-helper-v2-4-3c13ced536b2@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce MSM-specific DSC helpers | expand |
On 31/03/2023 21:49, Jessica Zhang wrote: > Correct the math for slice_last_group_size so that it matches the > calculations downstream. > > Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC") > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c > index b952f7d2b7f5..9312a8d7fbd9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c > @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc, > if (is_cmd_mode) > initial_lines += 1; > > - slice_last_group_size = 3 - (dsc->slice_width % 3); > + slice_last_group_size = dsc->slice_width % 3; > + > + if (slice_last_group_size == 0) > + slice_last_group_size = 3; Hmm. As I went on checking this against techpack: mod = dsc->slice_width % 3 mod | techpack | old | your_patch 0 | 2 | 3 | 3 1 | 0 | 2 | 1 2 | 1 | 1 | 2 So, obviously neither old nor new code match the calculations of the techpack. If we assume that sde_dsc_helper code is correct (which I have no reasons to doubt), then the proper code should be: slice_last_group_size = (dsc->slice_width + 2) % 3; Could you please doublecheck and adjust. > + > data = (initial_lines << 20); > data |= ((slice_last_group_size - 1) << 18); > /* bpp is 6.4 format, 4 LSBs bits are for fractional part */ >
On 4/2/2023 4:27 AM, Dmitry Baryshkov wrote: > On 31/03/2023 21:49, Jessica Zhang wrote: >> Correct the math for slice_last_group_size so that it matches the >> calculations downstream. >> >> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC") >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >> index b952f7d2b7f5..9312a8d7fbd9 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >> @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc >> *hw_dsc, >> if (is_cmd_mode) >> initial_lines += 1; >> - slice_last_group_size = 3 - (dsc->slice_width % 3); >> + slice_last_group_size = dsc->slice_width % 3; >> + >> + if (slice_last_group_size == 0) >> + slice_last_group_size = 3; > > Hmm. As I went on checking this against techpack: > > mod = dsc->slice_width % 3 > > mod | techpack | old | your_patch > 0 | 2 | 3 | 3 > 1 | 0 | 2 | 1 > 2 | 1 | 1 | 2 > > So, obviously neither old nor new code match the calculations of the > techpack. If we assume that sde_dsc_helper code is correct (which I have > no reasons to doubt), then the proper code should be: > > slice_last_group_size = (dsc->slice_width + 2) % 3; > > Could you please doublecheck and adjust. Hi Dmitry, The calculation should match the techpack calculation (I kept the `data |= ((slice_last_group_size - 1) << 18);` a few lines down). Thanks, Jessica Zhang > >> + >> data = (initial_lines << 20); >> data |= ((slice_last_group_size - 1) << 18); >> /* bpp is 6.4 format, 4 LSBs bits are for fractional part */ >> > > -- > With best wishes > Dmitry >
On 04/04/2023 00:45, Jessica Zhang wrote: > > > On 4/2/2023 4:27 AM, Dmitry Baryshkov wrote: >> On 31/03/2023 21:49, Jessica Zhang wrote: >>> Correct the math for slice_last_group_size so that it matches the >>> calculations downstream. >>> >>> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC") >>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >>> index b952f7d2b7f5..9312a8d7fbd9 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >>> @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc >>> *hw_dsc, >>> if (is_cmd_mode) >>> initial_lines += 1; >>> - slice_last_group_size = 3 - (dsc->slice_width % 3); >>> + slice_last_group_size = dsc->slice_width % 3; >>> + >>> + if (slice_last_group_size == 0) >>> + slice_last_group_size = 3; >> >> Hmm. As I went on checking this against techpack: >> >> mod = dsc->slice_width % 3 >> >> mod | techpack | old | your_patch >> 0 | 2 | 3 | 3 >> 1 | 0 | 2 | 1 >> 2 | 1 | 1 | 2 >> >> So, obviously neither old nor new code match the calculations of the >> techpack. If we assume that sde_dsc_helper code is correct (which I >> have no reasons to doubt), then the proper code should be: >> >> slice_last_group_size = (dsc->slice_width + 2) % 3; >> >> Could you please doublecheck and adjust. > > Hi Dmitry, > > The calculation should match the techpack calculation (I kept the `data > |= ((slice_last_group_size - 1) << 18);` a few lines down). And the techpack doesn't have -1. I think the following code piece would be more convenient as it is simpler: slice_last_group_size = (dsc->slice_width + 2) % 3; [...] data |= slice_last_group_size << 18; If you agree, could you please switch to it? > > Thanks, > > Jessica Zhang > >> >>> + >>> data = (initial_lines << 20); >>> data |= ((slice_last_group_size - 1) << 18); >>> /* bpp is 6.4 format, 4 LSBs bits are for fractional part */ >>> >> >> -- >> With best wishes >> Dmitry >>
On 4/3/2023 2:51 PM, Dmitry Baryshkov wrote: > On 04/04/2023 00:45, Jessica Zhang wrote: >> >> >> On 4/2/2023 4:27 AM, Dmitry Baryshkov wrote: >>> On 31/03/2023 21:49, Jessica Zhang wrote: >>>> Correct the math for slice_last_group_size so that it matches the >>>> calculations downstream. >>>> >>>> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC") >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >>>> index b952f7d2b7f5..9312a8d7fbd9 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >>>> @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc >>>> *hw_dsc, >>>> if (is_cmd_mode) >>>> initial_lines += 1; >>>> - slice_last_group_size = 3 - (dsc->slice_width % 3); >>>> + slice_last_group_size = dsc->slice_width % 3; >>>> + >>>> + if (slice_last_group_size == 0) >>>> + slice_last_group_size = 3; >>> >>> Hmm. As I went on checking this against techpack: >>> >>> mod = dsc->slice_width % 3 >>> >>> mod | techpack | old | your_patch >>> 0 | 2 | 3 | 3 >>> 1 | 0 | 2 | 1 >>> 2 | 1 | 1 | 2 >>> >>> So, obviously neither old nor new code match the calculations of the >>> techpack. If we assume that sde_dsc_helper code is correct (which I >>> have no reasons to doubt), then the proper code should be: >>> >>> slice_last_group_size = (dsc->slice_width + 2) % 3; >>> >>> Could you please doublecheck and adjust. >> >> Hi Dmitry, >> >> The calculation should match the techpack calculation (I kept the >> `data |= ((slice_last_group_size - 1) << 18);` a few lines down). > > And the techpack doesn't have -1. > > I think the following code piece would be more convenient as it is simpler: > > slice_last_group_size = (dsc->slice_width + 2) % 3; > [...] > data |= slice_last_group_size << 18; > > If you agree, could you please switch to it? Sure. Thanks, Jessica Zhang > >> >> Thanks, >> >> Jessica Zhang >> >>> >>>> + >>>> data = (initial_lines << 20); >>>> data |= ((slice_last_group_size - 1) << 18); >>>> /* bpp is 6.4 format, 4 LSBs bits are for fractional part */ >>>> >>> >>> -- >>> With best wishes >>> Dmitry >>> > > -- > With best wishes > Dmitry >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c index b952f7d2b7f5..9312a8d7fbd9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c @@ -56,7 +56,11 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc, if (is_cmd_mode) initial_lines += 1; - slice_last_group_size = 3 - (dsc->slice_width % 3); + slice_last_group_size = dsc->slice_width % 3; + + if (slice_last_group_size == 0) + slice_last_group_size = 3; + data = (initial_lines << 20); data |= ((slice_last_group_size - 1) << 18); /* bpp is 6.4 format, 4 LSBs bits are for fractional part */
Correct the math for slice_last_group_size so that it matches the calculations downstream. Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC") Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)