Re: [Intel-gfx] [PATCH v5 08/13] drm/omapdrm: Use regular fbdev I/O helpers

[Tomi Valkeinen]

On 30/05/2023 18:02, Thomas Zimmermann wrote:

Use the regular fbdev helpers for framebuffer I/O instead of DRM's
helpers. Omapdrm does not use damage handling, so DRM's fbdev helpers
are mere wrappers around the fbdev code.

By using fbdev helpers directly within each DRM fbdev emulation,
we can eventually remove DRM's wrapper functions entirely.

v4:
* use initializer macros for struct fb_ops
v2:
* use FB_SYS_HELPERS option

Signed-off-by: Thomas Zimmermann 
Acked-by: Sam Ravnborg 
Cc: Tomi Valkeinen 
---
  drivers/gpu/drm/omapdrm/Kconfig  |  1 +
  drivers/gpu/drm/omapdrm/omap_fbdev.c | 11 +++
  2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig
index 76ded1568bd0..b4ac76c9f31b 100644
--- a/drivers/gpu/drm/omapdrm/Kconfig
+++ b/drivers/gpu/drm/omapdrm/Kconfig
@@ -4,6 +4,7 @@ config DRM_OMAP
depends on DRM && OF
depends on ARCH_OMAP2PLUS
select DRM_KMS_HELPER
+   select FB_SYS_HELPERS if DRM_FBDEV_EMULATION
select VIDEOMODE_HELPERS
select HDMI
default n
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c 
b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index b950e93b3846..b7ccce0704a3 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -4,6 +4,8 @@
   * Author: Rob Clark 
   */
  
+#include 

+
  #include 
  #include 
  #include 
@@ -95,20 +97,13 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
  
  static const struct fb_ops omap_fb_ops = {

.owner = THIS_MODULE,
-
+   FB_DEFAULT_SYS_OPS,
.fb_check_var   = drm_fb_helper_check_var,
.fb_set_par = drm_fb_helper_set_par,
.fb_setcmap = drm_fb_helper_setcmap,
.fb_blank   = drm_fb_helper_blank,
.fb_pan_display = omap_fbdev_pan_display,
.fb_ioctl   = drm_fb_helper_ioctl,
-
-   .fb_read = drm_fb_helper_sys_read,
-   .fb_write = drm_fb_helper_sys_write,
-   .fb_fillrect = drm_fb_helper_sys_fillrect,
-   .fb_copyarea = drm_fb_helper_sys_copyarea,
-   .fb_imageblit = drm_fb_helper_sys_imageblit,
-
.fb_destroy = omap_fbdev_fb_destroy,
  };
  


Reviewed-by: Tomi Valkeinen 

 Tomi

Re: [Intel-gfx] [PATCH 16/22] drm/tilcdc: Use drm_mode_copy()

[Tomi Valkeinen]

On 18/02/2022 12:03, Ville Syrjala wrote:

From: Ville Syrjälä 

struct drm_display_mode embeds a list head, so overwriting
the full struct with another one will corrupt the list
(if the destination mode is on a list). Use drm_mode_copy()
instead which explicitly preserves the list head of
the destination mode.

Even if we know the destination mode is not on any list
using drm_mode_copy() seems decent as it sets a good
example. Bad examples of not using it might eventually
get copied into code where preserving the list head
actually matters.

Obviously one case not covered here is when the mode
itself is embedded in a larger structure and the whole
structure is copied. But if we are careful when copying
into modes embedded in structures I think we can be a
little more reassured that bogus list heads haven't been
propagated in.

@is_mode_copy@
@@
drm_mode_copy(...)
{
...
}

@depends on !is_mode_copy@
struct drm_display_mode *mode;
expression E, S;
@@
(
- *mode = E
+ drm_mode_copy(mode, )
|
- memcpy(mode, E, S)
+ drm_mode_copy(mode, E)
)

@depends on !is_mode_copy@
struct drm_display_mode mode;
expression E;
@@
(
- mode = E
+ drm_mode_copy(, )
|
- memcpy(, E, S)
+ drm_mode_copy(, E)
)

@@
struct drm_display_mode *mode;
@@
- &*mode
+ mode

Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
Signed-off-by: Ville Syrjälä 
---
  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 29890d704cb4..853c6b443fff 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -433,7 +433,7 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
  
  	set_scanout(crtc, fb);
  
-	crtc->hwmode = crtc->state->adjusted_mode;

+   drm_mode_copy(>hwmode, >state->adjusted_mode);
  
  	tilcdc_crtc->hvtotal_us =

tilcdc_mode_hvtotal(>hwmode);


Reviewed-by: Tomi Valkeinen 

 Tomi

Re: [Intel-gfx] [PATCH 01/21] MAINTAINERS: Add entry for fbdev core

[Tomi Valkeinen]

On 31/01/2022 23:05, Daniel Vetter wrote:

Ever since Tomi extracted the core code in 2014 it's been defacto me
maintaining this, with help from others from dri-devel and sometimes
Linus (but those are mostly merge conflicts):

$ git shortlog -ns  drivers/video/fbdev/core/ | head -n5
 35  Daniel Vetter
 23  Linus Torvalds
 10  Hans de Goede
  9  Dave Airlie
  6  Peter Rosin

I think ideally we'd also record that the various firmware fb drivers
(efifb, vesafb, ...) are also maintained in drm-misc because for the
past few years the patches have either been to fix handover issues
with drm drivers, or caused handover issues with drm drivers. So any
other tree just doesn't make sense. But also, there's plenty of
outdated MAINTAINER entries for these with people and git trees that
haven't been active in years, so maybe let's just leave them alone.
And furthermore distros are now adopting simpledrm as the firmware fb
driver, so hopefully the need to care about the fbdev firmware drivers
will go down going forward.

Note that drm-misc is group maintained, I expect that to continue like
we've done before, so no new expectations that patches all go through
my hands. That would be silly. This also means I'm happy to put any
other volunteer's name in the M: line, but otherwise git log says I'm
the one who's stuck with this.


Acked-by: Tomi Valkeinen 

 Tomi

Re: [Intel-gfx] [PATCH 06/11] drm/: drm_gem_plane_helper_prepare_fb is now the default

[Tomi Valkeinen]

On 21/05/2021 12:09, Daniel Vetter wrote:

No need to set it explicitly.

Signed-off-by: Daniel Vetter 
Cc: Laurentiu Palcu 
Cc: Lucas Stach 
Cc: Shawn Guo 
Cc: Sascha Hauer 
Cc: Pengutronix Kernel Team 
Cc: Fabio Estevam 
Cc: NXP Linux Team 
Cc: Philipp Zabel 
Cc: Paul Cercueil 
Cc: Chun-Kuang Hu 
Cc: Matthias Brugger 
Cc: Neil Armstrong 
Cc: Kevin Hilman 
Cc: Jerome Brunet 
Cc: Martin Blumenstingl 
Cc: Marek Vasut 
Cc: Stefan Agner 
Cc: Sandy Huang 
Cc: "Heiko Stübner" 
Cc: Yannick Fertre 
Cc: Philippe Cornu 
Cc: Benjamin Gaignard 
Cc: Maxime Coquelin 
Cc: Alexandre Torgue 
Cc: Maxime Ripard 
Cc: Chen-Yu Tsai 
Cc: Jernej Skrabec 
Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-m...@vger.kernel.org
Cc: linux-media...@lists.infradead.org
Cc: linux-amlo...@lists.infradead.org
Cc: linux-rockc...@lists.infradead.org
Cc: linux-st...@st-md-mailman.stormreply.com
Cc: linux-su...@lists.linux.dev
---
  drivers/gpu/drm/imx/dcss/dcss-plane.c   | 1 -
  drivers/gpu/drm/imx/ipuv3-plane.c   | 1 -
  drivers/gpu/drm/ingenic/ingenic-drm-drv.c   | 1 -
  drivers/gpu/drm/ingenic/ingenic-ipu.c   | 1 -
  drivers/gpu/drm/mediatek/mtk_drm_plane.c| 1 -
  drivers/gpu/drm/meson/meson_overlay.c   | 1 -
  drivers/gpu/drm/meson/meson_plane.c | 1 -
  drivers/gpu/drm/mxsfb/mxsfb_kms.c   | 2 --
  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 1 -
  drivers/gpu/drm/stm/ltdc.c  | 1 -
  drivers/gpu/drm/sun4i/sun4i_layer.c | 1 -
  drivers/gpu/drm/sun4i/sun8i_ui_layer.c  | 1 -
  drivers/gpu/drm/sun4i/sun8i_vi_layer.c  | 1 -
  drivers/gpu/drm/tidss/tidss_plane.c | 1 -
  14 files changed, 15 deletions(-)


For tidss:

Acked-by: Tomi Valkeinen 

 Tomi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 09/11] drm/omap: Follow implicit fencing in prepare_fb

[Tomi Valkeinen]

On 21/05/2021 12:09, Daniel Vetter wrote:

I guess no one ever tried running omap together with lima or panfrost,
not even sure that's possible. Anyway for consistency, fix this.

Signed-off-by: Daniel Vetter 
Cc: Tomi Valkeinen 
---
  drivers/gpu/drm/omapdrm/omap_plane.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c 
b/drivers/gpu/drm/omapdrm/omap_plane.c
index 801da917507d..512af976b7e9 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -6,6 +6,7 @@
  
  #include 

  #include 
+#include 
  #include 
  
  #include "omap_dmm_tiler.h"

@@ -29,6 +30,8 @@ static int omap_plane_prepare_fb(struct drm_plane *plane,
if (!new_state->fb)
return 0;
  
+	drm_gem_plane_helper_prepare_fb(plane, new_state);

+
return omap_framebuffer_pin(new_state->fb);
  }
  



Reviewed-by: Tomi Valkeinen 

 Tomi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] linux-next: manual merge of the drm tree with the drm-misc-fixes tree

[Tomi Valkeinen]

On 18/03/2021 03:02, Stephen Rothwell wrote:

Hi all,

Today's linux-next merge of the drm tree got a conflict in:

   drivers/gpu/drm/omapdrm/dss/dsi.c

between commit:

   690911544275 ("drm/omap: dsi: fix unsigned expression compared with zero")

from the drm-misc-fixes tree and commit:

   bbd13d6a7b2e ("drm/omap: dsi: fix unreachable code in dsi_vc_send_short()")

from the drm tree.

I fixed it up (these do basically the same thing, so I used the former
version) and can carry the fix as necessary. This is now fixed as far as
linux-next is concerned, but any non trivial conflicts should be mentioned
to your upstream maintainer when your tree is submitted for merging.
You may also want to consider cooperating with the maintainer of the
conflicting tree to minimise any particularly complex conflicts.
Yes, I messed that up. I accidentally merged a fix to drm-misc-fixes, 
but almost similar fix was already in drm-misc-next. Sorry about that.


 Tomi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] fbcon: Disable accelerated scrolling

[Tomi Valkeinen]

On 29/10/2020 15:22, Daniel Vetter wrote:
> So ever since syzbot discovered fbcon, we have solid proof that it's
> full of bugs. And often the solution is to just delete code and remove
> features, e.g.  50145474f6ef ("fbcon: remove soft scrollback code").
> 
> Now the problem is that most modern-ish drivers really only treat
> fbcon as an dumb kernel console until userspace takes over, and Oops
> printer for some emergencies. Looking at drm drivers and the basic
> vesa/efi fbdev drivers shows that only 3 drivers support any kind of
> acceleration:
> 
> - nouveau, seems to be enabled by default
> - omapdrm, when a DMM remapper exists using remapper rewriting for
>   y/xpanning
> - gma500, but that is getting deleted now for the GTT remapper trick,
>   and the accelerated copyarea never set the FBINFO_HWACCEL_COPYAREA
>   flag, so unused (and could be deleted already I think).
> 
> No other driver supportes accelerated fbcon. And fbcon is the only
> user of this accel code (it's not exposed as uapi through ioctls),
> which means we could garbage collect fairly enormous amounts of code
> if we kill this.
> 
> Plus because syzbot only runs on virtual hardware, and none of the
> drivers for that have acceleration, we'd remove a huge gap in testing.
> And there's no other even remotely comprehensive testing aside from
> syzbot.
> 
> This patch here just disables the acceleration code by always
> redrawing when scrolling. The plan is that once this has been merged
> for well over a year in released kernels, we can start to go around
> and delete a lot of code.
> 
> v2:
> - Drop a few more unused local variables, somehow I missed the
> compiler warnings (Sam)
> - Fix typo in comment (Jiri)
> - add a todo entry for the cleanup (Thomas)
> 
> v3: Remove more unused variables (0day)
> 
> Reviewed-by: Thomas Zimmermann 
> Reviewed-by: Greg Kroah-Hartman 
> Acked-by: Sam Ravnborg 
> Cc: Jiri Slaby 
> Cc: Bartlomiej Zolnierkiewicz 
> Cc: Greg Kroah-Hartman 
> Cc: Linus Torvalds 
> Cc: Ben Skeggs 
> Cc: nouv...@lists.freedesktop.org
> Cc: Tomi Valkeinen 
> Cc: Daniel Vetter 
> Cc: Jiri Slaby 
> Cc: "Gustavo A. R. Silva" 
> Cc: Tetsuo Handa 
> Cc: Peilin Ye 
> Cc: George Kennedy 
> Cc: Nathan Chancellor 
> Cc: Peter Rosin 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/gpu/todo.rst   | 18 +
>  drivers/video/fbdev/core/fbcon.c | 45 ++--
>  2 files changed, 26 insertions(+), 37 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 6b224ef14455..bec99341a904 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -277,6 +277,24 @@ Contact: Daniel Vetter, Noralf Tronnes
>  
>  Level: Advanced
>  
> +Garbage collect fbdev scrolling acceleration
> +
> +
> +Scroll acceleration is disabled in fbcon by hard-wiring p->scrollmode =
> +SCROLL_REDRAW. There's a ton of code this will allow us to remove:
> +- lots of code in fbcon.c
> +- a bunch of the hooks in fbcon_ops, maybe the remaining hooks could be 
> called
> +  directly instead of the function table (with a switch on p->rotate)
> +- fb_copyarea is unused after this, and can be deleted from all drivers
> +
> +Note that not all acceleration code can be deleted, since clearing and cursor
> +support is still accelerated, which might be good candidates for further
> +deletion projects.

Apparently omapdrm's accelerated panning has been broken for some time, and no 
one has noticed. It does:

strcmp(fbi->fix.id, MODULE_NAME), which is a comparison of omapdrmdrmfb == 
omapdrm and always fails.

Fixing that, and applying this patch, things work fine (unaccelerated, of 
course). I did notice a
single call to omap_fbdev_pan_display() when loading the drivers. This comes 
from fbcon_switch ->
bit_update_start -> fb_pan_display. Maybe this is from the clearing you mention 
above?

Reviewed-by: Tomi Valkeinen 

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 13/65] drm/omapdrm: Annotate dma-fence critical section in commit path

[Tomi Valkeinen]

