[linux-sunxi] Re: [PATCH v2] drm: sun4i: Add support for suspending the display driver

[Daniel Vetter]

On Tue, Oct 29, 2019 at 12:28:46PM +0100, Ondrej Jirman wrote:
> Shut down the display engine during suspend.
> 
> Signed-off-by: Ondrej Jirman 
> ---
> Changes in v2:
> - spaces -> tabs
> 
>  drivers/gpu/drm/sun4i/sun4i_drv.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c 
> b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index a5757b11b730..c519d7cfcf43 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -346,6 +346,27 @@ static int sun4i_drv_add_endpoints(struct device *dev,
>   return count;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int sun4i_drv_drm_sys_suspend(struct device *dev)
> +{
> + struct drm_device *drm = dev_get_drvdata(dev);
> +
> + return drm_mode_config_helper_suspend(drm);
> +}
> +
> +static int sun4i_drv_drm_sys_resume(struct device *dev)
> +{
> + struct drm_device *drm = dev_get_drvdata(dev);
> +
> + return drm_mode_config_helper_resume(drm);
> +}
> +#endif
> +
> +static const struct dev_pm_ops sun4i_drv_drm_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(sun4i_drv_drm_sys_suspend,
> + sun4i_drv_drm_sys_resume)
> +};

I wonder whether we should have these as default helpers somewhere,
there's probably a few drivers that could use them? It's just a handful of
lines we're saving here, but we have enough kms drivers to justify this
kind of stuff nowadays ...
-Daniel

> +
>  static int sun4i_drv_probe(struct platform_device *pdev)
>  {
>   struct component_match *match = NULL;
> @@ -418,6 +439,7 @@ static struct platform_driver sun4i_drv_platform_driver = 
> {
>   .driver = {
>   .name   = "sun4i-drm",
>   .of_match_table = sun4i_drv_of_table,
> + .pm = _drv_drm_pm_ops,
>   },
>  };
>  module_platform_driver(sun4i_drv_platform_driver);
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit 
https://groups.google.com/d/msgid/linux-sunxi/20191104174218.GL10326%40phenom.ffwll.local.

[linux-sunxi] Re: [PATCH v4 0/8] Allwinner H6 Mali GPU support

[Daniel Vetter]

On Sun, May 12, 2019 at 07:46:00PM +0200, peron.c...@gmail.com wrote:
> From: Clément Péron 
> 
> Hi,
> 
> The Allwinner H6 has a Mali-T720 MP2. The drivers are
> out-of-tree so this series only introduce the dt-bindings.

We do have an in-tree midgard driver now (since 5.2). Does this stuff work
together with your dt changes here?
-Daniel

> The first patch is from Neil Amstrong and has been already
> merged in linux-amlogic. It is required for this series.
> 
> The second patch is from Icenowy Zheng where I changed the
> order has required by Rob Herring.
> See: https://patchwork.kernel.org/patch/10699829/
> 
> Thanks,
> Clément
> 
> Changes in v4:
>  - Add Rob Herring reviewed-by tag
>  - Resent with correct Maintainers
> 
> Changes in v3 (Thanks to Maxime Ripard):
>  - Reauthor Icenowy for her patch
> 
> Changes in v2 (Thanks to Maxime Ripard):
>  - Drop GPU OPP Table
>  - Add clocks and clock-names in required
> 
> Clément Péron (6):
>   dt-bindings: gpu: mali-midgard: Add H6 mali gpu compatible
>   arm64: dts: allwinner: Add ARM Mali GPU node for H6
>   arm64: dts: allwinner: Add mali GPU supply for Pine H64
>   arm64: dts: allwinner: Add mali GPU supply for Beelink GS1
>   arm64: dts: allwinner: Add mali GPU supply for OrangePi Boards
>   arm64: dts: allwinner: Add mali GPU supply for OrangePi 3
> 
> Icenowy Zheng (1):
>   dt-bindings: gpu: add bus clock for Mali Midgard GPUs
> 
> Neil Armstrong (1):
>   dt-bindings: gpu: mali-midgard: Add resets property
> 
>  .../bindings/gpu/arm,mali-midgard.txt | 27 +++
>  .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |  5 
>  .../dts/allwinner/sun50i-h6-orangepi-3.dts|  5 
>  .../dts/allwinner/sun50i-h6-orangepi.dtsi |  5 
>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts |  5 ++++
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 14 ++
>  6 files changed, 61 insertions(+)
> 
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit 
https://groups.google.com/d/msgid/linux-sunxi/20190513151405.GW17751%40phenom.ffwll.local.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Daniel Vetter]

On Tue, Feb 05, 2019 at 11:24:19AM +0100, Thierry Reding wrote:
> On Tue, Feb 05, 2019 at 09:57:37AM +0100, Daniel Vetter wrote:
> > On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> > > On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > > > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding 
> > > > > > > >  wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick 
> > > > > > > > > wrote:
> > > > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to 
> > > > > > > > > > define panel
> > > > > > > > > > width/height or any modes/timings in dts. But this panel 
> > > > > > > > > > still may have
> > > > > > > > > > regulator and/or backlight.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Vasily Khoruzhick 
> > > > > > > > > > ---
> > > > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > > > > | 7 +++
> > > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > > >  create mode 100644 
> > > > > > > > > > Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > > >
> > > > > > > > > Please don't try to make panels look more generic than they 
> > > > > > > > > really are.
> > > > > > > > > You're going to have to provide a compatible string for your 
> > > > > > > > > device that
> > > > > > > > > is more specific than "panel-edp". You claim that you don't 
> > > > > > > > > need any
> > > > > > > > > extra information that is panel specific, but you don't know 
> > > > > > > > > that now.
> > > > > > > > > We have in the past thought that we didn't need things like 
> > > > > > > > > prepare
> > > > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > > > >
> > > > > > > > > Just do what everybody else does. Provide a specific 
> > > > > > > > > compatible string
> > > > > > > > > and match on that in the panel-simple driver. Even if you can 
> > > > > > > > > read all
> > > > > > > > > the video timings from an EDID EEPROM, you can still provide 
> > > > > > > > > a mode in
> > > > > > > > > the panel descriptor to serve as a fallback if for example 
> > > > > > > > > the EEPROM
> > > > > > > > > is faulty on some device.
> > > > > > > > 
> > > > > > > > Pinebook used several 768p panels that have slightly different 
> > > > > > > > timings
> > > > > > > > and recent batch uses 1080p panel.
> > > > > > > > 
> > > > > > > > What panel descriptor should I use as fallback?
> > > > > > > 
> > > > > > > You don't use panel descriptors as fallback. The simple-panel 
> > > > > > > driver
> > > > > > > will bind to a panel device and use the corresponding descriptor. 
> > > > > > > If
> > > > > > > your device tree contains the correct information, the descriptor 
> > > > > > > is
> > > > > > > correct for the panel you have.
> > > > > > > 
> > > > > > > In other words you need to ensure that you have the correct panel 
> > > > > > > in
> > > > > > > device tree for the board that you're using. This is exactly the 
> > > > 

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Daniel Vetter]

On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding 
> > > > > >  wrote:
> > > > > > >
> > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to 
> > > > > > > > define panel
> > > > > > > > width/height or any modes/timings in dts. But this panel still 
> > > > > > > > may have
> > > > > > > > regulator and/or backlight.
> > > > > > > >
> > > > > > > > Signed-off-by: Vasily Khoruzhick 
> > > > > > > > ---
> > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt| 7 
> > > > > > > > +++
> > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > >  create mode 100644 
> > > > > > > > Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > >
> > > > > > > Please don't try to make panels look more generic than they 
> > > > > > > really are.
> > > > > > > You're going to have to provide a compatible string for your 
> > > > > > > device that
> > > > > > > is more specific than "panel-edp". You claim that you don't need 
> > > > > > > any
> > > > > > > extra information that is panel specific, but you don't know that 
> > > > > > > now.
> > > > > > > We have in the past thought that we didn't need things like 
> > > > > > > prepare
> > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > >
> > > > > > > Just do what everybody else does. Provide a specific compatible 
> > > > > > > string
> > > > > > > and match on that in the panel-simple driver. Even if you can 
> > > > > > > read all
> > > > > > > the video timings from an EDID EEPROM, you can still provide a 
> > > > > > > mode in
> > > > > > > the panel descriptor to serve as a fallback if for example the 
> > > > > > > EEPROM
> > > > > > > is faulty on some device.
> > > > > > 
> > > > > > Pinebook used several 768p panels that have slightly different 
> > > > > > timings
> > > > > > and recent batch uses 1080p panel.
> > > > > > 
> > > > > > What panel descriptor should I use as fallback?
> > > > > 
> > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > your device tree contains the correct information, the descriptor is
> > > > > correct for the panel you have.
> > > > > 
> > > > > In other words you need to ensure that you have the correct panel in
> > > > > device tree for the board that you're using. This is exactly the same
> > > > > thing as for other devices.
> > > > > 
> > > > > One way to to this is to have separate device trees for each variant
> > > > > of the board that you want to support. Another variant may be to have
> > > > > a common device tree and then have some early firmware update the DTB
> > > > > with the correct panel information.
> > > > 
> > > > This would defeat the point of edp, which is to standardize the mess of
> > > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > > infrastructure already (using the OUI), I think if there's something 
> > > > that
> > > > doesn't work then we should quirk it there.
> > > 
> > > T

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Daniel Vetter]

On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding 
> > > >  wrote:
> > > > >
> > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > eDP panels usually have EDID EEPROM, so there's no need to define 
> > > > > > panel
> > > > > > width/height or any modes/timings in dts. But this panel still may 
> > > > > > have
> > > > > > regulator and/or backlight.
> > > > > >
> > > > > > Signed-off-by: Vasily Khoruzhick 
> > > > > > ---
> > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt| 7 
> > > > > > +++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > >  create mode 100644 
> > > > > > Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > >
> > > > > Please don't try to make panels look more generic than they really 
> > > > > are.
> > > > > You're going to have to provide a compatible string for your device 
> > > > > that
> > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > extra information that is panel specific, but you don't know that now.
> > > > > We have in the past thought that we didn't need things like prepare
> > > > > delay, but then we ran into situations where we did need them.
> > > > >
> > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > is faulty on some device.
> > > > 
> > > > Pinebook used several 768p panels that have slightly different timings
> > > > and recent batch uses 1080p panel.
> > > > 
> > > > What panel descriptor should I use as fallback?
> > > 
> > > You don't use panel descriptors as fallback. The simple-panel driver
> > > will bind to a panel device and use the corresponding descriptor. If
> > > your device tree contains the correct information, the descriptor is
> > > correct for the panel you have.
> > > 
> > > In other words you need to ensure that you have the correct panel in
> > > device tree for the board that you're using. This is exactly the same
> > > thing as for other devices.
> > > 
> > > One way to to this is to have separate device trees for each variant
> > > of the board that you want to support. Another variant may be to have
> > > a common device tree and then have some early firmware update the DTB
> > > with the correct panel information.
> > 
> > This would defeat the point of edp, which is to standardize the mess of
> > panels (at least somewhat) and avoid having to change the DT/ACPI
> > tables/firmware for every board you ship. Also, we do have DP quirking
> > infrastructure already (using the OUI), I think if there's something that
> > doesn't work then we should quirk it there.
> 
> The problem is that while the attempt may have been to standardize, it
> failed. It doesn't take into account any of the details such as timing
> between things like powering up the display and enabling the backlight
> or similar. I don't know how you'd want to "quirk" those kinds of
> requirements because they are highly panel specific.

Hm right, we get these from some firmware tables (and mix them with the
spec one, since some of the firmware values are nonsense). I don't even
know whether we can read the timings over dp aux somehow (you can power up
the panel with some pessimistic values to figure those out, and you only
need dp aux to work, which is much simpler than the entire panel).

> > What does make sense though imo is if we try not to stuff the edp panel
> > into panel-simple, because it's anything like a simple dumb panel. There's
> > also some integration awkwardness since with this panel you need to do dp
> > aux/i2c transactions to get at the information (edid alone isn't good
> > enough for 

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Daniel Vetter]

On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding  
> > wrote:
> > >
> > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > width/height or any modes/timings in dts. But this panel still may have
> > > > regulator and/or backlight.
> > > >
> > > > Signed-off-by: Vasily Khoruzhick 
> > > > ---
> > > >  .../devicetree/bindings/display/panel/panel-edp.txt| 7 +++
> > > >  1 file changed, 7 insertions(+)
> > > >  create mode 100644 
> > > > Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > >
> > > Please don't try to make panels look more generic than they really are.
> > > You're going to have to provide a compatible string for your device that
> > > is more specific than "panel-edp". You claim that you don't need any
> > > extra information that is panel specific, but you don't know that now.
> > > We have in the past thought that we didn't need things like prepare
> > > delay, but then we ran into situations where we did need them.
> > >
> > > Just do what everybody else does. Provide a specific compatible string
> > > and match on that in the panel-simple driver. Even if you can read all
> > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > is faulty on some device.
> > 
> > Pinebook used several 768p panels that have slightly different timings
> > and recent batch uses 1080p panel.
> > 
> > What panel descriptor should I use as fallback?
> 
> You don't use panel descriptors as fallback. The simple-panel driver
> will bind to a panel device and use the corresponding descriptor. If
> your device tree contains the correct information, the descriptor is
> correct for the panel you have.
> 
> In other words you need to ensure that you have the correct panel in
> device tree for the board that you're using. This is exactly the same
> thing as for other devices.
> 
> One way to to this is to have separate device trees for each variant
> of the board that you want to support. Another variant may be to have
> a common device tree and then have some early firmware update the DTB
> with the correct panel information.

This would defeat the point of edp, which is to standardize the mess of
panels (at least somewhat) and avoid having to change the DT/ACPI
tables/firmware for every board you ship. Also, we do have DP quirking
infrastructure already (using the OUI), I think if there's something that
doesn't work then we should quirk it there.

What does make sense though imo is if we try not to stuff the edp panel
into panel-simple, because it's anything like a simple dumb panel. There's
also some integration awkwardness since with this panel you need to do dp
aux/i2c transactions to get at the information (edid alone isn't good
enough for edp), and I'm not sure how exactly that's supposed to be
instantiated. Maybe a special function to instantiate an edp panel, which
takes both a DT node and the dp_aux controller would be much better,
instead of trying to auto-match against a DT compatible string and load a
panel driver which is almost all fake.

Or we teach dp_aux to register itself and somehow teach panel-edp how it
can get hold of the dp_aux channel it needs.

But fake mode in panel-simple sounds like the wrong approach. At least
from our experience with i915 (and I think other drivers supporting edp),
the standardization of panels for basic stuff at least worked.
Self-refresh seems an entirely different story unfortunately ... but again
quirking that for DP stuff should be done using the OUI I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH] drm/sun4i: wait on implicit fence before display

[Daniel Vetter]

On Thu, Nov 22, 2018 at 09:44:17AM +0800, Qiang Yu wrote:
> Render like lima will attach a fence to the framebuffer
> dma_buf, display like sun4i should wait it finish before
> show the framebuffer. Otherwise tearing will be observed.
> 
> Signed-off-by: Qiang Yu 

Thanks for submitting this fix, applied to drm-misc-next.
-Daniel

> ---
>  drivers/gpu/drm/sun4i/sun4i_layer.c| 2 ++
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 2 ++
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c 
> b/drivers/gpu/drm/sun4i/sun4i_layer.c
> index 750ad24de1d7..d68e663df9a0 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> @@ -12,6 +12,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "sun4i_backend.h"
> @@ -114,6 +115,7 @@ static void sun4i_backend_layer_atomic_update(struct 
> drm_plane *plane,
>  }
>  
>  static const struct drm_plane_helper_funcs sun4i_backend_layer_helper_funcs 
> = {
> + .prepare_fb = drm_gem_fb_prepare_fb,
>   .atomic_disable = sun4i_backend_layer_atomic_disable,
>   .atomic_update  = sun4i_backend_layer_atomic_update,
>  };
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c 
> b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> index 28c15c6ef1ef..7bc2ca2bd0c3 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -19,6 +19,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -287,6 +288,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane 
> *plane,
>  }
>  
>  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
> + .prepare_fb = drm_gem_fb_prepare_fb,
>   .atomic_check   = sun8i_ui_layer_atomic_check,
>   .atomic_disable = sun8i_ui_layer_atomic_disable,
>   .atomic_update  = sun8i_ui_layer_atomic_update,
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c 
> b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> index f4fe97813f94..815895795afd 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -315,6 +316,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane 
> *plane,
>  }
>  
>  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
> + .prepare_fb = drm_gem_fb_prepare_fb,
>   .atomic_check   = sun8i_vi_layer_atomic_check,
>   .atomic_disable = sun8i_vi_layer_atomic_disable,
>   .atomic_update  = sun8i_vi_layer_atomic_update,
> -- 
> 2.17.1
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH 9/9] [DO NOT MERGE] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Daniel Vetter]

On Thu, Oct 18, 2018 at 10:55 AM Laurent Pinchart
 wrote:
>
> Hi Icenowy,
>
> Thank you for the patch.
>
> On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng wrote:
> > From: Chen-Yu Tsai 
> >
> > The panels shipped with Allwinner devices are very "generic", i.e.
> > they do not have model numbers or reliable sources of information
> > for the timings (that we know of) other than the fex files shipped
> > on them. The dot clock frequency provided in the fex files have all
> > been rounded to the nearest MHz, as that is the unit used in them.
> >
> > We were using the simple panel "urt,umsh-8596md-t" as a substitute
> > for the A13 Q8 tablets in the absence of a specific model for what
> > may be many different but otherwise timing compatible panels. This
> > was usable without any visual artifacts or side effects, until the
> > dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> > rgb: Validate the clock rate").
> >
> > The reason this check fails is because the dotclock frequency for
> > this model is 33.26 MHz, which is not achievable with our dot clock
> > hardware, and the rate returned by clk_round_rate deviates slightly,
> > causing the driver to reject the display mode.
> >
> > The LCD panels have some tolerance on the dot clock frequency, even
> > if it's not specified in their datasheets.
> >
> > This patch adds a 5% tolerence to the dot clock check.
>
> Why do you think this shouldn't be merged ?

It pisses of a lot of people who really insist upon accurate timing. I
think a better fix would be to have a dotclock range in drm_panel, and
some magic to figure out which one of these we can actually do. Then
tell userspace that this is the mode is should request. That way
userspace knows what the actual dotclock/refresh rate is, and the
panel still works.
-Daniel

>
> > Signed-off-by: Chen-Yu Tsai 
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> > b/drivers/gpu/drm/sun4i/sun4i_rgb.c index bf068da6b12e..23bdc449eacc 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> > @@ -92,13 +92,14 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct
> > drm_encoder *crtc,
> >
> >   DRM_DEBUG_DRIVER("Vertical parameters OK\n");
> >
> > + /* Check against a 5% tolerance for the dot clock */
> >   tcon->dclk_min_div = 6;
> >   tcon->dclk_max_div = 127;
> >   rounded_rate = clk_round_rate(tcon->dclk, rate);
> > - if (rounded_rate < rate)
> > + if (rounded_rate < rate * 19 / 20 )
> >   return MODE_CLOCK_LOW;
> >
> > - if (rounded_rate > rate)
> > + if (rounded_rate > rate * 21 / 20)
> >   return MODE_CLOCK_HIGH;
> >
> >   DRM_DEBUG_DRIVER("Clock rate OK\n");
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: Planes enumeration with sun4i-drm driver

[Daniel Vetter]

On Mon, Oct 15, 2018 at 10:29 AM Maxime Ripard
 wrote:
> On Fri, Oct 12, 2018 at 06:47:13PM +0200, Paul Kocialkowski wrote:
> > I'm looking at the sun4i DRM driver these days (especially for
> > mainlining the VPU tiled format support through the frontend).
> >
> > The way things are done currently, all the possibly-supported plane
> > formats are listed in sun4i_backend_layer_formats and exposed as-is to
> > userspace for every plane. However, some of these formats cannot be
> > used on all planes at the same time and will be rejected when checking
> > the atomic commit.
> >
> > I find this a bit confusing from a userspace perspective.
> >
> > Not all constraints can be provided to userspace (e.g. the number of
> > planes we can effectively scale), but when it comes to formats, we have
> > that possibility. So perhaps it would make sense to only enumerate
> > formats as many times as we can support them.
> >
> > For instance, it could look like:
> > # plane 0: primary, RGB only
> > # plane 1: overlay, RGB + backend YUV formats
> > # plane 2: overlay, RGB + frontend YUV formats
> > # plane 3: overlay, RGB only
> >
> > That's not perfect either, because e.g. requesting a scaled RGB plane
> > will require the frontend and thus make it impossible to use the
> > frontend YUV formats that would still be described. But it feels like
> > it would be closer to representing hardware capabilities than our
> > current situation.
>
> That's really arguable. The hardware capabilities are that you can use
> any of those formats or features, on any plane, *but* on only one
> plane at the same time. What you describe isn't closer to it, it's
> just a way to workaround the issue we are facing (ie being able to
> show those constraints to userspace), and an imperfect workaround.
>
> > I am really not sure about the DRM subsystem policy about this, though.
> > Perhaps it was already decided that it's fine to deal with everything
> > at commit checking time and report more formats than the hardware can
> > really handle.
>
> It doesn't really matter what the DRM policy is here. Any change in
> the direction you suggest would be a regression from the userspace
> point of view and therefore would have to be reverted.

How exactly can this cause a regression? Do you have some userspace
that card-codes it's allocation of planes, which would then fail here
and worked beforehand?

> IIRC Weston tries to discover those constraints by brute-forcing
> atomic_check and figuring out a combination that works, and we would
> surely benefit some kind of API to expose composition constraints.

The current hints we have is to limit the features each plane exposes.
Currently there's no proposal to do stuff like "only x of y planes can
do $feature" at all. So yeah, Paul's idea doesn't seem entirely out of
hand to me, and for the "bug regressions" we need a concrete example
of what will break. Since we're fine-tuning the drm api all the time,
within the limits of what userspace expects :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH] drm/sun4i: Avoid failing to init fbdev without any connector

[Daniel Vetter]

On Tue, Aug 07, 2018 at 09:39:19PM +0200, Paul Kocialkowski wrote:
> Initializing and registering fbdev requires at least one DRM connector
> and will fail otherwise. In order to support headless setups (for
> instance for GPU rendering with the GBM backend, where a DRI card node
> is required to provide GEM memory reservation), add a check on the
> number of registered connectors before initializing fbdev.

sun4i is a pure kms driver, why exactly do you need it for gbm backed
rendering? What exactly is rendering here, and why does it insist on a
display card node, even if that display card node is 100% defunct?
-Daniel

> Signed-off-by: Paul Kocialkowski 
> ---
>  drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c 
> b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
> index 5f29850ef8ac..19a265e4a93a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
> @@ -49,6 +49,8 @@ static struct drm_mode_config_helper_funcs 
> sun4i_de_mode_config_helpers = {
>  
>  int sun4i_framebuffer_init(struct drm_device *drm)
>  {
> + int ret;
> +
>   drm_mode_config_reset(drm);
>  
>   drm->mode_config.max_width = 8192;
> @@ -57,7 +59,13 @@ int sun4i_framebuffer_init(struct drm_device *drm)
>   drm->mode_config.funcs = _de_mode_config_funcs;
>   drm->mode_config.helper_private = _de_mode_config_helpers;
>  
> - return drm_fb_cma_fbdev_init(drm, 32, 0);
> + if (drm->mode_config.num_connector > 0) {
> + ret = drm_fb_cma_fbdev_init(drm, 32, 0);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
>  }
>  
>  void sun4i_framebuffer_free(struct drm_device *drm)
> -- 
> 2.18.0
> 
> _______
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2 00/10] Allwinner H3/H5/A64(DE2) SimpleFB support

[Daniel Vetter]

On Thu, Nov 02, 2017 at 04:44:47PM +0100, Maxime Ripard wrote:
> On Thu, Nov 02, 2017 at 04:51:29PM +0800, Icenowy Zheng wrote:
> > 在 2017-10-27 23:06，Icenowy Zheng 写道：
> > > This patchset adds support for the SimpleFB on Allwinner SoCs with
> > > "Display Engine 2.0".
> > > 
> > > PATCH 1 to PATCH 3 are DE2 CCU fixes for H3/H5 SoCs.
> > > 
> > > PATCH 4 adds the pipeline strings for DE2 SimpleFB.
> > > 
> > > PATCH 5 to 7 adds necessary device tree nodes (DE2 CCU and SimpleFB)
> > > for H3/H5 SoCs.
> > > 
> > > PATCH 8 to 10 are for Allwinner A64 SoC to enable SimpleFB.
> > > 
> > > Icenowy Zheng (10):
> > >   dt-bindings: fix the binding of Allwinner DE2 CCU of A83T and H3
> > >   clk: sunxi-ng: add support for Allwinner H3 DE2 CCU
> > >   clk: sunxi-ng: fix the A64/H5 clock description of DE2 CCU
> > >   dt-bindings: simplefb-sunxi: add pipelines for DE2
> > >   ARM: sun8i: h3/h5: add DE2 CCU device node for H3
> > >   arm64: allwinner: h5: add compatible string for DE2 CCU
> > >   ARM: sunxi: h3/h5: add simplefb nodes
> > >   dt-bindings: add binding for A64 DE2 CCU SRAM
> > >   arm64: allwinner: a64: add DE2 CCU for A64 SoC
> > >   arm64: allwinner: a64: add simplefb for A64 SoC
> > 
> > Maxime, could you review and, if possible, apply the H3/5
> > part of this patchset?
> 
> This came a bit late, we're too close from the merge window
> now. Please resend them after -rc1 is out.

Just dropping here that drm-misc is open all the time, making for a much
better process for contributors :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v3 09/14] regmap: add iopoll-like polling macro for regmap_field

[Daniel Vetter]

On Wed, Oct 04, 2017 at 11:47:32AM +0100, Mark Brown wrote:
> On Fri, Sep 29, 2017 at 04:23:01PM +0800, Chen-Yu Tsai wrote:
> > This patch adds a macro regmap_field_read_poll_timeout that works
> > similar to the readx_poll_timeout defined in linux/iopoll.h, except
> > that this can also return the error value returned by a failed
> > regmap_field_read.
> 
> The following changes since commit 2bd6bf03f4c1c59381d62c61d03f6cc3fe71f66e:
> 
>   Linux 4.14-rc1 (2017-09-16 15:47:51 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git 
> tags/regmap-poll-field
> 
> for you to fetch changes up to 667063acb81931e2f8fd0cb91df9fcccad131d9a:
> 
>   regmap: add iopoll-like polling macro for regmap_field (2017-10-04 11:46:32 
> +0100)

Pulled into drm-misc-next for 4.15 since Maxime needs that for the sun4i
changes.

Thanks, Daniel

> 
> 
> regmap: Add field polling macro
> 
> 
> Chen-Yu Tsai (1):
>   regmap: add iopoll-like polling macro for regmap_field
> 
>  include/linux/regmap.h | 39 +++
>  1 file changed, 39 insertions(+)



> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2] drm/sun4i: Implement drm_driver lastclose to restore fbdev console

[Daniel Vetter]

On Mon, Jul 10, 2017 at 8:44 AM, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Sun, Jul 09, 2017 at 11:11:07PM +0800, Chen-Yu Tsai wrote:
>> On Sun, Jul 9, 2017 at 3:59 PM, Jonathan Liu <net...@gmail.com> wrote:
>> > The drm_driver lastclose callback is called when the last userspace
>> > DRM client has closed. Call drm_fbdev_cma_restore_mode to restore
>> > the fbdev console otherwise the fbdev console will stop working.
>> >
>> > Signed-off-by: Jonathan Liu <net...@gmail.com>
>>
>> This should have
>>
>> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
>>
>> Otherwise,
>>
>> Reviewed-by: Chen-Yu Tsai <w...@csie.org>
>
> And it should be sent to stable too.
>
> Can you resend it?

Fyi but patchwork auto-adds these tags. Feels a bit like overkill
asking for a resend for something you can trivially add yourself ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Daniel Vetter]

On Tue, Oct 25, 2016 at 08:44:22AM +0200, Daniel Vetter wrote:
> On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> > +static int de2_drm_bind(struct device *dev)
> > +{
> > +   struct drm_device *drm;
> > +   struct priv *priv;
> > +   int ret;
> > +
> > +   drm = drm_dev_alloc(_drm_driver, dev);

Oh and you might want to look into drm_dev_init, allows you to use
subclassing instead of pointer chasing for your driver-private data.
-Daniel

> > +   if (!drm)
> > +   return -ENOMEM;
> > +
> > +   priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +   if (!priv) {
> > +   dev_err(dev, "failed to allocate private area\n");
> > +   ret = -ENOMEM;
> > +   goto out1;
> > +   }
> > +
> > +   dev_set_drvdata(dev, drm);
> > +   drm->dev_private = priv;
> > +
> > +   drm_mode_config_init(drm);
> > +   drm->mode_config.min_width = 32;/* needed for cursor */
> > +   drm->mode_config.min_height = 32;
> > +   drm->mode_config.max_width = 1920;
> > +   drm->mode_config.max_height = 1080;
> > +   drm->mode_config.funcs = _mode_config_funcs;
> > +
> > +   drm->irq_enabled = true;
> > +
> > +   /* initialize the display engine */
> > +   priv->soc_type = (int) of_match_device(de2_drm_of_match, dev)->data;
> > +   ret = de2_de_init(priv, dev);
> > +   if (ret)
> > +   goto out2;
> > +
> > +   /* start the subdevices */
> > +   ret = component_bind_all(dev, drm);
> > +   if (ret < 0)
> > +       goto out2;
> > +
> > +   ret = drm_dev_register(drm, 0);
> 
> This needs to be the very last step in your driver load sequence.
> Kerneldoc explains why. Similar, but inverted for unloading:
> drm_dev_unregister is the very first thing you must call.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Daniel Vetter]

On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> +static int de2_drm_bind(struct device *dev)
> +{
> + struct drm_device *drm;
> + struct priv *priv;
> + int ret;
> +
> + drm = drm_dev_alloc(_drm_driver, dev);
> + if (!drm)
> + return -ENOMEM;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + dev_err(dev, "failed to allocate private area\n");
> + ret = -ENOMEM;
> + goto out1;
> + }
> +
> + dev_set_drvdata(dev, drm);
> + drm->dev_private = priv;
> +
> + drm_mode_config_init(drm);
> + drm->mode_config.min_width = 32;/* needed for cursor */
> + drm->mode_config.min_height = 32;
> + drm->mode_config.max_width = 1920;
> + drm->mode_config.max_height = 1080;
> + drm->mode_config.funcs = _mode_config_funcs;
> +
> + drm->irq_enabled = true;
> +
> + /* initialize the display engine */
> + priv->soc_type = (int) of_match_device(de2_drm_of_match, dev)->data;
> + ret = de2_de_init(priv, dev);
> + if (ret)
> + goto out2;
> +
> + /* start the subdevices */
> + ret = component_bind_all(dev, drm);
> + if (ret < 0)
> + goto out2;
> +
> + ret = drm_dev_register(drm, 0);

This needs to be the very last step in your driver load sequence.
Kerneldoc explains why. Similar, but inverted for unloading:
drm_dev_unregister is the very first thing you must call.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2 13/26] drm/fb_cma_helper: Remove implicit call to disable_unused_functions

[Daniel Vetter]

On Mon, Jan 25, 2016 at 12:19:27AM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Friday 15 January 2016 11:17:30 Daniel Vetter wrote:
> > On Fri, Jan 15, 2016 at 01:13:05AM +0200, Laurent Pinchart wrote:
> > > On Thursday 14 January 2016 16:24:56 Maxime Ripard wrote:
> > >> The drm_fbdev_cma_init function always calls the
> > >> drm_helper_disable_unused_functions. Since it's part of the usual probe
> > >> process, all the drivers using that helper will end up having their
> > >> encoder and CRTC disable functions called at probe if their device has
> > >> not been reported as enabled.
> > >> 
> > >> This could be fixed by reading out from the registers the current state
> > >> of the device if it is enabled, but even that will not handle the case
> > >> where the device is actually disabled.
> > >> 
> > >> Moreover, the drivers using the atomic modesetting expect that their
> > >> enable and disable callback to be called when the device is already
> > >> enabled or disabled (respectively).
> > >> 
> > >> We can however fix this issue by moving the call to
> > >> drm_helper_disable_unused_functions out of drm_fbdev_cma_init and make
> > >> the drivers needing it (all the drivers calling drm_fbdev_cma_init and
> > >> not using the atomic modesetting) explicitly call it.
> > > 
> > > I'd rather add it to all drivers that call drm_fbdev_cma_init(). All the
> > > atomic ones must have special code to cope with it that could be rendered
> > > useless by the removal of the drm_helper_disable_unused_functions() call.
> > > That code should be removed too, and it would be easier to check drivers
> > > one by one and fixing them individually (outside of this patch series,
> > > unless you insist ;-)) when removing the explicit
> > > drm_helper_disable_unused_functions() call.
> >
> > I had the same thought, but figured there's really no good reason ever to
> > do this. I suspect most of that disable_unused_function stuff we have all
> > over is supreme cargo-cult anyway ;-)
> 
> I'm not sure you got my point. Yes, the drm_helper_disable_unused_functions() 
> call should be removed from atomic drivers, but that will leave support code 
> added for the sole reason of avoiding the crash in the drivers. That code 
> will 
> likely not be noticed and will stay there rotting. If we added 
> drm_helper_disable_unused_functions() calls to all atomic drivers then driver 
> authors will hopefully check carefully if there's any support code that can 
> be 
> removed when removing the function call. It would act as a kind of FIXME 
> reminder.

drm_helper_disable_unused_functions() already prefers the ->disable
callbacks over dpms, like atomic helpers. I don't think there's any dead
code due to this change. The problem was that driver/hw blew up since it
was disabled when the hw was off already.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2 13/26] drm/fb_cma_helper: Remove implicit call to disable_unused_functions

[Daniel Vetter]

On Fri, Jan 15, 2016 at 01:13:05AM +0200, Laurent Pinchart wrote:
> Hi Maxime,
> 
> Thank you for the patch.
> 
> On Thursday 14 January 2016 16:24:56 Maxime Ripard wrote:
> > The drm_fbdev_cma_init function always calls the
> > drm_helper_disable_unused_functions. Since it's part of the usual probe
> > process, all the drivers using that helper will end up having their encoder
> > and CRTC disable functions called at probe if their device has not been
> > reported as enabled.
> > 
> > This could be fixed by reading out from the registers the current state of
> > the device if it is enabled, but even that will not handle the case where
> > the device is actually disabled.
> > 
> > Moreover, the drivers using the atomic modesetting expect that their enable
> > and disable callback to be called when the device is already enabled or
> > disabled (respectively).
> > 
> > We can however fix this issue by moving the call to
> > drm_helper_disable_unused_functions out of drm_fbdev_cma_init and make the
> > drivers needing it (all the drivers calling drm_fbdev_cma_init and not
> > using the atomic modesetting) explicitly call it.
> 
> I'd rather add it to all drivers that call drm_fbdev_cma_init(). All the 
> atomic ones must have special code to cope with it that could be rendered 
> useless by the removal of the drm_helper_disable_unused_functions() call. 
> That 
> code should be removed too, and it would be easier to check drivers one by 
> one 
> and fixing them individually (outside of this patch series, unless you insist 
> ;-)) when removing the explicit drm_helper_disable_unused_functions() call. 

I had the same thought, but figured there's really no good reason ever to
do this. I suspect most of that disable_unused_function stuff we have all
over is supreme cargo-cult anyway ;-)

> Other than that the patch looks fine to me.

So went ahead and applied to drm-misc.
-Daniel

> 
> > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> > ---
> >  drivers/gpu/drm/drm_fb_cma_helper.c | 3 ---
> >  drivers/gpu/drm/imx/imx-drm-core.c  | 1 +
> >  drivers/gpu/drm/sti/sti_drv.c   | 1 +
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 1 +
> >  4 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c index c19a62561183..daa98d881142
> > 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -348,9 +348,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct
> > drm_device *dev,
> > 
> > }
> > 
> > -   /* disable all the possible outputs/crtcs before entering KMS mode */
> > -   drm_helper_disable_unused_functions(dev);
> > -
> > ret = drm_fb_helper_initial_config(helper, preferred_bpp);
> > if (ret < 0) {
> > dev_err(dev->dev, "Failed to set initial hw configuration.\n");
> > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
> > b/drivers/gpu/drm/imx/imx-drm-core.c index 7b990b4e96d2..e1db57791fc9
> > 100644
> > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > @@ -312,6 +312,7 @@ static int imx_drm_driver_load(struct drm_device *drm,
> > unsigned long flags) dev_warn(drm->dev, "Invalid legacyfb_depth. 
> > Defaulting to 16bpp\n"); legacyfb_depth = 16;
> > }
> > +   drm_helper_disable_unused_functions(drm);
> > imxdrm->fbhelper = drm_fbdev_cma_init(drm, legacyfb_depth,
> > drm->mode_config.num_crtc, MAX_CRTC);
> > if (IS_ERR(imxdrm->fbhelper)) {
> > diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
> > index 1469987949d8..506b5626f3ed 100644
> > --- a/drivers/gpu/drm/sti/sti_drv.c
> > +++ b/drivers/gpu/drm/sti/sti_drv.c
> > @@ -160,6 +160,7 @@ static int sti_load(struct drm_device *dev, unsigned
> > long flags)
> > 
> > drm_mode_config_reset(dev);
> > 
> > +   drm_helper_disable_unused_functions(dev);
> > drm_fbdev_cma_init(dev, 32,
> >dev->mode_config.num_crtc,
> >dev->mode_config.num_connector);
> > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> > b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 876cad58b1f9..24be31d69701
> > 100644
> > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> > @@ -294,6 +294,7 @@ static int tilcdc_load(struct drm_device *dev, unsigned
> > long flags) break;
> > }
> &g

[linux-sunxi] Re: [PATCH 08/19] drm: Add Allwinner A10 Display Engine support

[Daniel Vetter]

On Wed, Nov 11, 2015 at 02:14:12PM -0800, Maxime Ripard wrote:
> Hi Daniel,
> 
> Thanks for your review!

Back from vacation, sorry for the delay.

> 
> On Fri, Oct 30, 2015 at 03:44:09PM +0100, Daniel Vetter wrote:
> > > +static void sun4i_crtc_disable(struct drm_crtc *crtc)
> > > +{
> > > + struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc);
> > > + struct sun4i_drv *drv = scrtc->drv;
> > > +
> > > + DRM_DEBUG_DRIVER("Disabling the CRTC\n");
> > > +
> > > + if (!scrtc->enabled)
> > > + return;
> > 
> > Please don't do this - the atomic state should always reflect the true hw
> > state (fix that up with either hw state readout or reset in the ->reset
> > callbacks), and the atomic helpers guarantee that they'll never call you
> > when not needed. If you don't trust them do a WARN_ON at least, but no
> > early silent returns.
> > 
> > Personally I'd just rip it out, it's too much trouble. And for debugging
> > the atomic helpers already trace it all (or at least should).
> 
> Ok. Is there a preferred way to do HW state readout, or should I just
> add that to my probe?

There's no preferred way yet (i.e. something supported in a standard
fashion through helpers). I'd place it in your various ->reset hooks
though. The idea from atomic helpers is that ->reset should make sure that
atomic state structures (i.e. drm_crtc/connector/plane_state or your
driver-specific subclasses of them) should match with the hw state. Either
by resetting the hw or by reading out the hw state inte freshly-allocated
structures.

> > > +static int sun4i_drv_connector_plug_all(struct drm_device *drm)
> > 
> > Laurent Pinchart has this as a rfc patch for drm core, please coordinate
> > with him and rebase on top of his patches.
> 
> Ack, I will.
> 
> > > +static int sun4i_drv_enable_vblank(struct drm_device *drm, int pipe)
> > > +{
> > > + struct sun4i_drv *drv = drm->dev_private;
> > > + struct sun4i_tcon *tcon = drv->tcon;
> > > +
> > > + DRM_DEBUG_DRIVER("Enabling VBLANK on pipe %d\n", pipe);
> > > +
> > > + sun4i_tcon_enable_vblank(tcon, true);
> > > +
> > > + return 0;
> > 
> > atomic helpers rely on enable_vblank failing correctly when the pipe is
> > off and vlbanks will never happen. You probably need a correct error code
> > here that checks crtc->state->active (well not that directly but something
> > derived from it, since the pointer chase would be racy).
> 
> Ok.
> 
> > I know it's a bit a mess since we don't have kms-native vblank driver
> > hooks yet and really the drm core should get this right for you. It'll
> > happen eventually, but drm_irq.c is a bit moldy ;-)
> 
> I can't really use drm_irq anyway, since we don't have a single
> interrupt for the VBLANK, but two, that we have to use depending on
> the output.

drm_irq has 2 parts: irq handling, which only supports 1 hw irq line, and
which is totally optional.

2nd part is the vblank support (all the drm_vblank and drm_crtc_vblank_)
functions, which are completely agnostic to how exactly your hw signals a
vblank for a given output. Those are mandatory if you want to support
vblanks (and really, you want to support vblanks). So whether you have 2
hw irq lines or 1 hw irq line with some additional status register doesn't
matter, since the hw irq -> logical drm_crtc mapping for vblank events is
done in the driver.

Yes I know we should split up the various bits in drm_irq, it's on my todo
;-)

> > > +static void sun4i_drv_disable_vblank(struct drm_device *drm, int pipe)
> > > +{
> > > + struct sun4i_drv *drv = drm->dev_private;
> > > + struct sun4i_tcon *tcon = drv->tcon;
> > > +
> > > + DRM_DEBUG_DRIVER("Disabling VBLANK on pipe %d\n", pipe);
> > > +
> > > + sun4i_tcon_enable_vblank(tcon, false);
> > > +}
> > > +
> > > +static int sun4i_drv_load(struct drm_device *drm, unsigned long flags)
> > 
> > load/unload callbacks are depracated since fundamentally racy, and we
> > can't fix that due to the pile of legacy dri1 drivers. Please use
> > drm_dev_alloc/register/unregister/unref functions instead, with the
> > load/unload code placed in between to avoid races with userspace seeing
> > the device/driver (e.g. in sysfs) while it's in a partially defunct state.
> > 
> > Relevant kerneldoc has the details, at least in linux-next.
> 
> Ok.
> 
> > > +static void sun4i_backend_layer_atomic_update(struct drm_plane *plane,
> > > +

[linux-sunxi] Re: [PATCH 00/19] drm: Add Allwinner A10 display engine support

[Daniel Vetter]

>   clk: sunxi: Add Allwinner R8 AHB gates support
>   drm/panel: simple: Add timings for the Olimex LCD-OLinuXino-4.3TS
>   drm: Add Allwinner A10 Display Engine support
>   drm: sun4i: Add DT bindings documentation
>   drm: sun4i: Add RGB output
>   drm: sun4i: Add composite output
>   drm: sun4i: tv: Add PAL output standard
>   drm: sun4i: tv: Add NTSC output standard
>   ARM: sun5i: dt: Add pll3 and pll7 clocks
>   ARM: sun5i: dt: Add display and TCON clocks
>   ARM: sun5i: dt: Add DRAM gates
>   ARM: sun5i: dt: Add display blocks to the DTSI
>   ARM: sun5i: r8: Add AHB gates to the DTSI
>   ARM: sun5i: chip: Enable the TV Encoder
> 
>  .../devicetree/bindings/drm/sunxi/sun4i-drm.txt| 122 
>  arch/arm/boot/dts/sun5i-a10s.dtsi  |   9 +-
>  arch/arm/boot/dts/sun5i-a13.dtsi   |   3 +-
>  arch/arm/boot/dts/sun5i-r8.dtsi|  46 +-
>  arch/arm/boot/dts/sun5i.dtsi   | 137 +
>  drivers/clk/sunxi/Makefile |   4 +
>  drivers/clk/sunxi/clk-simple-gates.c   |   4 +
>  drivers/clk/sunxi/clk-sun4i-display.c  | 199 +++
>  drivers/clk/sunxi/clk-sun4i-pll3.c |  84 +++
>  drivers/clk/sunxi/clk-sun4i-tcon-ch0.c | 173 ++
>  drivers/clk/sunxi/clk-sun4i-tcon-ch1.c | 167 ++
>  drivers/gpu/drm/Kconfig|   2 +
>  drivers/gpu/drm/Makefile   |   3 +-
>  drivers/gpu/drm/panel/panel-simple.c   |  26 +
>  drivers/gpu/drm/sun4i/Kconfig  |  14 +
>  drivers/gpu/drm/sun4i/Makefile |  11 +
>  drivers/gpu/drm/sun4i/sun4i_backend.c  | 271 +
>  drivers/gpu/drm/sun4i/sun4i_backend.h  | 159 ++
>  drivers/gpu/drm/sun4i/sun4i_crtc.c | 117 
>  drivers/gpu/drm/sun4i/sun4i_crtc.h |  31 ++
>  drivers/gpu/drm/sun4i/sun4i_drv.c  | 296 ++
>  drivers/gpu/drm/sun4i/sun4i_drv.h  |  30 +
>  drivers/gpu/drm/sun4i/sun4i_framebuffer.c  |  54 ++
>  drivers/gpu/drm/sun4i/sun4i_framebuffer.h  |  19 +
>  drivers/gpu/drm/sun4i/sun4i_layer.c| 111 
>  drivers/gpu/drm/sun4i/sun4i_layer.h|  30 +
>  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 243 
>  drivers/gpu/drm/sun4i/sun4i_rgb.h  |  18 +
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 478 
>  drivers/gpu/drm/sun4i/sun4i_tcon.h | 182 ++
>  drivers/gpu/drm/sun4i/sun4i_tv.c   | 619 
> +
>  drivers/gpu/drm/sun4i/sun4i_tv.h   |  18 +
>  32 files changed, 3673 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/drm/sunxi/sun4i-drm.txt
>  create mode 100644 drivers/clk/sunxi/clk-sun4i-display.c
>  create mode 100644 drivers/clk/sunxi/clk-sun4i-pll3.c
>  create mode 100644 drivers/clk/sunxi/clk-sun4i-tcon-ch0.c
>  create mode 100644 drivers/clk/sunxi/clk-sun4i-tcon-ch1.c
>  create mode 100644 drivers/gpu/drm/sun4i/Kconfig
>  create mode 100644 drivers/gpu/drm/sun4i/Makefile
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_backend.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_backend.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_crtc.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_crtc.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_drv.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_drv.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_framebuffer.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_framebuffer.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_layer.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_layer.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_rgb.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_rgb.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_tcon.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_tcon.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_tv.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_tv.h
> 
> -- 
> 2.6.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH 08/19] drm: Add Allwinner A10 Display Engine support

