On Fri, Mar 1, 2019 at 12:46 PM Noralf Trønnes <nor...@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 <nor...@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-...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
