| Message ID | 1447951610-12622-22-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote: > To avoid even more code duplication punt this all to the probe worker, > which needs some slight adjustment to also generate a uevent when the > status has changed to due connector->force. > > v2: Instead of running the output_poll_work (which is kinda the wrong > thing and a layering violation since it's an internal of the probe > helpers), or calling ->detect (which is again a layering violation > since it's used only by probe helpers) just call the official > ->fill_modes function, like a GET_CONNECTOR ioctl call. > > v3: Restore the accidentally removed forced-probe for echo "detect" > > force. > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index 9ac4ffa6cce3..df66d9447cb0 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, > { > struct drm_connector *connector = to_drm_connector(device); > struct drm_device *dev = connector->dev; > - enum drm_connector_status old_status; > + enum drm_connector_force old_force; > int ret; > > ret = mutex_lock_interruptible(&dev->mode_config.mutex); > if (ret) > return ret; > > - old_status = connector->status; > + old_force = connector->force; > > - if (sysfs_streq(buf, "detect")) { > + if (sysfs_streq(buf, "detect")) > connector->force = 0; > - connector->status = connector->funcs->detect(connector, true); > - } else if (sysfs_streq(buf, "on")) { > + else if (sysfs_streq(buf, "on")) > connector->force = DRM_FORCE_ON; > - } else if (sysfs_streq(buf, "on-digital")) { > + else if (sysfs_streq(buf, "on-digital")) > connector->force = DRM_FORCE_ON_DIGITAL; > - } else if (sysfs_streq(buf, "off")) { > + else if (sysfs_streq(buf, "off")) > connector->force = DRM_FORCE_OFF; > - } else > + else > ret = -EINVAL; > > - if (ret == 0 && connector->force) { > - if (connector->force == DRM_FORCE_ON || > - connector->force == DRM_FORCE_ON_DIGITAL) > - connector->status = connector_status_connected; > - else > - connector->status = connector_status_disconnected; > - if (connector->funcs->force) > - connector->funcs->force(connector); > - } > - > - if (old_status != connector->status) { > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", > + if (old_force != connector->force || !connector->force) { > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", > connector->base.id, > connector->name, > - old_status, connector->status); > + old_force, connector->force); > > - dev->mode_config.delayed_event = true; > - if (dev->mode_config.poll_enabled) > - schedule_delayed_work(&dev->mode_config.output_poll_work, > - 0); > + connector->funcs->fill_modes(connector, > + dev->mode_config.max_width, > + dev->mode_config.max_height); This now does fill_modes() before we call detect(). We rely on the ordering of detect() before doing fill_modes() as in many cases we use the EDID gathered in detect() to generate the modes (if I am not mistaken, I think we merged those patches to cache the EDID for a detection cycle). -Chris
On Thu, Nov 19, 2015 at 09:06:04PM +0000, Chris Wilson wrote: > On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote: > > To avoid even more code duplication punt this all to the probe worker, > > which needs some slight adjustment to also generate a uevent when the > > status has changed to due connector->force. > > > > v2: Instead of running the output_poll_work (which is kinda the wrong > > thing and a layering violation since it's an internal of the probe > > helpers), or calling ->detect (which is again a layering violation > > since it's used only by probe helpers) just call the official > > ->fill_modes function, like a GET_CONNECTOR ioctl call. > > > > v3: Restore the accidentally removed forced-probe for echo "detect" > > > force. > > > > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > > index 9ac4ffa6cce3..df66d9447cb0 100644 > > --- a/drivers/gpu/drm/drm_sysfs.c > > +++ b/drivers/gpu/drm/drm_sysfs.c > > @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, > > { > > struct drm_connector *connector = to_drm_connector(device); > > struct drm_device *dev = connector->dev; > > - enum drm_connector_status old_status; > > + enum drm_connector_force old_force; > > int ret; > > > > ret = mutex_lock_interruptible(&dev->mode_config.mutex); > > if (ret) > > return ret; > > > > - old_status = connector->status; > > + old_force = connector->force; > > > > - if (sysfs_streq(buf, "detect")) { > > + if (sysfs_streq(buf, "detect")) > > connector->force = 0; > > - connector->status = connector->funcs->detect(connector, true); > > - } else if (sysfs_streq(buf, "on")) { > > + else if (sysfs_streq(buf, "on")) > > connector->force = DRM_FORCE_ON; > > - } else if (sysfs_streq(buf, "on-digital")) { > > + else if (sysfs_streq(buf, "on-digital")) > > connector->force = DRM_FORCE_ON_DIGITAL; > > - } else if (sysfs_streq(buf, "off")) { > > + else if (sysfs_streq(buf, "off")) > > connector->force = DRM_FORCE_OFF; > > - } else > > + else > > ret = -EINVAL; > > > > - if (ret == 0 && connector->force) { > > - if (connector->force == DRM_FORCE_ON || > > - connector->force == DRM_FORCE_ON_DIGITAL) > > - connector->status = connector_status_connected; > > - else > > - connector->status = connector_status_disconnected; > > - if (connector->funcs->force) > > - connector->funcs->force(connector); > > - } > > - > > - if (old_status != connector->status) { > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", > > + if (old_force != connector->force || !connector->force) { > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", > > connector->base.id, > > connector->name, > > - old_status, connector->status); > > + old_force, connector->force); > > > > - dev->mode_config.delayed_event = true; > > - if (dev->mode_config.poll_enabled) > > - schedule_delayed_work(&dev->mode_config.output_poll_work, > > - 0); > > + connector->funcs->fill_modes(connector, > > + dev->mode_config.max_width, > > + dev->mode_config.max_height); > > This now does fill_modes() before we call detect(). We rely on the > ordering of detect() before doing fill_modes() as in many cases we use > the EDID gathered in detect() to generate the modes (if I am not > mistaken, I think we merged those patches to cache the EDID for a > detection cycle). ->fill_modes = drm_helper_probe_single_connector_modes which then calls ->detect. By going this way we avoid duplicating the "send uevent if anything changed" logic all over the place, and ->detect becomes purely a helper internal callback to get at the mode list for hotpluggeable outputs. Yes this means that ->detect should become a helper function, but that's quite an invasive change. -Daniel
On Fri, Nov 20, 2015 at 09:11:00AM +0100, Daniel Vetter wrote: > On Thu, Nov 19, 2015 at 09:06:04PM +0000, Chris Wilson wrote: > > On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote: > > > To avoid even more code duplication punt this all to the probe worker, > > > which needs some slight adjustment to also generate a uevent when the > > > status has changed to due connector->force. > > > > > > v2: Instead of running the output_poll_work (which is kinda the wrong > > > thing and a layering violation since it's an internal of the probe > > > helpers), or calling ->detect (which is again a layering violation > > > since it's used only by probe helpers) just call the official > > > ->fill_modes function, like a GET_CONNECTOR ioctl call. > > > > > > v3: Restore the accidentally removed forced-probe for echo "detect" > > > > force. > > > > > > > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > > > index 9ac4ffa6cce3..df66d9447cb0 100644 > > > --- a/drivers/gpu/drm/drm_sysfs.c > > > +++ b/drivers/gpu/drm/drm_sysfs.c > > > @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, > > > { > > > struct drm_connector *connector = to_drm_connector(device); > > > struct drm_device *dev = connector->dev; > > > - enum drm_connector_status old_status; > > > + enum drm_connector_force old_force; > > > int ret; > > > > > > ret = mutex_lock_interruptible(&dev->mode_config.mutex); > > > if (ret) > > > return ret; > > > > > > - old_status = connector->status; > > > + old_force = connector->force; > > > > > > - if (sysfs_streq(buf, "detect")) { > > > + if (sysfs_streq(buf, "detect")) > > > connector->force = 0; > > > - connector->status = connector->funcs->detect(connector, true); > > > - } else if (sysfs_streq(buf, "on")) { > > > + else if (sysfs_streq(buf, "on")) > > > connector->force = DRM_FORCE_ON; > > > - } else if (sysfs_streq(buf, "on-digital")) { > > > + else if (sysfs_streq(buf, "on-digital")) > > > connector->force = DRM_FORCE_ON_DIGITAL; > > > - } else if (sysfs_streq(buf, "off")) { > > > + else if (sysfs_streq(buf, "off")) > > > connector->force = DRM_FORCE_OFF; > > > - } else > > > + else > > > ret = -EINVAL; > > > > > > - if (ret == 0 && connector->force) { > > > - if (connector->force == DRM_FORCE_ON || > > > - connector->force == DRM_FORCE_ON_DIGITAL) > > > - connector->status = connector_status_connected; > > > - else > > > - connector->status = connector_status_disconnected; > > > - if (connector->funcs->force) > > > - connector->funcs->force(connector); > > > - } > > > - > > > - if (old_status != connector->status) { > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", > > > + if (old_force != connector->force || !connector->force) { > > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", > > > connector->base.id, > > > connector->name, > > > - old_status, connector->status); > > > + old_force, connector->force); > > > > > > - dev->mode_config.delayed_event = true; > > > - if (dev->mode_config.poll_enabled) > > > - schedule_delayed_work(&dev->mode_config.output_poll_work, > > > - 0); > > > + connector->funcs->fill_modes(connector, > > > + dev->mode_config.max_width, > > > + dev->mode_config.max_height); > > > > This now does fill_modes() before we call detect(). We rely on the > > ordering of detect() before doing fill_modes() as in many cases we use > > the EDID gathered in detect() to generate the modes (if I am not > > mistaken, I think we merged those patches to cache the EDID for a > > detection cycle). > > ->fill_modes = drm_helper_probe_single_connector_modes which then calls > ->detect. By going this way we avoid duplicating the "send uevent if > anything changed" logic all over the place, and ->detect becomes purely a > helper internal callback to get at the mode list for hotpluggeable > outputs. Ok, that struck me as a little surprising - I was thinking of lower level get_modes apparently. With that resolved, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > Yes this means that ->detect should become a helper function, but that's > quite an invasive change. And something like .fill_modes -> .probe (after removing .detect). -Chris
On Fri, Nov 20, 2015 at 09:25:14AM +0000, Chris Wilson wrote: > On Fri, Nov 20, 2015 at 09:11:00AM +0100, Daniel Vetter wrote: > > On Thu, Nov 19, 2015 at 09:06:04PM +0000, Chris Wilson wrote: > > > On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote: > > > > To avoid even more code duplication punt this all to the probe worker, > > > > which needs some slight adjustment to also generate a uevent when the > > > > status has changed to due connector->force. > > > > > > > > v2: Instead of running the output_poll_work (which is kinda the wrong > > > > thing and a layering violation since it's an internal of the probe > > > > helpers), or calling ->detect (which is again a layering violation > > > > since it's used only by probe helpers) just call the official > > > > ->fill_modes function, like a GET_CONNECTOR ioctl call. > > > > > > > > v3: Restore the accidentally removed forced-probe for echo "detect" > > > > > force. > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > > > > index 9ac4ffa6cce3..df66d9447cb0 100644 > > > > --- a/drivers/gpu/drm/drm_sysfs.c > > > > +++ b/drivers/gpu/drm/drm_sysfs.c > > > > @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, > > > > { > > > > struct drm_connector *connector = to_drm_connector(device); > > > > struct drm_device *dev = connector->dev; > > > > - enum drm_connector_status old_status; > > > > + enum drm_connector_force old_force; > > > > int ret; > > > > > > > > ret = mutex_lock_interruptible(&dev->mode_config.mutex); > > > > if (ret) > > > > return ret; > > > > > > > > - old_status = connector->status; > > > > + old_force = connector->force; > > > > > > > > - if (sysfs_streq(buf, "detect")) { > > > > + if (sysfs_streq(buf, "detect")) > > > > connector->force = 0; > > > > - connector->status = connector->funcs->detect(connector, true); > > > > - } else if (sysfs_streq(buf, "on")) { > > > > + else if (sysfs_streq(buf, "on")) > > > > connector->force = DRM_FORCE_ON; > > > > - } else if (sysfs_streq(buf, "on-digital")) { > > > > + else if (sysfs_streq(buf, "on-digital")) > > > > connector->force = DRM_FORCE_ON_DIGITAL; > > > > - } else if (sysfs_streq(buf, "off")) { > > > > + else if (sysfs_streq(buf, "off")) > > > > connector->force = DRM_FORCE_OFF; > > > > - } else > > > > + else > > > > ret = -EINVAL; > > > > > > > > - if (ret == 0 && connector->force) { > > > > - if (connector->force == DRM_FORCE_ON || > > > > - connector->force == DRM_FORCE_ON_DIGITAL) > > > > - connector->status = connector_status_connected; > > > > - else > > > > - connector->status = connector_status_disconnected; > > > > - if (connector->funcs->force) > > > > - connector->funcs->force(connector); > > > > - } > > > > - > > > > - if (old_status != connector->status) { > > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", > > > > + if (old_force != connector->force || !connector->force) { > > > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", > > > > connector->base.id, > > > > connector->name, > > > > - old_status, connector->status); > > > > + old_force, connector->force); > > > > > > > > - dev->mode_config.delayed_event = true; > > > > - if (dev->mode_config.poll_enabled) > > > > - schedule_delayed_work(&dev->mode_config.output_poll_work, > > > > - 0); > > > > + connector->funcs->fill_modes(connector, > > > > + dev->mode_config.max_width, > > > > + dev->mode_config.max_height); > > > > > > This now does fill_modes() before we call detect(). We rely on the > > > ordering of detect() before doing fill_modes() as in many cases we use > > > the EDID gathered in detect() to generate the modes (if I am not > > > mistaken, I think we merged those patches to cache the EDID for a > > > detection cycle). > > > > ->fill_modes = drm_helper_probe_single_connector_modes which then calls > > ->detect. By going this way we avoid duplicating the "send uevent if > > anything changed" logic all over the place, and ->detect becomes purely a > > helper internal callback to get at the mode list for hotpluggeable > > outputs. > > Ok, that struck me as a little surprising - I was thinking of lower level > get_modes apparently. > > With that resolved, > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Applied to drm-misc, thanks for the review. > > Yes this means that ->detect should become a helper function, but that's > > quite an invasive change. > > And something like .fill_modes -> .probe (after removing .detect). Hm, not sure. For panels we never really probe anything, the important bit is (at least for the caller in drm_crtc.c) that it fills in the connectore->modes list. Given that I think the current name is okish. -Daniel
On Tue, Nov 24, 2015 at 11:51:09AM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 09:25:14AM +0000, Chris Wilson wrote: > > And something like .fill_modes -> .probe (after removing .detect). > > Hm, not sure. For panels we never really probe anything, the important bit > is (at least for the caller in drm_crtc.c) that it fills in the > connectore->modes list. Given that I think the current name is okish. imo .fill_modes() does not imply that it communicates with the output, unlike say .detect(), .probe(), or .get_modes(). -Chris
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index a214a4a93b03..b7bdf12c54a5 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -147,6 +147,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect list_for_each_entry(mode, &connector->modes, head) mode->status = MODE_UNVERIFIED; + old_status = connector->status; + if (connector->force) { if (connector->force == DRM_FORCE_ON || connector->force == DRM_FORCE_ON_DIGITAL) @@ -156,33 +158,31 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect if (connector->funcs->force) connector->funcs->force(connector); } else { - old_status = connector->status; - connector->status = connector->funcs->detect(connector, true); + } + + /* + * Normally either the driver's hpd code or the poll loop should + * pick up any changes and fire the hotplug event. But if + * userspace sneaks in a probe, we might miss a change. Hence + * check here, and if anything changed start the hotplug code. + */ + if (old_status != connector->status) { + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", + connector->base.id, + connector->name, + old_status, connector->status); /* - * Normally either the driver's hpd code or the poll loop should - * pick up any changes and fire the hotplug event. But if - * userspace sneaks in a probe, we might miss a change. Hence - * check here, and if anything changed start the hotplug code. + * The hotplug event code might call into the fb + * helpers, and so expects that we do not hold any + * locks. Fire up the poll struct instead, it will + * disable itself again. */ - if (old_status != connector->status) { - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", - connector->base.id, - connector->name, - old_status, connector->status); - - /* - * The hotplug event code might call into the fb - * helpers, and so expects that we do not hold any - * locks. Fire up the poll struct instead, it will - * disable itself again. - */ - dev->mode_config.delayed_event = true; - if (dev->mode_config.poll_enabled) - schedule_delayed_work(&dev->mode_config.output_poll_work, - 0); - } + dev->mode_config.delayed_event = true; + if (dev->mode_config.poll_enabled) + schedule_delayed_work(&dev->mode_config.output_poll_work, + 0); } /* Re-enable polling in case the global poll config changed. */ diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 9ac4ffa6cce3..df66d9447cb0 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, { struct drm_connector *connector = to_drm_connector(device); struct drm_device *dev = connector->dev; - enum drm_connector_status old_status; + enum drm_connector_force old_force; int ret; ret = mutex_lock_interruptible(&dev->mode_config.mutex); if (ret) return ret; - old_status = connector->status; + old_force = connector->force; - if (sysfs_streq(buf, "detect")) { + if (sysfs_streq(buf, "detect")) connector->force = 0; - connector->status = connector->funcs->detect(connector, true); - } else if (sysfs_streq(buf, "on")) { + else if (sysfs_streq(buf, "on")) connector->force = DRM_FORCE_ON; - } else if (sysfs_streq(buf, "on-digital")) { + else if (sysfs_streq(buf, "on-digital")) connector->force = DRM_FORCE_ON_DIGITAL; - } else if (sysfs_streq(buf, "off")) { + else if (sysfs_streq(buf, "off")) connector->force = DRM_FORCE_OFF; - } else + else ret = -EINVAL; - if (ret == 0 && connector->force) { - if (connector->force == DRM_FORCE_ON || - connector->force == DRM_FORCE_ON_DIGITAL) - connector->status = connector_status_connected; - else - connector->status = connector_status_disconnected; - if (connector->funcs->force) - connector->funcs->force(connector); - } - - if (old_status != connector->status) { - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", + if (old_force != connector->force || !connector->force) { + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or reprobing\n", connector->base.id, connector->name, - old_status, connector->status); + old_force, connector->force); - dev->mode_config.delayed_event = true; - if (dev->mode_config.poll_enabled) - schedule_delayed_work(&dev->mode_config.output_poll_work, - 0); + connector->funcs->fill_modes(connector, + dev->mode_config.max_width, + dev->mode_config.max_height); } mutex_unlock(&dev->mode_config.mutex);
To avoid even more code duplication punt this all to the probe worker, which needs some slight adjustment to also generate a uevent when the status has changed to due connector->force. v2: Instead of running the output_poll_work (which is kinda the wrong thing and a layering violation since it's an internal of the probe helpers), or calling ->detect (which is again a layering violation since it's used only by probe helpers) just call the official ->fill_modes function, like a GET_CONNECTOR ioctl call. v3: Restore the accidentally removed forced-probe for echo "detect" > force. Cc: Chris Wilson <chris@chris-wilson.co.uk> Reported-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_probe_helper.c | 46 +++++++++++++++++++------------------- drivers/gpu/drm/drm_sysfs.c | 38 +++++++++++-------------------- 2 files changed, 36 insertions(+), 48 deletions(-)