On 23/10/2020 15:21, Daniel Vetter wrote:
> Nothing special, just put the end right after hw_done(). Note that in
> one path there's a wait for the flip/update to complete. But as far as
> I understand from comments and code that's only relevant for modesets,
> and skipped if there wasn't a modeset done on a given crtc.
> 
> For a bit more clarity pull the hw_done() call out of the if/else,
> that way it's a bit clearer flow. But happy to shuffle this around as
> is seen fit.
> 
> Signed-off-by: Daniel Vetter 
> Cc: Tomi Valkeinen 
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c 
> b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 2e598b8b72af..2b82a708eca6 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -68,6 +68,7 @@ static void omap_atomic_commit_tail(struct drm_atomic_state 
> *old_state)
>  {
>   struct drm_device *dev = old_state->dev;
>   struct omap_drm_private *priv = dev->dev_private;
> + bool fence_cookie = dma_fence_begin_signalling();
>  
>   priv->dispc_ops->runtime_get(priv->dispc);
>  
> @@ -90,8 +91,6 @@ static void omap_atomic_commit_tail(struct drm_atomic_state 
> *old_state)
>   omap_atomic_wait_for_completion(dev, old_state);
>  
>   drm_atomic_helper_commit_planes(dev, old_state, 0);
> -
> - drm_atomic_helper_commit_hw_done(old_state);
>   } else {
>   /*
>* OMAP3 DSS seems to have issues with the work-around above,
> @@ -101,10 +100,12 @@ static void omap_atomic_commit_tail(struct 
> drm_atomic_state *old_state)
>   drm_atomic_helper_commit_planes(dev, old_state, 0);
>  
>   drm_atomic_helper_commit_modeset_enables(dev, old_state);
> -
> - drm_atomic_helper_commit_hw_done(old_state);
>   }
>  
> + drm_atomic_helper_commit_hw_done(old_state);
> +
> + dma_fence_end_signalling(fence_cookie);
> +
>   /*
>* Wait for completion of the page flips to ensure that old buffers
>* can't be touched by the hardware anymore before cleaning up planes.
> 

Reviewed-by: Tomi Valkeinen 

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 10/20] drm/omapdrm: Introduce GEM object functions

[Tomi Valkeinen]

Hi,

On 13/08/2020 11:36, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in omapdrm.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c |  9 -
>  drivers/gpu/drm/omapdrm/omap_gem.c | 16 +++-
>  drivers/gpu/drm/omapdrm/omap_gem.h |  1 -
>  3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c 
> b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 53d5e184ee77..2e598b8b72af 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -521,12 +521,6 @@ static int dev_open(struct drm_device *dev, struct 
> drm_file *file)
>   return 0;
>  }
>  
> -static const struct vm_operations_struct omap_gem_vm_ops = {
> - .fault = omap_gem_fault,
> - .open = drm_gem_vm_open,
> - .close = drm_gem_vm_close,
> -};
> -
>  static const struct file_operations omapdriver_fops = {
>   .owner = THIS_MODULE,
>   .open = drm_open,
> @@ -549,10 +543,7 @@ static struct drm_driver omap_drm_driver = {
>  #endif
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_export = omap_gem_prime_export,
>   .gem_prime_import = omap_gem_prime_import,
> - .gem_free_object_unlocked = omap_gem_free_object,
> - .gem_vm_ops = _gem_vm_ops,
>   .dumb_create = omap_gem_dumb_create,
>   .dumb_map_offset = omap_gem_dumb_map_offset,
>   .ioctls = ioctls,
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c 
> b/drivers/gpu/drm/omapdrm/omap_gem.c
> index d0d12d5dd76c..d68dc63dea0a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -487,7 +487,7 @@ static vm_fault_t omap_gem_fault_2d(struct drm_gem_object 
> *obj,
>   * vma->vm_private_data points to the GEM object that is backing this
>   * mapping.
>   */
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf)
> +static vm_fault_t omap_gem_fault(struct vm_fault *vmf)
>  {
>   struct vm_area_struct *vma = vmf->vma;
>   struct drm_gem_object *obj = vma->vm_private_data;
> @@ -1169,6 +1169,18 @@ static bool omap_gem_validate_flags(struct drm_device 
> *dev, u32 flags)
>   return true;
>  }
>  
> +static const struct vm_operations_struct omap_gem_vm_ops = {
> + .fault = omap_gem_fault,
> + .open = drm_gem_vm_open,
> + .close = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs omap_gem_object_funcs = {
> + .free = omap_gem_free_object,
> + .export = omap_gem_prime_export,
> + .vm_ops = _gem_vm_ops,
> +};
> +
>  /* GEM buffer object constructor */
>  struct drm_gem_object *omap_gem_new(struct drm_device *dev,
>   union omap_gem_size gsize, u32 flags)
> @@ -1236,6 +1248,8 @@ struct drm_gem_object *omap_gem_new(struct drm_device 
> *dev,
>   size = PAGE_ALIGN(gsize.bytes);
>   }
>  
> + obj->funcs = _gem_object_funcs;
> +
>   /* Initialize the GEM object. */
>   if (!(flags & OMAP_BO_MEM_SHMEM)) {
>   drm_gem_private_object_init(dev, obj, size);
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.h 
> b/drivers/gpu/drm/omapdrm/omap_gem.h
> index 729b7812a815..9e6b5c8195d9 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.h
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.h
> @@ -69,7 +69,6 @@ struct dma_buf *omap_gem_prime_export(struct drm_gem_object 
> *obj, int flags);
>  struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>   struct dma_buf *buffer);
>  
> -vm_fault_t omap_gem_fault(struct vm_fault *vmf);
>  int omap_gem_roll(struct drm_gem_object *obj, u32 roll);
>  void omap_gem_cpu_sync_page(struct drm_gem_object *obj, int pgoff);
>  void omap_gem_dma_sync_buffer(struct drm_gem_object *obj,

omap_gem_free_object() can also be made static, and removed from omap_gem.h.

Tested on AM5 EVM.

Reviewed-by: Tomi Valkeinen 

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] linux-next: manual merge of the drm-misc tree with the drm-misc-fixes tree

[Tomi Valkeinen]

Hi Stephen,

On 23/04/2020 06:17, Stephen Rothwell wrote:
> Hi all,
> 
> On Tue, 21 Apr 2020 09:10:25 +0300 Tomi Valkeinen  
> wrote:
>>
>> On 21/04/2020 04:52, Stephen Rothwell wrote:
>>>
>>> Today's linux-next merge of the drm-misc tree got a conflict in:he drm-misc 
>>> tree with the drm-misc-fixes tree
>>>
>>> drivers/gpu/drm/tidss/tidss_encoder.c
>>>
>>> between commit:
>>>
>>> 9da67433f64e ("drm/tidss: fix crash related to accessing freed memory")
>>>
>>> from the drm-misc-fixes tree and commit:
>>>
>>> b28ad7deb2f2 ("drm/tidss: Use simple encoder")
>>>
>>> from the drm-misc tree.
>>>
>>> I fixed it up (I just used the latter version of this file) and can
>>
>> We need to use "drm/tidss: fix crash related to accessing freed memory" 
>> version.
>>
>>> carry the fix as necessary. This is now fixed as far as linux-next is
>>> concerned, but any non trivial conflicts should be mentioned to your
>>> upstream maintainer when your tree is submitted for merging.  You may
>>> also want to consider cooperating with the maintainer of the conflicting
>>> tree to minimise any particularly complex conflicts.
>>
>> I have fixed this with drm-misc's dim tool, so I presume the conflict goes 
>> away when drm-misc-fixes
>> is merged to drm-fixes, and drm-fixes is then at some point merged to 
>> drm-misc-next.
>>
>> It was a bit bad timing with the "drm/tidss: Use simple encoder", which 
>> removes the plumbing I
>> needed to implement the fix. So I effectively revert the "drm/tidss: Use 
>> simple encoder".
>>
>>Tomi
>>
> 
> This is now a conflict between the drm and drm-misc-fixes trees.

The fix ("drm/tidss: fix crash related to accessing freed memory") is in 
v5.7-rc3, and the conflicting change ("drm/tidss: Use simple encoder") in 
drm-next.

The conflict resolution in linux-next drops the fix and take the change from 
drm-next, which causes crash on module removal.

Here's the diff I made on top of linux-next to fix it. Essentially dropping the 
simple-encoder change, and reapplying the fix. This should be fixed in drm-next 
when Dave next time pulls in Linus' branch.

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c 
b/drivers/gpu/drm/tidss/tidss_encoder.c
index 4c0558286f5e..e624cdcbb567 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -56,25 +56,38 @@ static int tidss_encoder_atomic_check(struct drm_encoder 
*encoder,
return 0;
 }
 
+static void tidss_encoder_destroy(struct drm_encoder *encoder)
+{
+   drm_encoder_cleanup(encoder);
+   kfree(encoder);
+}
+
 static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
.atomic_check = tidss_encoder_atomic_check,
 };
 
+static const struct drm_encoder_funcs encoder_funcs = {
+   .destroy = tidss_encoder_destroy,
+};
+
 struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
 u32 encoder_type, u32 possible_crtcs)
 {
struct drm_encoder *enc;
int ret;
 
-   enc = devm_kzalloc(tidss->dev, sizeof(*enc), GFP_KERNEL);
+   enc = kzalloc(sizeof(*enc), GFP_KERNEL);
if (!enc)
return ERR_PTR(-ENOMEM);
 
enc->possible_crtcs = possible_crtcs;
 
-   ret = drm_simple_encoder_init(>ddev, enc, encoder_type);
-   if (ret < 0)
+   ret = drm_encoder_init(>ddev, enc, _funcs,
+  encoder_type, NULL);
+   if (ret < 0) {
+   kfree(enc);
return ERR_PTR(ret);
+   }
 
drm_encoder_helper_add(enc, _helper_funcs);
 


 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 25/59] drm/tidss: Delete tidss->saved_state

[Tomi Valkeinen]

On 15/04/2020 10:40, Daniel Vetter wrote:

Not used anymore since the switch to suspend/resume helpers.

Tested-by: Jyri Sarha 
Acked-by: Sam Ravnborg 
Signed-off-by: Daniel Vetter 
Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
---
  drivers/gpu/drm/tidss/tidss_drv.h | 2 --
  1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.h 
b/drivers/gpu/drm/tidss/tidss_drv.h
index b23cd95c8d78..3b0a3d87b7c4 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -29,8 +29,6 @@ struct tidss_device {
  
  	spinlock_t wait_lock;	/* protects the irq masks */

dispc_irq_t irq_mask;   /* enabled irqs in addition to wait_list */
-
-   struct drm_atomic_state *saved_state;
  };
  
  #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)




Reviewed-by: Tomi Valkeinen 

 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 23/59] drm/tidss: Use devm_drm_dev_alloc

[Tomi Valkeinen]

On 15/04/2020 10:39, Daniel Vetter wrote:

Already using devm_drm_dev_init, so very simple replacment.

Tested-by: Jyri Sarha 
Acked-by: Sam Ravnborg 
Signed-off-by: Daniel Vetter 
Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
---
  drivers/gpu/drm/tidss/tidss_drv.c | 15 ---
  1 file changed, 4 insertions(+), 11 deletions(-)


Reviewed-by: Tomi Valkeinen 

 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 24/59] drm/tidss: Don't use drm_device->dev_private

[Tomi Valkeinen]

On 15/04/2020 10:39, Daniel Vetter wrote:

Upcasting using a container_of macro is more typesafe, faster and
easier for the compiler to optimize.

Tested-by: Jyri Sarha 
Acked-by: Sam Ravnborg 
Signed-off-by: Daniel Vetter 
Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
---
  drivers/gpu/drm/tidss/tidss_crtc.c  | 16 
  drivers/gpu/drm/tidss/tidss_drv.c   |  2 --
  drivers/gpu/drm/tidss/tidss_drv.h   |  2 ++
  drivers/gpu/drm/tidss/tidss_irq.c   | 12 ++--
  drivers/gpu/drm/tidss/tidss_kms.c   |  2 +-
  drivers/gpu/drm/tidss/tidss_plane.c |  6 +++---
  6 files changed, 20 insertions(+), 20 deletions(-)


Reviewed-by: Tomi Valkeinen 

 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] linux-next: manual merge of the drm-misc tree with the drm-misc-fixes tree

[Tomi Valkeinen]

Hi,

On 21/04/2020 04:52, Stephen Rothwell wrote:

Hi all,

Today's linux-next merge of the drm-misc tree got a conflict in:he drm-misc 
tree with the drm-misc-fixes tree

   drivers/gpu/drm/tidss/tidss_encoder.c

between commit:

   9da67433f64e ("drm/tidss: fix crash related to accessing freed memory")

from the drm-misc-fixes tree and commit:

   b28ad7deb2f2 ("drm/tidss: Use simple encoder")

from the drm-misc tree.

I fixed it up (I just used the latter version of this file) and can


We need to use "drm/tidss: fix crash related to accessing freed memory" version.


carry the fix as necessary. This is now fixed as far as linux-next is
concerned, but any non trivial conflicts should be mentioned to your
upstream maintainer when your tree is submitted for merging.  You may
also want to consider cooperating with the maintainer of the conflicting
tree to minimise any particularly complex conflicts.


I have fixed this with drm-misc's dim tool, so I presume the conflict goes away when drm-misc-fixes 
is merged to drm-fixes, and drm-fixes is then at some point merged to drm-misc-next.


It was a bit bad timing with the "drm/tidss: Use simple encoder", which removes the plumbing I 
needed to implement the fix. So I effectively revert the "drm/tidss: Use simple encoder".


 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/15] drm/omapdrm: Drop dma_buf->k(un)map

[Tomi Valkeinen]

On 18/11/2019 12:35, Daniel Vetter wrote:
> No in-tree users left.
> 
> Note that this is one of the few (if only) implementations of dma-buf
> that provided a kmap, but not a vmap implemenation. Given that the
> only real user (in-tree at least) of kmap was tegra, and it's
> impossible to buy a chip with tegra host1x and ompadrm on the same
> SoC, there's no problem here.
> 
> Signed-off-by: Daniel Vetter 
> Cc: Tomi Valkeinen 
> ---
>   drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 21 -
>   1 file changed, 21 deletions(-)

We're using dma_buf_kmap in TI kernel, in some rpmsg code:

https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/drivers/rpmsg/rpmsg_rpc_dmabuf.c?h=ti-linux-4.19.y

I'm not familiar with the code, Cc'ing Suman.

In any case, if no one else needs dmabuf kmap, I would guess that TI doesn't 
need it either... Presuming that's the case, for the omapdrm part:

Acked-by: Tomi Valkeinen 

Also, don't downplay the 32bit kernels, there are still plenty of users for 
arm32, and will be for a long time =)

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-misc-next

[Tomi Valkeinen]

Hi,

On 18/10/2019 23:11, Sean Paul wrote:

On Fri, Oct 18, 2019 at 9:46 AM Tomi Valkeinen  wrote:


Hi Sean,

On 17/10/2019 22:26, Sean Paul wrote:


concern for those. The omap OMAP_BO_MEM_* changes though I don't think have
really reached non-TI eyes. There's no link in the commit message to a UAPI
implementation and the only reference I could find is libkmsxx which can set
them through the python bindings. Since this is TI-specific gunk in TI-specific
headers I'm inclined to let it pass, but I would have liked to have this
conversation upfront. I figured I'd call this out so you have final say.


There was some discussion about that a few years back when I initially
sent the patches, but now that I look, the discussion died before really
even starting.

This is what I said about userspace implementation:


Yes, unfortunately that is not going to happen. I don't see how this
could be used in a generic system like Weston or X. It's meant for very
specific use cases, where you know exactly the requirements of your
application and the capabilities of the whole system, and optimize based
on that.


Thanks for the context, Tomi.

Indeed it looks like the discussion died, but Laurent still brought up
the u/s requirement and the patch was effectively dead until Daniel or
Dave weighed in. I'd expect at least some outreach before pushing the
patch directly 2+ years later. Has anything changed since then?


There were new review rounds for the series this summer & autumn, but 
no, nothing else. I haven't specifically pinged anyone about the UAPI 
changes.


This series introduces three new flags to an already existing UAPI, and, 
for whatever reason, this didn't register to me as a new UAPI, even if 
Laurent asked about it some years back.


So, my mistake.

The flags are added in a single patch, so I can easily push a revert for 
that if this patch is not acceptable. The rest of the series is cleanup.



I know this feature is used by customers, but I don't have access to
their applications.


Unfortunately, and as you know, that is insufficient/irrelevant for
introducing new UAPI. So the question is if the libkmsxx bindings
constitute opensource userspace, it's really thin IMO.


Ok. Well, I know and understand the general rule here. But perhaps I 
haven't taken it as strictly as needed.


I have to say I don't quite understand why this rule would be always 
strictly held, though.


These flags, for example, what should we do with them?
- They provide a proven, real use-case benefit.
- They are specific to SoCs with OMAP DSS and DMM IPs.
- They are relatively simple.
- All the details, including the details about the HW, are public.
- Using them makes sense only in cases where you are tuning your system 
to your application, and you must know the resource needs of all the 
apps in your system. So they cannot really be used in any generic setup, 
or at least I have no idea how.


Does the history and experience say that such specific case as above 
cases don't really exist, and we can always have a generic UAPI (or, in 
worst case, a device specific UAPI) which can be used from X/Weston/or-such?


Or do things like above exist, but they need to carried in vendors' kernels?

 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-misc-next

[Tomi Valkeinen]

Hi Sean,

On 17/10/2019 22:26, Sean Paul wrote:


concern for those. The omap OMAP_BO_MEM_* changes though I don't think have
really reached non-TI eyes. There's no link in the commit message to a UAPI
implementation and the only reference I could find is libkmsxx which can set
them through the python bindings. Since this is TI-specific gunk in TI-specific
headers I'm inclined to let it pass, but I would have liked to have this
conversation upfront. I figured I'd call this out so you have final say.


There was some discussion about that a few years back when I initially 
sent the patches, but now that I look, the discussion died before really 
even starting.


This is what I said about userspace implementation:


Yes, unfortunately that is not going to happen. I don't see how this
could be used in a generic system like Weston or X. It's meant for very
specific use cases, where you know exactly the requirements of your
application and the capabilities of the whole system, and optimize based
on that.
I know this feature is used by customers, but I don't have access to 
their applications.


 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/omapdrm: Substitute format_is_yuv() with format->is_yuv

[Tomi Valkeinen]

Hi,

On 18/07/18 13:17, Ville Syrjälä wrote:
> On Tue, Jul 17, 2018 at 06:13:45PM +0100, Ayan Kumar Halder wrote:
>> drm_format_info table has a field 'is_yuv' to denote if the format
>> is yuv or not. The driver is expected to use this instead of
>> having a function for the same purpose.
>>
>> Signed-off-by: Ayan Kumar halder 
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dispc.c | 26 ++
>>  1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c 
>> b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> index 84f274c..8d2d7a4 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> @@ -1140,18 +1140,6 @@ static void dispc_ovl_set_color_mode(struct 
>> dispc_device *dispc,
>>  REG_FLD_MOD(dispc, DISPC_OVL_ATTRIBUTES(plane), m, 4, 1);
>>  }
>>  
>> -static bool format_is_yuv(u32 fourcc)
>> -{
>> -switch (fourcc) {
>> -case DRM_FORMAT_YUYV:
>> -case DRM_FORMAT_UYVY:
>> -case DRM_FORMAT_NV12:
>> -return true;
>> -default:
>> -return false;
>> -}
>> -}
>> -
>>  static void dispc_ovl_configure_burst_type(struct dispc_device *dispc,
>> enum omap_plane_id plane,
>> enum omap_dss_rotation_type rotation)
>> @@ -1910,11 +1898,14 @@ static void dispc_ovl_set_scaling_uv(struct 
>> dispc_device *dispc,
>>  int scale_x = out_width != orig_width;
>>  int scale_y = out_height != orig_height;
>>  bool chroma_upscale = plane != OMAP_DSS_WB;
>> +const struct drm_format_info *info;
>> +
>> +info = drm_format_info(fourcc);
> 
> Not sure Tomi wants drm usage (apart from the fourccs) inside the
> dss code.

Seems like I have missed this. No, I don't have anything against drm
usage inside dss. That's the way we've been moving after we managed to
get rid of omapfb links.

I'll pick this up.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/13] drm/omapdrm: Nuke omap_framebuffer_get_next_connector()

[Tomi Valkeinen]

On 09/04/18 11:41, Daniel Vetter wrote:
> On Fri, Apr 06, 2018 at 09:10:43AM +0300, Tomi Valkeinen wrote:
>> On 05/04/18 19:50, Daniel Vetter wrote:
>>> On Thu, Apr 05, 2018 at 06:13:58PM +0300, Ville Syrjala wrote:
>>>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>>>
>>>> omap_framebuffer_get_next_connector() uses plane->fb which we want to
>>>> deprecate for atomic drivers. As omap_framebuffer_get_next_connector()
>>>> is unused just nuke the entire function.
>>>>
>>>> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
>>>> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>>
>>> Yeah was slightly worried how to fix up this one, but we're lucky!
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
>>
>> I tried to remove it just a week ago, but Sebastian said that it's used
>> by a unmerged series about DSI command mode displays, so I dropped the
>> patch.
>>
>> In the unmerged series, it's used by omap_framebuffer_dirty() ([PATCHv3
>> 3/8] drm/omap: add support for manually updated displays). So we have a
>> framebuffer, and we want to know which crtcs need to be flushed.
> 
> You can't do that in atomic drivers.
> 
> You need to take appropriate locks and do the full ->state->fb deref.
> Also, there's a gazillion of copies for generic framebuffer_dirty
> implementations floating around, pleas try to coordinate with those.

Ok. In that case we need to refresh the manual update series to do
things differently. For this patch:

Reviewed-by: Tomi Valkeinen <tomi.valkei...@ti.com>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/13] drm/omapdrm: Nuke omap_framebuffer_get_next_connector()

[Tomi Valkeinen]

On 05/04/18 19:50, Daniel Vetter wrote:
> On Thu, Apr 05, 2018 at 06:13:58PM +0300, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>
>> omap_framebuffer_get_next_connector() uses plane->fb which we want to
>> deprecate for atomic drivers. As omap_framebuffer_get_next_connector()
>> is unused just nuke the entire function.
>>
>> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
>> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> 
> Yeah was slightly worried how to fix up this one, but we're lucky!
> 
> Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>

I tried to remove it just a week ago, but Sebastian said that it's used
by a unmerged series about DSI command mode displays, so I dropped the
patch.

In the unmerged series, it's used by omap_framebuffer_dirty() ([PATCHv3
3/8] drm/omap: add support for manually updated displays). So we have a
framebuffer, and we want to know which crtcs need to be flushed.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/8] drm/omap: Simplify the rotation-on-crtc hack

[Tomi Valkeinen]

On 31/07/17 14:48, Laurent Pinchart wrote:

>>> The comment about read lock is only valid when the plane is bound to the
>>> crtc. In general this is not always the case. You can only peak at
>>> plane->state when crtc->state->plane_mask & BIT(drm_plane_index(plane))
>>> is true.
>>>
>>> I think we might need -EDEADLK handling for getprop then, even though it's
>>> not optimal. :(
>>
>> Well both the old an new way only worked because we grab all the locks
>> unconditionally. And I'd really want to avoid get_prop being anything
>> but a simple lookup. Unfortuantely that breaks omapdrm, so no idea
>> what exactly to do here.
>>
>> In a way adding properties without standardizing them across drivers
>> first was a really bad idea, because then we have disjoint sets of
>> uapi expectations, and there's just no way to make that work.
>>
>> I guess one radical approach might be to make this the "standard", and
>> just redirect rotation from the CRTC to the primary plane.
>>
>> Or omapdrm needs to duplicate the property properly, and update one if
>> the other is set. I think that's probably the most workable approach.
> 
> Maybe the first question we should answer is whether this hack is still 
> needed 
> in the omapdrm driver. Tomi, do you know whether userspace still needs this ?

The omap X driver uses legacy modesetting and the rotation property for
the crtc.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Tomi Valkeinen]

On 28/03/17 17:22, Daniel Vetter wrote:

>> This is a bit related to the other annoyance, which is that we don't
>> reset properties when a DRM app quits. I think the state of the HW
>> should be restored to exactly the same state as it was (including
>> turning off the crtcs). I'm planning to have a look at this whenever I
>> happen to find some time...
> 
> I have a blog post with all the ideas from last time around we've
> discussed this:
> 
> http://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
> 
> Consensus was that we'll look at this again when someone has a real use
> case that needs it. And then maybe still decide that they just need a
> system compositor :-)

Thanks for the link. I now see this is pretty messed up (but necessary)
stuff we have =).

The situation on our embedded devices is often a bit simpler: no system
compositors, one VT, (hopefully soon) no fbdev, and we have a clear
reset state.

Reading the text makes me think the state should be somehow explicitly
passed from one compositor to the other, which would keep that state
active on the kernel side, but if that's even sanely possible, I have no
idea...

I guess I'll have to deal with this in the userspace for now, and I'll
add a helper to kms++ library which will disable and reset everything it
can.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Tomi Valkeinen]

On 27/03/17 10:43, Daniel Vetter wrote:

> With atomic we've stopped killing the entire CRTC when you the last
> userspace reference for the framebuffer on the primary plane disappears,
> but just shut down the primary plane. Assuming the driver can do that, we
> fall back to full CRTC shutdown if not. That avoids a pile of flickering
> and unecessary modesets.
> 
> But yeah downside is that the crtc is active even when unloading. Note

Not only that, but also when you stop a DRM userspace app and don't
start a new one, the crtc stays active. Maybe that's not so common
scenario, but I wouldn't be that surprised if on embedded world someone
only runs a DRM app when they're showing something, and no app in
between the runs.

What makes the behavior a bit funny is that if I boot without fbdev
emulation, the crtcs stay off (as they should). But run a DRM app, quit
it, and the crtcs are on.

This is a bit related to the other annoyance, which is that we don't
reset properties when a DRM app quits. I think the state of the HW
should be restored to exactly the same state as it was (including
turning off the crtcs). I'm planning to have a look at this whenever I
happen to find some time...

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Tomi Valkeinen]

On 21/03/17 18:41, Daniel Vetter wrote:
> The trouble here is that it does multiple atomic commits under one
> drm_modeset_lock_all, which breaks the behind-the-scenes acquire
> context magic that function pulls off. It's much better to have one
> overall atomic commit. That we still have multiple atomic commits
> prevents us from adding some pretty useful debug checks to the atomic
> machinery.
> 
> Hence it is really a bad idea to call the legacy
> drm_crtc_force_disable_all() function. There's 2 atomic drivers using
> this still, nouveau and tinydrm. To fix this, introduce a new
> drm_atomic_helper_shutdown() by extracting the code from i915.
> 
> While at it improve kernel-doc and catch future offenders by
> sprinkling a WARN_ON into the legacy function. We should probably move
> those into the legacy modeset helpers, too ...
> 
> v2: Make it compile on arm drivers too (Noralf).
> 
> v3: Correct kerneldoc to point at _disable_all().
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Acked-by: Noralf Trønnes <nor...@tronnes.org>
> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Cc: Noralf Trønnes <nor...@tronnes.org>
> Cc: Ben Skeggs <bske...@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>

Tested-by: Tomi Valkeinen <tomi.valkei...@ti.com>

As a side note, I find it a bit odd that when fbdev is disabled, the
crtcs stays enabled even when DRM userspace app quits. Or is that just
omapdrm behavior? I presume not, as this shutdown on unload would not be
needed otherwise.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 08/19] drm/tilcdc: Drop calls to modeset_lock_crtc

[Tomi Valkeinen]

On 22/03/17 23:50, Daniel Vetter wrote:
> Again this is an internal helper, not the official way to lock a crtc.
> 
> Cc: Jyri Sarha <jsa...@ti.com>
> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkei...@ti.com>

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] gpu: drm: drivers: Convert printk(KERN_ to pr_

[Tomi Valkeinen]

On 28/02/17 14:55, Joe Perches wrote:
> Use a more common logging style.
> 
> Miscellanea:
> 
> o Coalesce formats and realign arguments
> o Neaten a few macros now using pr_
> 
> Signed-off-by: Joe Perches <j...@perches.com>

For omap:

Acked-by: Tomi Valkeinen <tomi.valkei...@ti.com>

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/4] video: Add new aspect ratios for HDMI 2.0

[Tomi Valkeinen]

On 09/08/16 17:55, Shashank Sharma wrote:
> HDMI 2.0/CEA-861-F introduces two new aspect ratios:
> - 64:27
> - 256:135
> 
> This patch adds enumeration for the new aspect ratios
> in the existing aspect ratio list.
> 
> V2: rebase
> 
> Signed-off-by: Shashank Sharma <shashank.sha...@intel.com>
> Reviewed-by: Sean Paul <seanp...@chromium.org>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> Cc: Emil Velikov <emil.l.veli...@gmail.com>
> ---
>  drivers/video/hdmi.c | 4 
>  include/linux/hdmi.h | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 1626892..1cf907e 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -533,6 +533,10 @@ hdmi_picture_aspect_get_name(enum hdmi_picture_aspect 
> picture_aspect)
>   return "4:3";
>   case HDMI_PICTURE_ASPECT_16_9:
>   return "16:9";
> + case HDMI_PICTURE_ASPECT_64_27:
> + return "64:27";
> + case HDMI_PICTURE_ASPECT_256_135:
> + return "256:135";
>   case HDMI_PICTURE_ASPECT_RESERVED:
>   return "Reserved";
>   }
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index e974420..edbb4fc 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -78,6 +78,8 @@ enum hdmi_picture_aspect {
>   HDMI_PICTURE_ASPECT_NONE,
>   HDMI_PICTURE_ASPECT_4_3,
>   HDMI_PICTURE_ASPECT_16_9,
> + HDMI_PICTURE_ASPECT_64_27,
> + HDMI_PICTURE_ASPECT_256_135,
>   HDMI_PICTURE_ASPECT_RESERVED,
>  };
>  
> 

Acked-by: Tomi Valkeinen <tomi.valkei...@ti.com>

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 07/15] drm/omap: Use per-plane rotation property

[Tomi Valkeinen]

On 06/10/16 13:30, Ville Syrjälä wrote:
> On Thu, Oct 06, 2016 at 12:59:17PM +0300, Tomi Valkeinen wrote:
>>
>> On 26/09/16 19:30, ville.syrj...@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>>
>>> The global mode_config.rotation_property is going away, switch over to
>>> per-plane rotation_property.
>>>
>>> Not sure I got the annoying crtc rotation_property handling right.
>>> Might work, or migth not.
>>>
>>> v2: Drop the BIT()
>>> Don't create rotation property twice for each primary plane
>>>
>>> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
>>> Cc: Rob Clark <robdcl...@gmail.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>> ---
>>
>>
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c 
>>> b/drivers/gpu/drm/omapdrm/omap_plane.c
>>> index 6ddaa5ea4b6b..b272f810989e 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>>> @@ -211,9 +211,16 @@ void omap_plane_install_properties(struct drm_plane 
>>> *plane,
>>> struct omap_drm_private *priv = dev->dev_private;
>>>  
>>> if (priv->has_dmm) {
>>> -   struct drm_property *prop = dev->mode_config.rotation_property;
>>> -
>>> -   drm_object_attach_property(obj, prop, DRM_ROTATE_0);
>>> +   if (!plane->rotation_property)
>>> +   drm_plane_create_rotation_property(plane,
>>> +  DRM_ROTATE_0,
>>> +  DRM_ROTATE_0 | 
>>> DRM_ROTATE_90 |
>>> +  DRM_ROTATE_180 | 
>>> DRM_ROTATE_270 |
>>> +  DRM_REFLECT_X | 
>>> DRM_REFLECT_Y);
>>> +
>>> +   if (plane->rotation_property && obj != >base)
>>> +   drm_object_attach_property(obj, 
>>> plane->rotation_property,
>>> +  DRM_ROTATE_0);
>>
>> I think this could use a short comment, as it's not obvious wth is going
>> on here =).
> 
> /* Attach the rotation property also to the crtc object */ ?

Yes, sounds fine to me.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 07/15] drm/omap: Use per-plane rotation property

[Tomi Valkeinen]

On 26/09/16 19:30, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> 
> The global mode_config.rotation_property is going away, switch over to
> per-plane rotation_property.
> 
> Not sure I got the annoying crtc rotation_property handling right.
> Might work, or migth not.
> 
> v2: Drop the BIT()
> Don't create rotation property twice for each primary plane
> 
> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
> Cc: Rob Clark <robdcl...@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> ---


> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c 
> b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 6ddaa5ea4b6b..b272f810989e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -211,9 +211,16 @@ void omap_plane_install_properties(struct drm_plane 
> *plane,
>   struct omap_drm_private *priv = dev->dev_private;
>  
>   if (priv->has_dmm) {
> - struct drm_property *prop = dev->mode_config.rotation_property;
> -
> - drm_object_attach_property(obj, prop, DRM_ROTATE_0);
> + if (!plane->rotation_property)
> + drm_plane_create_rotation_property(plane,
> +DRM_ROTATE_0,
> +DRM_ROTATE_0 | 
> DRM_ROTATE_90 |
> +DRM_ROTATE_180 | 
> DRM_ROTATE_270 |
> +DRM_REFLECT_X | 
> DRM_REFLECT_Y);
> +
> + if (plane->rotation_property && obj != >base)
> + drm_object_attach_property(obj, 
> plane->rotation_property,
> +    DRM_ROTATE_0);

I think this could use a short comment, as it's not obvious wth is going
on here =).

Otherwise both omap patches look fine, and test fine.

Reviewed-by: Tomi Valkeinen <tomi.valkei...@ti.com>

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/15] drm/omap: Use per-plane rotation property

[Tomi Valkeinen]

On 12/08/16 19:04, Ville Syrjälä wrote:
> On Thu, Aug 11, 2016 at 04:33:32PM +0300, Ville Syrjälä wrote:
>> On Thu, Aug 11, 2016 at 02:32:44PM +0300, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 22/07/16 16:43, ville.syrj...@linux.intel.com wrote:
>>>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>>>
>>>> The global mode_config.rotation_property is going away, switch over to
>>>> per-plane rotation_property.
>>>>
>>>> Not sure I got the annoying crtc rotation_property handling right.
>>>> Might work, or migth not.
>>>
>>> I think something is funny with this patch or the series. I fetched your
>>> branch, and with your series, it looks like the primary planes lose all
>>> their props. modetest says:
>>>
>>> could not get plane 26 properties: Invalid argument
>>> could not get plane 30 properties: Invalid argument
>>
>> Hmm. Weird. Is it really the get props ioctl that fails?
>>
>> The first EINVAL I can spot there is
>> if (!obj->properties) {
>>   ret = -EINVAL;
>>   goto out_unref;
>>  }
>> which definitely makes no sense since this is assigned
>> as plane->base.properties = >properties. So can't be that unless
>> we manage to clear the pointer somehow after the init.
>>
>> The only other direct EINVAL I see there is if
>>  drm_object_property_get_value(obj->properties->properties[i])
>> fails to find the passed prop in the properties array. Which clearly
>> can't happen since we got it from the array in the first place. Also,
>> clearly that code is rather inefficient, perhaps someone should rewrite
>> it a bit.
>>
>> Can't quite see how this could fail for the plane in other ways. But I
>> might be blind.
> 
> I tried to think on this a bit more, and the only think I came up with was
> that we end up doing the drm_plane_create_rotation_property() twice for the
> primary planes. I tried that on i915 but it'd didn't result in anything bad
> AFAICS. Would leak a bit, but so what :P
> 
> Dunno, I guess you could try something like:
> 
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -211,11 +211,12 @@ void omap_plane_install_properties(struct drm_plane 
> *plane,
> struct omap_drm_private *priv = dev->dev_private;
>  
> if (priv->has_dmm) {
> -   drm_plane_create_rotation_property(plane,
> -  BIT(DRM_ROTATE_0),
> -  BIT(DRM_ROTATE_0) | 
> BIT(DRM_ROTATE_90) |
> -  BIT(DRM_ROTATE_180) | 
> BIT(DRM_ROTATE_270) |
> -  BIT(DRM_REFLECT_X) | 
> BIT(DRM_REFLECT_Y));
> +   if (!plane->rotation_property)
> +   drm_plane_create_rotation_property(plane,
> +  BIT(DRM_ROTATE_0),
> +  BIT(DRM_ROTATE_0) 
> | BIT(DRM_ROTATE_90) |
> +  
> BIT(DRM_ROTATE_180) | BIT(DRM_ROTATE_270) |
> +  BIT(DRM_REFLECT_X) 
> | BIT(DRM_REFLECT_Y));

This fixes the problem... Obviously something gets confused if the
property is created twice. Perhaps the first one gets stored somewhere,
and gets used somehow, even if the latter property is the "real" one,
attached to the plane? Just a guess...

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/15] drm/omap: Use per-plane rotation property

[Tomi Valkeinen]

Hi,

On 22/07/16 16:43, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> The global mode_config.rotation_property is going away, switch over to
> per-plane rotation_property.
> 
> Not sure I got the annoying crtc rotation_property handling right.
> Might work, or migth not.

I think something is funny with this patch or the series. I fetched your
branch, and with your series, it looks like the primary planes lose all
their props. modetest says:

could not get plane 26 properties: Invalid argument
could not get plane 30 properties: Invalid argument

and

Planes:
id  crtcfb  CRTC x,yx,y gamma size  possible
crtcs
26  28  55  0,0 0,0 0   0x0001
  formats: RG16 RX12 XR12 RA12 AR12 XR15 AR15 RG24 RX24 XR24 RA24 AR24
  no properties found
30  0   0   0,0 0,0 0   0x0002
  formats: RG16 RX12 XR12 RA12 AR12 XR15 AR15 RG24 RX24 XR24 RA24 AR24
NV12 YUYV UYVY
  no properties found

I didn't look closer yet.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 16/22] drm/omap: Nuke close hooks

[Tomi Valkeinen]

Hi Daniel,

On 11/01/16 23:41, Daniel Vetter wrote:

> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c 
> b/drivers/gpu/drm/omapdrm/omap_drv.c
> index dfafdb602ad2..603a65498b40 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -175,17 +175,6 @@ static int omap_atomic_commit(struct drm_device *dev,
>   priv->commit.pending |= commit->crtcs;
>   spin_unlock(>commit.lock);
>  
> - /* Keep track of all CRTC events to unlink them in preclose(). */
> - spin_lock_irqsave(>event_lock, flags);
> - for (i = 0; i < dev->mode_config.num_crtc; ++i) {
> - struct drm_crtc_state *cstate = state->crtc_states[i];
> -
> - if (cstate && cstate->event)
> - list_add_tail(>event->base.link,
> -   >commit.events);
> - }
> - spin_unlock_irqrestore(>event_lock, flags);
> -
>   /* Swap the state, this is the point of no return. */
>   drm_atomic_helper_swap_state(dev, state);

'flags' local needs to be removed from omap_atomic_commit, otherwise:

drivers/gpu/drm/omapdrm/omap_drv.c:145:16: error: unused variable 'flags'

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 20/22] drm/tilcdc: Nuke preclose hook

[Tomi Valkeinen]

On 12/01/16 17:12, Daniel Vetter wrote:

> Different approaches to the same problem:
> 
> - omap just unlinks the event from fpriv and still process it normally.
>   But then before sending it out it checks whether the fpriv is still
>   there or not and either sends it, or deletes the event directly. This
>   way the vblank_put is always called from the worker/irq handler as part
>   of the event processing.
> 
>   This is the same approach I implemented in core with this series.
> 
> - tilcdc (and most other drivers) entirely destroy the event in the
>   preclose hook, which means they must also release any other resources
>   acquired as part of that event. Therefore they have the vblank_put here.
>   But the vblank_put is obviously also in the normal event processing
>   paths, so with the new approach of only unlinking it we can handle this
>   without any special cases in the driver.
> 
> I hope this explains what's going on. Since you're about driver maintainer
> no. 3 with the same question: Can you pls review the kerneldoc and make
> sure it explains this well? I tried to improve it already a bit after
> Laurent's/Thomas' questions.

Ok, makes sense. I think the kernel doc is fine.

I wasn't able to test tilcdc, as it seems to crash on drm-next at the
moment... Need to debug that first.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 16/22] drm/omap: Nuke close hooks

[Tomi Valkeinen]

On 11/01/16 23:41, Daniel Vetter wrote:
> Again since the core takes care of this we can remove them. While at
> it also remove the postclose hook, it's empty.
> 
> v2: Laurent pointed me at even more code to delete.
> 
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
> Acked-by: Daniel Stone <dani...@collabora.com>
> Reviewed-by: Alex Deucher <alexander.deuc...@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 13 +---
>  drivers/gpu/drm/omapdrm/omap_drv.c  | 41 
> -
>  drivers/gpu/drm/omapdrm/omap_drv.h  |  1 -
>  3 files changed, 1 insertion(+), 54 deletions(-)

Acked-by: Tomi Valkeinen <tomi.valkei...@ti.com>

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 20/22] drm/tilcdc: Nuke preclose hook

[Tomi Valkeinen]

On 11/01/16 23:41, Daniel Vetter wrote:
> Again since the drm core takes care of event unlinking/disarming this
> is now just needless code.
> 
> v2: Fixup misplaced hunks.
> 
> Cc: Rob Clark 
> Acked-by: Daniel Stone 
> Reviewed-by: Alex Deucher  (v1)
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 20 
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  8 
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  1 -
>  3 files changed, 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 7d07733bdc86..4802da8e6d6f 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -662,26 +662,6 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
>   return IRQ_HANDLED;
>  }
>  
> -void tilcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file 
> *file)
> -{
> - struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> - struct drm_pending_vblank_event *event;
> - struct drm_device *dev = crtc->dev;
> - unsigned long flags;
> -
> - /* Destroy the pending vertical blanking event associated with the
> -  * pending page flip, if any, and disable vertical blanking interrupts.
> -  */
> - spin_lock_irqsave(>event_lock, flags);
> - event = tilcdc_crtc->event;
> - if (event && event->base.file_priv == file) {
> - tilcdc_crtc->event = NULL;
> - event->base.destroy(>base);
> - drm_vblank_put(dev, 0);
> - }
> - spin_unlock_irqrestore(>event_lock, flags);
> -}
> -

Hmm, looks fine, but when I was comparing the omapdrm change and this
one, I see tilcdc doing drm_vblank_put() in the removed code but omapdrm
doesn't.

The other patches that nuke preclose hooks also contain vblank_put. Will
there be a vblank_put call missing here, or will there be an extra
vblank_put call happening somewhere on omapdrm?

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

On 25/08/15 16:45, Daniel Vetter wrote:
> When the usual fbcon legacy options are enabled we have
> ->register_framebuffer
>   ->fb notifier chain calls into fbcon
> ->fbcon sets up console on new fbi
>   ->fbi->set_par
> ->drm_fb_helper_set_par exercises full kms api
> 
> And because of locking inversion hilarity all of register_framebuffer
> is done with the console lock held. Which means that the first time on
> driver load we exercise _all_ the kms code (all probe paths and
> modeset paths for everything connected) is under the console lock.
> That means if anything goes belly-up in that big pile of code nothing
> ever reaches logfiles (and the machine is dead).
> 
> Usual tactic to debug that is to temporarily remove those console_lock
> calls to be able to capture backtraces. I'm fed up writing this patch
> and recompiling kernels. Hence this patch here to add an unsafe,
> kernel-taining option to do this at runtime.

I think this was never merged. This was part 4 of 4, were there
dependencies or...? Should I apply this for 4.5?

But then... I think my issues with console lock have been later at
runtime, not at register. Maybe we need a module option to disable the
console lock altogether. I wonder how much havoc that might create, though.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

On 01/09/15 17:34, Rob Clark wrote:

> I hadn't had a chance to look at it further yet..  I think Daniel
> claimed it worked for him, but he was probably on intel-next, where I
> was on drm-next at the time which seemed to be having some unrelated
> i915 issues (when I was trying to debug atomic fb-helper patches).  So
> can't really say that the issue I had was actually related to this
> patch.  I'll try again later this week or next, when hopefully i915 in
> drm-next is in better shape..

Rob, did you have a chance to test this?

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

On 25/08/15 22:24, Rob Clark wrote:
> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vet...@ffwll.ch> wrote:
>> When the usual fbcon legacy options are enabled we have
>> ->register_framebuffer
>>   ->fb notifier chain calls into fbcon
>> ->fbcon sets up console on new fbi
>>   ->fbi->set_par
>> ->drm_fb_helper_set_par exercises full kms api
>>
>> And because of locking inversion hilarity all of register_framebuffer
>> is done with the console lock held. Which means that the first time on
>> driver load we exercise _all_ the kms code (all probe paths and
>> modeset paths for everything connected) is under the console lock.
>> That means if anything goes belly-up in that big pile of code nothing
>> ever reaches logfiles (and the machine is dead).
>>
>> Usual tactic to debug that is to temporarily remove those console_lock
>> calls to be able to capture backtraces. I'm fed up writing this patch
>> and recompiling kernels. Hence this patch here to add an unsafe,
>> kernel-taining option to do this at runtime.
>>
>> Cc: Jean-Christophe Plagniol-Villard <plagn...@jcrosoft.com>
>> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
>> Cc: linux-fb...@vger.kernel.org
>> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> 
> This one was causing me some problems, if I tried to enable
> lockless_register_fb.  It *looks* like it should work, so I'm not
> quite sure what the deal is.  But I'm 110% fan of getting something
> like this working, because console_lock is pretty much the bane of kms
> developer's existence..
> 
> I'll have to debug further on a system where I can see more than the
> bottom three lines of the second to last backtrace..

Any idea if anyone has ever looked at properly fixing this?

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] fbdev: Debug knob to register without holding console_lock

[Tomi Valkeinen]

On 01/09/15 17:34, Rob Clark wrote:
> On Tue, Sep 1, 2015 at 6:32 AM, Tomi Valkeinen <tomi.valkei...@ti.com> wrote:
>>
>>
>> On 25/08/15 22:24, Rob Clark wrote:
>>> On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter <daniel.vet...@ffwll.ch> 
>>> wrote:
>>>> When the usual fbcon legacy options are enabled we have
>>>> ->register_framebuffer
>>>>   ->fb notifier chain calls into fbcon
>>>> ->fbcon sets up console on new fbi
>>>>   ->fbi->set_par
>>>> ->drm_fb_helper_set_par exercises full kms api
>>>>
>>>> And because of locking inversion hilarity all of register_framebuffer
>>>> is done with the console lock held. Which means that the first time on
>>>> driver load we exercise _all_ the kms code (all probe paths and
>>>> modeset paths for everything connected) is under the console lock.
>>>> That means if anything goes belly-up in that big pile of code nothing
>>>> ever reaches logfiles (and the machine is dead).
>>>>
>>>> Usual tactic to debug that is to temporarily remove those console_lock
>>>> calls to be able to capture backtraces. I'm fed up writing this patch
>>>> and recompiling kernels. Hence this patch here to add an unsafe,
>>>> kernel-taining option to do this at runtime.
>>>>
>>>> Cc: Jean-Christophe Plagniol-Villard <plagn...@jcrosoft.com>
>>>> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
>>>> Cc: linux-fb...@vger.kernel.org
>>>> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
>>>
>>> This one was causing me some problems, if I tried to enable
>>> lockless_register_fb.  It *looks* like it should work, so I'm not
>>> quite sure what the deal is.  But I'm 110% fan of getting something
>>> like this working, because console_lock is pretty much the bane of kms
>>> developer's existence..
>>>
>>> I'll have to debug further on a system where I can see more than the
>>> bottom three lines of the second to last backtrace..
>>
>> Any idea if anyone has ever looked at properly fixing this?
> 
> I hadn't had a chance to look at it further yet..  I think Daniel
> claimed it worked for him, but he was probably on intel-next, where I
> was on drm-next at the time which seemed to be having some unrelated
> i915 issues (when I was trying to debug atomic fb-helper patches).  So
> can't really say that the issue I had was actually related to this
> patch.  I'll try again later this week or next, when hopefully i915 in
> drm-next is in better shape..

Oh, I didn't mean this patch, but the whole console lock in general.
I've also banged my head to a wall because of it =).

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/atomic: pass old crtc state to atomic_begin/flush.

[Tomi Valkeinen]

Hi,

On 21/07/15 14:28, Maarten Lankhorst wrote:
 In intel it's useful to keep track of some state changes with old
 crtc state vs new state, for example to disable initial planes or
 when a modeset's prevented during fastboot.
 
 Cc: dri-de...@lists.freedesktop.org
 Signed-off-by: Maarten Lankhorst maarten.lankho...@linux.intel.com
 ---
  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c |  6 --
  drivers/gpu/drm/drm_atomic_helper.c|  8 
  drivers/gpu/drm/drm_plane_helper.c |  4 ++--
  drivers/gpu/drm/i915/intel_display.c   | 10 ++
  drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c   |  6 --
  drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c   |  6 --
  drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  6 --
  drivers/gpu/drm/sti/sti_drm_crtc.c |  6 --
  drivers/gpu/drm/tegra/dc.c |  6 --
  include/drm/drm_crtc_helper.h  |  6 --
  10 files changed, 40 insertions(+), 24 deletions(-)

Looks like this broke omapdrm in linux-next:

drivers/gpu/drm/omapdrm/omap_crtc.c:470:2: error: initialization from
incompatible pointer type [-Werror]
drivers/gpu/drm/omapdrm/omap_crtc.c:470:2: error: (near initialization
for ‘omap_crtc_helper_funcs.atomic_begin’) [-Werror]
drivers/gpu/drm/omapdrm/omap_crtc.c:471:2: error: initialization from
incompatible pointer type [-Werror]
drivers/gpu/drm/omapdrm/omap_crtc.c:471:2: error: (near initialization
for ‘omap_crtc_helper_funcs.atomic_flush’) [-Werror]

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] video/fbdev: Always built-in video= cmdline parsing

[Tomi Valkeinen]

Hi,

On 06/08/14 15:52, Daniel Vetter wrote:
 In drm/i915 we want to get at the video= cmdline modes even when we
 don't have fbdev support enabled, so that users can always override
 the kernel's initial mode selection.
 
 But that gives us a direct depency upon the parsing code in the fbdev
 subsystem. Since it's so little code just extract these 2 functions
 and always build them in.
 
 Whiel at it fix the checkpatch fail in this code.
 
 v2: Also move fb_mode_option. Spotted by the kbuild.
 
 v3: Review from Geert:
 - Keep the old copyright notice from fb_mem.c, although I have no
 idea what exactly applies.
 - Only compile this when needed.
 
 Cc: Geert Uytterhoeven ge...@linux-m68k.org
 Cc: Plagniol-Villard plagn...@jcrosoft.com
 Cc: Tomi Valkeinen tomi.valkei...@ti.com
 Cc: linux-fb...@vger.kernel.org
 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 
 --
 
 I prefer if we can merge this through drm-next since we'll use it
 there in follow-up patches.
 -Daniel
 ---
  drivers/video/fbdev/Kconfig   |   4 ++
  drivers/video/fbdev/core/Makefile |   1 +
  drivers/video/fbdev/core/fb_cmdline.c | 110 
 ++
  drivers/video/fbdev/core/fbmem.c  |  92 
  drivers/video/fbdev/core/modedb.c |   3 -
  5 files changed, 115 insertions(+), 95 deletions(-)
  create mode 100644 drivers/video/fbdev/core/fb_cmdline.c

Sorry for late response.

Looks fine for me, and I'm fine merging it via drm-next.

Acked-by: Tomi Valkeinen tomi.valkei...@ti.com

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Kick out vga console

[Tomi Valkeinen]

Hi,

On 06/06/14 18:20, Daniel Vetter wrote:
 Tomi/Jean can you please ack merging this through drm-intel trees? It
 just exports the vga and dummy consoles so that i915 can do what it
 needs to do.

Acked-by: Tomi Valkeinen tomi.valkei...@ti.com

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] fbcon: Avoid corrupting active workqueue entries

[Tomi Valkeinen]

Hi,

On 2013-12-17 16:27, Chris Wilson wrote:
 We attempt to reschedule an active work which can itself corrupt the
 workqueue lists, and we may then free the work item whilst the task is
 still pending. Both of these may lead to a system deadlock and an
 unresponsive machine without any outputs for a panic to even be shown.

This gives the following warning:

drivers/video/console/fbcon.c:407:20: warning: unused variable ‘ops’
[-Wunused-variable]

I can fix that. Or maybe it's better if you send a v2 with that fixed,
so that this gets properly to stable-kernel.

However, I wouldn't mind an acked-by or reviewed-by from someone.

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx