RE: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface configuration

[Wen He]

> -----Original Message-----
> From: Liviu Dudau <liviu.dudau@xxxxxxx>
> Sent: 2019å7æ18æ 17:37
> To: Wen He <wen.he_1@xxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> brian.starkey@xxxxxxx; airlied@xxxxxxxx; daniel@xxxxxxxx; Leo Li
> <leoyang.li@xxxxxxx>
> Subject: Re: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface
> configuration
> 
> Caution: EXT Email
> 
> On Thu, Jul 18, 2019 at 03:29:48AM +0000, Wen He wrote:
> >
> >
> > > -----Original Message-----
> > > From: Liviu Dudau <liviu.dudau@xxxxxxx>
> > > Sent: 2019å7æ17æ 19:22
> > > To: Wen He <wen.he_1@xxxxxxx>
> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > brian.starkey@xxxxxxx; airlied@xxxxxxxx; daniel@xxxxxxxx; Leo Li
> > > <leoyang.li@xxxxxxx>
> > > Subject: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS
> > > interface configuration
> > >
> > >
> > > Hi Wen,
> > >
> >
> > Hi Liviu,
> 
> Hi Wen,
> 
> >
> > > On Wed, Jul 17, 2019 at 05:23:53PM +0800, Wen He wrote:
> > > > Configure the display Quality of service (QoS) levels to high
> > > > priority if the level is defined as high as in DTS. The ARQOS for
> > > > DP500 is driven from the "RQOS" register, needed to program the
> > > > RQOS register value < 7 for the 4k resolution flicker to disappear on the
> LS1028A platform.
> > >
> > > Thanks for taking time to come up with a more generic patch for your
> issue!
> > >
> >
> > Thanks for the review and comments,
> >
> > > I have a question: what happens if you program the
> > > MALIDP500_RQOS_QUALITY register to 0xd000d000 for all pixelclocks?
> > >
> >
> > We can't do that, because:
> > 1. The flicker issue only reproduced in 4k@60.
> 
> Can you clarify? Does this mean that with the RQOS setting of 0xd000d000
> you don't see flicker at lower resolutions, or that you haven't tested at other
> resolution than 4k@60?

I mean that I didn't see flicker issue at lower resolutions without the RQOS setting
of 0xd000d000.

The ROQS register default value's 0x00010001, in this case, only found the flicker
issue can be reproduced in 4k@60, other lower resolutions not found this issue.
So If setting the RQOS register value's to 0xd001d001, 4k flicker issue will be resolve.

> 
> > 2. This configuration will be reduced DDR benchmark performance data,
> > because the LCDC and DDR are both to connect CCI-400. we have to make
> > sure that DDR performance at first when work together with other
> resolutions.
> 
> Hmm, I'm not sure I'm sharing the same view of the problem. You are writing
> into DP500's QoS registers, which don't control how CCI-400 or DDR are going
> to behave. Now I agree that a more aggressive QoS from DP500 is going to
> lead to more contention to the DDR, but maybe you can look into CCI-400's
> QoS settings and tweak the bandwidth allocation there as well.

Yes, I agree with you, but after discussed with our design team, I chose them advice
that only change the DP500's QoS.
Here're comments from our design team:
-------------------
There are multiple registers that impact QOS values in the system, particularly as it
relates to transactions from the LCD Controller.
"The âbestâ approach (which in theory should result in the LCD Controller having
the bandwidth it needs to avoid buffer underrun, while also having the minimal impact
on the system) is to use the dynamic QoS information (ARQOS) coming from the LCD
Controller. To do this, you need to program the LCD Controllerâs âRQOSâ register. 
See the attached DP-500 TRM, Section 2.4.6, âDisplay Quality of Service Interfaceâ
and Section 4.2.9, âDisplay engine quality of service registersâ.
-------------------

> 
> >
> > > Also, some suggestions further down:
> > >
> > > >
> > > > Signed-off-by: Wen He <wen.he_1@xxxxxxx>
> > > > ---
> > > > change in v2:
> > > >         - add new implementation for 4k flicker issue on the
> > > > LS1028A
> > > >
> > > >  drivers/gpu/drm/arm/malidp_drv.c  |  5 +++++
> > > >  drivers/gpu/drm/arm/malidp_hw.c   | 13 +++++++++++++
> > > >  drivers/gpu/drm/arm/malidp_hw.h   |  3 +++
> > > >  drivers/gpu/drm/arm/malidp_regs.h | 12 ++++++++++++
> > > >  4 files changed, 33 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> > > > b/drivers/gpu/drm/arm/malidp_drv.c
> > > > index f25ec4382277..d2b2cf52ac87 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > > > @@ -818,6 +818,11 @@ static int malidp_bind(struct device *dev)
> > > >
> > > >       malidp->core_id = version;
> > > >
> > > > +     hwdev->arqos_high_level = false;
> > >
> > > This is not needed.
> > >
> > > > +
> > > > +     hwdev->arqos_high_level = of_property_read_bool(dev->of_node,
> > > > +
> > > > + "arm,malidp-arqos-high-level");
> > >
> > > I think it would be better to have this property as a u32 value,
> > > rather than a boolean, and put the value that we want to program
> > > MALIDP_RQOS_QUALITY with in there.
> >
> > I really thought that, but I've tested DDR performance for 4k
> > resolution with different RQOS setting, the best test performance data
> > is when set the RQOS register value's 0xd000d000. So the value is
> > fixed, I think the Boolean type is better used here, that's why I did it.
> 
> Yes, it is fixed for your platform, but not fixed for other ODMs that might
> decide to use the LS1028A chip to create a board. They might use different
> DDRs, or their PCB traces might have different lengths. I think it is still valuable
> to put the 0xd000d000 value into the device tree and read it from there. For
> LS1028A NXP boards it will do then the right thing.
> 

