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:
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
