On Fri, Mar 1, 2019 at 12:46 PM Noralf Trønnes <noralf@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx