Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180221144919.GN5453@intel.com>
References: <20180221143730.30285-1-robdclark@gmail.com> <20180221143730.30285-2-robdclark@gmail.com>
 <20180221144919.GN5453@intel.com>
From:   Rob Clark <robdclark@gmail.com>
Date:   Wed, 21 Feb 2018 09:54:49 -0500
Message-ID: <CAF6AEGuYmLRphz1FusqaOqyz=mGtbvn7oOsxNYKg3MEMOvYO1A@mail.gmail.com>
Subject: Re: [PATCH 1/4] drm/atomic: integrate modeset lock with private objects
To:     =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>
Cc:     dri-devel <dri-devel@lists.freedesktop.org>,
        David Airlie <airlied@linux.ie>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Feb 21, 2018 at 9:49 AM, Ville Syrj=C3=A4l=C3=A4
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Feb 21, 2018 at 09:37:21AM -0500, Rob Clark wrote:
>> Follow the same pattern of locking as with other state objects.  This
>> avoids boilerplate in the driver.
>
> I'm not sure we really want to do this. What if the driver wants a
> custom locking scheme for this state?

That seems like something we want to discourage, ie. all the more
reason for this patch.

There is no reason drivers could not split their global state into
multiple private objs's, each with their own lock, for more fine
grained locking.  That is basically the only valid reason I can think
of for "custom locking".

(And ofc drivers could add there own locks in addition to what is done
by core, but I'd rather look at that on a case by case basis, rather
than it being part of the boilerplate in each driver.)

BR,
-R

>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c | 9 ++++++++-
>>  include/drm/drm_atomic.h     | 5 +++++
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index fc8c4da409ff..004e621ab307 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -1078,6 +1078,8 @@ drm_atomic_private_obj_init(struct drm_private_obj=
 *obj,
>>  {
>>       memset(obj, 0, sizeof(*obj));
>>
>> +     drm_modeset_lock_init(&obj->lock);
>> +
>>       obj->state =3D state;
>>       obj->funcs =3D funcs;
>>  }
>> @@ -1093,6 +1095,7 @@ void
>>  drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>>  {
>>       obj->funcs->atomic_destroy_state(obj, obj->state);
>> +     drm_modeset_lock_fini(&obj->lock);
>>  }
>>  EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>>
>> @@ -1113,7 +1116,7 @@ struct drm_private_state *
>>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>>                                struct drm_private_obj *obj)
>>  {
>> -     int index, num_objs, i;
>> +     int index, num_objs, i, ret;
>>       size_t size;
>>       struct __drm_private_objs_state *arr;
>>       struct drm_private_state *obj_state;
>> @@ -1122,6 +1125,10 @@ drm_atomic_get_private_obj_state(struct drm_atomi=
c_state *state,
>>               if (obj =3D=3D state->private_objs[i].ptr)
>>                       return state->private_objs[i].state;
>>
>> +     ret =3D drm_modeset_lock(&obj->lock, state->acquire_ctx);
>> +     if (ret)
>> +             return ERR_PTR(ret);
>> +
>>       num_objs =3D state->num_private_objs + 1;
>>       size =3D sizeof(*state->private_objs) * num_objs;
>>       arr =3D krealloc(state->private_objs, size, GFP_KERNEL);
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 09076a625637..9ae53b73c9d2 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -218,6 +218,11 @@ struct drm_private_state_funcs {
>>   * &drm_modeset_lock is required to duplicate and update this object's =
state.
>>   */
>>  struct drm_private_obj {
>> +     /**
>> +      * @lock: Modeset lock to protect the state object.
>> +      */
>> +     struct drm_modeset_lock lock;
>> +
>>       /**
>>        * @state: Current atomic state for this driver private object.
>>        */
>> --
>> 2.14.3
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrj=C3=A4l=C3=A4
> Intel OTC