Re: [PATCH v4 01/14] drm/atomic: export drm_atomic_replace_property_blob

[Peter Rosin]

On 2017-07-11 10:01, Daniel Vetter wrote:
> On Thu, Jul 06, 2017 at 02:20:35PM +0200, Peter Rosin wrote:
>> While at it, add some words in the kernel-doc about the 'replaced' arg and
>> remove a faulty kernel-doc comment on the return value.
>>
>> Also remove a redundant return statement.
>>
>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/drm_atomic.c | 17 +++++++++--------
>>  include/drm/drm_atomic.h     |  4 ++++
>>  2 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 09ca662..b7d9696 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -414,13 +414,15 @@ EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
>>   * @new_blob: the new blob to replace with
>>   * @replaced: whether the blob has been replaced
>>   *
>> - * RETURNS:
>> - * Zero on success, error code on failure
>> + * Note that you are required to initialize @replaced to false before the
>> + * call, since it is only set to true when the blob property is changed and
>> + * not set to false when the property is not changed. This enables a series
>> + * of calls to be made where you are interested in if any property is
>> + * replaced, but not care so much about exactly which of them was replaced.
>>   */
>> -static void
>> -drm_atomic_replace_property_blob(struct drm_property_blob **blob,
>> -				 struct drm_property_blob *new_blob,
>> -				 bool *replaced)
>> +void drm_atomic_replace_property_blob(struct drm_property_blob **blob,
>> +				      struct drm_property_blob *new_blob,
>> +				      bool *replaced)
> 
> Yes I know I'm super-annoying, but this function now feels misplaced. It
> has nothing to do with atomic, it just replaces a pointer to a blob with
> anther pointer. I think it'd be much better if we move this function to
> drm_property.c, and rename it to drm_property_replace_blob.

Right, good judgement. Regarding incremental reviewing, I had it coming
because I am guilty too... :-) Anyway, no problem!

> Second, instead of typing a huge paragraph explaining how replaced works,
> I think the better option would be to drop that parameter and instead
> return a boolean indicating whether the blob was replaced or not.

Right. And again, good judgement.

> That's a bit more work, but imo functions which are exported need to pass
> a higher barrier wrt api polish.

Will fix these issues in v5.

> Thanks, Daniel

Cheers,
Peter

>>  {
>>  	struct drm_property_blob *old_blob = *blob;
>>  
>> @@ -432,9 +434,8 @@ drm_atomic_replace_property_blob(struct drm_property_blob **blob,
>>  		drm_property_blob_get(new_blob);
>>  	*blob = new_blob;
>>  	*replaced = true;
>> -
>> -	return;
>>  }
>> +EXPORT_SYMBOL(drm_atomic_replace_property_blob);
>>  
>>  static int
>>  drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index dcc8e0c..8b32ea5 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -321,6 +321,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>>  		struct drm_connector_state *state, struct drm_property *property,
>>  		uint64_t val);
>>  
>> +void drm_atomic_replace_property_blob(struct drm_property_blob **blob,
>> +				      struct drm_property_blob *new_blob,
>> +				      bool *replaced);
>> +
>>  void * __must_check
>>  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>>  			      void *obj,
>> -- 
>> 2.1.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>