Re: [PATCH 09/13] drm: Tighten locking in drm_mode_getconnector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 16, 2016 at 10:03:32AM -0500, Sean Paul wrote:
> On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> > - Modeset state needs mode_config->connection mutex, that covers
> >   figuring out the encoder, and reading properties (since in the
> >   atomic case those need to look at connector->state).
> >
> > - Don't hold any locks for stuff that's invariant (i.e. possible
> >   connectors).
> >
> > - Same for connector lookup and unref, those don't need any locks.
> >
> > - And finally the probe stuff is only protected by mode_config->mutex.
> >
> > While at it updated the kerneldoc for these fields in drm_connector
> > and add docs explaining what's protected by which locks.
> >
> 
> 
> Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>

Merged up to this patch to drm-misc, thanks for the reviews to everyone.
-Daniel

> 
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_connector.c | 92 ++++++++++++++++++++---------------------
> >  include/drm/drm_connector.h     | 23 +++++++++--
> >  2 files changed, 63 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 0d4728704099..44b556d5d40c 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1160,43 +1160,65 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> >
> >         memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo));
> >
> > -       mutex_lock(&dev->mode_config.mutex);
> > -
> >         connector = drm_connector_lookup(dev, out_resp->connector_id);
> > -       if (!connector) {
> > -               ret = -ENOENT;
> > -               goto out_unlock;
> > -       }
> > +       if (!connector)
> > +               return -ENOENT;
> > +
> > +       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > +       encoder = drm_connector_get_encoder(connector);
> > +       if (encoder)
> > +               out_resp->encoder_id = encoder->base.id;
> > +       else
> > +               out_resp->encoder_id = 0;
> > +
> > +       ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
> > +                       (uint32_t __user *)(unsigned long)(out_resp->props_ptr),
> > +                       (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
> > +                       &out_resp->count_props);
> > +       drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > +       if (ret)
> > +               goto out_unref;
> >
> >         for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> >                 if (connector->encoder_ids[i] != 0)
> >                         encoders_count++;
> >
> > +       if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
> > +               copied = 0;
> > +               encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
> > +               for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > +                       if (connector->encoder_ids[i] != 0) {
> > +                               if (put_user(connector->encoder_ids[i],
> > +                                            encoder_ptr + copied)) {
> > +                                       ret = -EFAULT;
> > +                                       goto out_unref;
> > +                               }
> > +                               copied++;
> > +                       }
> > +               }
> > +       }
> > +       out_resp->count_encoders = encoders_count;
> > +
> > +       out_resp->connector_id = connector->base.id;
> > +       out_resp->connector_type = connector->connector_type;
> > +       out_resp->connector_type_id = connector->connector_type_id;
> > +
> > +       mutex_lock(&dev->mode_config.mutex);
> >         if (out_resp->count_modes == 0) {
> >                 connector->funcs->fill_modes(connector,
> >                                              dev->mode_config.max_width,
> >                                              dev->mode_config.max_height);
> >         }
> >
> > -       /* delayed so we get modes regardless of pre-fill_modes state */
> > -       list_for_each_entry(mode, &connector->modes, head)
> > -               if (drm_mode_expose_to_userspace(mode, file_priv))
> > -                       mode_count++;
> > -
> > -       out_resp->connector_id = connector->base.id;
> > -       out_resp->connector_type = connector->connector_type;
> > -       out_resp->connector_type_id = connector->connector_type_id;
> >         out_resp->mm_width = connector->display_info.width_mm;
> >         out_resp->mm_height = connector->display_info.height_mm;
> >         out_resp->subpixel = connector->display_info.subpixel_order;
> >         out_resp->connection = connector->status;
> >
> > -       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > -       encoder = drm_connector_get_encoder(connector);
> > -       if (encoder)
> > -               out_resp->encoder_id = encoder->base.id;
> > -       else
> > -               out_resp->encoder_id = 0;
> > +       /* delayed so we get modes regardless of pre-fill_modes state */
> > +       list_for_each_entry(mode, &connector->modes, head)
> > +               if (drm_mode_expose_to_userspace(mode, file_priv))
> > +                       mode_count++;
> >
> >         /*
> >          * This ioctl is called twice, once to determine how much space is
> > @@ -1219,36 +1241,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> >                 }
> >         }
> >         out_resp->count_modes = mode_count;
> > -
> > -       ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
> > -                       (uint32_t __user *)(unsigned long)(out_resp->props_ptr),
> > -                       (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
> > -                       &out_resp->count_props);
> > -       if (ret)
> > -               goto out;
> > -
> > -       if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
> > -               copied = 0;
> > -               encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
> > -               for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > -                       if (connector->encoder_ids[i] != 0) {
> > -                               if (put_user(connector->encoder_ids[i],
> > -                                            encoder_ptr + copied)) {
> > -                                       ret = -EFAULT;
> > -                                       goto out;
> > -                               }
> > -                               copied++;
> > -                       }
> > -               }
> > -       }
> > -       out_resp->count_encoders = encoders_count;
> > -
> >  out:
> > -       drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > -
> > -       drm_connector_unreference(connector);
> > -out_unlock:
> >         mutex_unlock(&dev->mode_config.mutex);
> > +out_unref:
> > +       drm_connector_unreference(connector);
> >
> >         return ret;
> >  }
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index a24559ef8bb7..9af4141165d3 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -559,9 +559,6 @@ struct drm_cmdline_mode {
> >   * @interlace_allowed: can this connector handle interlaced modes?
> >   * @doublescan_allowed: can this connector handle doublescan?
> >   * @stereo_allowed: can this connector handle stereo modes?
> > - * @modes: modes available on this connector (from fill_modes() + user)
> > - * @status: one of the drm_connector_status enums (connected, not, or unknown)
> > - * @probed_modes: list of modes derived directly from the display
> >   * @funcs: connector control functions
> >   * @edid_blob_ptr: DRM property containing EDID if present
> >   * @properties: property tracking for this connector
> > @@ -631,11 +628,27 @@ struct drm_connector {
> >          * Protected by @mutex.
> >          */
> >         bool registered;
> > +
> > +       /**
> > +        * @modes:
> > +        * Modes available on this connector (from fill_modes() + user).
> > +        * Protected by dev->mode_config.mutex.
> > +        */
> >         struct list_head modes; /* list of modes on this connector */
> >
> > +       /**
> > +        * @status:
> > +        * One of the drm_connector_status enums (connected, not, or unknown).
> > +        * Protected by dev->mode_config.mutex.
> > +        */
> >         enum drm_connector_status status;
> >
> > -       /* these are modes added by probing with DDC or the BIOS */
> > +       /**
> > +        * @probed_modes:
> > +        * These are modes added by probing with DDC or the BIOS, before
> > +        * filtering is applied. Used by the probe helpers.Protected by
> > +        * dev->mode_config.mutex.
> > +        */
> >         struct list_head probed_modes;
> >
> >         /**
> > @@ -644,6 +657,8 @@ struct drm_connector {
> >          * flat panels in embedded systems, the driver should initialize the
> >          * display_info.width_mm and display_info.height_mm fields with the
> >          * physical size of the display.
> > +        *
> > +        * Protected by dev->mode_config.mutex.
> >          */
> >         struct drm_display_info display_info;
> >         const struct drm_connector_funcs *funcs;
> > --
> > 2.11.0
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux