Hi,

On 23/09/2021 10:06, Neil Armstrong wrote:

From: Benoit Parrot bparrot@ti.com

We currently assume that an overlay has the same maximum width and maximum height as the overlay manager. This assumption is incorrect. On some variants the overlay manager maximum width is twice the maximum width that the overlay can handle. We need to add the appropriate data per variant as well as export a helper function to retrieve the data so check can be made dynamically in omap_plane_atomic_check().

Signed-off-by: Benoit Parrot bparrot@ti.com Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/omapdrm/dss/dispc.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/omapdrm/dss/dss.h | 2 ++ drivers/gpu/drm/omapdrm/omap_plane.c | 14 ++++++++++++++ 3 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index 3c4a4991e45a..bdecec8f4d88 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -92,6 +92,8 @@ struct dispc_features { u8 mgr_height_start; u16 mgr_width_max; u16 mgr_height_max;

• u16 ovl_width_max;
• u16 ovl_height_max; unsigned long max_lcd_pclk; unsigned long max_tv_pclk; unsigned int max_downscale;

@@ -2599,6 +2601,12 @@ static int dispc_ovl_calc_scaling(struct dispc_device *dispc, return 0; }

+void dispc_ovl_get_max_size(struct dispc_device *dispc, u16 *width, u16 *height) +{

• *width = dispc->feat->ovl_width_max;
• *height = dispc->feat->ovl_height_max;

+}

• static int dispc_ovl_setup_common(struct dispc_device *dispc, enum omap_plane_id plane, enum omap_overlay_caps caps,

@@ -4240,6 +4248,8 @@ static const struct dispc_features omap24xx_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048,

• .ovl_width_max = 2048,
• .ovl_height_max = 2048, .max_lcd_pclk = 66500000, .max_downscale = 2, /*

@@ -4278,6 +4288,8 @@ static const struct dispc_features omap34xx_rev1_0_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048,

• .ovl_width_max = 2048,
• .ovl_height_max = 2048, .max_lcd_pclk = 173000000, .max_tv_pclk = 59000000, .max_downscale = 4,

@@ -4313,6 +4325,8 @@ static const struct dispc_features omap34xx_rev3_0_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048,

• .ovl_width_max = 2048,
• .ovl_height_max = 2048, .max_lcd_pclk = 173000000, .max_tv_pclk = 59000000, .max_downscale = 4,

@@ -4348,6 +4362,8 @@ static const struct dispc_features omap36xx_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048,

• .ovl_width_max = 2048,
• .ovl_height_max = 2048, .max_lcd_pclk = 173000000, .max_tv_pclk = 59000000, .max_downscale = 4,

@@ -4383,6 +4399,8 @@ static const struct dispc_features am43xx_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048,

• .ovl_width_max = 2048,
• .ovl_height_max = 2048, .max_lcd_pclk = 173000000, .max_tv_pclk = 59000000, .max_downscale = 4,

@@ -4418,6 +4436,8 @@ static const struct dispc_features omap44xx_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048,

• .ovl_width_max = 2048,
• .ovl_height_max = 2048, .max_lcd_pclk = 170000000, .max_tv_pclk = 185625000, .max_downscale = 4,

@@ -4457,6 +4477,8 @@ static const struct dispc_features omap54xx_dispc_feats = { .mgr_height_start = 27, .mgr_width_max = 4096, .mgr_height_max = 4096,

• .ovl_width_max = 2048,
• .ovl_height_max = 4096, .max_lcd_pclk = 170000000, .max_tv_pclk = 192000000, .max_downscale = 4,

diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h b/drivers/gpu/drm/omapdrm/dss/dss.h index a547527bb2f3..14c39f7c3988 100644 --- a/drivers/gpu/drm/omapdrm/dss/dss.h +++ b/drivers/gpu/drm/omapdrm/dss/dss.h @@ -397,6 +397,8 @@ int dispc_get_num_mgrs(struct dispc_device *dispc); const u32 *dispc_ovl_get_color_modes(struct dispc_device *dispc, enum omap_plane_id plane);

+void dispc_ovl_get_max_size(struct dispc_device *dispc, u16 *width, u16 *height);

• u32 dispc_read_irqstatus(struct dispc_device *dispc); void dispc_clear_irqstatus(struct dispc_device *dispc, u32 mask); void dispc_write_irqenable(struct dispc_device *dispc, u32 mask);

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 512af976b7e9..d0a67b7ed1a0 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -109,11 +109,18 @@ static int omap_plane_atomic_check(struct drm_plane *plane, { struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);

• struct omap_drm_private *priv = plane->dev->dev_private; struct drm_crtc_state *crtc_state;
• u16 width, height;
• u32 width_fp, height_fp;

I think naming these max_w/max_width etc. would be better.

Tomi

On 23/09/2021 10:06, Neil Armstrong wrote:

From: Benoit Parrot bparrot@ti.com

Split out the hardware overlay specifics from omap_plane. To start, the hw overlays are statically assigned to planes.

The goal is to eventually assign hw overlays dynamically to planes during plane->atomic_check() based on requested caps (scaling, YUV, etc). And then perform hw overlay re-assignment if required.

Signed-off-by: Benoit Parrot bparrot@ti.com Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/omapdrm/Makefile | 1 + drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++- drivers/gpu/drm/omapdrm/omap_drv.h | 4 ++ drivers/gpu/drm/omapdrm/omap_overlay.c | 87 ++++++++++++++++++++++++++ drivers/gpu/drm/omapdrm/omap_overlay.h | 31 +++++++++ drivers/gpu/drm/omapdrm/omap_plane.c | 42 ++++++------- 6 files changed, 151 insertions(+), 23 deletions(-) create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.c create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.h

diff --git a/drivers/gpu/drm/omapdrm/Makefile b/drivers/gpu/drm/omapdrm/Makefile index 21e8277ff88f..710b4e0abcf0 100644 --- a/drivers/gpu/drm/omapdrm/Makefile +++ b/drivers/gpu/drm/omapdrm/Makefile @@ -9,6 +9,7 @@ omapdrm-y := omap_drv.o \ omap_debugfs.o \ omap_crtc.o \ omap_plane.o \

• omap_overlay.o \ omap_encoder.o \ omap_fb.o \ omap_gem.o \

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index f86e20578143..b994014b22e8 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -583,10 +583,14 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)

omap_gem_init(ddev);

• ret = omap_hwoverlays_init(priv);
• if (ret)

• goto err_gem_deinit;
  

• ret = omap_modeset_init(ddev); if (ret) { dev_err(priv->dev, "omap_modeset_init failed: ret=%d\n", ret);

• goto err_gem_deinit;
  

• goto err_free_overlays;
  

  }

  /* Initialize vblank handling, start with all CRTCs disabled. */

@@ -618,6 +622,8 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) omap_fbdev_fini(ddev); err_cleanup_modeset: omap_modeset_fini(ddev); +err_free_overlays:

• omap_hwoverlays_destroy(priv); err_gem_deinit: omap_gem_deinit(ddev); destroy_workqueue(priv->wq);

@@ -642,6 +648,7 @@ static void omapdrm_cleanup(struct omap_drm_private *priv) drm_atomic_helper_shutdown(ddev);

omap_modeset_fini(ddev);

• omap_hwoverlays_destroy(priv); omap_gem_deinit(ddev);

  destroy_workqueue(priv->wq);

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 591d4c273f02..b4d9c2062723 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -24,6 +24,7 @@ #include "omap_gem.h" #include "omap_irq.h" #include "omap_plane.h" +#include "omap_overlay.h"

#define DBG(fmt, ...) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__) #define VERB(fmt, ...) if (0) DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) /* verbose debug */ @@ -57,6 +58,9 @@ struct omap_drm_private { unsigned int num_planes; struct drm_plane *planes[8];

• unsigned int num_ovls;

• struct omap_hw_overlay *overlays[8];

• struct drm_fb_helper *fbdev;

  struct workqueue_struct *wq;

diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.c b/drivers/gpu/drm/omapdrm/omap_overlay.c new file mode 100644 index 000000000000..2b1416d2aad2 --- /dev/null +++ b/drivers/gpu/drm/omapdrm/omap_overlay.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Benoit Parrot, bparrot@ti.com

Extra comma there, and similar case below.

• */

+#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_plane_helper.h>

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

+/*

• • overlay funcs
• */

+static const char * const overlay_id_to_name[] = {

• [OMAP_DSS_GFX] = "gfx",
• [OMAP_DSS_VIDEO1] = "vid1",
• [OMAP_DSS_VIDEO2] = "vid2",
• [OMAP_DSS_VIDEO3] = "vid3",

+};

I was expecting to see the name array to be removed from omap_plane.c as it's moved here, but that's not the case. Why is that? Especially as after this series these names make no sense with the planes.

+static void omap_overlay_destroy(struct omap_hw_overlay *overlay) +{

• kfree(overlay);

+}

+static struct omap_hw_overlay *omap_overlay_init(enum omap_plane_id overlay_id,

• 				 enum omap_overlay_caps caps)
  

+{

• struct omap_hw_overlay *overlay;
• overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
• if (!overlay)

• return ERR_PTR(-ENOMEM);
  

• overlay->name = overlay_id_to_name[overlay_id];
• overlay->overlay_id = overlay_id;
• overlay->caps = caps;
• /*

• * When this is called priv->num_crtcs is not known yet.
  

• * Use a safe mask value to start with, it will get updated to the
  

• * proper value after the first use.
  

• */
  

• overlay->possible_crtcs = 0xff;

This sounds like a hack. Why do we need possible_crtcs anyway? If I'm not mistaken, on all DSS versions any overlay can be used on any ctrtc. On the DRM plane level we need the possible_crtc as the DRM framework needs that (i.e. we just always set all crtcs available for all planes), but why is it needed here?

• return overlay;

+}

+int omap_hwoverlays_init(struct omap_drm_private *priv) +{

• static const enum omap_plane_id hw_plane_ids[] = {

• 	OMAP_DSS_GFX, OMAP_DSS_VIDEO1,
  

• 	OMAP_DSS_VIDEO2, OMAP_DSS_VIDEO3,
  

• };
• u32 num_overlays = dispc_get_num_ovls(priv->dispc);
• enum omap_overlay_caps caps;
• int i, ret;
• for (i = 0; i < num_overlays; i++) {

• struct omap_hw_overlay *overlay;
  

• caps = dispc_ovl_get_caps(priv->dispc, hw_plane_ids[i]);
  

• overlay = omap_overlay_init(hw_plane_ids[i], caps);
  

• if (IS_ERR(overlay)) {
  

• 	ret = PTR_ERR(overlay);
  

• 	dev_err(priv->dev, "failed to construct overlay for %s (%d)\n",
  

• 		overlay_id_to_name[i], ret);
  

• 	return ret;
  

• }
  

I think this leaks memory if omap_overlay_init() fails.

• overlay->idx = priv->num_ovls;
  

• priv->overlays[priv->num_ovls++] = overlay;
  

• }
• return 0;

+}

+void omap_hwoverlays_destroy(struct omap_drm_private *priv) +{

• int i;
• for (i = 0; i < priv->num_ovls; i++) {

• omap_overlay_destroy(priv->overlays[i]);
  

• priv->overlays[i] = NULL;
  

• }

+} diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.h b/drivers/gpu/drm/omapdrm/omap_overlay.h new file mode 100644 index 000000000000..892fecb67adb --- /dev/null +++ b/drivers/gpu/drm/omapdrm/omap_overlay.h @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Benoit Parrot, bparrot@ti.com
• */

+#ifndef __OMAPDRM_OVERLAY_H__ +#define __OMAPDRM_OVERLAY_H__

+#include <linux/types.h>

+enum drm_plane_type;

+struct drm_device; +struct drm_mode_object; +struct drm_plane;

+/* Used to associate a HW overlay/plane to a plane */ +struct omap_hw_overlay {

• int idx;

unsigned int.

• const char *name;
• enum omap_plane_id overlay_id;

Perhaps just "id" is fine. You don't have "overlay_name" there either, but just "name".

Tomi

On 12/10/2021 10:13, Tomi Valkeinen wrote:

On 23/09/2021 10:06, Neil Armstrong wrote:

...

From: Benoit Parrot bparrot@ti.com

In preparation to add omap plane state specific extensions we need to subclass drm_plane_state and add the relevant helpers.

The addition of specific extension will be done separately.

Signed-off-by: Benoit Parrot bparrot@ti.com Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/omapdrm/omap_plane.c | 38 +++++++++++++++++++++++++---   1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 0df5381cc015..bda794b4c915 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -16,6 +16,13 @@    * plane funcs    */   +#define to_omap_plane_state(x) container_of(x, struct omap_plane_state, base)

+struct omap_plane_state { +    /* Must be first. */ +    struct drm_plane_state base; +};

#define to_omap_plane(x) container_of(x, struct omap_plane, base)     struct omap_plane { @@ -207,11 +214,17 @@ void omap_plane_install_properties(struct drm_plane *plane,   static void omap_plane_reset(struct drm_plane *plane)   {       struct omap_plane *omap_plane = to_omap_plane(plane); +    struct omap_plane_state *omap_state;   -    drm_atomic_helper_plane_reset(plane); -    if (!plane->state) +    if (plane->state) +        drm_atomic_helper_plane_destroy_state(plane, plane->state);

+    omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL); +    if (!omap_state)           return;   +    __drm_atomic_helper_plane_reset(plane, &omap_state->base);

/*        * Set the zpos default depending on whether we are a primary or overlay        * plane. @@ -222,6 +235,25 @@ static void omap_plane_reset(struct drm_plane *plane)       plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;   }   +static struct drm_plane_state * +omap_plane_atomic_duplicate_state(struct drm_plane *plane) +{ +    struct omap_plane_state *state; +    struct omap_plane_state *copy;

+    if (WARN_ON(!plane->state)) +        return NULL;

+    state = to_omap_plane_state(plane->state); +    copy = kmemdup(state, sizeof(*state), GFP_KERNEL); +    if (!copy) +        return NULL;

+    __drm_atomic_helper_plane_duplicate_state(plane, &copy->base);

+    return &copy->base; +}

omap_crtc.c has similar, but slightly different, functions. I think it would be good to use the same style in omap_plane, or, if the approach above is better, change omap_crtc to match the style here.

Indeed the crtc version is better, I used the same style.

Thanks, Neil

Tomi

On 23/09/2021 10:06, Neil Armstrong wrote:

From: Benoit Parrot bparrot@ti.com

Global shared resources (like hw overlays) for omapdrm are implemented as a part of atomic state using the drm_private_obj infrastructure available in the atomic core.

omap_global_state is introduced as a drm atomic private object. The two funcs omap_get_global_state() and omap_get_existing_global_state() are the two variants that will be used to access omap_global_state.

drm_mode_config_init() needs to be called earlier because it creates/initializes the private_obj link list maintained by the atomic framework. The private_obj link list has to exist prior to calling drm_atomic_private_obj_init(). Similarly the cleanup handler are reordered appropriately.

I'm not really familiar with the private object. Did you check how current drivers use it? These patches are 3 years old, and things might have changed around the private object.

Signed-off-by: Benoit Parrot bparrot@ti.com Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/omapdrm/omap_drv.c | 91 +++++++++++++++++++++++++++++- drivers/gpu/drm/omapdrm/omap_drv.h | 21 +++++++ 2 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index b994014b22e8..c7912374d393 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -128,6 +128,82 @@ static const struct drm_mode_config_funcs omap_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit, };

+/* Global/shared object state funcs */

+/*

• • This is a helper that returns the private state currently in operation.
• • Note that this would return the "old_state" if called in the atomic check
• • path, and the "new_state" after the atomic swap has been done.
• */

+struct omap_global_state * +omap_get_existing_global_state(struct omap_drm_private *priv) +{

• return to_omap_global_state(priv->glob_obj.state);

+}

+/*

• • This acquires the modeset lock set aside for global state, creates
• • a new duplicated private object state.
• */

+struct omap_global_state *__must_check +omap_get_global_state(struct drm_atomic_state *s) +{

• struct omap_drm_private *priv = s->dev->dev_private;
• struct drm_private_state *priv_state;
• priv_state = drm_atomic_get_private_obj_state(s, &priv->glob_obj);
• if (IS_ERR(priv_state))

• return ERR_CAST(priv_state);
  

• return to_omap_global_state(priv_state);

+}

+static struct drm_private_state * +omap_global_duplicate_state(struct drm_private_obj *obj) +{

• struct omap_global_state *state;
• state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
• if (!state)

• return NULL;
  

• __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
• return &state->base;

+}

+static void omap_global_destroy_state(struct drm_private_obj *obj,

• 		      struct drm_private_state *state)
  

+{

• struct omap_global_state *omap_state = to_omap_global_state(state);
• kfree(omap_state);

+}

+static const struct drm_private_state_funcs omap_global_state_funcs = {

• .atomic_duplicate_state = omap_global_duplicate_state,
• .atomic_destroy_state = omap_global_destroy_state,

+};

+static int omap_global_obj_init(struct drm_device *dev) +{

• struct omap_drm_private *priv = dev->dev_private;
• struct omap_global_state *state;
• state = kzalloc(sizeof(*state), GFP_KERNEL);
• if (!state)

• return -ENOMEM;
  

• drm_atomic_private_obj_init(dev, &priv->glob_obj, &state->base,

• 		    &omap_global_state_funcs);
  

• return 0;

+}

+static void omap_global_obj_fini(struct omap_drm_private *priv) +{

• drm_atomic_private_obj_fini(&priv->glob_obj);

+}

• static void omap_disconnect_pipelines(struct drm_device *ddev) { struct omap_drm_private *priv = ddev->dev_private;

@@ -231,8 +307,6 @@ static int omap_modeset_init(struct drm_device *dev) if (!omapdss_stack_is_ready()) return -EPROBE_DEFER;

• drm_mode_config_init(dev);
• ret = omap_modeset_init_properties(dev); if (ret < 0) return ret;

@@ -583,10 +657,16 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)

omap_gem_init(ddev);

• ret = omap_hwoverlays_init(priv);

• drm_mode_config_init(ddev);

• ret = omap_global_obj_init(ddev); if (ret) goto err_gem_deinit;

• ret = omap_hwoverlays_init(priv);

• if (ret)

• goto err_free_priv_obj;
  

• ret = omap_modeset_init(ddev); if (ret) { dev_err(priv->dev, "omap_modeset_init failed: ret=%d\n", ret);

@@ -624,7 +704,10 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) omap_modeset_fini(ddev); err_free_overlays: omap_hwoverlays_destroy(priv); +err_free_priv_obj:

• omap_global_obj_fini(priv); err_gem_deinit:
• drm_mode_config_cleanup(ddev); omap_gem_deinit(ddev); destroy_workqueue(priv->wq); omap_disconnect_pipelines(ddev);

@@ -649,6 +732,8 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)