Hmm, I think you are right, should be use value to instead of the Boolean.

> >
> > >
> > > Also, you need to add the documentation for this optional property
> > > in Documentation/devicetree/bindings/display/arm,malidp.txt.
> >
> > Understand, I will generate another patch to add this change for the DT
> bindings.
> >
> > >
> >
> > > > +
> > > >       /* set the number of lines used for output of RGB data */
> > > >       ret = of_property_read_u8_array(dev->of_node,
> > > >
> > > > "arm,malidp-output-port-lines", diff --git
> > > > a/drivers/gpu/drm/arm/malidp_hw.c
> > > > b/drivers/gpu/drm/arm/malidp_hw.c index 50af399d7f6f..eaa1658cd86b
> > > > 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > > > @@ -374,6 +374,19 @@ static void malidp500_modeset(struct
> > > malidp_hw_device *hwdev, struct videomode *
> > > >               malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_ILACED,
> > > MALIDP_DE_DISPLAY_FUNC);
> > > >       else
> > > >               malidp_hw_clearbits(hwdev,
> MALIDP_DISP_FUNC_ILACED,
> > > > MALIDP_DE_DISPLAY_FUNC);
> > > > +
> > > > +     /*
> > > > +      *  Program the RQoS register to increasing QoS levels for
> > > > +      *  the 4k resolution flicker to disappear on the LS1028A.
> > >
> > > Looks like two sentences got smashed into one, the result is a bit hard to
> read.
> > >
> >
> > Ha, maybe should be described like this:
> > "Program the RQoS register to avoid 4k resolution flicker on the LS1028A."
> 
> Yes, that is much easier to understand.
> 

Thank you for the detailed review.

Best Regards,
Wen

> Thank you,
> Liviu
> 
> 
> >
> > Best Regards,
> > Wen
> >
> > > Best regards,
> > > Liviu
> > >
> > > > +      */
> > > > +     if (hwdev->arqos_high_level) {
> > > > +             val = RED_ARQOS_VALUE | GREEN_ARQOS_VALUE;
> > > > +
> > > > +             if (mode->pixelclock == 594000000)
> > > > +                     malidp_hw_setbits(hwdev, val,
> > > MALIDP500_RQOS_QUALITY);
> > > > +             else
> > > > +                     malidp_hw_clearbits(hwdev, val,
> > > MALIDP500_RQOS_QUALITY);
> > > > +     }
> > > >  }
> > > >
> > > >  int malidp_format_get_bpp(u32 fmt) diff --git
> > > > a/drivers/gpu/drm/arm/malidp_hw.h
> > > > b/drivers/gpu/drm/arm/malidp_hw.h index
> 968a65eed371..b8baba60508a
> > > > 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_hw.h
> > > > +++ b/drivers/gpu/drm/arm/malidp_hw.h
> > > > @@ -251,6 +251,9 @@ struct malidp_hw_device {
> > > >
> > > >       /* size of memory used for rotating layers, up to two banks
> > > > available
> > > */
> > > >       u32 rotation_memory[2];
> > > > +
> > > > +     /* priority level of RQOS register used for driven the ARQOS signal
> */
> > > > +     bool arqos_high_level;
> > > >  };
> > > >
> > > >  static inline u32 malidp_hw_read(struct malidp_hw_device *hwdev,
> > > > u32
> > > > reg) diff --git a/drivers/gpu/drm/arm/malidp_regs.h
> > > > b/drivers/gpu/drm/arm/malidp_regs.h
> > > > index 993031542fa1..08842142b3b2 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_regs.h
> > > > +++ b/drivers/gpu/drm/arm/malidp_regs.h
> > > > @@ -210,6 +210,18 @@
> > > >  #define MALIDP500_CONFIG_VALID               0x00f00
> > > >  #define MALIDP500_CONFIG_ID          0x00fd4
> > > >
> > > > +/*
> > > > + * The quality of service (QoS) register on the DP500. RQOS
> > > > +register values
> > > > + * are driven by the ARQOS signal, using AXI transacations,
> > > > +dependent on the
> > > > + * FIFO input level.
> > > > + * The RQOS register can also set QoS levels for:
> > > > + *    - RED_ARQOS   @ A 4-bit signal value for close to underflow
> > > conditions
> > > > + *    - GREEN_ARQOS @ A 4-bit signal value for normal conditions
> > > > + */
> > > > +#define MALIDP500_RQOS_QUALITY          0x00500
> > > > +#define RED_ARQOS_VALUE                 (0xd << 12)
> > > > +#define GREEN_ARQOS_VALUE               (0xd << 28)
> > > > +
> > > >  /* register offsets and bits specific to DP550/DP650 */
> > > >  #define MALIDP550_ADDR_SPACE_SIZE    0x10000
> > > >  #define MALIDP550_DE_CONTROL         0x00010
> > > > --
> > > > 2.17.1
> > > >
> > >
> > > --
> > > ====================
> > > | I would like to |
> > > | fix the world,  |
> > > | but they're not |
> > > | giving me the   |
> > >  \ source code!  /
> > >   ---------------
> > >     Â\_(ã)_/Â
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     Â\_(ã)_/Â