> -----Original Message----- > From: liviu.dudau@xxxxxxx <liviu.dudau@xxxxxxx> > Sent: 2019年3月30日 0:11 > To: Wen He <wen.he_1@xxxxxxx> > Cc: brian.starkey@xxxxxxx; malidp@xxxxxxxxxxxx; > dri-devel@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/2] drm/arm/malidp: Setting QoS priority for 4k to avoid > flicker issue > > On Fri, Mar 29, 2019 at 08:20:00AM +0000, Wen He wrote: > > > > > > > -----Original Message----- > > > From: liviu.dudau@xxxxxxx [mailto:liviu.dudau@xxxxxxx] > > > Sent: 2019年3月29日 0:39 > > > To: Wen He <wen.he_1@xxxxxxx> > > > Cc: brian.starkey@xxxxxxx; malidp@xxxxxxxxxxxx; > > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > > Subject: Re: [PATCH 2/2] drm/arm/malidp: Setting QoS priority for 4k > > > to avoid flicker issue > > > > > > Hi Wen, > > > > > > On Thu, Mar 28, 2019 at 03:56:58AM +0000, Wen He wrote: > > > > When we running DDR benchmark test will able to observed flicker > > > > issue in 4k@60 resolution on monitor. In LS1028A SoC, need > > > > increasing DP500 priority to avoid that issue. > > > > > > > > Signed-off-by: Wen He <wen.he_1@xxxxxxx> > > > > --- > > > > drivers/gpu/drm/arm/malidp_hw.c | 13 +++++++++++++ > > > > drivers/gpu/drm/arm/malidp_regs.h | 8 ++++++++ > > > > 2 files changed, 21 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/arm/malidp_hw.c > > > > b/drivers/gpu/drm/arm/malidp_hw.c index > 8df12e9a33bb..a5263488eb02 > > > > 100644 > > > > --- a/drivers/gpu/drm/arm/malidp_hw.c > > > > +++ b/drivers/gpu/drm/arm/malidp_hw.c > > > > @@ -378,6 +378,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); > > > > + > > > > +#ifdef CONFIG_ARCH_LAYERSCAPE > > > > + /* Setting QoS for 4k resolution to avoid flicker issue */ > > > > + if (mode->hactive == 3840 > > > > + && mode->vactive == 2160) > > > > > > I'm not keen on this check, it only enables this for one 3840x2160. > > > What if the output is 2160x3840? Does it matter what pixel clock you use? > > > Can the same issue be seen if (for example) you use 120Hz refresh > > > rate but on a smaller output resolution? > > > > > > > Hi liviu, > > Hi Wen, > > > > > Let me clearly it. > > Currently, we only supported four resolutions (480p@60, 720p@60, > 1080@60, 4k@60) for LS1028A Display port. > > All of the refresh rate is 60MHz. so that impossible will be there resolution > output's 2160x3840 and refresh rate is 120MHz. > > Yes, but you are asking to merge this into the mainline driver which supports > more resolutions than that. > > > > > > I think it's worth thinking this in terms of bandwidth and not > > > hardcode for one resolution. > > > > > > > Yes, you are right. > > But only 4k resolution have the flicker issue, so this is a special problem. > > I think that hardcode is better than dynamically adjusting bandwidth. > > If you want to do that for your BSP, then it's fine. For mainline, I think we can > do better and think in more generic terms. > > > > > > Also, I think setting the QoS bits should be a bit more generic, i.e > > > if the modeset requires a certain bandwidth you should write a value > > > read from a device tree, for example? > > > > > > > + malidp_hw_setbits(hwdev, GREEN_ARQOS_CONFIG > > > > + | RED_ARQOS_CONFIG, MALIDP500_RQOS_QUALITY); > > > > + else > > > > + malidp_hw_clearbits(hwdev, GREEN_ARQOS_CONFIG > > > > + | RED_ARQOS_CONFIG, MALIDP500_RQOS_QUALITY); > > > > + > > > > + malidp_hw_setbits(hwdev, CONFIG_VALID, > MALIDP500_CONFIG_VALID); > > > > > > malidp500_modeset() will be called with MALIDP_CFG_VALID set anyway, > > > so you should not need this. I don't know what bit 1 in > > > MALIDP500_CONFIG_VALID does for LS1028A, I don't have that spec. > > > > > > > About this, I got this step from our design team, So just to make sure > > the configuration works that writing value 0x3 to > MALIDP500_CONFIG_VALID. > > Bit 1 in MALIDP500_CONFIG_VALID is not defined in the Arm spec, so don't > know what the NXP design team has done there, might be worth talking to > them. > > > > > Is there can always to valid configuration? If yes, I can remove this line. > > Yes, before we enter modeset we set bit 0 of MALIDP500_CONFIG_VALID and > we clear it after modeset is finished. DP500 is a bit weird, you need to look at > CONFIG_VALID as somewhat inverted as a signal from the software point of > view. > It is probably easier to think of it in the way the code was written, i.e. we > "enter config mode" before modeset and then "exit config mode" > afterwards. Hi Liviu, This patch is not a rigorous implementation for the mainline, I will use the new Implementation to send next version of the patch. Thanks for these comments. Best Regards, Wen > > > > > > > > > > +#endif > > > > } > > > > > > > > int malidp_format_get_bpp(u32 fmt) diff --git > > > > a/drivers/gpu/drm/arm/malidp_regs.h > > > > b/drivers/gpu/drm/arm/malidp_regs.h > > > > index a0dd6e1676a8..8dcf7e9f46ce 100644 > > > > --- a/drivers/gpu/drm/arm/malidp_regs.h > > > > +++ b/drivers/gpu/drm/arm/malidp_regs.h > > > > @@ -213,6 +213,14 @@ > > > > #define MALIDP500_DC_IRQ_BASE 0x00f00 > > > > #define MALIDP500_CONFIG_VALID 0x00f00 > > > > #define MALIDP500_CONFIG_ID 0x00fd4 > > > > +#ifdef CONFIG_ARCH_LAYERSCAPE > > > > +#define MALIDP500_RQOS_QUALITY 0x00500 > > > > +/* Increasing QoS level signal */ > > > > +#define GREEN_ARQOS_CONFIG (0xd << 28) > > > > +/* Close to underflow QoS level signal */ > > > > +#define RED_ARQOS_CONFIG (0xd << 12) > > > > +#define CONFIG_VALID 0x3 > > > > > > What are these values? Is it something NXP has added to the DP500? > > > If so, I think these should have some LAYERSCAPE (or LS) in their > > > name. Also, rather than hardcoding the values, would it not be > > > better to read them form device tree, to allow for fine tuning or variability > between IP deployments? > > > > > > Best regards, > > > Liviu > > > > > > > These values are QoS registers of DP500. > > I have already put CONFIG_ARCH_LAYERSCAPE definition to specify > architecture. > > > > Regarding your suggestion, I think device tree also can meet this, but > > that is Non-standard writing in here, because not have any properties > > define to describe QoS in arm,malidp device bindings file. > > What I was thinking was that you could add the properties and the > documentation ;) > > Best regards, > Liviu > > > > > Best Regards, > > Wen > > > > > > > +#endif > > > > > > > > /* register offsets and bits specific to DP550/DP650 */ > > > > #define MALIDP550_ADDR_SPACE_SIZE 0x10000 > > > > -- > > > > 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! / > --------------- > ¯\_(ツ)_/¯ _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