[Daniel Vetter]

On Fri, Oct 30, 2015 at 03:20:54PM +0100, Maxime Ripard wrote:
> The Allwinner A10 and subsequent SoCs share the same display pipeline, with
> variations in the number of controllers (1 or 2), or the presence or not of
> some output (HDMI, TV, VGA) or not.
> 
> This hardware supports 4 layers and 32 sprites, even though we only support
> one primary layer for now.
> 
> Signed-off-by: Maxime Ripard 

Quickly (well, very quickly because jetlag and between travels) read
through this. Looks good overall, bunch of comments below.

Cheers, Daniel

> ---
>  drivers/gpu/drm/Kconfig   |   2 +
>  drivers/gpu/drm/Makefile  |   3 +-
>  drivers/gpu/drm/sun4i/Kconfig |  14 +
>  drivers/gpu/drm/sun4i/Makefile|   8 +
>  drivers/gpu/drm/sun4i/sun4i_backend.c | 271 +
>  drivers/gpu/drm/sun4i/sun4i_backend.h | 159 ++
>  drivers/gpu/drm/sun4i/sun4i_crtc.c| 117 
>  drivers/gpu/drm/sun4i/sun4i_crtc.h|  31 ++
>  drivers/gpu/drm/sun4i/sun4i_drv.c | 281 ++
>  drivers/gpu/drm/sun4i/sun4i_drv.h |  30 ++
>  drivers/gpu/drm/sun4i/sun4i_framebuffer.c |  54 
>  drivers/gpu/drm/sun4i/sun4i_framebuffer.h |  19 ++
>  drivers/gpu/drm/sun4i/sun4i_layer.c   | 111 +++
>  drivers/gpu/drm/sun4i/sun4i_layer.h   |  30 ++
>  drivers/gpu/drm/sun4i/sun4i_tcon.c| 478 
> ++
>  drivers/gpu/drm/sun4i/sun4i_tcon.h| 182 
>  16 files changed, 1789 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/sun4i/Kconfig
>  create mode 100644 drivers/gpu/drm/sun4i/Makefile
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_backend.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_backend.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_crtc.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_crtc.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_drv.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_drv.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_framebuffer.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_framebuffer.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_layer.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_layer.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_tcon.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_tcon.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 1a0a8df2eed8..ecf93fafa33c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -239,6 +239,8 @@ source "drivers/gpu/drm/rcar-du/Kconfig"
>  
>  source "drivers/gpu/drm/shmobile/Kconfig"
>  
> +source "drivers/gpu/drm/sun4i/Kconfig"
> +
>  source "drivers/gpu/drm/omapdrm/Kconfig"
>  
>  source "drivers/gpu/drm/tilcdc/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 45e7719846b1..2e5f547db672 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -1,4 +1,4 @@
> -#
> +
>  # Makefile for the drm device driver.  This driver provides support for the
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>  
> @@ -58,6 +58,7 @@ obj-$(CONFIG_DRM_ARMADA) += armada/
>  obj-$(CONFIG_DRM_ATMEL_HLCDC)+= atmel-hlcdc/
>  obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
>  obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
> +obj-$(CONFIG_DRM_SUN4I) += sun4i/
>  obj-$(CONFIG_DRM_OMAP)   += omapdrm/
>  obj-y+= tilcdc/
>  obj-$(CONFIG_DRM_QXL) += qxl/
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> new file mode 100644
> index ..99510e64e91a
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -0,0 +1,14 @@
> +config DRM_SUN4I
> + tristate "DRM Support for Allwinner A10 Display Engine"
> + depends on DRM && ARM
> + depends on ARCH_SUNXI || COMPILE_TEST
> + select DRM_GEM_CMA_HELPER
> + select DRM_KMS_HELPER
> + select DRM_KMS_CMA_HELPER
> + select DRM_PANEL
> + select REGMAP_MMIO
> + select VIDEOMODE_HELPERS
> + help
> +   Choose this option if you have an Allwinner SoC with a
> +   Display Engine. If M is selected the module will be called
> +   sun4i-drm.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> new file mode 100644
> index ..bc2df12beb42
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -0,0 +1,8 @@
> +sun4i-drm-y += sun4i_backend.o
> +sun4i-drm-y += sun4i_crtc.o
> +sun4i-drm-y += sun4i_drv.o
> +sun4i-drm-y += sun4i_framebuffer.o
> +sun4i-drm-y += sun4i_layer.o
> +sun4i-drm-y += sun4i_tcon.o
> +
> +obj-$(CONFIG_DRM_SUN4I)  += sun4i-drm.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c 
> b/drivers/gpu/drm/sun4i/sun4i_backend.c
> new file mode 100644
> index ..74eac55f1244
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -0,0 +1,271 @@
> +/*
> + * Copyright (C) 2015 Free