Hi
Am 21.04.20 um 12:45 schrieb Daniel Vetter:
> On Mon, Apr 20, 2020 at 3:37 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>>
>> Hi
>>
>> Am 15.04.20 um 09:39 schrieb Daniel Vetter:
>>> Add a new macro helper to combine the usual init sequence in drivers,
>>> consisting of a kzalloc + devm_drm_dev_init + drmm_add_final_kfree
>>> triplet. This allows us to remove the rather unsightly
>>> drmm_add_final_kfree from all currently merged drivers.
>>>
>>> The kerneldoc is only added for this new function. Existing kerneldoc
>>> and examples will be udated at the very end, since once all drivers
>>> are converted over to devm_drm_dev_alloc we can unexport a lot of
>>> interim functions and make the documentation for driver authors a lot
>>> cleaner and less confusing. There will be only one true way to
>>> initialize a drm_device at the end of this, which is going to be
>>> devm_drm_dev_alloc.
>>>
>>> v2:
>>> - Actually explain what this is for in the commit message (Sam)
>>> - Fix checkpatch issues (Sam)
>>>
>>> Acked-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
>>> Cc: Noralf Trønnes <noralf@xxxxxxxxxxx>
>>> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
>>> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
>>> Cc: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
>>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>
> Thanks for taking a look, some questions on your suggestions below.
>
>> Sorry for being late. A number of nits are listed below. In any case:
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>
>> Best regards
>> Thomas
>>
>>> ---
>>> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++
>>> include/drm/drm_drv.h | 33 +++++++++++++++++++++++++++++++++
>>> 2 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 1bb4f636b83c..8e1813d2a12e 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -739,6 +739,29 @@ int devm_drm_dev_init(struct device *parent,
>>> }
>>> EXPORT_SYMBOL(devm_drm_dev_init);
>>>
>>> +void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver,
>>> + size_t size, size_t offset)
>>
>> Maybe rename 'offset' of 'dev_offset' to make the relationship clear.
>
> Hm, I see the point of this (and the dev_field below, although I'd go
> with dev_member there for some consistency with other macros using
> offset_of or container_of), but I'm not sure about the dev_ prefix.
> Drivers use that sometimes for the struct device *, and usage for
> struct drm_device * is also very inconsistent. I've seen ddev, drm,
> dev and base (that one only for embedded structs ofc). So not sure
> which prefix to pick, aside from dev_ seems the most confusing. Got
> ideas?
We have pdev for the PCI device, dev for the abstract device, and things
like mdev for struct mga_device in mgag200. So I'd go with ddev. I don't
like drm, because it could be anything in DRM. I guess struct drm_driver
is more 'drm' than struct drm_device.
But all of this is bikeshedding. It's probably best to keep the patch
as-is, and maybe rename variables later if we ever find consent on the
naming.
>
>>> +{
>>> + void *container;
>>> + struct drm_device *drm;
>>> + int ret;
>>> +
>>> + container = kzalloc(size, GFP_KERNEL);
>>> + if (!container)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + drm = container + offset;
>>
>> While convenient, I somewhat dislike the use of void* variables. I'd use
>> unsigned char* for container and do an explicit cast to struct
>> drm_device* here.
>
> I thought ever since C89 the explicit recommendation for untyped
> pointer math has been void *, and no longer char *, with the spec
> being explicit that void * pointer math works exactly like char *. So
From how I understand the C spec, I think it's the other way around. I
had to look up the sections from the C11 spec:
Sec 6.5.6 Additive operators
2 For addition, either both operands shall have arithmetic type, or
one operand shall be a pointer to a complete object type and the
other shall have integer type. (Incrementing is equivalent to adding
1.)
About void it says that it's not a complete type.
Sec 6.2.5 Types
19 The void type comprises an empty set of values; it is an incomplete
object type that cannot be completed.
Arithmetic on void* is a gcc extension AFAIK.
> not clear on why you think char * is preferred here. I'm also not
> aware of any other kernel code that casts to char * for untyped
> pointer math. So unless you have some supporting evidence, I'll skip
> this one, ok?
I'm really just bikeshedding on things I'd have done differently.
Best regards
Thomas
>
> Thanks, Daniel
>
>>> + ret = devm_drm_dev_init(parent, drm, driver);
>>> + if (ret) {
>>> + kfree(container);
>>> + return ERR_PTR(ret);
>>> + }
>>> + drmm_add_final_kfree(drm, container);
>>> +
>>> + return container;
>>> +}
>>> +EXPORT_SYMBOL(__devm_drm_dev_alloc);
>>> +
>>> /**
>>> * drm_dev_alloc - Allocate new DRM device
>>> * @driver: DRM driver to allocate device for
>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index e7c6ea261ed1..f07f15721254 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -626,6 +626,39 @@ int devm_drm_dev_init(struct device *parent,
>>> struct drm_device *dev,
>>> struct drm_driver *driver);
>>>
>>> +void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver,
>>> + size_t size, size_t offset);
>>> +
>>> +/**
>>> + * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance
>>> + * @parent: Parent device object
>>> + * @driver: DRM driver
>>> + * @type: the type of the struct which contains struct &drm_device
>>> + * @member: the name of the &drm_device within @type.
>>> + *
>>> + * This allocates and initialize a new DRM device. No device registration is done.
>>> + * Call drm_dev_register() to advertice the device to user space and register it
>>> + * with other core subsystems. This should be done last in the device
>>> + * initialization sequence to make sure userspace can't access an inconsistent
>>> + * state.
>>> + *
>>> + * The initial ref-count of the object is 1. Use drm_dev_get() and
>>> + * drm_dev_put() to take and drop further ref-counts.
>>> + *
>>> + * It is recommended that drivers embed &struct drm_device into their own device
>>> + * structure.
>>> + *
>>> + * Note that this manages the lifetime of the resulting &drm_device
>>> + * automatically using devres. The DRM device initialized with this function is
>>> + * automatically put on driver detach using drm_dev_put().
>>> + *
>>> + * RETURNS:
>>> + * Pointer to new DRM device, or ERR_PTR on failure.
>>> + */
>>> +#define devm_drm_dev_alloc(parent, driver, type, member) \
>>
>> I'd replace 'member' with 'dev_field' to make the relation ship clear.
>>
>>> + ((type *) __devm_drm_dev_alloc(parent, driver, sizeof(type), \
>>> + offsetof(type, member)))
>>> +
>>> struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>>> struct device *parent);
>>> int drm_dev_register(struct drm_device *dev, unsigned long flags);
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