omap_modeset_fini(ddev); omap_hwoverlays_destroy(priv);

• omap_global_obj_fini(priv);

• drm_mode_config_cleanup(ddev); omap_gem_deinit(ddev);

  destroy_workqueue(priv->wq);

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index b4d9c2062723..280cdd27bc8e 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -14,6 +14,7 @@ #include "dss/omapdss.h" #include "dss/dss.h"

+#include <drm/drm_atomic.h> #include <drm/drm_gem.h> #include <drm/omap_drm.h>

@@ -41,6 +42,15 @@ struct omap_drm_pipeline { unsigned int alias_id; };

+/*

• • Global private object state for tracking resources that are shared across
• • multiple kms objects (planes/crtcs/etc).
• */

+#define to_omap_global_state(x) container_of(x, struct omap_global_state, base)

Add empty line here.

+struct omap_global_state {

• struct drm_private_state base;

+};

• struct omap_drm_private { struct drm_device *ddev; struct device *dev;

@@ -61,6 +71,13 @@ struct omap_drm_private { unsigned int num_ovls; struct omap_hw_overlay *overlays[8];

• /*

• * Global private object state, Do not access directly, use
  

• * omap_global_get_state()
  

• */
  

• struct drm_modeset_lock glob_obj_lock;

This is not used... What am I missing?

• struct drm_private_obj glob_obj;

• struct drm_fb_helper *fbdev;

  struct workqueue_struct *wq;

@@ -88,5 +105,9 @@ struct omap_drm_private {

void omap_debugfs_init(struct drm_minor *minor); +struct omap_global_state *__must_check +omap_get_global_state(struct drm_atomic_state *s); +struct omap_global_state * +omap_get_existing_global_state(struct omap_drm_private *priv);

These could also be separated by empty lines. At least to my eyes it gets confusing if those declarations are not separated.

Tomi

On 12/10/2021 16:23, Neil Armstrong wrote:

...

...

+    struct drm_private_obj glob_obj;

struct drm_fb_helper *fbdev;         struct workqueue_struct *wq; @@ -88,5 +105,9 @@ struct omap_drm_private {       void omap_debugfs_init(struct drm_minor *minor); +struct omap_global_state *__must_check +omap_get_global_state(struct drm_atomic_state *s); +struct omap_global_state * +omap_get_existing_global_state(struct omap_drm_private *priv);

These could also be separated by empty lines. At least to my eyes it gets confusing if those declarations are not separated.

Atomic states can be extremely confusing, and hard to track. I checked and they do what they are documented for...

The omap_get_existing_global_state() is the most confusing since the result depends if we are in an atomic transaction of not.

So here I was just talking about the cosmetics, how the lines above look like. I have trouble seeing where the function declaration starts and where it ends without looking closely, as both lines of the declaration start at the first column, and there are no empty lines between the declarations.

But now that you mention, yes, the states are confusing =). And this series is somewhat difficult. I think it's important for future maintainability to include explanations and comments in this series for the confusing parts (plane-overlay mapping and state handling, mostly).

Tomi

On 12/10/2021 15:34, Tomi Valkeinen wrote:

On 23/09/2021 10:06, Neil Armstrong wrote:

...

From: Benoit Parrot bparrot@ti.com

(re)assign the hw overlays to planes based on required caps, and to handle situations where we could not modify an in-use plane.

This means all planes advertise the superset of formats and properties. Userspace must (as always) use atomic TEST_ONLY step for atomic updates, as not all planes may be available for use on every frame.

The mapping of hwoverlays to plane is stored in omap_global_state, so that state updates are atomically committed in the same way that plane/etc state updates are managed.  This is needed because the omap_plane_state keeps a pointer to the hwoverlay, and we don't want global state to become out of sync with the plane state if an atomic update fails, we hit deadlock/ backoff scenario, etc.  The use of global_state_lock keeps multiple parallel updates which both re-assign hwoverlays properly serialized.

Signed-off-by: Benoit Parrot bparrot@ti.com Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/omapdrm/omap_drv.h     |   3 +   drivers/gpu/drm/omapdrm/omap_overlay.c | 140 ++++++++++++++++++++   drivers/gpu/drm/omapdrm/omap_overlay.h |   9 ++   drivers/gpu/drm/omapdrm/omap_plane.c   | 170 ++++++++++++++++++++-----   4 files changed, 287 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 280cdd27bc8e..2d5928f05a23 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -49,6 +49,9 @@ struct omap_drm_pipeline {   #define to_omap_global_state(x) container_of(x, struct omap_global_state, base)   struct omap_global_state {       struct drm_private_state base;

+    /* global atomic state of assignment between overlays and planes */ +    struct drm_plane *hwoverlay_to_plane[8];   };     struct omap_drm_private { diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.c b/drivers/gpu/drm/omapdrm/omap_overlay.c index 2b1416d2aad2..f1a23c2203aa 100644 --- a/drivers/gpu/drm/omapdrm/omap_overlay.c +++ b/drivers/gpu/drm/omapdrm/omap_overlay.c @@ -21,6 +21,146 @@ static const char * const overlay_id_to_name[] = {       [OMAP_DSS_VIDEO3] = "vid3",   };   +static struct omap_hw_overlay * +omap_plane_find_free_overlay(struct drm_device *dev, +                 struct drm_plane *hwoverlay_to_plane[], +                 u32 caps, u32 fourcc, u32 crtc_mask) +{ +    struct omap_drm_private *priv = dev->dev_private; +    int i;

+    DBG("caps: %x fourcc: %x crtc: %x", caps, fourcc, crtc_mask);

+    for (i = 0; i < priv->num_ovls; i++) { +        struct omap_hw_overlay *cur = priv->overlays[i];

+        DBG("%d: id: %d cur->caps: %x cur->crtc: %x", +            cur->idx, cur->overlay_id, cur->caps, cur->possible_crtcs);

+        /* skip if already in-use */ +        if (hwoverlay_to_plane[cur->idx]) +            continue;

+        /* check if allowed on crtc */ +        if (!(cur->possible_crtcs & crtc_mask)) +            continue;

+        /* skip if doesn't support some required caps: */ +        if (caps & ~cur->caps) +            continue;

+        /* check supported format */ +        if (!dispc_ovl_color_mode_supported(priv->dispc, +                            cur->overlay_id, fourcc)) +            continue;

+        return cur; +    }

+    DBG("no match"); +    return NULL; +}

+int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane, +            u32 caps, u32 fourcc, u32 crtc_mask, +            struct omap_hw_overlay **overlay) +{ +    struct omap_drm_private *priv = s->dev->dev_private; +    struct omap_global_state *new_global_state, *old_global_state; +    struct drm_plane **overlay_map; +    struct omap_hw_overlay *ovl;

+    new_global_state = omap_get_global_state(s); +    if (IS_ERR(new_global_state)) +        return PTR_ERR(new_global_state);

+    /* +     * grab old_state after omap_get_global_state(), +     * since now we hold lock: +     */ +    old_global_state = omap_get_existing_global_state(priv); +    DBG("new_global_state: %p old_global_state: %p", +        new_global_state, old_global_state);

+    overlay_map = new_global_state->hwoverlay_to_plane;

+    if (!*overlay) { +        ovl = omap_plane_find_free_overlay(s->dev, overlay_map, +                           caps, fourcc, crtc_mask); +        if (!ovl) +            return -ENOMEM;

+        ovl->possible_crtcs = crtc_mask; +        overlay_map[ovl->idx] = plane; +        *overlay = ovl;

+        DBG("%s: assign to plane %s caps %x on crtc %x", +            (*overlay)->name, plane->name, caps, crtc_mask); +    }

+    return 0; +}

Why would one call this function with overlay == NULL? What does the function do in that case?

This function is only called with !*overlay, no ide why it would be called with *overlay != NULL... I'll try to simplify this.

...

+void omap_overlay_release(struct drm_atomic_state *s, +              struct drm_plane *plane, +              struct omap_hw_overlay *overlay)

It feels odd to pass both plane and overlay here. If we're unassigning (is that a verb? =) the overlay, isn't it enough to just pass either the plane or the overlay?

Passing the overlay is necessary when having 2 overlays for a plane, but the plane parameter is definitely unneeded here.

...

+{ +    struct omap_global_state *state = omap_get_global_state(s); +    struct drm_plane **overlay_map = state->hwoverlay_to_plane;

+    if (!overlay) +        return;

+    if (WARN_ON(!overlay_map[overlay->idx])) +        return; +    /* +     * Check that the overlay we are releasing is actually +     * assigned to the plane we are trying to release it from. +     */ +    if (overlay_map[overlay->idx] == plane) { +        DBG("%s: release from plane %s", overlay->name, plane->name);

+        overlay_map[overlay->idx] = NULL; +    }

So it's normal that overlay_map[overlay->idx] != plane? When does that happen?

It's impossible since the overlay is taken from the plane state, will simplify this, remove the plane parameter and remove the unnecessary checks.

...

+}

+void omap_overlay_disable(struct drm_atomic_state *s, +              struct drm_plane *plane, +              struct omap_hw_overlay *overlay) +{ +    struct omap_drm_private *priv = s->dev->dev_private; +    struct drm_plane **overlay_map; +    struct omap_global_state *old_state;

+    old_state = omap_get_existing_global_state(priv); +    overlay_map = old_state->hwoverlay_to_plane;

+    if (!overlay) +        return;

You can move the if above to the beginning of the func.

...

+    /* +     * Check that the overlay we are trying to disable has not +     * been re-assigned to another plane already +     */ +    if (!overlay_map[overlay->idx]) { +        DBG("%s: on %s disabled", overlay->name, plane->name);

+        /* disable the overlay */ +        dispc_ovl_enable(priv->dispc, overlay->overlay_id, false);

+        /* +         * Since we are disabling this overlay in this +         * atomic cycle we can reset the available crtcs +         * it can be used on +         */ +        overlay->possible_crtcs = (1 << priv->num_pipes) - 1;

This is changing data in the overlay struct itself. Shouldn't the mutable data be in the atomic state?

I'm also a bit confused on what the "possible_crtcs" is. But I think it's either a bitmask of all the crtcs when the overlay is free, or a mask of a single crtc when allocated.

I think a variable that's either 0 when the overlay is not assigned, or the mask of the crtc when in use, would be more clear. Also with some other name (active_crtc?).

I dropped entirely this possible_crtcs.

...

+    }

+    /* +     * Otherwise the overlay is still in use so leave it alone +     */ +}

This function also has interesting behavior, possibly not disabling the hw overlay. I think these new exposed overlay functions could use a comment above each of them, explaining what they do.

I renamed this function to "omap_overlay_update_state()" which will disable an overlay if not more used. This name looks much more appropriate.

...

static void omap_overlay_destroy(struct omap_hw_overlay *overlay)   {       kfree(overlay); diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.h b/drivers/gpu/drm/omapdrm/omap_overlay.h index 892fecb67adb..d5033ee481c2 100644 --- a/drivers/gpu/drm/omapdrm/omap_overlay.h +++ b/drivers/gpu/drm/omapdrm/omap_overlay.h @@ -28,4 +28,13 @@ struct omap_hw_overlay {     int omap_hwoverlays_init(struct omap_drm_private *priv);   void omap_hwoverlays_destroy(struct omap_drm_private *priv); +int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane, +            u32 caps, u32 fourcc, u32 crtc_mask, +            struct omap_hw_overlay **overlay); +void omap_overlay_release(struct drm_atomic_state *s, +              struct drm_plane *plane, +              struct omap_hw_overlay *overlay); +void omap_overlay_disable(struct drm_atomic_state *s, +              struct drm_plane *plane, +              struct omap_hw_overlay *overlay);   #endif /* __OMAPDRM_OVERLAY_H__ */ diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index bda794b4c915..4b400a8bfe9e 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -8,6 +8,7 @@   #include <drm/drm_atomic_helper.h>   #include <drm/drm_gem_atomic_helper.h>   #include <drm/drm_plane_helper.h> +#include <drm/drm_fourcc.h>     #include "omap_dmm_tiler.h"   #include "omap_drv.h" @@ -21,6 +22,8 @@   struct omap_plane_state {       /* Must be first. */       struct drm_plane_state base;

+    struct omap_hw_overlay *overlay;   };     #define to_omap_plane(x) container_of(x, struct omap_plane, base) @@ -29,8 +32,6 @@ struct omap_plane {       struct drm_plane base;       enum omap_plane_id id;       const char *name;

-    struct omap_hw_overlay *overlay;   };     static int omap_plane_prepare_fb(struct drm_plane *plane, @@ -58,10 +59,27 @@ static void omap_plane_atomic_update(struct drm_plane *plane,       struct omap_plane *omap_plane = to_omap_plane(plane);       struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,                                          plane); -    enum omap_plane_id ovl_id = omap_plane->overlay->overlay_id; +    struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, +                                       plane); +    struct omap_plane_state *new_omap_state; +    struct omap_plane_state *old_omap_state;       struct omap_overlay_info info; +    enum omap_plane_id ovl_id;       int ret;   +    new_omap_state = to_omap_plane_state(new_state); +    old_omap_state = to_omap_plane_state(old_state);

+    /* Cleanup previously held overlay if needed */ +    omap_overlay_disable(old_state->state, plane, old_omap_state->overlay);

+    if (!new_omap_state->overlay) { +        DBG("[PLANE:%d:%s] overlay_id: ??? (%p)", plane->base.id, plane->name, +            new_omap_state->overlay);

I wonder what the ??? means here.

Already in earlier patches I wondered if the DBG prints were good or not, but I thought I'll comment on those after I can run this and see what they actually print. But some of them felt like debug prints used during development, not something to leave in the production code (at least in the form they were).

I changed the print to something more interesting.

...

+        return; +    }

+    ovl_id = new_omap_state->overlay->overlay_id;       DBG("%s, crtc=%p fb=%p", omap_plane->name, new_state->crtc,           new_state->fb);   @@ -80,9 +98,9 @@ static void omap_plane_atomic_update(struct drm_plane *plane,       /* update scanout: */       omap_framebuffer_update_scanout(new_state->fb, new_state, &info);   -    DBG("%dx%d -> %dx%d (%d)", info.width, info.height, -            info.out_width, info.out_height, -            info.screen_width); +    DBG("%s: %dx%d -> %dx%d (%d)", +            new_omap_state->overlay->name, info.width, info.height, +            info.out_width, info.out_height, info.screen_width);       DBG("%d,%d %pad %pad", info.pos_x, info.pos_y,               &info.paddr, &info.p_uv_addr);   @@ -105,55 +123,66 @@ static void omap_plane_atomic_disable(struct drm_plane *plane,   {       struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,                                          plane); -    struct omap_drm_private *priv = plane->dev->dev_private; -    struct omap_plane *omap_plane = to_omap_plane(plane); -    enum omap_plane_id ovl_id = omap_plane->overlay->overlay_id; +    struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, +                                       plane); +    struct omap_plane_state *new_omap_state; +    struct omap_plane_state *old_omap_state;

+    new_omap_state = to_omap_plane_state(new_state); +    old_omap_state = to_omap_plane_state(old_state);

+    if (!old_omap_state->overlay) +        return;         new_state->rotation = DRM_MODE_ROTATE_0; -    new_state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : omap_plane->id; +    new_state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY +            ? 0 : old_omap_state->overlay->overlay_id;

I'm not sure if I'm right here, but... doesn't the above mean that the plane's zpos can change, depending on which hw overlay it happened to use?

I'll need to investigate more on this.

...

-    dispc_ovl_enable(priv->dispc, ovl_id, false); +    omap_overlay_disable(old_state->state, plane, old_omap_state->overlay); +    new_omap_state->overlay = NULL;   }   +#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))   static int omap_plane_atomic_check(struct drm_plane *plane,                      struct drm_atomic_state *state)   {       struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,                                            plane); +    struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, +                                         plane); +    struct drm_crtc *crtc;       struct omap_drm_private *priv = plane->dev->dev_private; +    struct omap_plane_state *omap_state = to_omap_plane_state(new_plane_state); +    struct omap_global_state *omap_overlay_global_state; +    u32 crtc_mask; +    u32 fourcc; +    u32 caps = 0; +    bool new_hw_overlay = false; +    int min_scale, max_scale; +    int ret;       struct drm_crtc_state *crtc_state;       u16 width, height;       u32 width_fp, height_fp;   -    if (!new_plane_state->fb) -        return 0; +    omap_overlay_global_state = omap_get_global_state(state); +    if (IS_ERR(omap_overlay_global_state)) +        return PTR_ERR(omap_overlay_global_state); +    DBG("%s: omap_overlay_global_state: %p", plane->name, +        omap_overlay_global_state);         dispc_ovl_get_max_size(priv->dispc, &width, &height);       width_fp = width << 16;       height_fp = height << 16;   -    /* crtc should only be NULL when disabling (i.e., !new_plane_state->fb) */ -    if (WARN_ON(!new_plane_state->crtc)) +    crtc = new_plane_state->crtc ? new_plane_state->crtc : plane->state->crtc; +    if (!crtc)           return 0;   -    crtc_state = drm_atomic_get_existing_crtc_state(state, -                            new_plane_state->crtc); +    crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);       /* we should have a crtc state if the plane is attached to a crtc */       if (WARN_ON(!crtc_state))           return 0;   -    if (!crtc_state->enable) -        return 0;

-    if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0) -        return -EINVAL;

-    if (new_plane_state->crtc_x + new_plane_state->crtc_w > crtc_state->adjusted_mode.hdisplay) -        return -EINVAL;

-    if (new_plane_state->crtc_y + new_plane_state->crtc_h > crtc_state->adjusted_mode.vdisplay) -        return -EINVAL;

/* Make sure dimensions are within bounds. */       if (new_plane_state->src_h > height_fp || new_plane_state->crtc_h > height)           return -EINVAL; @@ -161,9 +190,81 @@ static int omap_plane_atomic_check(struct drm_plane *plane,       if (new_plane_state->src_w > width_fp || new_plane_state->crtc_w > width)           return -EINVAL;   -    if (new_plane_state->rotation != DRM_MODE_ROTATE_0 && -        !omap_framebuffer_supports_rotation(new_plane_state->fb)) -        return -EINVAL;

+    /* +     * Note: these are just sanity checks to filter out totally bad scaling +     * factors. The real limits must be calculated case by case, and +     * unfortunately we currently do those checks only at the commit +     * phase in dispc. +     */ +    min_scale = FRAC_16_16(1, 8); +    max_scale = FRAC_16_16(8, 1);

+    ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, +                          min_scale, max_scale, +                          true, true);

This piece here feels a bit out of context (wrt. to this patch). Why do we need it here, when adding support to hw overlays?

And I think it's fine to use the above helper, but maybe it can be in a separate patch earlier?

I'll move it out of this patch.

...

+    if (ret) +        return ret;

+    DBG("%s: check (%d -> %d)", plane->name, +        old_plane_state->visible, new_plane_state->visible);

Here's an example of the debug prints I mentioned earlier. I think this prints:

"GFX: check (1 -> 0)"

I'm fine with terse debug prints, but... maybe something like "visible 1 -> 0" would be much better? =)

Fixed

...

+    if (new_plane_state->visible) { +        if (new_plane_state->rotation != DRM_MODE_ROTATE_0 && +            !omap_framebuffer_supports_rotation(new_plane_state->fb)) +            return -EINVAL;

+        if ((new_plane_state->src_w >> 16) != new_plane_state->crtc_w || +            (new_plane_state->src_h >> 16) != new_plane_state->crtc_h) +            caps |= OMAP_DSS_OVL_CAP_SCALE;

+        fourcc = new_plane_state->fb->format->format; +        crtc_mask = drm_crtc_mask(new_plane_state->crtc);

+        /* +         * (re)allocate hw overlay if we don't have one or +         * there is a caps mismatch +         */ +        if (!omap_state->overlay || +            (caps & ~omap_state->overlay->caps)) { +            new_hw_overlay = true; +        } else { +            /* check if allowed on crtc */ +            if (!(omap_state->overlay->possible_crtcs & crtc_mask)) +                new_hw_overlay = true;

+            /* check supported format */ +            if (!dispc_ovl_color_mode_supported(priv->dispc, +                        omap_state->overlay->overlay_id, +                        fourcc)) +                new_hw_overlay = true; +        }

+        if (new_hw_overlay) { +            struct omap_hw_overlay *old_ovl = omap_state->overlay; +            struct omap_hw_overlay *new_ovl = NULL;

+            omap_overlay_release(state, plane, old_ovl);

+            ret = omap_overlay_assign(state, plane, caps, +                          fourcc, crtc_mask, &new_ovl); +            if (ret) { +                DBG("%s: failed to assign hw_overlay(s)!", +                    plane->name); +                omap_state->overlay = NULL; +                return ret; +            }

+            omap_state->overlay = new_ovl; +        } +    } else { +        omap_overlay_release(state, plane, omap_state->overlay); +        omap_state->overlay = NULL; +    }

+    if (omap_state->overlay) +        DBG("plane: %s overlay_id: %d", plane->name, +            omap_state->overlay->overlay_id);         return 0;   } @@ -230,7 +331,7 @@ static void omap_plane_reset(struct drm_plane *plane)        * plane.        */       plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY -               ? 0 : omap_plane->overlay->overlay_id; +               ? 0 : omap_plane->id;

Hmm... On plane reset we now set the zpos to plane->id, but on disable we reset zpos to overlay->id. And before this patch we set the vice versa. I don't follow.

I'll aswell need to investigate more on this.

Tomi

Neil