Re: [v1] drm/arm/mali-dp: Add a loop around the second set CVAL and try 5 times
On Wed, May 08, 2019 at 10:58:18AM +, Wen He wrote:
> This patch trying to fix monitor freeze issue caused by drm error
> 'flip_done timed out' on LS1028A platform. this set try is make a loop
> around the second setting CVAL and try like 5 times before giveing up.
>
> Signed-off-by: Liviu
> Signed-off-by: Wen He
Acked-by: Liviu Dudau
I will pull this into my mali-dp tree and send it as fixes after v5.2-rc1.
Best regards,
Liviu
> ---
> drivers/gpu/drm/arm/malidp_drv.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> b/drivers/gpu/drm/arm/malidp_drv.c
> index 21725c9b9f5e..18cb7f134f4e 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -192,6 +192,7 @@ static void malidp_atomic_commit_hw_done(struct
> drm_atomic_state *state)
> {
> struct drm_device *drm = state->dev;
> struct malidp_drm *malidp = drm->dev_private;
> + int loop = 5;
>
> malidp->event = malidp->crtc.state->event;
> malidp->crtc.state->event = NULL;
> @@ -206,8 +207,18 @@ static void malidp_atomic_commit_hw_done(struct
> drm_atomic_state *state)
> drm_crtc_vblank_get(&malidp->crtc);
>
> /* only set config_valid if the CRTC is enabled */
> - if (malidp_set_and_wait_config_valid(drm) < 0)
> + if (malidp_set_and_wait_config_valid(drm) < 0) {
> + /*
> + * make a loop around the second CVAL setting and
> + * try 5 times before giving up.
> + */
> + while (loop--) {
> + if (!malidp_set_and_wait_config_valid(drm))
> + break;
> + }
> DRM_DEBUG_DRIVER("timed out waiting for updated
> configuration\n");
> + }
> +
> } else if (malidp->event) {
> /* CRTC inactive means vblank IRQ is disabled, send event
> directly */
> spin_lock_irq(&drm->event_lock);
> --
> 2.17.1
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
___
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate
Hi Wen,
On Wed, May 15, 2019 at 02:42:08AM +, Wen He wrote:
> Disable checking for required pixel clock rate if ARCH_LAYERSCPAE
> is enable.
>
> Signed-off-by: Alison Wang
> Signed-off-by: Wen He
> ---
> change in description:
> - This check that only supported one pixel clock required clock rate
> compare with dts node value. but we have supports 4 pixel clock
> for ls1028a board.
So, your DT says your pixel clock provider is a fixed clock? If you support
more than one rate, you should instead use a real provider for it. How do you
support the 4 pixel clocks?
Also, not sure what the paragraph above is meant to be. Should it be part of
the commit message?
Best regards,
Liviu
> drivers/gpu/drm/arm/malidp_crtc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c
> b/drivers/gpu/drm/arm/malidp_crtc.c
> index 56aad288666e..bb79223d9981 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -36,11 +36,13 @@ static enum drm_mode_status malidp_crtc_mode_valid(struct
> drm_crtc *crtc,
>
> if (req_rate) {
> rate = clk_round_rate(hwdev->pxlclk, req_rate);
> +#ifndef CONFIG_ARCH_LAYERSCAPE
> if (rate != req_rate) {
> DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
>req_rate);
> return MODE_NOCLOCK;
> }
> +#endif
> }
>
> return MODE_OK;
> --
> 2.17.1
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate
On Thu, May 16, 2019 at 08:10:21AM +, Wen He wrote: > > > > -Original Message- > > From: [email protected] [mailto:[email protected]] > > Sent: 2019年5月15日 23:46 > > To: Wen He > > Cc: [email protected]; [email protected]; Leo Li > > > > Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel > > clock rate > > > > > > Hi Wen, > > Hi Liviu, > > > > > On Wed, May 15, 2019 at 02:42:08AM +, Wen He wrote: > > > Disable checking for required pixel clock rate if ARCH_LAYERSCPAE is > > > enable. > > > > > > Signed-off-by: Alison Wang > > > Signed-off-by: Wen He > > > --- > > > change in description: > > > - This check that only supported one pixel clock required clock rate > > > compare with dts node value. but we have supports 4 pixel clock > > > for ls1028a board. > > > > So, your DT says your pixel clock provider is a fixed clock? If you support > > more > > than one rate, you should instead use a real provider for it. How do you > > support the 4 pixel clocks? > > > > Yes , the DT node only can provided one pixel clock by using a fixed clock. > But we Display Port controller support 4 or more resolutions, each of which > requires a set of pixel clocks to drive, and we hope they can switch any > resolution > we want by some program every times. That program can't be some userspace application, because it will have to make changes to the hardware and the kernel will not know that things have changed under its feet. That leaves the option of the bootloader or some other kernel module doing the changes. If you have another kernel module that knows how to change clocks, that should be implemented using the common clocks infrastructure, at which time you can put it in the DT as the clock provider for the pixelclock. If the bootloader does the changes, then the bootloader should edit the DT and set the correct value for the pixel clock. Regardless, with your change and on your platform the user can request any resolution and the driver will silently fail to set that resolution. One other problem is the one Robin raised, where the kernel is compiled for multiple platforms, like what various Linux distributions do. That kernel will either work on other SoC or not, depending on what CONFIG_ARCH_LAYERSCAPE is set to. In summary, for this patch, it's a NAK. There are proper ways of achieving what you need, but this patch is not. Best regards, Liviu > > For example, if we set that fixed pixel clock is 2700 (27Mhz), but user > hope can see > a group 1080p resolution penguins during startup , and hope playing a 4k > video once > system boot up done. > Btw, In our board, the 1080p resolution is driven by a 148.5Mhz pixel clock, > 4k is driven > by a 594Mhz. 27Mhz only can drive 480p resolution. > > To meet the above user requirements, I was to setup following steps, > 1. Add the "video=1920x1080-32@60" to bootargs command line [specify penguins > size] > 2. Play a 4K video with 4k resolution when system boot up done. > > > Also, not sure what the paragraph above is meant to be. Should it be part of > > the commit message? > > > > These comments just want to let you know. > > > Best regards, > > Liviu > > > > > > > drivers/gpu/drm/arm/malidp_crtc.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c > > > b/drivers/gpu/drm/arm/malidp_crtc.c > > > index 56aad288666e..bb79223d9981 100644 > > > --- a/drivers/gpu/drm/arm/malidp_crtc.c > > > +++ b/drivers/gpu/drm/arm/malidp_crtc.c > > > @@ -36,11 +36,13 @@ static enum drm_mode_status > > > malidp_crtc_mode_valid(struct drm_crtc *crtc, > > > > > According to our pixel configuration above, > Now the variable req_rate value is 14850 or 5940, another variable > rate value is > 2700, so we will get a warning and display will cannot works well. > > We're not sure which resolution are user want, and we also can't just offered > one resolution > to user. so I remove this check on our board, maybe it's not good change. > > I want to know do you have other good suggestion? Thanks. > > Best Regards, > Wen > > > > if (req_rate) { > > > rate = clk_round_rate(hwdev->pxlclk, req_rate); > > > +#ifndef CONFIG_ARCH_LAYERSCAPE > > > if (rate != req_rate) { > > > DRM_DEBUG_DRIVER("pxlclk
Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate
On Fri, May 17, 2019 at 10:37:01AM +, Wen He wrote: > > > > -Original Message- > > From: Robin Murphy [mailto:[email protected]] > > Sent: 2019年5月16日 18:45 > > To: Wen He ; [email protected]; > > [email protected]; [email protected] > > Cc: Leo Li > > Subject: Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required > > pixel clock rate > > > > > > On 16/05/2019 10:42, Wen He wrote: > > > > > > > > >> -Original Message- > > >> From: Robin Murphy [mailto:[email protected]] > > >> Sent: 2019年5月16日 1:14 > > >> To: Wen He ; [email protected]; > > >> [email protected]; [email protected] > > >> Cc: Leo Li > > >> Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for > > >> required pixel clock rate > > >> > > >> Caution: EXT Email > > >> > > >> On 15/05/2019 03:42, Wen He wrote: > > >>> Disable checking for required pixel clock rate if ARCH_LAYERSCPAE is > > >>> enable. > > >>> > > >>> Signed-off-by: Alison Wang > > >>> Signed-off-by: Wen He > > >>> --- > > >>> change in description: > > >>>- This check that only supported one pixel clock required clock > > rate > > >>>compare with dts node value. but we have supports 4 pixel clock > > >>>for ls1028a board. > > >>>drivers/gpu/drm/arm/malidp_crtc.c | 2 ++ > > >>>1 file changed, 2 insertions(+) > > >>> > > >>> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c > > >>> b/drivers/gpu/drm/arm/malidp_crtc.c > > >>> index 56aad288666e..bb79223d9981 100644 > > >>> --- a/drivers/gpu/drm/arm/malidp_crtc.c > > >>> +++ b/drivers/gpu/drm/arm/malidp_crtc.c > > >>> @@ -36,11 +36,13 @@ static enum drm_mode_status > > >>> malidp_crtc_mode_valid(struct drm_crtc *crtc, > > >>> > > >>>if (req_rate) { > > >>>rate = clk_round_rate(hwdev->pxlclk, req_rate); > > >>> +#ifndef CONFIG_ARCH_LAYERSCAPE > > >> > > >> What about multiplatform builds? The kernel config doesn't tell you > > >> what hardware you're actually running on. > > >> > > > > > > Hi Robin, > > > > > > Thanks for your reply. > > > > > > In fact, Only one platform integrates this IP when > > CONFIG_ARCH_LAYERSCAPE is set. > > > Although this are not good ways, but I think it won't be a problem under > > multiplatform builds. > > > > My point is that ARCH_LAYERSCAPE is going to be enabled in distribution > > kernels along with everything else, so you're effectively removing this > > check for > > all other vendors' Mali-DP implementations as well, which is probably not > > OK. > > > > Furthermore, if LS1028A really only supports 4 specific modes as the BSP > > documentation I found claims, then surely you'd want a *more* specific check > > here, rather than no check at all? > > Hi Robin, > > Thanks for your comments. > > Yes, As you said, now LS1028A only supports 4 modes, and we use three clocks > to support > all four modes. In fact, this was really the point. > > However, we can only enable one mode to meet the check statement in this > place. > > For example, If user has a 1080p monitor, we must be set the pixel > fixed-clock to 148.5MHz. > if user want to choice 4k monitor, we also to be change the pixel fixed-clock > to 594MHz in > DT node. In reality, We have no way of knowing what kind of monitor the user > wants. Right? How does your DT know which monitor the user is going to plug in? Like I've said, if you expose the mechanism you use to set the fixed-clock to a certain value via the clk provider then you will be able to switch automatically to that frequency without your patch. Best regards, Liviu > > Moreover, user cannot to change screen resolution in this case, I don't think > this place is > reasonable. we need to supporting Ubuntu , Wayland and other embedded GU, so > we need > to switch the resolutions. > > Maybe it's that most android device used, and android system always only need > one > resolution. > > Best Regards, > Wen > > > > > Robin. -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯
Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate
On Fri, May 17, 2019 at 10:38:00AM +, Wen He wrote: > > > > -Original Message- > > From: [email protected] [mailto:[email protected]] > > Sent: 2019年5月16日 16:24 > > To: Wen He > > Cc: [email protected]; [email protected]; Leo Li > > > > Subject: Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required > > pixel clock rate > > > > Caution: EXT Email > > > > On Thu, May 16, 2019 at 08:10:21AM +0000, Wen He wrote: > > > > > > > > > > -Original Message- > > > > From: [email protected] [mailto:[email protected]] > > > > Sent: 2019年5月15日 23:46 > > > > To: Wen He > > > > Cc: [email protected]; [email protected]; > > > > Leo Li > > > > Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for > > > > required pixel clock rate > > > > > > > > > > > > Hi Wen, > > > > > > Hi Liviu, > > > > > Hi Liviu, > > > > > > > > > On Wed, May 15, 2019 at 02:42:08AM +, Wen He wrote: > > > > > Disable checking for required pixel clock rate if ARCH_LAYERSCPAE > > > > > is enable. > > > > > > > > > > Signed-off-by: Alison Wang > > > > > Signed-off-by: Wen He > > > > > --- > > > > > change in description: > > > > > - This check that only supported one pixel clock required clock > > rate > > > > > compare with dts node value. but we have supports 4 pixel clock > > > > > for ls1028a board. > > > > > > > > So, your DT says your pixel clock provider is a fixed clock? If you > > > > support more than one rate, you should instead use a real provider > > > > for it. How do you support the 4 pixel clocks? > > > > > > > > > > Yes , the DT node only can provided one pixel clock by using a fixed > > > clock. > > > But we Display Port controller support 4 or more resolutions, each of > > > which requires a set of pixel clocks to drive, and we hope they can > > > switch any resolution we want by some program every times. > > > > That program can't be some userspace application, because it will have to > > make changes to the hardware and the kernel will not know that things have > > changed under its feet. That leaves the option of the bootloader or some > > other > > kernel module doing the changes. > > > > If you have another kernel module that knows how to change clocks, that > > should be implemented using the common clocks infrastructure, at which time > > you can put it in the DT as the clock provider for the pixelclock. > > > > Hi Liviu, Hi Wen, > > Yes, I think you are right, and even though we didn't implement clocks prepare > /enable/disable interface, but we can notification hardware to change pixel > clock > by determining the required pixel clock in each mode once had modeset event > in DP driver. > > But the point is how do we meet the conditions for the clock rate check in > here? You don't need to change anything in this driver, your clock provider will only set the values it supports. So in malidp_crtc_atomic_enable() we set the desired pxlclk, but your provider will drop/refuse the change, so in malidp_crtc_mode_valid() only the 4 supported resolutions will pass, all other modes will fail. > > As you know, I don't, I still don't have a LS1028A platform ;) Best regards, Liviu > we LS1028A supports 4 modes, every resolution change will to > determine whether the hardware supports the pixel clock required for the > resolution > by calling this function malidp_crtc_mode_valid() . assume if we put > fixed-clock in DT > node that will can't meet this checking. > > Best Regards, > Wen > > > If the bootloader does the changes, then the bootloader should edit the DT > > and set the correct value for the pixel clock. Regardless, with your change > > and > > on your platform the user can request any resolution and the driver will > > silently fail to set that resolution. > > > > One other problem is the one Robin raised, where the kernel is compiled for > > multiple platforms, like what various Linux distributions do. That kernel > > will > > either work on other SoC or not, depending on what > > CONFIG_ARCH_LAYERSCAPE is set to. > > > > In summary, for this patch, it's a NAK. There are pr
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 +, 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 > --- > 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? I think it's worth thinking this in terms of bandwidth and not hardcode for one resolution. 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. > +#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_BASE0x00f00 > #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_VALID0x3 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 > +#endif > > /* register offsets and bits specific to DP550/DP650 */ > #define MALIDP550_ADDR_SPACE_SIZE0x1 > -- > 2.17.1 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 +, Wen He wrote: > > > > -Original Message- > > From: [email protected] [mailto:[email protected]] > > Sent: 2019年3月29日 0:39 > > To: Wen He > > Cc: [email protected]; [email protected]; > > [email protected] > > 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 +, 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 > > > --- > > > 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. > > > > > > +#endif > > > } > > > > > > int malidp_form
[PATCH 1/8] drm: mali-dp: Set the drm->irq_enabled flag to match driver's state.
On Tue, Oct 11, 2016 at 03:26:02PM +0100, Brian Starkey wrote:
> From: Liviu Dudau
>
> Mali DP driver does not use drm_irq_{un,}install() function so the
> drm->irq_enabled flag does not get set automatically.
> drm_wait_vblank() checks the value of the flag among other functions.
>
> Signed-off-by: Liviu Dudau
> ---
>
> Hi,
>
> This series is a bunch of small driver-internal fixes and cleanup for
> Mali-DP.
For the whole series, on the patches not already Signed-off-by me or acked:
Acked-by: Liviu Dudau
Many thanks,
Liviu
>
> -Brian
>
> drivers/gpu/drm/arm/malidp_drv.c |3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> b/drivers/gpu/drm/arm/malidp_drv.c
> index 9280358..7987ebd 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -377,6 +377,8 @@ static int malidp_bind(struct device *dev)
> if (ret < 0)
> goto irq_init_fail;
>
> + drm->irq_enabled = true;
> +
> ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> if (ret < 0) {
> DRM_ERROR("failed to initialise vblank\n");
> @@ -402,6 +404,7 @@ fbdev_fail:
> vblank_fail:
> malidp_se_irq_fini(drm);
> malidp_de_irq_fini(drm);
> + drm->irq_enabled = false;
> irq_init_fail:
> component_unbind_all(dev, drm);
> bind_fail:
> --
> 1.7.9.5
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
Re: [PATCH] drm/arcpgu: Get rid of "encoder-slave" property
On Wed, Mar 29, 2017 at 01:34:00PM +, Alexey Brodkin wrote: > Hi Liviu, Rob, Hi Alexey, > > On Fri, 2017-03-03 at 18:21 +, [email protected] wrote: > > On Fri, Mar 03, 2017 at 05:48:19PM +, Alexey Brodkin wrote: > > > > > > Hi Liviu, > > > > > > On Fri, 2017-03-03 at 16:28 +, Liviu Dudau wrote: > > > > > > > > On Fri, Mar 03, 2017 at 06:19:24PM +0300, Alexey Brodkin wrote: > > > > > > > > > > > > > > > - /* find the encoder node and initialize it */ > > > > > - encoder_node = of_parse_phandle(drm->dev->of_node, > > > > > "encoder-slave", 0); > > > > > - if (encoder_node) { > > > > > - ret = arcpgu_drm_hdmi_init(drm, encoder_node); > > > > > - of_node_put(encoder_node); > > > > > + /* There is only one output port inside each device, find it */ > > > > > + port = of_graph_get_next_endpoint(pdev->dev.of_node, NULL); > > > > > + > > > > > + if (port) { > > > > > + if (of_device_is_available(port)) > > > > > + encoder = of_graph_get_remote_port_parent(port); > > > > > + of_node_put(port); > > > > > + } > > > > > > > > You must've been looking at some old version. Current version in -next > > > > uses > > > > of_graph_get_remote_node() to replace all those lines you have added > > > > (see Rob > > > > Herring's series to introduce of_graph_get_remote_node() function) > > > > > > Hm, I'm not on Linus' master tree [1] and so I thought I was quite up to > > > date :) > > > Still I made a check of linux-next and don't see any changes in > > > "drivers/gpu/drm/arm" compared to Linus' tree. > > > > > > [1] > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__git.kernel.org_cgit_linux_kernel_git_torvalds_linux.git_commit_drivers_gpu_drm_arm-3Fid-3D > > > e4563f6ba71792c77aeccb2092cc23149b44e642&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=SI66ngnnXy33ncb8m5H4La2 > > > T1SzSEiiP7hc_XsRahEc&s=uaswjVXcjYDrUosOkO_UpTMqJMWTT-LLPrg5JE6-t-8&e= > > > [2] > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__git.kernel.org_cgit_linux_kernel_git_next_linux-2Dnext.git_commit_drivers_gpu_drm_arm-3Fid > > > -3De4563f6ba71792c77aeccb2092cc23149b44e642&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=SI66ngnnXy33ncb8m5H4 > > > La2T1SzSEiiP7hc_XsRahEc&s=hl9Y6s3K9LwLL1M2WnL3ODax_V-ZRh8k1iTiyctIqU4&e= > > > > > > Could you please clarify which exact tree did you mean? > > > > Sorry, I thought the series got pulled by one of the DRM trees, but it > > looks like > > I was wrong. I was carrying a private copy in my internal tree, waiting for > > the > > moment when it got pulled into drm-next or drm-misc-next. > > > > Rob, do you have an update on your series introducing > > of_graph_get_remote_node() ? > > For some reason I cannot find any relevant commits in linux-next tree even > today. > Could you please point me to either any random git tree with mentioned above > change or > maybe just mailing list where this patch was sent? Not sure why Rob hasn't added it to linux-next, but (according to Rob) this is the latest version: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=of-graph-helpers Best regards, Liviu > > I'd like to implement the same fix in ARCPGU and call it a day finally. > > -Alexey -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/arcpgu: Get rid of "encoder-slave" property
On Fri, Mar 03, 2017 at 05:48:19PM +, Alexey Brodkin wrote:
> Hi Liviu,
>
> On Fri, 2017-03-03 at 16:28 +, Liviu Dudau wrote:
> > On Fri, Mar 03, 2017 at 06:19:24PM +0300, Alexey Brodkin wrote:
> > >
> > > - /* find the encoder node and initialize it */
> > > - encoder_node = of_parse_phandle(drm->dev->of_node, "encoder-slave", 0);
> > > - if (encoder_node) {
> > > - ret = arcpgu_drm_hdmi_init(drm, encoder_node);
> > > - of_node_put(encoder_node);
> > > + /* There is only one output port inside each device, find it */
> > > + port = of_graph_get_next_endpoint(pdev->dev.of_node, NULL);
> > > +
> > > + if (port) {
> > > + if (of_device_is_available(port))
> > > + encoder = of_graph_get_remote_port_parent(port);
> > > + of_node_put(port);
> > > + }
> >
> > You must've been looking at some old version. Current version in -next uses
> > of_graph_get_remote_node() to replace all those lines you have added (see
> > Rob
> > Herring's series to introduce of_graph_get_remote_node() function)
>
> Hm, I'm not on Linus' master tree [1] and so I thought I was quite up to date
> :)
> Still I made a check of linux-next and don't see any changes in
> "drivers/gpu/drm/arm" compared to Linus' tree.
>
> [1]
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/arm?id=e4563f6ba71792c77aeccb2092cc23149b44e642
> [2]
> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/arm?id=e4563f6ba71792c77aeccb2092cc23149b44e642
>
> Could you please clarify which exact tree did you mean?
Sorry, I thought the series got pulled by one of the DRM trees, but it looks
like
I was wrong. I was carrying a private copy in my internal tree, waiting for the
moment when it got pulled into drm-next or drm-misc-next.
Rob, do you have an update on your series introducing
of_graph_get_remote_node() ?
Best regards,
Liviu
>
> Anyways I just tried to rebase my patch on top of linux-next tree and now
> video output is broken for me - I only see some garbage on top of the screen
> so I'll need to investigate it first before moving forward with stuff you
> proposed :)
>
> -Alexey
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
___
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: mali-dp: Don't set DRM_PLANE_COMMIT_ACTIVE_ONLY
On Mon, Oct 24, 2016 at 10:52:28AM +0100, Brian Starkey wrote: > We need to explicitly disable our planes, so don't set the flag which > would otherwise skip the plane disable when the CRTC is disabled. > > Signed-off-by: Brian Starkey Acked-by: Liviu Dudau > --- > drivers/gpu/drm/arm/malidp_drv.c |3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c > b/drivers/gpu/drm/arm/malidp_drv.c > index 9ed938e..f7bb763 100644 > --- a/drivers/gpu/drm/arm/malidp_drv.c > +++ b/drivers/gpu/drm/arm/malidp_drv.c > @@ -102,8 +102,7 @@ static void malidp_atomic_commit_tail(struct > drm_atomic_state *state) >*/ > malidp_mw_atomic_commit(drm, state); > > - drm_atomic_helper_commit_planes(drm, state, > - DRM_PLANE_COMMIT_ACTIVE_ONLY); > + drm_atomic_helper_commit_planes(drm, state, 0); > > malidp_atomic_commit_hw_done(state); > > -- > 1.7.9.5 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ã)_/¯
[PATCH] drm/arm: mark symbols static where possible
On Mon, Oct 24, 2016 at 12:18:37PM +0200, Arnd Bergmann wrote:
> On Saturday, October 22, 2016 5:13:01 PM CEST Baoyou Xie wrote:
> > We get 2 warnings when building kernel with W=1:
> > drivers/gpu/drm/arm/malidp_planes.c:49:25: warning: no previous prototype
> > for 'malidp_duplicate_plane_state' [-Wmissing-prototypes]
> > drivers/gpu/drm/arm/malidp_planes.c:66:6: warning: no previous prototype
> > for 'malidp_destroy_plane_state' [-Wmissing-prototypes]
> >
> > In fact, both functions are only used in the file in which they are
> > declared and don't need a declaration, but can be made static.
> > So this patch marks these functions with 'static'.
> >
> > Signed-off-by: Baoyou Xie
> > ---
> ...
> > @@ -63,7 +64,7 @@ struct drm_plane_state
> > *malidp_duplicate_plane_state(struct drm_plane *plane)
> > return &state->base;
> > }
> >
> > -void malidp_destroy_plane_state(struct drm_plane *plane,
> > +static void malidp_destroy_plane_state(struct drm_plane *plane,
> > struct drm_plane_state *state)
> > {
> > struct malidp_plane_state *m_state = to_malidp_plane_state(state);
> >
>
> The change looks correct, but I notice that the two lines of the
> declaration are no longer aligned.
>
> The file follows the normal style of aligning the argument list
> in the second line to line up with the opening '(', so please keep
> it that way.
>
> Arnd
>
Fixed it locally and pushed it into my public repo @
git://linux-arm.org/linux-ld.git for-upstream/mali-dp
I will send it to David Airlie as a pull request together with the other
patches/cleanups.
Many thanks,
Liviu
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH] drm/arm: mark symbols static where possible
On Tue, Sep 06, 2016 at 03:23:28PM +0800, Baoyou Xie wrote:
> We get 2 warnings when building kernel with W=1:
> drivers/gpu/drm/arm/malidp_planes.c:49:25: warning: no previous prototype for
> 'malidp_duplicate_plane_state' [-Wmissing-prototypes]
> drivers/gpu/drm/arm/malidp_planes.c:66:6: warning: no previous prototype for
> 'malidp_destroy_plane_state' [-Wmissing-prototypes]
>
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> So this patch marks these functions with 'static'.
>
> Signed-off-by: Baoyou Xie
Acked-by: Liviu Dudau
> ---
> drivers/gpu/drm/arm/malidp_planes.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c
> b/drivers/gpu/drm/arm/malidp_planes.c
> index 725098d..5f8bece 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -46,7 +46,8 @@ static void malidp_de_plane_destroy(struct drm_plane *plane)
> devm_kfree(plane->dev->dev, mp);
> }
>
> -struct drm_plane_state *malidp_duplicate_plane_state(struct drm_plane *plane)
> +static struct
> +drm_plane_state *malidp_duplicate_plane_state(struct drm_plane *plane)
> {
> struct malidp_plane_state *state, *m_state;
>
> @@ -63,7 +64,7 @@ struct drm_plane_state *malidp_duplicate_plane_state(struct
> drm_plane *plane)
> return &state->base;
> }
>
> -void malidp_destroy_plane_state(struct drm_plane *plane,
> +static void malidp_destroy_plane_state(struct drm_plane *plane,
> struct drm_plane_state *state)
> {
> struct malidp_plane_state *m_state = to_malidp_plane_state(state);
> --
> 2.7.4
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH] Revert "drm/i2c: tda998x: don't register the connector"
On Fri, Sep 23, 2016 at 12:18:12AM -0700, Sean Paul wrote:
> This reverts commit 6a2925ea12006911c8180a89feda6d040873ed18.
>
> commit 6a2925ea12006911c8180a89feda6d040873ed18
> Author: Brian Starkey
> Date: Mon Jul 25 11:55:48 2016 +0100
>
> drm/i2c: tda998x: don't register the connector
>
> [seanpaul]
> Patch isn't fully baked, and apparently causing issues in hdlcd. Revert
> until this is sorted.
I would argue that the comment is not correct, patch is fine, it is
just the dependent code is not ready to work with the patch.
Otherwise, thanks for doing this and sorry for the noise.
Best regards,
Liviu
>
> Signed-off-by: Sean Paul
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 088900d..9798d40 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1584,6 +1584,7 @@ const struct drm_connector_helper_funcs
> tda998x_connector_helper_funcs = {
>
> static void tda998x_connector_destroy(struct drm_connector *connector)
> {
> + drm_connector_unregister(connector);
> drm_connector_cleanup(connector);
> }
>
> @@ -1655,10 +1656,16 @@ static int tda998x_bind(struct device *dev, struct
> device *master, void *data)
> if (ret)
> goto err_connector;
>
> + ret = drm_connector_register(&priv->connector);
> + if (ret)
> + goto err_sysfs;
> +
> drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
>
> return 0;
>
> +err_sysfs:
> + drm_connector_cleanup(&priv->connector);
> err_connector:
> drm_encoder_cleanup(&priv->encoder);
> err_encoder:
> @@ -1671,6 +1678,7 @@ static void tda998x_unbind(struct device *dev, struct
> device *master,
> {
> struct tda998x_priv *priv = dev_get_drvdata(dev);
>
> + drm_connector_unregister(&priv->connector);
> drm_connector_cleanup(&priv->connector);
> drm_encoder_cleanup(&priv->encoder);
> tda998x_destroy(priv);
> --
> 2.8.0.rc3.226.g39d4020
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH] drm: arm: mark symbols static where possible
Hi Baoyou,
On Sun, Sep 25, 2016 at 03:20:24PM +0800, Baoyou Xie wrote:
> We get 2 warnings when building kernel with W=1:
> drivers/gpu/drm/arm/malidp_planes.c:49:25: warning: no previous prototype for
> 'malidp_duplicate_plane_state' [-Wmissing-prototypes]
> drivers/gpu/drm/arm/malidp_planes.c:66:6: warning: no previous prototype for
> 'malidp_destroy_plane_state' [-Wmissing-prototypes]
>
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> So this patch marks these functions with 'static'.
>
> Signed-off-by: Baoyou Xie
I've already acked this patch:
https://lists.freedesktop.org/archives/dri-devel/2016-September/117624.html
What is going on between you and Ville Syrjälä? Both are posting the same
patch.
Best regards,
Liviu
> ---
> drivers/gpu/drm/arm/malidp_planes.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c
> b/drivers/gpu/drm/arm/malidp_planes.c
> index 725098d..5f8bece 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -46,7 +46,8 @@ static void malidp_de_plane_destroy(struct drm_plane *plane)
> devm_kfree(plane->dev->dev, mp);
> }
>
> -struct drm_plane_state *malidp_duplicate_plane_state(struct drm_plane *plane)
> +static struct
> +drm_plane_state *malidp_duplicate_plane_state(struct drm_plane *plane)
> {
> struct malidp_plane_state *state, *m_state;
>
> @@ -63,7 +64,7 @@ struct drm_plane_state *malidp_duplicate_plane_state(struct
> drm_plane *plane)
> return &state->base;
> }
>
> -void malidp_destroy_plane_state(struct drm_plane *plane,
> +static void malidp_destroy_plane_state(struct drm_plane *plane,
> struct drm_plane_state *state)
> {
> struct malidp_plane_state *m_state = to_malidp_plane_state(state);
> --
> 2.7.4
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs
On Thu, May 05, 2016 at 05:13:38PM +0100, Robin Murphy wrote:
> The current PM ops simply unconditionally enable/disable the HDLCD,
> which proves problematic when there is no display plugged in - since
> without a crtc the hardware itself is still in an uninitialised state,
> coming out of suspend results in it being enabled without a valid
> framebuffer address, which typically results in it trying to scan out
> from bus address 0 and flooding the system with error interrupts.
>
> Fix this by checking the crtc state on resume, and only enabling the
> hardware if it's actually supposed to be. For the sake of consistency,
> do the same on the suspend path as well, although there it's merely a
> case of skipping unnecessary work.
>
> CC: Liviu Dudau
Acked-by: Liviu Dudau
Thanks for the patch, Robin!
Liviu
> Signed-off-by: Robin Murphy
> ---
> drivers/gpu/drm/arm/hdlcd_crtc.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c
> b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index fef1b04c2aab..bf6ff5e48adc 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -296,12 +296,14 @@ static struct drm_plane *hdlcd_plane_init(struct
> drm_device *drm)
>
> void hdlcd_crtc_suspend(struct drm_crtc *crtc)
> {
> - hdlcd_crtc_disable(crtc);
> + if (crtc->state->active)
> + hdlcd_crtc_disable(crtc);
> }
>
> void hdlcd_crtc_resume(struct drm_crtc *crtc)
> {
> - hdlcd_crtc_enable(crtc);
> + if (crtc->state->active)
> + hdlcd_crtc_enable(crtc);
> }
>
> int hdlcd_setup_crtc(struct drm_device *drm)
> --
> 2.8.1.dirty
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH 2/2] drm: hdlcd: Suspend/resume only active crtcs
On Thu, May 05, 2016 at 07:06:02PM +0200, Daniel Vetter wrote:
> On Thu, May 05, 2016 at 05:13:38PM +0100, Robin Murphy wrote:
> > The current PM ops simply unconditionally enable/disable the HDLCD,
> > which proves problematic when there is no display plugged in - since
> > without a crtc the hardware itself is still in an uninitialised state,
> > coming out of suspend results in it being enabled without a valid
> > framebuffer address, which typically results in it trying to scan out
> > from bus address 0 and flooding the system with error interrupts.
> >
> > Fix this by checking the crtc state on resume, and only enabling the
> > hardware if it's actually supposed to be. For the sake of consistency,
> > do the same on the suspend path as well, although there it's merely a
> > case of skipping unnecessary work.
> >
> > CC: Liviu Dudau
> > Signed-off-by: Robin Murphy
> > ---
> > drivers/gpu/drm/arm/hdlcd_crtc.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > index fef1b04c2aab..bf6ff5e48adc 100644
> > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > @@ -296,12 +296,14 @@ static struct drm_plane *hdlcd_plane_init(struct
> > drm_device *drm)
> >
> > void hdlcd_crtc_suspend(struct drm_crtc *crtc)
> > {
> > - hdlcd_crtc_disable(crtc);
> > + if (crtc->state->active)
> > + hdlcd_crtc_disable(crtc);
> > }
> >
> > void hdlcd_crtc_resume(struct drm_crtc *crtc)
> > {
> > - hdlcd_crtc_enable(crtc);
> > + if (crtc->state->active)
> > + hdlcd_crtc_enable(crtc);
> > }
>
> If you use the atomic helpers to suspend/resume your entire display
> pipeline these callbacks shouldn't even be needed at all. Tried just
> removing them?
Yes, I need to cleanup the PM code in HDLCD.
Thanks,
Liviu
> -Daniel
>
> >
> > int hdlcd_setup_crtc(struct drm_device *drm)
> > --
> > 2.8.1.dirty
> >
> > ___
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH 1/2] drm: hdlcd: Skip PM callbacks if unbound
On Fri, May 06, 2016 at 04:54:17PM +0200, Thierry Reding wrote: > On Thu, May 05, 2016 at 05:13:37PM +0100, Robin Murphy wrote: > > Until an encoder component is available to trigger the HDLCD's component > > master bind callback, the drm_device we keep in drvdata isn't allocated. > > Since it is quite possible to, say, hibernate the system without having > > loaded the encoder driver, the PM callbacks need to check for a valid > > drm_device before trying to take the lock and suspend/resume the crtcs, > > otherwise this happens: > > > > [ 51.779092] Unable to handle kernel NULL pointer dereference at virtual > > address 0200 > > [ 51.787143] pgd = 800974462000 > > [ 51.790514] [0200] *pgd= > > [ 51.794768] Internal error: Oops: 9604 [#1] PREEMPT SMP > > [ 51.800291] Modules linked in: > > [ 51.803325] CPU: 4 PID: 2098 Comm: systemd-sleep Not tainted 4.6.0-rc6+ > > #4 > > [ 51.810140] Hardware name: ARM Juno development board (r1) (DT) > > [ 51.816008] task: 80097443 ti: 80097380c000 task.ti: > > 80097380c000 > > [ 51.823433] PC is at mutex_lock+0x14/0x60 > > [ 51.827411] LR is at drm_modeset_lock_all+0x3c/0xe0 > > [ 51.832246] pc : [] lr : [] pstate: > > 4145 > > [ 51.839577] sp : 80097380faf0 > > [ 51.842859] x29: 80097380faf0 x28: > > [ 51.848132] x27: 0002 x26: 08e01000 > > [ 51.853405] x25: 084cc000 x24: 08dcb408 > > [ 51.858678] x23: 08e7c000 x22: 8009760e8870 > > [ 51.863962] x21: 0200 x20: > > [ 51.869244] x19: 0200 x18: 800079357dd0 > > [ 51.874527] x17: 8a00ab88 x16: 0018 > > [ 51.879809] x15: 0018 x14: > > [ 51.885091] x13: 0002 x12: > > [ 51.890374] x11: 087ff904 x10: 0840 > > [ 51.895656] x9 : x8 : 800973821680 > > [ 51.900938] x7 : x6 : 003f > > [ 51.906220] x5 : 0040 x4 : > > [ 51.911502] x3 : 0004 x2 : > > [ 51.916784] x1 : 0001 x0 : 0200 > > ... > > [ 52.339406] [] mutex_lock+0x14/0x60 > > [ 52.344425] [] drm_modeset_lock_all+0x3c/0xe0 > > [ 52.350305] [] hdlcd_pm_suspend+0x2c/0x90 > > [ 52.355842] [] platform_pm_suspend+0x24/0x58 > > [ 52.361638] [] dpm_run_callback.isra.4+0x20/0x68 > > [ 52.367778] [] __device_suspend+0xe4/0x238 > > [ 52.373400] [] dpm_suspend+0x10c/0x240 > > [ 52.378679] [] dpm_suspend_start+0x70/0x80 > > [ 52.384302] [] suspend_devices_and_enter+0x9c/0x480 > > [ 52.390699] [] pm_suspend+0x1d0/0x240 > > [ 52.395890] [] state_store+0x80/0x100 > > [ 52.401084] [] kobj_attr_store+0x14/0x28 > > [ 52.406535] [] sysfs_kf_write+0x48/0x58 > > [ 52.411899] [] kernfs_fop_write+0xbc/0x188 > > [ 52.417521] [] __vfs_write+0x1c/0xe0 > > [ 52.422626] [] vfs_write+0x8c/0x1a8 > > [ 52.427644] [] SyS_write+0x44/0xa0 > > [ 52.432578] [] __sys_trace_return+0x0/0x4 > > [ 52.438114] Code: 910003fd f9000bf3 aa0003f3 f9800011 (885ffc01) > > [ 52.444229] ---[ end trace 34b87a5fb9453f65 ]--- > > > > Acked-by: Liviu Dudau > > Signed-off-by: Robin Murphy > > --- > > drivers/gpu/drm/arm/hdlcd_drv.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c > > b/drivers/gpu/drm/arm/hdlcd_drv.c > > index 734899c4e4bb..86a5eea4cf53 100644 > > --- a/drivers/gpu/drm/arm/hdlcd_drv.c > > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c > > @@ -498,7 +498,7 @@ static int __maybe_unused hdlcd_pm_suspend(struct > > device *dev) > > struct drm_device *drm = dev_get_drvdata(dev); > > struct drm_crtc *crtc; > > > > - if (pm_runtime_suspended(dev)) > > + if (pm_runtime_suspended(dev) || !drm) > > return 0; > > > > drm_modeset_lock_all(drm); > > @@ -513,7 +513,7 @@ static int __maybe_unused hdlcd_pm_resume(struct device > > *dev) > > struct drm_device *drm = dev_get_drvdata(dev); > > struct drm_crtc *crtc; > > > > - if (!pm_runtime_suspended(dev)) > > + if (!pm_runtime_suspended(dev) || !drm) > > return 0; > > Perhaps rather than this workaround you should only enable runtime PM on > this after the master has been bound? Yes, that is probably a better solution, even if the patch is going to be much bigger. Best regards, Liviu > > Thierry -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ã)_/¯
[PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper
On Thu, Feb 25, 2016 at 12:09:39PM +0200, Jyri Sarha wrote:
> Hi,
> Based on discussion around this patch:
>
> https://lists.freedesktop.org/archives/dri-devel/2016-February/100685.html
>
> I think the patch below should be applied to tda988x development branch.
> Would you take it or do you prefer some other approach?
You should send it to David Airlie after you get an Ack from Russell.
Best regards,
Liviu
>
> Best regards,
> Jyri
>
> On 01/16/16 22:17, Jyri Sarha wrote:
> >Choose between atomic or non atomic connector dpms helper. If tda998x
> >is connected to a drm driver that does not support atomic modeset
> >calling drm_atomic_helper_connector_dpms() causes a crash when the
> >connectors atomic state is not initialized. The patch implements a
> >driver specific connector dpms helper that calls
> >drm_atomic_helper_connector_dpms() if driver supports DRIVER_ATOMIC
> >and otherwise it calls the legacy drm_helper_connector_dpms().
> >
> >Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
> >modesetting").
> >
> >Signed-off-by: Jyri Sarha
> >---
> >
> >Ok, so this is the second approach to solve this issue. The first
> >attempt can be found here [1] with the follow from Liviu Dudau that
> >suggested this approach.
> >
> >It just makes me wonder if drm_atomic_helper_connector_dpms() should
> >call the legacy callback automatically if DRIVER_ATOMIC is not set or
> >at least bail out gracefully with an error message. Then again it may
> >be overkill if the tda998x is the only driver that need to support
> >both situations.
> >
> >Best regards,
> >Jyri
> >
> >[1] http://www.spinics.net/lists/dri-devel/msg98514.html
> >
> > drivers/gpu/drm/i2c/tda998x_drv.c | 10 +-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> >b/drivers/gpu/drm/i2c/tda998x_drv.c
> >index 012d36d..bb7d507 100644
> >--- a/drivers/gpu/drm/i2c/tda998x_drv.c
> >+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> >@@ -1382,8 +1382,16 @@ static void tda998x_connector_destroy(struct
> >drm_connector *connector)
> > drm_connector_cleanup(connector);
> > }
> >
> >+static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
> >+{
> >+if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
> >+return drm_atomic_helper_connector_dpms(connector, mode);
> >+else
> >+return drm_helper_connector_dpms(connector, mode);
> >+}
> >+
> > static const struct drm_connector_funcs tda998x_connector_funcs = {
> >-.dpms = drm_atomic_helper_connector_dpms,
> >+.dpms = tda998x_connector_dpms,
> > .reset = drm_atomic_helper_connector_reset,
> > .fill_modes = drm_helper_probe_single_connector_modes,
> > .detect = tda998x_connector_detect,
> >
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH RFC] drm/i2c: tda998x: Only set atomic funcs if bound to atomic drm driver
On Fri, Jan 15, 2016 at 02:55:26PM +0200, Jyri Sarha wrote:
> Only set atomic connector callbacks if the tda998x driver is bound to
> a drm driver that supports atomic modesetting. At least
> drm_crtc_helper_set_config() calls connectors dpms callback without
> knowing that it may assume atomic modeset support. Calling
> drm_atomic_helper_connector_dpms() causes a crash with a driver that
> does not have atomic state initialized.
Hi Jyri,
>
> Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
> modesetting").
First, sorry for the breakage, I did not had any other board to test the
change on.
>
> Signed-off-by: Jyri Sarha
> ---
> I am not sure if this is the best way to fix the issue, but after
> 9736e988d328 Beaglebone-Black HDMI is totally broken and this fixes
> the issue. I am currently working on atomic modesetting for tilcdc,
> but there is no way it makes it to 4.5 release.
Yes, I'm not a big fan of the change either. I would've thought that
you only need to update the .dpms pointer, all other hooks added are
specific to atomic operations (right?). Would it not be simpler to have
a tda998x_connector_dpms() function that calls the appropriate .dpms
function based on the feature check?
Best regards,
Liviu
>
> Best regards,
> Jyri
>
> drivers/gpu/drm/i2c/tda998x_drv.c | 17 +++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 012d36d..67d1408 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1382,7 +1382,7 @@ static void tda998x_connector_destroy(struct
> drm_connector *connector)
> drm_connector_cleanup(connector);
> }
>
> -static const struct drm_connector_funcs tda998x_connector_funcs = {
> +static const struct drm_connector_funcs tda998x_connector_atomic_funcs = {
> .dpms = drm_atomic_helper_connector_dpms,
> .reset = drm_atomic_helper_connector_reset,
> .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -1392,15 +1392,28 @@ static const struct drm_connector_funcs
> tda998x_connector_funcs = {
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> +static const struct drm_connector_funcs tda998x_connector_funcs = {
> + .dpms = drm_helper_connector_dpms,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .detect = tda998x_connector_detect,
> + .destroy = tda998x_connector_destroy,
> +};
> +
> static int tda998x_bind(struct device *dev, struct device *master, void
> *data)
> {
> struct tda998x_encoder_params *params = dev->platform_data;
> struct i2c_client *client = to_i2c_client(dev);
> struct drm_device *drm = data;
> struct tda998x_priv *priv;
> + const struct drm_connector_funcs *connector_funcs;
> u32 crtcs = 0;
> int ret;
>
> + if (drm_core_check_feature(drm, DRIVER_ATOMIC))
> + connector_funcs = &tda998x_connector_atomic_funcs;
> + else
> + connector_funcs = &tda998x_connector_funcs;
> +
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
> @@ -1437,7 +1450,7 @@ static int tda998x_bind(struct device *dev, struct
> device *master, void *data)
> drm_connector_helper_add(&priv->connector,
>&tda998x_connector_helper_funcs);
> ret = drm_connector_init(drm, &priv->connector,
> - &tda998x_connector_funcs,
> + connector_funcs,
>DRM_MODE_CONNECTOR_HDMIA);
> if (ret)
> goto err_connector;
> --
> 1.9.1
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH] Revert "drm/rockchip: Convert the probe function to the generic drm_of_component_probe()"
On Tue, Nov 10, 2015 at 04:47:19PM +0800, Mark Yao wrote:
> This reverts commit 52f5eb60940de889ce98a876f6933b574ead3225.
>
> Rockchip drm can't work with generic drm_of_component_probe now
>
> Signed-off-by: Mark Yao
Acked-by: Liviu Dudau
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81
> +--
> 1 file changed, 75 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index d26e0cc..f22e1e1 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -19,7 +19,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -419,6 +418,29 @@ static int compare_of(struct device *dev, void *data)
> return dev->of_node == np;
> }
>
> +static void rockchip_add_endpoints(struct device *dev,
> +struct component_match **match,
> +struct device_node *port)
> +{
> + struct device_node *ep, *remote;
> +
> + for_each_child_of_node(port, ep) {
> + remote = of_graph_get_remote_port_parent(ep);
> + if (!remote || !of_device_is_available(remote)) {
> + of_node_put(remote);
> + continue;
> + } else if (!of_device_is_available(remote->parent)) {
> + dev_warn(dev, "parent device of %s is not available\n",
> + remote->full_name);
> + of_node_put(remote);
> + continue;
> + }
> +
> + component_match_add(dev, match, compare_of, remote);
> + of_node_put(remote);
> + }
> +}
> +
> static int rockchip_drm_bind(struct device *dev)
> {
> struct drm_device *drm;
> @@ -461,14 +483,61 @@ static const struct component_master_ops
> rockchip_drm_ops = {
>
> static int rockchip_drm_platform_probe(struct platform_device *pdev)
> {
> - int ret = drm_of_component_probe(&pdev->dev, compare_of,
> - &rockchip_drm_ops);
> + struct device *dev = &pdev->dev;
> + struct component_match *match = NULL;
> + struct device_node *np = dev->of_node;
> + struct device_node *port;
> + int i;
>
> - /* keep compatibility with old code that was returning -ENODEV */
> - if (ret == -EINVAL)
> + if (!np)
> return -ENODEV;
> + /*
> + * Bind the crtc ports first, so that
> + * drm_of_find_possible_crtcs called from encoder .bind callbacks
> + * works as expected.
> + */
> + for (i = 0;; i++) {
> + port = of_parse_phandle(np, "ports", i);
> + if (!port)
> + break;
> +
> + if (!of_device_is_available(port->parent)) {
> + of_node_put(port);
> + continue;
> + }
>
> - return ret;
> + component_match_add(dev, &match, compare_of, port->parent);
> + of_node_put(port);
> + }
> +
> + if (i == 0) {
> + dev_err(dev, "missing 'ports' property\n");
> + return -ENODEV;
> + }
> +
> + if (!match) {
> + dev_err(dev, "No available vop found for display-subsystem.\n");
> + return -ENODEV;
> + }
> + /*
> + * For each bound crtc, bind the encoders attached to its
> + * remote endpoint.
> + */
> + for (i = 0;; i++) {
> + port = of_parse_phandle(np, "ports", i);
> + if (!port)
> + break;
> +
> + if (!of_device_is_available(port->parent)) {
> + of_node_put(port);
> + continue;
> + }
> +
> + rockchip_add_endpoints(dev, &match, port);
> + of_node_put(port);
> + }
> +
> + return component_master_add_with_match(dev, &rockchip_drm_ops, match);
> }
>
> static int rockchip_drm_platform_remove(struct platform_device *pdev)
> --
> 1.7.9.5
>
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
HDLCD crashes with 6d910bfa809e
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
> Hi Daniel, Liviu,
Hi Robin,
>
> Having just inadvertently merged -next into my working branch, I find
> dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely
> affecting my board's ability to boot ;)
>
> Since I (intentionally) don't have sufficient CMA to create a framebuffer,
> drm_gem_cma_create() fails, unconditionally calls the now-NULL
> drm->driver->gem_free_object() in its cleanup path, and fiery death
> ensues...
Thanks for reporting this. What other changes other than reducing the CMA
allocation size do you have that I might need in order to reproduce this?
Best regards,
Liviu
>
> Regards,
> Robin.
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
HDLCD crashes with 6d910bfa809e
On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
> Hi Liviu,
>
> On 07/06/16 14:35, liviu.dudau at arm.com wrote:
> >On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
> >>Having just inadvertently merged -next into my working branch, I find
> >>dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely
> >>affecting my board's ability to boot ;)
> >>
> >>Since I (intentionally) don't have sufficient CMA to create a framebuffer,
> >>drm_gem_cma_create() fails, unconditionally calls the now-NULL
> >>drm->driver->gem_free_object() in its cleanup path, and fiery death
> >>ensues...
> >
> >Thanks for reporting this. What other changes other than reducing the CMA
> >allocation size do you have that I might need in order to reproduce this?
>
> I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
> + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
> job:
>
> [3.032402] hdlcd 7ff6.hdlcd: bound 0-0070 (ops tda998x_ops)
> [3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [3.044970] [drm] No driver support for vblank timestamp query.
> [3.076973] hdlcd 7ff6.hdlcd: failed to allocate buffer with size
> 768
> [3.084081] Bad mode in Synchronous Abort handler detected, code
> 0x8604 -- IABT (current EL)
> [3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted
> 4.7.0-rc1-next-20160602 #686
> [3.100682] Hardware name: ARM Juno development board (r1) (DT)
> [3.106567] Workqueue: deferwq deferred_probe_work_func
> [3.111761] task: 8009768a3e80 ti: 8009768e8000 task.ti:
> 8009768e8000
> [3.119198] PC is at 0x0
> [3.121720] LR is at drm_gem_cma_create+0x128/0x130
> ...and so on.
>
> Today's -next, on the other hand, dodges the bullet entirely:
>
> [2.903645] [drm] found ARM HDLCD version r0p0
> [2.908122] hdlcd 7ff6.hdlcd: master bind failed: -22
> [2.913505] tda998x: probe of 0-0070 failed with error -22
> [2.919141] [drm] found ARM HDLCD version r0p0
> [2.923609] hdlcd 7ff5.hdlcd: master bind failed: -22
> [2.928991] tda998x: probe of 0-0071 failed with error -22
Yes, if fails in of_reserved_mem_device_init(). Tracking now to see
which of the conditions at line 311 are the culprits.
Best regards,
Liviu
>
> Oh well...
>
> Robin.
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
HDLCD crashes with 6d910bfa809e
On Tue, Jun 07, 2016 at 03:14:04PM +0100, liviu.dudau at arm.com wrote:
> On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
> > Hi Liviu,
> >
> > On 07/06/16 14:35, liviu.dudau at arm.com wrote:
> > >On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
> > >>Having just inadvertently merged -next into my working branch, I find
> > >>dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely
> > >>affecting my board's ability to boot ;)
> > >>
> > >>Since I (intentionally) don't have sufficient CMA to create a framebuffer,
> > >>drm_gem_cma_create() fails, unconditionally calls the now-NULL
> > >>drm->driver->gem_free_object() in its cleanup path, and fiery death
> > >>ensues...
> > >
> > >Thanks for reporting this. What other changes other than reducing the CMA
> > >allocation size do you have that I might need in order to reproduce this?
> >
> > I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
> > + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
> > job:
> >
> > [3.032402] hdlcd 7ff6.hdlcd: bound 0-0070 (ops tda998x_ops)
> > [3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> > [3.044970] [drm] No driver support for vblank timestamp query.
> > [3.076973] hdlcd 7ff6.hdlcd: failed to allocate buffer with size
> > 768
> > [3.084081] Bad mode in Synchronous Abort handler detected, code
> > 0x8604 -- IABT (current EL)
> > [3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted
> > 4.7.0-rc1-next-20160602 #686
> > [3.100682] Hardware name: ARM Juno development board (r1) (DT)
> > [3.106567] Workqueue: deferwq deferred_probe_work_func
> > [3.111761] task: 8009768a3e80 ti: 8009768e8000 task.ti:
> > 8009768e8000
> > [3.119198] PC is at 0x0
> > [3.121720] LR is at drm_gem_cma_create+0x128/0x130
> > ...and so on.
> >
> > Today's -next, on the other hand, dodges the bullet entirely:
> >
> > [2.903645] [drm] found ARM HDLCD version r0p0
> > [2.908122] hdlcd 7ff6.hdlcd: master bind failed: -22
> > [2.913505] tda998x: probe of 0-0070 failed with error -22
> > [2.919141] [drm] found ARM HDLCD version r0p0
> > [2.923609] hdlcd 7ff5.hdlcd: master bind failed: -22
> > [2.928991] tda998x: probe of 0-0071 failed with error -22
>
> Yes, if fails in of_reserved_mem_device_init(). Tracking now to see
> which of the conditions at line 311 are the culprits.
Bah, discard the line number, old cached version in my editor.
Liviu
>
> Best regards,
> Liviu
>
> >
> > Oh well...
> >
> > Robin.
> >
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
HDLCD crashes with 6d910bfa809e
On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
> Hi Liviu,
>
> On 07/06/16 14:35, liviu.dudau at arm.com wrote:
> >On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
> >>Having just inadvertently merged -next into my working branch, I find
> >>dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely
> >>affecting my board's ability to boot ;)
> >>
> >>Since I (intentionally) don't have sufficient CMA to create a framebuffer,
> >>drm_gem_cma_create() fails, unconditionally calls the now-NULL
> >>drm->driver->gem_free_object() in its cleanup path, and fiery death
> >>ensues...
> >
> >Thanks for reporting this. What other changes other than reducing the CMA
> >allocation size do you have that I might need in order to reproduce this?
>
> I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
> + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
> job:
>
> [3.032402] hdlcd 7ff6.hdlcd: bound 0-0070 (ops tda998x_ops)
> [3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [3.044970] [drm] No driver support for vblank timestamp query.
> [3.076973] hdlcd 7ff6.hdlcd: failed to allocate buffer with size
> 768
> [3.084081] Bad mode in Synchronous Abort handler detected, code
> 0x8604 -- IABT (current EL)
> [3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted
> 4.7.0-rc1-next-20160602 #686
> [3.100682] Hardware name: ARM Juno development board (r1) (DT)
> [3.106567] Workqueue: deferwq deferred_probe_work_func
> [3.111761] task: 8009768a3e80 ti: 8009768e8000 task.ti:
> 8009768e8000
> [3.119198] PC is at 0x0
> [3.121720] LR is at drm_gem_cma_create+0x128/0x130
> ...and so on.
>
> Today's -next, on the other hand, dodges the bullet entirely:
>
> [2.903645] [drm] found ARM HDLCD version r0p0
> [2.908122] hdlcd 7ff6.hdlcd: master bind failed: -22
> [2.913505] tda998x: probe of 0-0070 failed with error -22
> [2.919141] [drm] found ARM HDLCD version r0p0
> [2.923609] hdlcd 7ff5.hdlcd: master bind failed: -22
> [2.928991] tda998x: probe of 0-0071 failed with error -22
OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from
when
there is no "memory-region" phandle in the DT: before it used to return
-ENODEV, now
it returns -EINVAL.
Marek, I quite liked the old behaviour to detect if the DT had the optional
(from
my driver's point of view) "memory-region" phandle. Plus the check for dev is
superfluous
when using of_reserved_mem_device_init() as that uses dev->of_node for np so it
would
crash before the check anyway. Maybe move the check there?
Until then I suggest reverting the 59ce4039727ef40 commit.
Best regards,
Liviu
>
> Oh well...
>
> Robin.
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH] drm/fb_cma_helper: Implement fb_mmap callback
On Tue, Jun 07, 2016 at 04:43:05PM +0200, Daniel Vetter wrote:
> On Tue, Jun 07, 2016 at 01:18:09PM +0100, Robin Murphy wrote:
> > In the absence of an fb_mmap callback, the fbdev code falls back to a
> > naive implementation which relies upon the DMA address being the same
> > as the physical address, and the buffer being physically contiguous
> > from there. Whilst this often holds for standard CMA allocations via
> > the platform's regular DMA ops, if the allocation is provided by an
> > IOMMU then such assumptions can fall apart spectacularly.
> >
> > To resolve this, reroute the fb_mmap call to the appropriate DMA API
> > implementation, as per the other cma_helper calls.
> >
> > Acked-by: Daniel Vetter
> > Signed-off-by: Robin Murphy
> > ---
> >
> > Resending rebased to 4.7-rc1 with Daniel's ack. I know Russell raised
> > some concerns about the general way fb_cma_helper uses fb_info[1], but
> > AFAICS that's a longstanding separate problem orthogonal to this patch.
>
> Do you want me to pull this in through drm-misc, or will this land through
> some driver tree? Note that I'll do -misc pulls about every week, so if
> you don't send your pull this week I'd like to pull it in to avoid
> hilarity^W needless conflicts.
> -Daniel
Hi Daniel,
This patch is for drm core, so no driver can carry it (?).
Please pull it into your drm-misc.
Best regards,
Liviu
>
> >
> > Robin.
> >
> > [1]:http://thread.gmane.org/gmane.comp.video.dri.devel/149288
> >
> > drivers/gpu/drm/drm_fb_cma_helper.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c
> > index 172cafe11c71..a25afc068d3f 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -23,6 +23,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #define DEFAULT_FBDEFIO_DELAY_MS 50
> > @@ -297,6 +298,12 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void
> > *arg)
> > EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show);
> > #endif
> >
> > +static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct
> > *vma)
> > +{
> > + return dma_mmap_writecombine(info->device, vma, info->screen_base,
> > +info->fix.smem_start, info->fix.smem_len);
> > +}
> > +
> > static struct fb_ops drm_fbdev_cma_ops = {
> > .owner = THIS_MODULE,
> > .fb_fillrect= drm_fb_helper_sys_fillrect,
> > @@ -307,6 +314,7 @@ static struct fb_ops drm_fbdev_cma_ops = {
> > .fb_blank = drm_fb_helper_blank,
> > .fb_pan_display = drm_fb_helper_pan_display,
> > .fb_setcmap = drm_fb_helper_setcmap,
> > + .fb_mmap= drm_fb_cma_mmap,
> > };
> >
> > static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> > --
> > 2.8.1.dirty
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
HDLCD crashes with 6d910bfa809e
On Wed, Jun 08, 2016 at 08:58:33AM +0200, Marek Szyprowski wrote:
> Hi All,
Hi Marek,
>
>
> On 2016-06-07 16:34, liviu.dudau at arm.com wrote:
> >On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
> >>Hi Liviu,
> >>
> >>On 07/06/16 14:35, liviu.dudau at arm.com wrote:
> >>>On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
> Having just inadvertently merged -next into my working branch, I find
> dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely
> affecting my board's ability to boot ;)
>
> Since I (intentionally) don't have sufficient CMA to create a framebuffer,
> drm_gem_cma_create() fails, unconditionally calls the now-NULL
> drm->driver->gem_free_object() in its cleanup path, and fiery death
> ensues...
> >>>Thanks for reporting this. What other changes other than reducing the CMA
> >>>allocation size do you have that I might need in order to reproduce this?
> >>I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
> >>+ DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
> >>job:
> >>
> >>[3.032402] hdlcd 7ff6.hdlcd: bound 0-0070 (ops tda998x_ops)
> >>[3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> >>[3.044970] [drm] No driver support for vblank timestamp query.
> >>[3.076973] hdlcd 7ff6.hdlcd: failed to allocate buffer with size
> >>768
> >>[3.084081] Bad mode in Synchronous Abort handler detected, code
> >>0x8604 -- IABT (current EL)
> >>[3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted
> >>4.7.0-rc1-next-20160602 #686
> >>[3.100682] Hardware name: ARM Juno development board (r1) (DT)
> >>[3.106567] Workqueue: deferwq deferred_probe_work_func
> >>[3.111761] task: 8009768a3e80 ti: 8009768e8000 task.ti:
> >>8009768e8000
> >>[3.119198] PC is at 0x0
> >>[3.121720] LR is at drm_gem_cma_create+0x128/0x130
> >>...and so on.
> >>
> >>Today's -next, on the other hand, dodges the bullet entirely:
> >>
> >>[2.903645] [drm] found ARM HDLCD version r0p0
> >>[2.908122] hdlcd 7ff6.hdlcd: master bind failed: -22
> >>[2.913505] tda998x: probe of 0-0070 failed with error -22
> >>[2.919141] [drm] found ARM HDLCD version r0p0
> >>[2.923609] hdlcd 7ff5.hdlcd: master bind failed: -22
> >>[2.928991] tda998x: probe of 0-0071 failed with error -22
> >OK, the problem is that commit 59ce4039727ef40 has changed the behaviour
> >from when
> >there is no "memory-region" phandle in the DT: before it used to return
> >-ENODEV, now
> >it returns -EINVAL.
> >
> >Marek, I quite liked the old behaviour to detect if the DT had the optional
> >(from
> >my driver's point of view) "memory-region" phandle. Plus the check for dev
> >is superfluous
> >when using of_reserved_mem_device_init() as that uses dev->of_node for np so
> >it would
> >crash before the check anyway. Maybe move the check there?
> >
> >Until then I suggest reverting the 59ce4039727ef40 commit.
>
> I've just send a fix for this issue. I'm sorry for the regression. I hope
> the fix fill
> quickly get into next to solve your problem.
Thanks for the patch, however I have some comments to it.
>
> The additional check for null dev make sense, because the new function
> of_reserved_mem_device_init_by_idx can be called with any device and node
> pointer not
> embedded with it, so I would like to keep it safe.
And I have to admit I find that scary. Why do you accept any node that is *not*
related to
the device? If you want just the ability to parse multiple "memory-region"
phandles (where
are the bindings defined for that?) I would have modified
of_reserved_mem_device_init() to
the the parsing and accept either the single entry style or a node with multiple
"memory-region" phandles in it. Otherwise I can steal the "memory-region" of
another device
and that device would have no idea that I have done this.
Can you point me to the latest thread where this patch has been discussed?
Best regards,
Liviu
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
HDLCD crashes with 6d910bfa809e
On Wed, Jun 08, 2016 at 12:01:20PM +0200, Marek Szyprowski wrote: > Hi Liviu, Hi Marek, [HDLCD bug report content removed] > >>>OK, the problem is that commit 59ce4039727ef40 has changed the behaviour > >>>from when > >>>there is no "memory-region" phandle in the DT: before it used to return > >>>-ENODEV, now > >>>it returns -EINVAL. > >>> > >>>Marek, I quite liked the old behaviour to detect if the DT had the > >>>optional (from > >>>my driver's point of view) "memory-region" phandle. Plus the check for dev > >>>is superfluous > >>>when using of_reserved_mem_device_init() as that uses dev->of_node for np > >>>so it would > >>>crash before the check anyway. Maybe move the check there? > >>> > >>>Until then I suggest reverting the 59ce4039727ef40 commit. > >>I've just send a fix for this issue. I'm sorry for the regression. I hope > >>the fix fill > >>quickly get into next to solve your problem. > >Thanks for the patch, however I have some comments to it. > > > >>The additional check for null dev make sense, because the new function > >>of_reserved_mem_device_init_by_idx can be called with any device and node > >>pointer not > >>embedded with it, so I would like to keep it safe. > >And I have to admit I find that scary. Why do you accept any node that is > >*not* related to > >the device? If you want just the ability to parse multiple "memory-region" > >phandles (where > >are the bindings defined for that?) I would have modified > >of_reserved_mem_device_init() to > >the the parsing and accept either the single entry style or a node with > >multiple > >"memory-region" phandles in it. Otherwise I can steal the "memory-region" of > >another device > >and that device would have no idea that I have done this. > > The idea is not to steal memory region from another device, but to let > driver to use multiple > regions assigned to the supported device with dma-mapping api. To use them > with dma-mapping > subsystem, one needs separate struct device for each reserved region. The > idea is to create > child devices of the device, which has memory-region property. Then for each > such child > device, driver can call of_reserved_mem_device_init_by_idx() to enable usage > of dma-mapping > api based on the selected reserved region. Such approach was already used > for long time in > the media/platform/s5p-mfc driver and now it has been converted to use > generic reserved > memory regions. > > If you feel scary about this approach, maybe a check if the device provided > to > of_reserved_mem_device_init_by_idx() function is one of the successors of > the device hidden > behind the provided node pointer (or points to the same device)? That would not hurt to have. > > >Can you point me to the latest thread where this patch has been discussed? > > This patch is a part of "Exynos: MFC driver: reserved memory cleanup and > IOMMU support" thread > and has been around for a while: > https://www.mail-archive.com/linux-media at vger.kernel.org/msg97645.html Thanks! I'm not subscribed to linux-media and way behind a lot of other MLs, so I have not noticed it. I've had a look at the patchset, found one case where you might leak some memory. I also think you could've had an of_reserved_mem_device_init_list() function that inits all the "memory-region" nodes and returns a list struct reserved_mem* that the driver can then assign to whatever internal variables it has. That way you can validate that all the "memory-region" phandles are used by the same parent device. Best regards, Liviu > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ã)_/¯
[PATCH] drm/atomic-helpers: Clear up cleanup_done a bit
On Wed, Jun 15, 2016 at 12:08:29PM +0200, Daniel Vetter wrote:
> It's not obvious at first sight that this is a fastpath, make that
> clearer with a goto. Fallout from a discussion with Liviu on irc.
>
> Cc: Liviu.Dudau at arm.com
> Acked-by: Liviu.Dudau at arm.com
Nope, I did not agree to *all* those changes, only the drm_atomic_helper.c one
:)
Best regards,
Liviu
> Signed-off-by: Daniel Vetter
> ---
> .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.h | 4 ++--
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 ---
> drivers/gpu/drm/drm_atomic_helper.c| 8 +++-
> include/drm/drm_crtc.h | 23
> --
> 4 files changed, 9 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> index ec4036a09f3e..a625b9137da2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> @@ -187,12 +187,12 @@ int init_pipelines(struct device_queue_manager *dqm,
> unsigned int get_first_pipe(struct device_queue_manager *dqm);
> unsigned int get_pipes_num(struct device_queue_manager *dqm);
>
> -extern inline unsigned int get_sh_mem_bases_32(struct kfd_process_device
> *pdd)
> +static inline unsigned int get_sh_mem_bases_32(struct kfd_process_device
> *pdd)
> {
> return (pdd->lds_base >> 16) & 0xFF;
> }
>
> -extern inline unsigned int
> +static inline unsigned int
> get_sh_mem_bases_nybble_64(struct kfd_process_device *pdd)
> {
> return (pdd->lds_base >> 60) & 0x0E;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index d0d5f4baf72d..80113c335966 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -617,10 +617,7 @@ int kgd2kfd_resume(struct kfd_dev *kfd);
> int kfd_init_apertures(struct kfd_process *process);
>
> /* Queue Context Management */
> -inline uint32_t lower_32(uint64_t x);
> -inline uint32_t upper_32(uint64_t x);
> struct cik_sdma_rlc_registers *get_sdma_mqd(void *mqd);
> -inline uint32_t get_sdma_base_addr(struct cik_sdma_rlc_registers *m);
>
> int init_queue(struct queue **q, struct queue_properties properties);
> void uninit_queue(struct queue *q);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 716aa535eb98..0556c95b7ddb 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1579,11 +1579,8 @@ void drm_atomic_helper_commit_cleanup_done(struct
> drm_atomic_state *state)
> /* commit_list borrows our reference, need to remove before we
>* clean up our drm_atomic_state. But only after it actually
>* completed, otherwise subsequent commits won't stall
> properly. */
> - if (try_wait_for_completion(&commit->flip_done)) {
> - list_del(&commit->commit_entry);
> - spin_unlock(&crtc->commit_lock);
> - continue;
> - }
> + if (try_wait_for_completion(&commit->flip_done))
> + goto del_commit:
>
> spin_unlock(&crtc->commit_lock);
>
> @@ -1597,6 +1594,7 @@ void drm_atomic_helper_commit_cleanup_done(struct
> drm_atomic_state *state)
> crtc->base.id, crtc->name);
>
> spin_lock(&crtc->commit_lock);
> +del_commit:
> list_del(&commit->commit_entry);
> spin_unlock(&crtc->commit_lock);
> }
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3c84ddc7e4c8..a39e1f17a20e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2820,29 +2820,14 @@ static inline void drm_connector_unreference(struct
> drm_connector *connector)
> #define drm_for_each_crtc(crtc, dev) \
> list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
>
> -static inline void
> -assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
> -{
> - /*
> - * The connector hotadd/remove code currently grabs both locks when
> - * updating lists. Hence readers need only hold either of them to be
> - * safe and the check amounts to
> - *
> - * WARN_ON(not_holding(A) && not_holding(B)).
> - */
> - WARN_ON(!mutex_is_locked(&mode_config->mutex) &&
> - !drm_modeset_is_locked(&mode_config->connection_mutex));
> -}
> -
> #define drm_for_each_connector(connector, dev) \
> /* loop to wrap everything into a srcu read-side critical section */
> \
> - for (bool __conn_loop_srcu = true,
> \
> - int __conn_loop_srcu_ret =
> srcu_read_lock(&drm_connector_list_srcu);\
> - __conn_loop_srcu; __conn_loop_srcu = false,
> \
> + for (in
[PATCH] drm/atomic-helpers: Clear up cleanup_done a bit
On Wed, Jun 15, 2016 at 12:24:26PM +0200, Daniel Vetter wrote:
> It's not obvious at first sight that this is a fastpath, make that
> clearer with a goto. Fallout from a discussion with Liviu on irc.
>
> v2: Drop bogus hunks that crept in.
>
> Cc: Liviu.Dudau at arm.com
> Acked-by: Liviu.Dudau at arm.com
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 716aa535eb98..0556c95b7ddb 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1579,11 +1579,8 @@ void drm_atomic_helper_commit_cleanup_done(struct
> drm_atomic_state *state)
> /* commit_list borrows our reference, need to remove before we
>* clean up our drm_atomic_state. But only after it actually
>* completed, otherwise subsequent commits won't stall
> properly. */
> - if (try_wait_for_completion(&commit->flip_done)) {
> - list_del(&commit->commit_entry);
> - spin_unlock(&crtc->commit_lock);
> - continue;
> - }
> + if (try_wait_for_completion(&commit->flip_done))
> + goto del_commit:
s/:/;/
Liviu
>
> spin_unlock(&crtc->commit_lock);
>
> @@ -1597,6 +1594,7 @@ void drm_atomic_helper_commit_cleanup_done(struct
> drm_atomic_state *state)
> crtc->base.id, crtc->name);
>
> spin_lock(&crtc->commit_lock);
> +del_commit:
> list_del(&commit->commit_entry);
> spin_unlock(&crtc->commit_lock);
> }
> --
> 2.8.1
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH] drm/i2c: tda998x: Choose between atomic or non atomic dpms helper
On Sat, Jan 16, 2016 at 10:17:54PM +0200, Jyri Sarha wrote:
> Choose between atomic or non atomic connector dpms helper. If tda998x
> is connected to a drm driver that does not support atomic modeset
> calling drm_atomic_helper_connector_dpms() causes a crash when the
> connectors atomic state is not initialized. The patch implements a
> driver specific connector dpms helper that calls
> drm_atomic_helper_connector_dpms() if driver supports DRIVER_ATOMIC
> and otherwise it calls the legacy drm_helper_connector_dpms().
>
> Fixes commit 9736e988d328 ("drm/i2c: tda998x: Add support for atomic
> modesetting").
>
> Signed-off-by: Jyri Sarha
Looks much cleaner to me.
Acked-by: Liviu Dudau
> ---
>
> Ok, so this is the second approach to solve this issue. The first
> attempt can be found here [1] with the follow from Liviu Dudau that
> suggested this approach.
>
> It just makes me wonder if drm_atomic_helper_connector_dpms() should
> call the legacy callback automatically if DRIVER_ATOMIC is not set or
> at least bail out gracefully with an error message. Then again it may
> be overkill if the tda998x is the only driver that need to support
> both situations.
I would guess that in general it makes sense for the core code to do what
you suggest, but the idea is to try to encourage the drivers to convert
to atomic ops rather than give them too many crutches. And the tda998x will
need this for a short period of time until tilcdc converts as well.
Best regards,
Liviu
>
> Best regards,
> Jyri
>
> [1] http://www.spinics.net/lists/dri-devel/msg98514.html
>
> drivers/gpu/drm/i2c/tda998x_drv.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 012d36d..bb7d507 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1382,8 +1382,16 @@ static void tda998x_connector_destroy(struct
> drm_connector *connector)
> drm_connector_cleanup(connector);
> }
>
> +static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
> +{
> + if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
> + return drm_atomic_helper_connector_dpms(connector, mode);
> + else
> + return drm_helper_connector_dpms(connector, mode);
> +}
> +
> static const struct drm_connector_funcs tda998x_connector_funcs = {
> - .dpms = drm_atomic_helper_connector_dpms,
> + .dpms = tda998x_connector_dpms,
> .reset = drm_atomic_helper_connector_reset,
> .fill_modes = drm_helper_probe_single_connector_modes,
> .detect = tda998x_connector_detect,
> --
> 1.9.1
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ã)_/¯
[PATCH] drm: hdlcd: Work properly in big-endian mode
On Wed, Dec 07, 2016 at 03:31:40PM +, Robin Murphy wrote: > Under a big-endian kernel, colours on the framebuffer all come out a > delightful shade of wrong, So you are saying that wrong is only a 1 bit value? > since we fail to take the reversed byte > order into account. Fortunately, the HDLCD has a control bit to make it > automatically byteswap big-endian data; let's use it as appropriate. > > Signed-off-by: Robin Murphy Change looks good to me, but as Daniel has mentioned, we should have failed the modesetting as the buffer we are passed should be in the wrong fourcc format. Any way I can play with a big-endian filesystem that you can share? Best regards, Liviu > --- > drivers/gpu/drm/arm/hdlcd_crtc.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c > b/drivers/gpu/drm/arm/hdlcd_crtc.c > index 28341b32067f..eceb7bed5dd0 100644 > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c > @@ -63,6 +63,7 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) > uint32_t pixel_format; > struct simplefb_format *format = NULL; > int i; > + u32 reg; > > pixel_format = crtc->primary->state->fb->pixel_format; > > @@ -76,7 +77,11 @@ static int hdlcd_set_pxl_fmt(struct drm_crtc *crtc) > > /* HDLCD uses 'bytes per pixel', zero means 1 byte */ > btpp = (format->bits_per_pixel + 7) / 8; > - hdlcd_write(hdlcd, HDLCD_REG_PIXEL_FORMAT, (btpp - 1) << 3); > + reg = (btpp - 1) << 3; > +#ifdef __BIG_ENDIAN > + reg |= HDLCD_PIXEL_FMT_BIG_ENDIAN; > +#endif > + hdlcd_write(hdlcd, HDLCD_REG_PIXEL_FORMAT, reg); > > /* >* The format of the HDLCD_REG__SELECT register is: > -- > 2.10.2.dirty > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ã)_/¯
Re: 回复: [PATCH] drm/komeda: add NV12 format to support writeback layer type
Hi Liu, Sorry about the delay, I was on holiday until 28th and while cleaning up the backlog I've accidentally marked the email as read and did not reply. On Fri, Sep 08, 2023 at 08:11:44AM +, Liu Lucas/刘保柱 wrote: > Hi all, > > Do you have any suggestions for the patch I submitted? Please also let > me know, thank you! > > Best Regards, > baozhu.liu > -邮件原件- > 发件人: baozhu.liu > 发送时间: 2023年8月29日 17:30 > 收件人: [email protected]; [email protected]; [email protected] > 抄送: [email protected]; [email protected]; Liu > Lucas/刘保柱 > 主题: [PATCH] drm/komeda: add NV12 format to support writeback layer type > > When testing the d71 writeback layer function, the output format is set to > NV12, and the following error message is displayed: > > [drm:komeda_fb_is_layer_supported] Layer TYPE: 4 doesn't support fb FMT: NV12 > little-endian (0x3231564e) with modifier: 0x0.. > > Check the d71 data manual, writeback layer output formats includes NV12 > format. > > Signed-off-by: baozhu.liu Acked-by: Liviu Dudau I will push the patch this week into drm-misc-next. Best regards, Liviu > --- > drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c > b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c > index 6c56f5662bc7..80973975bfdb 100644 > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c > @@ -521,7 +521,7 @@ static struct komeda_format_caps d71_format_caps_table[] > = { > {__HW_ID(5, 1), DRM_FORMAT_YUYV,RICH, Rot_ALL_H_V, > LYT_NM, AFB_TH}, /* afbc */ > {__HW_ID(5, 2), DRM_FORMAT_YUYV,RICH, Flip_H_V, > 0, 0}, > {__HW_ID(5, 3), DRM_FORMAT_UYVY,RICH, Flip_H_V, > 0, 0}, > - {__HW_ID(5, 6), DRM_FORMAT_NV12,RICH, Flip_H_V, > 0, 0}, > + {__HW_ID(5, 6), DRM_FORMAT_NV12,RICH_WB,Flip_H_V, > 0, 0}, > {__HW_ID(5, 6), DRM_FORMAT_YUV420_8BIT, RICH, Rot_ALL_H_V, > LYT_NM, AFB_TH}, /* afbc */ > {__HW_ID(5, 7), DRM_FORMAT_YUV420, RICH, Flip_H_V, > 0, 0}, > /* YUV 10bit*/ > -- > 2.17.1 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯
Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure
Hi, On Wed, Aug 13, 2025 at 10:04:22AM +, Kandpal, Suraj wrote: > > > > }; > > > > > > I still don't like that. This really doesn't belong here. If anything, > > > the drm_connector for writeback belongs to drm_crtc. > > > > Why? We already have generic HDMI field inside drm_connector. I am really > > hoping to be able to land DP parts next to it. In theory we can have a DVI- > > specific entry there (e.g. with the subconnector type). > > The idea is not to limit how the drivers subclass those structures. > > > > I don't see a good case why WB should deviate from that design. > > > > > If the issue is that some drivers need a custom drm_connector > > > subclass, then I'd rather turn the connector field of > > > drm_writeback_connector into a pointer. > > > > Having a pointer requires additional ops in order to get drm_connector from > > WB code and vice versa. Having drm_connector_wb inside drm_connector > > saves us from those ops (which don't manifest for any other kind of > > structure). > > Nor will it take any more space since union will reuse space already taken > > up by > > HDMI part. > > > > > > > Seems like this thread has died. We need to get a conclusion on the design. > Laurent do you have any issue with the design given Dmitry's explanation as > to why this > Design is good for drm_writeback_connector. I'm with Laurent here. The idea for drm_connector (and a lot of drm structures) are to be used as base "classes" for extended structures. I don't know why HDMI connector ended up inside drm_connector as not all connectors have HDMI functionality, but that's a cleanup for another day. drm_writeback_connector uses the 'base' drm_connector only for a few things, mostly in __drm_writeback_connector_init() and prepare_job()/cleanup_job(). In _init() we just setup the properties and the encoder after we disable interlacing. prepare_job()/cleanup_job() is another workaround to be to some custom ops some drivers might want for signalling. So we should be able to convert the 'base' drm_connector to a pointer relatively easy. We shouldn't need to get to the drm_connector from a drm_writeback_connector() outside drm_writeback.c. Then it looks like what we need is a __drm_writeback_connector_init_with_connector() where we can pass a base pointer and remember it. Maybe an extra parameter to existing init functions, or a new one that skips the encoder initialisation entirely. Best regards, Liviu > > Regards, > Suraj Kandpal > > > > > I plan to add drm_connector_dp in a similar way, covering DP needs > > > > (currently WIP). > > > > -- > > With best wishes > > Dmitry -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯
