Re: [PATCH 1/5] drm: Have the crtc code only reference master from legacy nodes

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

 



Hi

On Thu, Mar 13, 2014 at 11:57 AM, Thomas Hellstrom
<thellstrom@xxxxxxxxxx> wrote:
> control- and render nodes are intended to be master-less.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_crtc.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 3b7d32d..c9d895a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1398,7 +1398,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>         uint32_t __user *crtc_id;
>         uint32_t __user *connector_id;
>         uint32_t __user *encoder_id;
> -       struct drm_mode_group *mode_group;
> +       struct drm_mode_group *mode_group = NULL;

Why not move this mode_group=NULL; into the if() condition below?
Avoids global initialization and makes the "if() - else" parts easier
to read as you directly see that mode_group is set to NULL for
non-legacy nodes.

>
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>                 return -EINVAL;
> @@ -1429,8 +1429,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>         mutex_unlock(&file_priv->fbs_lock);
>
>         drm_modeset_lock_all(dev);
> -       mode_group = &file_priv->master->minor->mode_group;
> -       if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
> +       if (file_priv->minor->type != DRM_MINOR_LEGACY) {
>
>                 list_for_each(lh, &dev->mode_config.crtc_list)
>                         crtc_count++;
> @@ -1442,6 +1441,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>                         encoder_count++;
>         } else {
>
> +               mode_group = &file_priv->master->minor->mode_group;
>                 crtc_count = mode_group->num_crtcs;
>                 connector_count = mode_group->num_connectors;
>                 encoder_count = mode_group->num_encoders;
> @@ -1456,7 +1456,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>         if (card_res->count_crtcs >= crtc_count) {
>                 copied = 0;
>                 crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
> -               if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
> +               if (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

You set mode_group=NULL with your patch, so I think it's much easier
to read if you replace all these minor-id-tests with "if
(!mode_group)". This moves the group-selection to the head of the
function and makes the remaining parts just work on the selected group
(or global if NULL).

Same for the two conditions below..

Apart from that:
Reviewed-by: David Herrmann <dh.herrmann@xxxxxxxxx>

Thanks
David

>                         list_for_each_entry(crtc, &dev->mode_config.crtc_list,
>                                             head) {
>                                 DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
> @@ -1483,7 +1483,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>         if (card_res->count_encoders >= encoder_count) {
>                 copied = 0;
>                 encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
> -               if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
> +               if (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

>                         list_for_each_entry(encoder,
>                                             &dev->mode_config.encoder_list,
>                                             head) {
> @@ -1514,7 +1514,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>         if (card_res->count_connectors >= connector_count) {
>                 copied = 0;
>                 connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
> -               if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
> +               if (file_priv->minor->type != DRM_MINOR_LEGACY) {

if (!mode_group) {

>                         list_for_each_entry(connector,
>                                             &dev->mode_config.connector_list,
>                                             head) {
> @@ -2715,7 +2715,8 @@ int drm_mode_getfb(struct drm_device *dev,
>         r->bpp = fb->bits_per_pixel;
>         r->pitch = fb->pitches[0];
>         if (fb->funcs->create_handle) {
> -               if (file_priv->is_master || capable(CAP_SYS_ADMIN)) {
> +               if (file_priv->is_master || capable(CAP_SYS_ADMIN) ||
> +                   file_priv->minor->type == DRM_MINOR_CONTROL) {
>                         ret = fb->funcs->create_handle(fb, file_priv,
>                                                        &r->handle);
>                 } else {
> --
> 1.7.10.4
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://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