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 <ch...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
