On Fri, 28 Jul 2023 at 22:25, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 7/27/2023 8:26 AM, Dmitry Baryshkov wrote:
> > On 27/07/2023 18:24, Abhinav Kumar wrote:
> >>
> >>
> >> On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote:
> >>> On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar
> >>> <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> >>>>> Switch to using data from MDSS driver to program the SSPP fetch and
> >>>>> UBWC
> >>>>> configuration.
> >>>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >>>>> ---
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 7 +++++--
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++++++++++++++-
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 +
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 ++-
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 ++
> >>>>> 6 files changed, 36 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> >>>>> index cf70a9bd1034..bfd82c2921af 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> >>>>> @@ -8,6 +8,8 @@
> >>>>> #include "dpu_hw_sspp.h"
> >>>>> #include "dpu_kms.h"
> >>>>>
> >>>>> +#include "msm_mdss.h"
> >>>>> +
> >>>>> #include <drm/drm_file.h>
> >>>>>
> >>>>> #define DPU_FETCH_CONFIG_RESET_VALUE 0x00000087
> >>>>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct
> >>>>> dpu_sw_pipe *pipe,
> >>>>> DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
> >>>>> DPU_FETCH_CONFIG_RESET_VALUE |
> >>>>> ctx->ubwc->highest_bank_bit << 18);
> >>>>
> >>>> Does this needs to be protected with if ctx->ubwc check?
> >>>
> >>> Yes... And it should have been already.
> >>>
> >>>>
> >>>>> - switch (ctx->ubwc->ubwc_version) {
> >>>>> - case DPU_HW_UBWC_VER_10:
> >>>>> + switch (ctx->ubwc->ubwc_enc_version) {
> >>>>> + case UBWC_1_0:
> >>>>
> >>>> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
> >>>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in
> >>>> the
> >>>> catalog for the encoder version in the first place? Because looking at
> >>>> the registers UBWC_x_x is the correct value.
> >>>>
> >>>> If we cannot find the reason why, it should be noted in the commit text
> >>>> that the values we are using did change.
> >>>
> >>> Huh? This is just an enum. It isn't a part of uABI, nor it is written
> >>> to the hardware.
> >>>
> >>
> >> The reason is that, this switch case is moving from comparing one set
> >> of values to totally different ones. So atleast that should be noted.
> >>
> >> First thing that struck me was this point because the enums UBWC_x_x
> >> and DPU_HW_UBWC_VER_xx dont match.
> >>
> >
> > Do you have any proposed text in mind?
> >
>
> I was doing some checking about this. The issue was that when this enum
> was made, it missed using the SDE_HW_UBWC_VER macro
>
>
> 75 enum {
> 76 DPU_HW_UBWC_VER_10 = 0x100,
> 77 DPU_HW_UBWC_VER_20 = 0x200,
> 78 DPU_HW_UBWC_VER_30 = 0x300,
> 79 DPU_HW_UBWC_VER_40 = 0x400,
> 80 };
> 81
>
> so something like this:
>
> 183 */
> 184 enum {
> 185 SDE_HW_UBWC_VER_10 = SDE_HW_UBWC_VER(0x100),
> 186 SDE_HW_UBWC_VER_20 = SDE_HW_UBWC_VER(0x200),
> 187 SDE_HW_UBWC_VER_30 = SDE_HW_UBWC_VER(0x300),
> 188 SDE_HW_UBWC_VER_40 = SDE_HW_UBWC_VER(0x400),
> 189 SDE_HW_UBWC_VER_43 = SDE_HW_UBWC_VER(0x431),
> 190 };
>
> This macro handles that conversion under the hood.
>
> So I would write something like this
>
> "This also corrects the usage of UBWC version which was incorrect from
> the beginning because of the enum storing the DPU_HW_UBWC_*** missed out
> the conversion to the full UBWC version"
I don't like the word 'correcting', as it makes one think there was an
issue beforehand. There were no real issues. So I'll try to cook
something up for the next iteration.
--
With best wishes
Dmitry
