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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DB7PR04MB51952D8288917D8C2ECFDBEAE2C80@DB7PR04MB5195.eurprd04.prod.outlook.com>
Date:   Thu, 18 Jul 2019 03:29:48 +0000
From:   Wen He <wen.he_1@....com>
To:     Liviu Dudau <liviu.dudau@....com>
CC:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "brian.starkey@....com" <brian.starkey@....com>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        "daniel@...ll.ch" <daniel@...ll.ch>, Leo Li <leoyang.li@....com>
Subject: RE: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface
 configuration



> -----Original Message-----
> From: Liviu Dudau <liviu.dudau@....com>
> Sent: 2019年7月17日 19:22
> To: Wen He <wen.he_1@....com>
> Cc: dri-devel@...ts.freedesktop.org; linux-kernel@...r.kernel.org;
> brian.starkey@....com; airlied@...ux.ie; daniel@...ll.ch; Leo Li
> <leoyang.li@....com>
> Subject: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface
> configuration
> 
> 
> Hi Wen,
> 

Hi Liviu,

> 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.
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.

> Also, some suggestions further down:
> 
> >
> > Signed-off-by: Wen He <wen.he_1@....com>
> > ---
> > 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.

> 
> 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."

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!  /
>   ---------------
>     ¯\_(ツ)_/¯