Am 11.09.23 um 20:29 schrieb John Stultz:
> On Mon, Sep 11, 2023 at 3:14 AM Christian König
> <[email protected]> wrote:
>> Am 11.09.23 um 04:30 schrieb Yong Wu:
>>> From: John Stultz <[email protected]>
>>>
>>> This allows drivers who don't want to create their own
>>> DMA-BUF exporter to be able to allocate DMA-BUFs directly
>>> from existing DMA-BUF Heaps.
>>>
>>> There is some concern that the premise of DMA-BUF heaps is
>>> that userland knows better about what type of heap memory
>>> is needed for a pipeline, so it would likely be best for
>>> drivers to import and fill DMA-BUFs allocated by userland
>>> instead of allocating one themselves, but this is still
>>> up for debate.
>> The main design goal of having DMA-heaps in the first place is to avoid
>> per driver allocation and this is not necessary because userland know
>> better what type of memory it wants.
>>
>> The background is rather that we generally want to decouple allocation
>> from having a device driver connection so that we have better chance
>> that multiple devices can work with the same memory.
> Yep, very much agreed, and this is what the comment above is trying to describe.
>
> Ideally user-allocated buffers would be used to ensure driver's don't
> create buffers with constraints that limit which devices the buffers
> might later be shared with.
>
> However, this patch was created as a hold-over from the old ION logic
> to help vendors transition to dmabuf heaps, as vendors had situations
> where they still wanted to export dmabufs that were not to be
> generally shared and folks wanted to avoid duplication of logic
> already in existing heaps. At the time, I never pushed it upstream as
> there were no upstream users. But I think if there is now a potential
> upstream user, it's worth having the discussion to better understand
> the need.
Yeah, that indeed makes much more sense.
When existing drivers want to avoid their own handling and move their
memory management over to using DMA-heaps even for internal allocations
then no objections from my side. That is certainly something we should
aim for if possible.
But what we should try to avoid is that newly merged drivers provide
both a driver specific UAPI and DMA-heaps. The justification that this
makes it easier to transit userspace to the new UAPI doesn't really count.
That would be adding UAPI already with a plan to deprecate it and that
is most likely not helpful considering that UAPI must be supported
forever as soon as it is upstream.
> So I think this patch is a little confusing in this series, as I don't
> see much of it actually being used here (though forgive me if I'm
> missing it).
>
> Instead, It seems it get used in a separate patch series here:
> https://lore.kernel.org/all/[email protected]/
Please try to avoid stuff like that it is really confusing and eats
reviewers time.
Regards,
Christian.
>
> Yong, I appreciate you sending this out! But maybe if the secure heap
> submission doesn't depend on this functionality, I might suggest
> moving this patch (or at least the majority of it) to be part of the
> vcodec series instead? That way reviewers will have more context for
> how the code being added is used?
>
> thanks
> -john
On Tue, 2023-09-12 at 09:06 +0200, Christian König wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Am 11.09.23 um 20:29 schrieb John Stultz:
> > On Mon, Sep 11, 2023 at 3:14 AM Christian König
> > <[email protected]> wrote:
> >> Am 11.09.23 um 04:30 schrieb Yong Wu:
> >>> From: John Stultz <[email protected]>
> >>>
> >>> This allows drivers who don't want to create their own
> >>> DMA-BUF exporter to be able to allocate DMA-BUFs directly
> >>> from existing DMA-BUF Heaps.
> >>>
> >>> There is some concern that the premise of DMA-BUF heaps is
> >>> that userland knows better about what type of heap memory
> >>> is needed for a pipeline, so it would likely be best for
> >>> drivers to import and fill DMA-BUFs allocated by userland
> >>> instead of allocating one themselves, but this is still
> >>> up for debate.
> >> The main design goal of having DMA-heaps in the first place is to
> avoid
> >> per driver allocation and this is not necessary because userland
> know
> >> better what type of memory it wants.
> >>
> >> The background is rather that we generally want to decouple
> allocation
> >> from having a device driver connection so that we have better
> chance
> >> that multiple devices can work with the same memory.
> > Yep, very much agreed, and this is what the comment above is trying
> to describe.
> >
> > Ideally user-allocated buffers would be used to ensure driver's
> don't
> > create buffers with constraints that limit which devices the
> buffers
> > might later be shared with.
> >
> > However, this patch was created as a hold-over from the old ION
> logic
> > to help vendors transition to dmabuf heaps, as vendors had
> situations
> > where they still wanted to export dmabufs that were not to be
> > generally shared and folks wanted to avoid duplication of logic
> > already in existing heaps. At the time, I never pushed it upstream
> as
> > there were no upstream users. But I think if there is now a
> potential
> > upstream user, it's worth having the discussion to better
> understand
> > the need.
>
> Yeah, that indeed makes much more sense.
>
> When existing drivers want to avoid their own handling and move
> their
> memory management over to using DMA-heaps even for internal
> allocations
> then no objections from my side. That is certainly something we
> should
> aim for if possible.
Thanks.
>
> But what we should try to avoid is that newly merged drivers provide
> both a driver specific UAPI and DMA-heaps. The justification that
> this
> makes it easier to transit userspace to the new UAPI doesn't really
> count.
>
> That would be adding UAPI already with a plan to deprecate it and
> that
> is most likely not helpful considering that UAPI must be supported
> forever as soon as it is upstream.
Sorry, I didn't understand this. I think we have not change the UAPI.
Which code are you referring to?
>
> > So I think this patch is a little confusing in this series, as I
> don't
> > see much of it actually being used here (though forgive me if I'm
> > missing it).
> >
> > Instead, It seems it get used in a separate patch series here:
> >
> https://lore.kernel.org/all/[email protected]/
>
> Please try to avoid stuff like that it is really confusing and eats
> reviewers time.
My fault, I thought dma-buf and media belonged to the different tree,
so I send them separately. The cover letter just said "The consumers of
the new heap and new interface are our codecs and DRM, which will be
sent upstream soon", and there was no vcodec link at that time.
In the next version, we will put the first three patches into the
vcodec patchset.
Thanks.
>
> Regards,
> Christian.
>
> >
> > Yong, I appreciate you sending this out! But maybe if the secure
> heap
> > submission doesn't depend on this functionality, I might suggest
> > moving this patch (or at least the majority of it) to be part of
> the
> > vcodec series instead? That way reviewers will have more context
> for
> > how the code being added is used?
Will do.
Thanks.
> >
> > thanks
> > -john
>
Am 12.09.23 um 10:52 schrieb Yong Wu (吴勇):
> [SNIP]
>> But what we should try to avoid is that newly merged drivers provide
>> both a driver specific UAPI and DMA-heaps. The justification that
>> this
>> makes it easier to transit userspace to the new UAPI doesn't really
>> count.
>>
>> That would be adding UAPI already with a plan to deprecate it and
>> that
>> is most likely not helpful considering that UAPI must be supported
>> forever as soon as it is upstream.
> Sorry, I didn't understand this. I think we have not change the UAPI.
> Which code are you referring to?
Well, what do you need this for if not a new UAPI?
My assumption here is that you need to export the DMA-heap allocation
function so that you can server an UAPI in your new driver. Or what else
is that good for?
As far as I understand you try to upstream your new vcodec driver. So
while this change here seems to be a good idea to clean up existing
drivers it doesn't look like a good idea for a newly created driver.
Regards,
Christian.
>>> So I think this patch is a little confusing in this series, as I
>> don't
>>> see much of it actually being used here (though forgive me if I'm
>>> missing it).
>>>
>>> Instead, It seems it get used in a separate patch series here:
>>>
>> https://lore.kernel.org/all/[email protected]/
>>
>> Please try to avoid stuff like that it is really confusing and eats
>> reviewers time.
> My fault, I thought dma-buf and media belonged to the different tree,
> so I send them separately. The cover letter just said "The consumers of
> the new heap and new interface are our codecs and DRM, which will be
> sent upstream soon", and there was no vcodec link at that time.
>
> In the next version, we will put the first three patches into the
> vcodec patchset.
>
> Thanks.
>
Le mardi 12 septembre 2023 à 16:46 +0200, Christian König a écrit :
> Am 12.09.23 um 10:52 schrieb Yong Wu (吴勇):
> > [SNIP]
> > > But what we should try to avoid is that newly merged drivers provide
> > > both a driver specific UAPI and DMA-heaps. The justification that
> > > this
> > > makes it easier to transit userspace to the new UAPI doesn't really
> > > count.
> > >
> > > That would be adding UAPI already with a plan to deprecate it and
> > > that
> > > is most likely not helpful considering that UAPI must be supported
> > > forever as soon as it is upstream.
> > Sorry, I didn't understand this. I think we have not change the UAPI.
> > Which code are you referring to?
>
> Well, what do you need this for if not a new UAPI?
>
> My assumption here is that you need to export the DMA-heap allocation
> function so that you can server an UAPI in your new driver. Or what else
> is that good for?
>
> As far as I understand you try to upstream your new vcodec driver. So
> while this change here seems to be a good idea to clean up existing
> drivers it doesn't look like a good idea for a newly created driver.
MTK VCODEC has been upstream for quite some time now. The other patchset is
trying to add secure decoding/encoding support to that existing upstream driver.
Regarding the uAPI, it seems that this addition to dmabuf heap internal API is
exactly the opposite. By making heaps available to drivers, modification to the
V4L2 uAPI is being reduced to adding "SECURE_MODE" + "SECURE_HEAP_ID" controls
(this is not debated yet has an approach). The heaps is being used internally in
replacement to every allocation, user visible or not.
Nicolas
>
> Regards,
> Christian.
>
> > > > So I think this patch is a little confusing in this series, as I
> > > don't
> > > > see much of it actually being used here (though forgive me if I'm
> > > > missing it).
> > > >
> > > > Instead, It seems it get used in a separate patch series here:
> > > >
> > > https://lore.kernel.org/all/[email protected]/
> > >
> > > Please try to avoid stuff like that it is really confusing and eats
> > > reviewers time.
> > My fault, I thought dma-buf and media belonged to the different tree,
> > so I send them separately. The cover letter just said "The consumers of
> > the new heap and new interface are our codecs and DRM, which will be
> > sent upstream soon", and there was no vcodec link at that time.
> >
> > In the next version, we will put the first three patches into the
> > vcodec patchset.
> >
> > Thanks.
> >
>
Am 12.09.23 um 16:58 schrieb Nicolas Dufresne:
> Le mardi 12 septembre 2023 à 16:46 +0200, Christian König a écrit :
>> Am 12.09.23 um 10:52 schrieb Yong Wu (吴勇):
>>> [SNIP]
>>>> But what we should try to avoid is that newly merged drivers provide
>>>> both a driver specific UAPI and DMA-heaps. The justification that
>>>> this
>>>> makes it easier to transit userspace to the new UAPI doesn't really
>>>> count.
>>>>
>>>> That would be adding UAPI already with a plan to deprecate it and
>>>> that
>>>> is most likely not helpful considering that UAPI must be supported
>>>> forever as soon as it is upstream.
>>> Sorry, I didn't understand this. I think we have not change the UAPI.
>>> Which code are you referring to?
>> Well, what do you need this for if not a new UAPI?
>>
>> My assumption here is that you need to export the DMA-heap allocation
>> function so that you can server an UAPI in your new driver. Or what else
>> is that good for?
>>
>> As far as I understand you try to upstream your new vcodec driver. So
>> while this change here seems to be a good idea to clean up existing
>> drivers it doesn't look like a good idea for a newly created driver.
> MTK VCODEC has been upstream for quite some time now. The other patchset is
> trying to add secure decoding/encoding support to that existing upstream driver.
>
> Regarding the uAPI, it seems that this addition to dmabuf heap internal API is
> exactly the opposite. By making heaps available to drivers, modification to the
> V4L2 uAPI is being reduced to adding "SECURE_MODE" + "SECURE_HEAP_ID" controls
> (this is not debated yet has an approach). The heaps is being used internally in
> replacement to every allocation, user visible or not.
Thanks a lot for that explanation, I was really wondering what the use
case for this is if it's not to serve new UAPI.
In this case I don't see any reason why we shouldn't do it. It's indeed
much cleaner.
Christian.
>
> Nicolas
>
>> Regards,
>> Christian.
>>
>>>>> So I think this patch is a little confusing in this series, as I
>>>> don't
>>>>> see much of it actually being used here (though forgive me if I'm
>>>>> missing it).
>>>>>
>>>>> Instead, It seems it get used in a separate patch series here:
>>>>>
>>>> https://lore.kernel.org/all/[email protected]/
>>>>
>>>> Please try to avoid stuff like that it is really confusing and eats
>>>> reviewers time.
>>> My fault, I thought dma-buf and media belonged to the different tree,
>>> so I send them separately. The cover letter just said "The consumers of
>>> the new heap and new interface are our codecs and DRM, which will be
>>> sent upstream soon", and there was no vcodec link at that time.
>>>
>>> In the next version, we will put the first three patches into the
>>> vcodec patchset.
>>>
>>> Thanks.
>>>