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;elseconnector->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