[Intel-gfx] [RFC v4 12/25] drm/i915: Add drm_driver->initial_client_display callback

Daniel Vetter daniel at ffwll.ch
Fri Mar 1 11:51:47 UTC 2019


On Fri, Mar 1, 2019 at 12:46 PM Noralf Trønnes <noralf at tronnes.org> wrote:
>
>
>
> Den 16.04.2018 10.38, skrev Daniel Vetter:
> > On Sat, Apr 14, 2018 at 01:53:05PM +0200, Noralf Trønnes wrote:
> >> As part of moving the modesetting code out of drm_fb_helper and into
> >> drm_client, the drm_fb_helper_funcs->initial_config callback needs to go.
> >> Replace it with a drm_driver->initial_client_display callback that can
> >> work for all in-kernel clients.
> >>
> >> TODO:
> >> - Add a patch that moves the function out of intel_fbdev.c since it's not
> >>   fbdev specific anymore.
> >>
> >> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> >
> > So the reason we originally added this callback for i915 fast boot was
> > that there wasn't any atomic around yet. And it was all an experiment to
> > figure out how to best go about designing fastboot.
> >
> > But now we have fbdev, and fastboot design is also pretty clear:
> >
> > 1. driver loads
> > 2. driver reads out current hw state, reconstructs a full atomic state for
> > everything and stuffs it into connector/crtc/plane->state pointers.
> > 3. fbdev and any other client read out current state (with some caveats)
> > and just take it over.
> >
> > What non-fastboot drivers do:
> > 1. drivers load
> > 2. reset both hw and sw state to everything off.
> >
> > Now the intel_fb_initial_config is all generic code really, and it will
> > neatly fall back to the default config if everything is off. This means we
> > could:
> > 1. Move the intel_fb_initial_config into the fbdev helpers.
> > 2. Nuke the ->initial_config callback.
> >
> > And pronto! every driver which implements hw state readout will get fbdev
> > fastboot for free.
> >
>
> I'm back working on this now and there's one intel specific thing:
> num_pipes, in that function that I don't know how to handle when moving
> the code into the fbdev helper:

num_pipes == dev->mode_config.num_crtcs. Or should at least if we
managed to set up everything correctly. Not exactly sure why we've
used that intel-ism there, shouldn't have any effect when switching
over.
-Daniel

>
> static bool intel_fb_initial_config(...)
> {
> [...]
>         /*
>          * If the BIOS didn't enable everything it could, fall back to have the
>          * same user experiencing of lighting up as much as possible like the
>          * fbdev helper library.
>          */
>         if (num_connectors_enabled != num_connectors_detected &&
>             num_connectors_enabled < INTEL_INFO(dev_priv)->num_pipes) {
>                 DRM_DEBUG_KMS("fallback: Not all outputs enabled\n");
>                 DRM_DEBUG_KMS("Enabled: %i, detected: %i\n", num_connectors_enabled,
>                               num_connectors_detected);
>                 fallback = true;
>         }
>
> Noralf.
>
> > And since you've already rewritting the intel code to use drm_client, it's
> > practically done already. Just need to s/intel_/drm_fbdev_helper_ or
> > something like that :-)
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/drm_fb_helper.c    |  19 +++++--
> >>  drivers/gpu/drm/i915/i915_drv.c    |   1 +
> >>  drivers/gpu/drm/i915/intel_drv.h   |  11 ++++
> >>  drivers/gpu/drm/i915/intel_fbdev.c | 113 ++++++++++++++++++-------------------
> >>  include/drm/drm_drv.h              |  21 +++++++
> >>  5 files changed, 104 insertions(+), 61 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >> index b992f59dad30..5407bf6dc8c0 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -2103,6 +2103,20 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
> >>      /* prevent concurrent modification of connector_count by hotplug */
> >>      lockdep_assert_held(&fb_helper->lock);
> >>
> >> +    mutex_lock(&dev->mode_config.mutex);
> >> +    if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> >> +            DRM_DEBUG_KMS("No connectors reported connected with modes\n");
> >> +
> >> +    if (dev->driver->initial_client_display) {
> >> +            display = dev->driver->initial_client_display(dev, width, height);
> >> +            if (display) {
> >> +                    drm_client_display_free(fb_helper->display);
> >> +                    fb_helper->display = display;
> >> +                    mutex_unlock(&dev->mode_config.mutex);
> >> +                    return;
> >> +            }
> >> +    }
> >> +
> >>      crtcs = kcalloc(fb_helper->connector_count,
> >>                      sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
> >>      modes = kcalloc(fb_helper->connector_count,
> >> @@ -2120,9 +2134,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
> >>      if (IS_ERR(display))
> >>              goto out;
> >>
> >> -    mutex_lock(&fb_helper->dev->mode_config.mutex);
> >> -    if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> >> -            DRM_DEBUG_KMS("No connectors reported connected with modes\n");
> >>      drm_enable_connectors(fb_helper, enabled);
> >>
> >>      if (!(fb_helper->funcs->initial_config &&
> >> @@ -2144,7 +2155,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
> >>
> >>              drm_pick_crtcs(fb_helper, crtcs, modes, 0, width, height);
> >>      }
> >> -    mutex_unlock(&fb_helper->dev->mode_config.mutex);
> >>
> >>      /* need to set the modesets up here for use later */
> >>      /* fill out the connector<->crtc mappings into the modesets */
> >> @@ -2182,6 +2192,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
> >>      drm_client_display_free(fb_helper->display);
> >>      fb_helper->display = display;
> >>  out:
> >> +    mutex_unlock(&dev->mode_config.mutex);
> >>      kfree(crtcs);
> >>      kfree(modes);
> >>      kfree(offsets);
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 07c07d55398b..b746c0cbaa4b 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -2857,6 +2857,7 @@ static struct drm_driver driver = {
> >>
> >>      .dumb_create = i915_gem_dumb_create,
> >>      .dumb_map_offset = i915_gem_mmap_gtt,
> >> +    .initial_client_display = i915_initial_client_display,
> >>      .ioctls = i915_ioctls,
> >>      .num_ioctls = ARRAY_SIZE(i915_ioctls),
> >>      .fops = &i915_driver_fops,
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index d4368589b355..f77f510617c5 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1720,6 +1720,9 @@ extern void intel_fbdev_fini(struct drm_i915_private *dev_priv);
> >>  extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
> >>  extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
> >>  extern void intel_fbdev_restore_mode(struct drm_device *dev);
> >> +struct drm_client_display *
> >> +i915_initial_client_display(struct drm_device *dev, unsigned int width,
> >> +                        unsigned int height);
> >>  #else
> >>  static inline int intel_fbdev_init(struct drm_device *dev)
> >>  {
> >> @@ -1749,6 +1752,14 @@ static inline void intel_fbdev_output_poll_changed(struct drm_device *dev)
> >>  static inline void intel_fbdev_restore_mode(struct drm_device *dev)
> >>  {
> >>  }
> >> +
> >> +static inline struct drm_client_display *
> >> +i915_initial_client_display(struct drm_device *dev, unsigned int width,
> >> +                        unsigned int height)
> >> +{
> >> +    return NULL;
> >> +}
> >> +
> >>  #endif
> >>
> >>  /* intel_fbc.c */
> >> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> >> index a4ab8575a72e..b7f44c9475a8 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> >> @@ -38,6 +38,7 @@
> >>  #include <linux/vga_switcheroo.h>
> >>
> >>  #include <drm/drmP.h>
> >> +#include <drm/drm_client.h>
> >>  #include <drm/drm_crtc.h>
> >>  #include <drm/drm_fb_helper.h>
> >>  #include "intel_drv.h"
> >> @@ -287,18 +288,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >>      return ret;
> >>  }
> >>
> >> -static struct drm_fb_helper_crtc *
> >> -intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
> >> -{
> >> -    int i;
> >> -
> >> -    for (i = 0; i < fb_helper->crtc_count; i++)
> >> -            if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
> >> -                    return &fb_helper->crtc_info[i];
> >> -
> >> -    return NULL;
> >> -}
> >> -
> >>  /*
> >>   * Try to read the BIOS display configuration and use it for the initial
> >>   * fb configuration.
> >> @@ -326,44 +315,48 @@ intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
> >>   * is in VGA mode we need to recalculate watermarks and set a new high-res
> >>   * framebuffer anyway.
> >>   */
> >> -static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >> -                                struct drm_fb_helper_crtc **crtcs,
> >> -                                struct drm_display_mode **modes,
> >> -                                struct drm_fb_offset *offsets,
> >> -                                bool *enabled, int width, int height)
> >> +struct drm_client_display *
> >> +i915_initial_client_display(struct drm_device *dev, unsigned int width,
> >> +                        unsigned int height)
> >>  {
> >> -    struct drm_i915_private *dev_priv = to_i915(fb_helper->dev);
> >> +    struct drm_i915_private *dev_priv = to_i915(dev);
> >>      unsigned long conn_configured, conn_seq, mask;
> >> -    unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
> >> -    int i, j;
> >> -    bool *save_enabled;
> >> -    bool fallback = true, ret = true;
> >> +    bool fallback = true, *enabled = NULL;
> >> +    struct drm_client_display *display;
> >> +    struct drm_connector **connectors;
> >> +    int i, connector_count;
> >> +    unsigned int count;
> >>      int num_connectors_enabled = 0;
> >>      int num_connectors_detected = 0;
> >>      struct drm_modeset_acquire_ctx ctx;
> >>
> >> -    save_enabled = kcalloc(count, sizeof(bool), GFP_KERNEL);
> >> -    if (!save_enabled)
> >> -            return false;
> >> +    display = drm_client_display_create(dev);
> >> +    if (IS_ERR(display))
> >> +            return NULL;
> >>
> >>      drm_modeset_acquire_init(&ctx, 0);
> >>
> >> -    while (drm_modeset_lock_all_ctx(fb_helper->dev, &ctx) != 0)
> >> +    while (drm_modeset_lock_all_ctx(dev, &ctx) != 0)
> >>              drm_modeset_backoff(&ctx);
> >>
> >> -    memcpy(save_enabled, enabled, count);
> >> +    connector_count = drm_connector_get_all(dev, &connectors);
> >> +    if (connector_count < 1)
> >> +            goto bail;
> >> +
> >> +    enabled = drm_connector_get_enabled_status(connectors, connector_count);
> >> +    if (!enabled)
> >> +            goto bail;
> >> +
> >> +    count = min(connector_count, BITS_PER_LONG);
> >>      mask = GENMASK(count - 1, 0);
> >>      conn_configured = 0;
> >>  retry:
> >>      conn_seq = conn_configured;
> >>      for (i = 0; i < count; i++) {
> >> -            struct drm_fb_helper_connector *fb_conn;
> >> -            struct drm_connector *connector;
> >> +            struct drm_connector *connector = connectors[i];
> >> +            struct drm_display_mode *mode;
> >> +            struct drm_mode_set *modeset;
> >>              struct drm_encoder *encoder;
> >> -            struct drm_fb_helper_crtc *new_crtc;
> >> -
> >> -            fb_conn = fb_helper->connector_info[i];
> >> -            connector = fb_conn->connector;
> >>
> >>              if (conn_configured & BIT(i))
> >>                      continue;
> >> @@ -402,16 +395,13 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >>
> >>              num_connectors_enabled++;
> >>
> >> -            new_crtc = intel_fb_helper_crtc(fb_helper,
> >> -                                            connector->state->crtc);
> >> -
> >>              /*
> >>               * Make sure we're not trying to drive multiple connectors
> >>               * with a single CRTC, since our cloning support may not
> >>               * match the BIOS.
> >>               */
> >> -            for (j = 0; j < count; j++) {
> >> -                    if (crtcs[j] == new_crtc) {
> >> +            drm_client_display_for_each_modeset(modeset, display) {
> >> +                    if (modeset->connectors[0] == connector) {
> >>                              DRM_DEBUG_KMS("fallback: cloned configuration\n");
> >>                              goto bail;
> >>                      }
> >> @@ -421,28 +411,26 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >>                            connector->name);
> >>
> >>              /* go for command line mode first */
> >> -            modes[i] = drm_connector_pick_cmdline_mode(connector);
> >> +            mode = drm_connector_pick_cmdline_mode(connector);
> >>
> >>              /* try for preferred next */
> >> -            if (!modes[i]) {
> >> +            if (!mode) {
> >>                      DRM_DEBUG_KMS("looking for preferred mode on connector %s %d\n",
> >>                                    connector->name, connector->has_tile);
> >> -                    modes[i] = drm_connector_has_preferred_mode(connector,
> >> -                                                                width,
> >> -                                                                height);
> >> +                    mode = drm_connector_has_preferred_mode(connector,
> >> +                                                            width, height);
> >>              }
> >>
> >>              /* No preferred mode marked by the EDID? Are there any modes? */
> >> -            if (!modes[i] && !list_empty(&connector->modes)) {
> >> +            if (!mode && !list_empty(&connector->modes)) {
> >>                      DRM_DEBUG_KMS("using first mode listed on connector %s\n",
> >>                                    connector->name);
> >> -                    modes[i] = list_first_entry(&connector->modes,
> >> -                                                struct drm_display_mode,
> >> -                                                head);
> >> +                    mode = list_first_entry(&connector->modes,
> >> +                                            struct drm_display_mode, head);
> >>              }
> >>
> >>              /* last resort: use current mode */
> >> -            if (!modes[i]) {
> >> +            if (!mode) {
> >>                      /*
> >>                       * IMPORTANT: We want to use the adjusted mode (i.e.
> >>                       * after the panel fitter upscaling) as the initial
> >> @@ -458,16 +446,26 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >>                       */
> >>                      DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> >>                                    connector->name);
> >> -                    modes[i] = &connector->state->crtc->mode;
> >> +                    mode = &connector->state->crtc->mode;
> >>              }
> >> -            crtcs[i] = new_crtc;
> >> +
> >> +            modeset = drm_client_display_find_modeset(display, connector->state->crtc);
> >> +            if (WARN_ON(!modeset))
> >> +                    goto bail;
> >> +
> >> +            modeset->mode = drm_mode_duplicate(dev, mode);
> >> +            drm_connector_get(connector);
> >> +            modeset->connectors[0] = connector;
> >> +            modeset->num_connectors = 1;
> >> +            modeset->x = 0;
> >> +            modeset->y = 0;
> >>
> >>              DRM_DEBUG_KMS("connector %s on [CRTC:%d:%s]: %dx%d%s\n",
> >>                            connector->name,
> >>                            connector->state->crtc->base.id,
> >>                            connector->state->crtc->name,
> >> -                          modes[i]->hdisplay, modes[i]->vdisplay,
> >> -                          modes[i]->flags & DRM_MODE_FLAG_INTERLACE ? "i" :"");
> >> +                          mode->hdisplay, mode->vdisplay,
> >> +                          mode->flags & DRM_MODE_FLAG_INTERLACE ? "i" : "");
> >>
> >>              fallback = false;
> >>              conn_configured |= BIT(i);
> >> @@ -492,19 +490,20 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >>      if (fallback) {
> >>  bail:
> >>              DRM_DEBUG_KMS("Not using firmware configuration\n");
> >> -            memcpy(enabled, save_enabled, count);
> >> -            ret = false;
> >> +            drm_client_display_free(display);
> >> +            display = NULL;
> >>      }
> >>
> >>      drm_modeset_drop_locks(&ctx);
> >>      drm_modeset_acquire_fini(&ctx);
> >>
> >> -    kfree(save_enabled);
> >> -    return ret;
> >> +    drm_connector_put_all(connectors, connector_count);
> >> +    kfree(enabled);
> >> +
> >> +    return display;
> >>  }
> >>
> >>  static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> >> -    .initial_config = intel_fb_initial_config,
> >>      .fb_probe = intelfb_create,
> >>  };
> >>
> >> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >> index 7e545f5f94d3..13356e6fd40c 100644
> >> --- a/include/drm/drm_drv.h
> >> +++ b/include/drm/drm_drv.h
> >> @@ -32,6 +32,7 @@
> >>
> >>  #include <drm/drm_device.h>
> >>
> >> +struct drm_client_display;
> >>  struct drm_file;
> >>  struct drm_gem_object;
> >>  struct drm_master;
> >> @@ -553,6 +554,26 @@ struct drm_driver {
> >>                          struct drm_device *dev,
> >>                          uint32_t handle);
> >>
> >> +    /**
> >> +     * @initial_client_config:
> >> +     *
> >> +     * Driver callback to setup an initial fbdev display configuration.
> >> +     * Drivers can use this callback to tell the fbdev emulation what the
> >> +     * preferred initial configuration is. This is useful to implement
> >> +     * smooth booting where the fbdev (and subsequently all userspace) never
> >> +     * changes the mode, but always inherits the existing configuration.
> >> +     *
> >> +     * This callback is optional.
> >> +     *
> >> +     * RETURNS:
> >> +     *
> >> +     * The driver should return true if a suitable initial configuration has
> >> +     * been filled out and false when the fbdev helper should fall back to
> >> +     * the default probing logic.
> >> +     */
> >> +    struct drm_client_display *(*initial_client_display)(struct drm_device *dev,
> >> +                                    unsigned int width, unsigned int height);
> >> +
> >>      /**
> >>       * @gem_vm_ops: Driver private ops for this object
> >>       */
> >> --
> >> 2.15.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >



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


More information about the Intel-gfx mailing list