[RFC,libdrm 1/3] tegra: Add stream library
On 01/02/2013 02:31 PM, Terje Bergstr?m wrote: > On 02.01.2013 04:44, Mark Zhang wrote: >> Agree. If we are able to do something dynamically, normally that'll be >> better. >> >> Terje, we can get the Tegra version in FUSE. I think we don't need this >> kind of try-catch logics. > > We'd need to have in user space a map of SoC version -> number of sync > points. As kernel already has this and checks for invalid fences, I > don't think user space needs the same check. > > We're anyway talking about a possibility of coding error, nothing that > can happen on properly working software, so I wouldn't want to add any > code that isn't absolutely necessary to catch that. > Ah, yes, my bad. It's libdrm, not host1x driver. Mark > Terje >
[RFC,libdrm 1/3] tegra: Add stream library
On 12/31/2012 02:22 PM, Terje Bergstr?m wrote: > On 28.12.2012 22:48, Thierry Reding wrote: >> I disagree. We shouldn't be hiding this kind of detail behind an #ifdef. >> Instead it should be detected at runtime. Otherwise you'll need to build >> different versions of libdrm for every generation of Tegra. That may be >> fine for NVIDIA provided BSPs, but distributions would have a very hard >> time dealing with that. What we want is software that works unmodified >> on as many generations of Tegra as possible. > > I agree. The fences will be rejected by kernel and kernel knows about > number of sync points in each host1x revision. So, we could just submit > and look at return code. > Agree. If we are able to do something dynamically, normally that'll be better. Terje, we can get the Tegra version in FUSE. I think we don't need this kind of try-catch logics. Mark > Terje >
[RFC,libdrm 1/3] tegra: Add stream library
On 02.01.2013 04:44, Mark Zhang wrote: > Agree. If we are able to do something dynamically, normally that'll be > better. > > Terje, we can get the Tegra version in FUSE. I think we don't need this > kind of try-catch logics. We'd need to have in user space a map of SoC version -> number of sync points. As kernel already has this and checks for invalid fences, I don't think user space needs the same check. We're anyway talking about a possibility of coding error, nothing that can happen on properly working software, so I wouldn't want to add any code that isn't absolutely necessary to catch that. Terje
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 01/02/2013 02:31 PM, Terje Bergström wrote: > On 02.01.2013 04:44, Mark Zhang wrote: >> Agree. If we are able to do something dynamically, normally that'll be >> better. >> >> Terje, we can get the Tegra version in FUSE. I think we don't need this >> kind of try-catch logics. > > We'd need to have in user space a map of SoC version -> number of sync > points. As kernel already has this and checks for invalid fences, I > don't think user space needs the same check. > > We're anyway talking about a possibility of coding error, nothing that > can happen on properly working software, so I wouldn't want to add any > code that isn't absolutely necessary to catch that. > Ah, yes, my bad. It's libdrm, not host1x driver. Mark > Terje > ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 02.01.2013 04:44, Mark Zhang wrote: > Agree. If we are able to do something dynamically, normally that'll be > better. > > Terje, we can get the Tegra version in FUSE. I think we don't need this > kind of try-catch logics. We'd need to have in user space a map of SoC version -> number of sync points. As kernel already has this and checks for invalid fences, I don't think user space needs the same check. We're anyway talking about a possibility of coding error, nothing that can happen on properly working software, so I wouldn't want to add any code that isn't absolutely necessary to catch that. Terje ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 12/31/2012 02:22 PM, Terje Bergström wrote: > On 28.12.2012 22:48, Thierry Reding wrote: >> I disagree. We shouldn't be hiding this kind of detail behind an #ifdef. >> Instead it should be detected at runtime. Otherwise you'll need to build >> different versions of libdrm for every generation of Tegra. That may be >> fine for NVIDIA provided BSPs, but distributions would have a very hard >> time dealing with that. What we want is software that works unmodified >> on as many generations of Tegra as possible. > > I agree. The fences will be rejected by kernel and kernel knows about > number of sync points in each host1x revision. So, we could just submit > and look at return code. > Agree. If we are able to do something dynamically, normally that'll be better. Terje, we can get the Tegra version in FUSE. I think we don't need this kind of try-catch logics. Mark > Terje > ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC,libdrm 1/3] tegra: Add stream library
On 28.12.2012 22:48, Thierry Reding wrote: > I disagree. We shouldn't be hiding this kind of detail behind an #ifdef. > Instead it should be detected at runtime. Otherwise you'll need to build > different versions of libdrm for every generation of Tegra. That may be > fine for NVIDIA provided BSPs, but distributions would have a very hard > time dealing with that. What we want is software that works unmodified > on as many generations of Tegra as possible. I agree. The fences will be rejected by kernel and kernel knows about number of sync points in each host1x revision. So, we could just submit and look at return code. Terje
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 28.12.2012 22:48, Thierry Reding wrote: > I disagree. We shouldn't be hiding this kind of detail behind an #ifdef. > Instead it should be detected at runtime. Otherwise you'll need to build > different versions of libdrm for every generation of Tegra. That may be > fine for NVIDIA provided BSPs, but distributions would have a very hard > time dealing with that. What we want is software that works unmodified > on as many generations of Tegra as possible. I agree. The fences will be rejected by kernel and kernel knows about number of sync points in each host1x revision. So, we could just submit and look at return code. Terje ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC,libdrm 1/3] tegra: Add stream library
On Fri, Dec 28, 2012 at 09:45:48AM +0200, Arto Merilainen wrote:
> On 12/28/2012 08:47 AM, Mark Zhang wrote:
> >>+int tegra_fence_is_valid(const struct tegra_fence *fence)
> >>+{
> >>+int valid = fence ? 1 : 0;
> >>+valid = valid && fence->id != (uint32_t) -1;
> >>+valid = valid && fence->id < 32;
> >
> >Hardcode here. Assume always has 32 syncpts.
> >Change to a micro wrapped with tegra version ifdef will be better.
> >
>
> You are correct. I will fix this.
I disagree. We shouldn't be hiding this kind of detail behind an #ifdef.
Instead it should be detected at runtime. Otherwise you'll need to build
different versions of libdrm for every generation of Tegra. That may be
fine for NVIDIA provided BSPs, but distributions would have a very hard
time dealing with that. What we want is software that works unmodified
on as many generations of Tegra as possible.
Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:
[RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 04:50 PM, Arto Merilainen wrote:
> On 12/28/2012 09:57 AM, Mark Zhang wrote:
>> On 12/28/2012 03:45 PM, Arto Merilainen wrote:
>>> On 12/28/2012 08:47 AM, Mark Zhang wrote:
> +
> +/* Add fences */
> +if (num_fences) {
> +
> +tegra_stream_push(stream,
> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
> +host1x_uclass_wait_syncpt_r(), num_fences));
> +
> +for (; num_fences; num_fences--, fence++) {
> +assert(tegra_fence_is_valid(fence));
This is useless. We already add "1 + num_fences" to num_words above. So
move this "assert" before adding "1 + num_fences" to num_words makes
sense.
>>>
>>> The assertion checks the validity of a single fence - not if there is
>>> room in the command buffer.
>>>
>>> The goal is to prevent having invalid fences in the command stream. If
>>> this check were not here it would be possible to initialise a fence with
>>> tegra_fence_clear() and put that fence into the stream.
>>
>> My idea is, if one fence is invalid, then we should not count this in
>> "num_words". In current code, if one fence is invalid, then this fence
>> will not be pushed into the command stream, and the "num_words" shows a
>> wrong command buffer size.
>>
>> So I think we should:
>> - validate the fences, remove the invalid fence
>> - update num_words
>> - then you don't need to check fence here - I mean, before push a host1x
>> syncpt wait command into the active buffer of stream.
>>
>
> In my opinion asking tegra_stream_begin() to put a bad fence into the
> stream is a case we should never be. assert() kills the application
> immediately (in debug builds) and usually this helps the programmer for
> 1) finding bugs 2) not doing bad code.
>
Yep, I agree. But in release builds, assert does nothing. So this
checking doesn't make sense and also a wrong fence will be pushed into
command buffer silently. And we always use release version in real
products, so we can't count on this "assert".
> "Silencing" is not a good solution especially in this case:
> tegra_stream_flush() returns an invalid fence when flushing fails. If
> the application chains submits (i.e. do a blit and then do another using
> the output of the first blit) it is crucial to be sure the first submit
> has been performed before starting the second one.
>
Yes. So I suggest doing fence checking at the beginning of the
"tegra_stream_begin", if invalid fence found, return an error.
> - Arto
[RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 03:45 PM, Arto Merilainen wrote:
> On 12/28/2012 08:47 AM, Mark Zhang wrote:
>>> +int tegra_fence_is_valid(const struct tegra_fence *fence)
>>> +{
>>> +int valid = fence ? 1 : 0;
>>> +valid = valid && fence->id != (uint32_t) -1;
>>> +valid = valid && fence->id < 32;
>>
>> Hardcode here. Assume always has 32 syncpts.
>> Change to a micro wrapped with tegra version ifdef will be better.
>>
>
> You are correct. I will fix this.
>
>>> +return valid;
>>> +}
>>> +
>> [...]
>>> +
>>> +/* Add fences */
>>> +if (num_fences) {
>>> +
>>> +tegra_stream_push(stream,
>>> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
>>> +host1x_uclass_wait_syncpt_r(), num_fences));
>>> +
>>> +for (; num_fences; num_fences--, fence++) {
>>> +assert(tegra_fence_is_valid(fence));
>>
>> This is useless. We already add "1 + num_fences" to num_words above. So
>> move this "assert" before adding "1 + num_fences" to num_words makes
>> sense.
>>
>
> The assertion checks the validity of a single fence - not if there is
> room in the command buffer.
>
> The goal is to prevent having invalid fences in the command stream. If
> this check were not here it would be possible to initialise a fence with
> tegra_fence_clear() and put that fence into the stream.
My idea is, if one fence is invalid, then we should not count this in
"num_words". In current code, if one fence is invalid, then this fence
will not be pushed into the command stream, and the "num_words" shows a
wrong command buffer size.
So I think we should:
- validate the fences, remove the invalid fence
- update num_words
- then you don't need to check fence here - I mean, before push a host1x
syncpt wait command into the active buffer of stream.
>
>>> +
>>> +tegra_stream_push(stream,
>>> nvhost_class_host_wait_syncpt(fence->id,
>>> +fence->value));
>>> +}
>>> +}
>>> +
>>> +if (class_id)
>>> +tegra_stream_push(stream, nvhost_opcode_setclass(class_id,
>>> 0, 0));
>>> +
>>> +return 0;
>>> +}
>>> +
>> [...]
>>> +
>>> +#endif /* TEGRA_DRMIF_H */
>>>
>
> - Arto
[RFC,libdrm 1/3] tegra: Add stream library
I just skimmed the code of libdrm while I'm trying to understand the
host1x driver. So below is what I found.
Mark
On 12/13/2012 10:01 PM, Arto Meril?inen wrote:
> From: Arto Merilainen
>
> This patch introduces tegra stream library. The library is used for
> buffer management, command stream construction and work
> synchronization.
>
[...]
> +
> +/*
> + * tegra_fence_is_valid(fence)
> + *
> + * Check validity of a fence. We just check that the fence range
> + * is valid w.r.t. host1x hardware.
> + */
> +
> +int tegra_fence_is_valid(const struct tegra_fence *fence)
> +{
> +int valid = fence ? 1 : 0;
> +valid = valid && fence->id != (uint32_t) -1;
> +valid = valid && fence->id < 32;
Hardcode here. Assume always has 32 syncpts.
Change to a micro wrapped with tegra version ifdef will be better.
> +return valid;
> +}
> +
[...]
> +
> +/* Add fences */
> +if (num_fences) {
> +
> +tegra_stream_push(stream,
> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
> +host1x_uclass_wait_syncpt_r(), num_fences));
> +
> +for (; num_fences; num_fences--, fence++) {
> +assert(tegra_fence_is_valid(fence));
This is useless. We already add "1 + num_fences" to num_words above. So
move this "assert" before adding "1 + num_fences" to num_words makes sense.
> +
> +tegra_stream_push(stream,
> nvhost_class_host_wait_syncpt(fence->id,
> +fence->value));
> +}
> +}
> +
> +if (class_id)
> +tegra_stream_push(stream, nvhost_opcode_setclass(class_id, 0, 0));
> +
> +return 0;
> +}
> +
[...]
> +
> +#endif /* TEGRA_DRMIF_H */
>
Re: [RFC,libdrm 1/3] tegra: Add stream library
On Fri, Dec 28, 2012 at 09:45:48AM +0200, Arto Merilainen wrote:
> On 12/28/2012 08:47 AM, Mark Zhang wrote:
> >>+int tegra_fence_is_valid(const struct tegra_fence *fence)
> >>+{
> >>+int valid = fence ? 1 : 0;
> >>+valid = valid && fence->id != (uint32_t) -1;
> >>+valid = valid && fence->id < 32;
> >
> >Hardcode here. Assume always has 32 syncpts.
> >Change to a micro wrapped with tegra version ifdef will be better.
> >
>
> You are correct. I will fix this.
I disagree. We shouldn't be hiding this kind of detail behind an #ifdef.
Instead it should be detected at runtime. Otherwise you'll need to build
different versions of libdrm for every generation of Tegra. That may be
fine for NVIDIA provided BSPs, but distributions would have a very hard
time dealing with that. What we want is software that works unmodified
on as many generations of Tegra as possible.
Thierry
pgpRWXYzm7WZc.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 11:04 AM, Mark Zhang wrote: > On 12/28/2012 04:50 PM, Arto Merilainen wrote: >> >> In my opinion asking tegra_stream_begin() to put a bad fence into the >> stream is a case we should never be. assert() kills the application >> immediately (in debug builds) and usually this helps the programmer for >> 1) finding bugs 2) not doing bad code. >> > > Yep, I agree. But in release builds, assert does nothing. So this > checking doesn't make sense and also a wrong fence will be pushed into > command buffer silently. And we always use release version in real > products, so we can't count on this "assert". The only pro of using assert is low (=no :-) ) overhead in release builds. > >> "Silencing" is not a good solution especially in this case: >> tegra_stream_flush() returns an invalid fence when flushing fails. If >> the application chains submits (i.e. do a blit and then do another using >> the output of the first blit) it is crucial to be sure the first submit >> has been performed before starting the second one. >> > > Yes. So I suggest doing fence checking at the beginning of the > "tegra_stream_begin", if invalid fence found, return an error. > This sounds reasonable. - Arto
[RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 09:57 AM, Mark Zhang wrote:
> On 12/28/2012 03:45 PM, Arto Merilainen wrote:
>> On 12/28/2012 08:47 AM, Mark Zhang wrote:
+
+/* Add fences */
+if (num_fences) {
+
+tegra_stream_push(stream,
+nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
+host1x_uclass_wait_syncpt_r(), num_fences));
+
+for (; num_fences; num_fences--, fence++) {
+assert(tegra_fence_is_valid(fence));
>>>
>>> This is useless. We already add "1 + num_fences" to num_words above. So
>>> move this "assert" before adding "1 + num_fences" to num_words makes
>>> sense.
>>>
>>
>> The assertion checks the validity of a single fence - not if there is
>> room in the command buffer.
>>
>> The goal is to prevent having invalid fences in the command stream. If
>> this check were not here it would be possible to initialise a fence with
>> tegra_fence_clear() and put that fence into the stream.
>
> My idea is, if one fence is invalid, then we should not count this in
> "num_words". In current code, if one fence is invalid, then this fence
> will not be pushed into the command stream, and the "num_words" shows a
> wrong command buffer size.
>
> So I think we should:
> - validate the fences, remove the invalid fence
> - update num_words
> - then you don't need to check fence here - I mean, before push a host1x
> syncpt wait command into the active buffer of stream.
>
In my opinion asking tegra_stream_begin() to put a bad fence into the
stream is a case we should never be. assert() kills the application
immediately (in debug builds) and usually this helps the programmer for
1) finding bugs 2) not doing bad code.
"Silencing" is not a good solution especially in this case:
tegra_stream_flush() returns an invalid fence when flushing fails. If
the application chains submits (i.e. do a blit and then do another using
the output of the first blit) it is crucial to be sure the first submit
has been performed before starting the second one.
- Arto
[RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 08:47 AM, Mark Zhang wrote:
>> +int tegra_fence_is_valid(const struct tegra_fence *fence)
>> +{
>> +int valid = fence ? 1 : 0;
>> +valid = valid && fence->id != (uint32_t) -1;
>> +valid = valid && fence->id < 32;
>
> Hardcode here. Assume always has 32 syncpts.
> Change to a micro wrapped with tegra version ifdef will be better.
>
You are correct. I will fix this.
>> +return valid;
>> +}
>> +
> [...]
>> +
>> +/* Add fences */
>> +if (num_fences) {
>> +
>> +tegra_stream_push(stream,
>> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
>> +host1x_uclass_wait_syncpt_r(), num_fences));
>> +
>> +for (; num_fences; num_fences--, fence++) {
>> +assert(tegra_fence_is_valid(fence));
>
> This is useless. We already add "1 + num_fences" to num_words above. So
> move this "assert" before adding "1 + num_fences" to num_words makes sense.
>
The assertion checks the validity of a single fence - not if there is
room in the command buffer.
The goal is to prevent having invalid fences in the command stream. If
this check were not here it would be possible to initialise a fence with
tegra_fence_clear() and put that fence into the stream.
>> +
>> +tegra_stream_push(stream,
>> nvhost_class_host_wait_syncpt(fence->id,
>> +fence->value));
>> +}
>> +}
>> +
>> +if (class_id)
>> +tegra_stream_push(stream, nvhost_opcode_setclass(class_id, 0, 0));
>> +
>> +return 0;
>> +}
>> +
> [...]
>> +
>> +#endif /* TEGRA_DRMIF_H */
>>
- Arto
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 11:04 AM, Mark Zhang wrote: On 12/28/2012 04:50 PM, Arto Merilainen wrote: In my opinion asking tegra_stream_begin() to put a bad fence into the stream is a case we should never be. assert() kills the application immediately (in debug builds) and usually this helps the programmer for 1) finding bugs 2) not doing bad code. Yep, I agree. But in release builds, assert does nothing. So this checking doesn't make sense and also a wrong fence will be pushed into command buffer silently. And we always use release version in real products, so we can't count on this "assert". The only pro of using assert is low (=no :-) ) overhead in release builds. "Silencing" is not a good solution especially in this case: tegra_stream_flush() returns an invalid fence when flushing fails. If the application chains submits (i.e. do a blit and then do another using the output of the first blit) it is crucial to be sure the first submit has been performed before starting the second one. Yes. So I suggest doing fence checking at the beginning of the "tegra_stream_begin", if invalid fence found, return an error. This sounds reasonable. - Arto ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 04:50 PM, Arto Merilainen wrote:
> On 12/28/2012 09:57 AM, Mark Zhang wrote:
>> On 12/28/2012 03:45 PM, Arto Merilainen wrote:
>>> On 12/28/2012 08:47 AM, Mark Zhang wrote:
> +
> +/* Add fences */
> +if (num_fences) {
> +
> +tegra_stream_push(stream,
> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
> +host1x_uclass_wait_syncpt_r(), num_fences));
> +
> +for (; num_fences; num_fences--, fence++) {
> +assert(tegra_fence_is_valid(fence));
This is useless. We already add "1 + num_fences" to num_words above. So
move this "assert" before adding "1 + num_fences" to num_words makes
sense.
>>>
>>> The assertion checks the validity of a single fence - not if there is
>>> room in the command buffer.
>>>
>>> The goal is to prevent having invalid fences in the command stream. If
>>> this check were not here it would be possible to initialise a fence with
>>> tegra_fence_clear() and put that fence into the stream.
>>
>> My idea is, if one fence is invalid, then we should not count this in
>> "num_words". In current code, if one fence is invalid, then this fence
>> will not be pushed into the command stream, and the "num_words" shows a
>> wrong command buffer size.
>>
>> So I think we should:
>> - validate the fences, remove the invalid fence
>> - update num_words
>> - then you don't need to check fence here - I mean, before push a host1x
>> syncpt wait command into the active buffer of stream.
>>
>
> In my opinion asking tegra_stream_begin() to put a bad fence into the
> stream is a case we should never be. assert() kills the application
> immediately (in debug builds) and usually this helps the programmer for
> 1) finding bugs 2) not doing bad code.
>
Yep, I agree. But in release builds, assert does nothing. So this
checking doesn't make sense and also a wrong fence will be pushed into
command buffer silently. And we always use release version in real
products, so we can't count on this "assert".
> "Silencing" is not a good solution especially in this case:
> tegra_stream_flush() returns an invalid fence when flushing fails. If
> the application chains submits (i.e. do a blit and then do another using
> the output of the first blit) it is crucial to be sure the first submit
> has been performed before starting the second one.
>
Yes. So I suggest doing fence checking at the beginning of the
"tegra_stream_begin", if invalid fence found, return an error.
> - Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 09:57 AM, Mark Zhang wrote:
On 12/28/2012 03:45 PM, Arto Merilainen wrote:
On 12/28/2012 08:47 AM, Mark Zhang wrote:
+
+/* Add fences */
+if (num_fences) {
+
+tegra_stream_push(stream,
+nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
+host1x_uclass_wait_syncpt_r(), num_fences));
+
+for (; num_fences; num_fences--, fence++) {
+assert(tegra_fence_is_valid(fence));
This is useless. We already add "1 + num_fences" to num_words above. So
move this "assert" before adding "1 + num_fences" to num_words makes
sense.
The assertion checks the validity of a single fence - not if there is
room in the command buffer.
The goal is to prevent having invalid fences in the command stream. If
this check were not here it would be possible to initialise a fence with
tegra_fence_clear() and put that fence into the stream.
My idea is, if one fence is invalid, then we should not count this in
"num_words". In current code, if one fence is invalid, then this fence
will not be pushed into the command stream, and the "num_words" shows a
wrong command buffer size.
So I think we should:
- validate the fences, remove the invalid fence
- update num_words
- then you don't need to check fence here - I mean, before push a host1x
syncpt wait command into the active buffer of stream.
In my opinion asking tegra_stream_begin() to put a bad fence into the
stream is a case we should never be. assert() kills the application
immediately (in debug builds) and usually this helps the programmer for
1) finding bugs 2) not doing bad code.
"Silencing" is not a good solution especially in this case:
tegra_stream_flush() returns an invalid fence when flushing fails. If
the application chains submits (i.e. do a blit and then do another using
the output of the first blit) it is crucial to be sure the first submit
has been performed before starting the second one.
- Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 03:45 PM, Arto Merilainen wrote:
> On 12/28/2012 08:47 AM, Mark Zhang wrote:
>>> +int tegra_fence_is_valid(const struct tegra_fence *fence)
>>> +{
>>> +int valid = fence ? 1 : 0;
>>> +valid = valid && fence->id != (uint32_t) -1;
>>> +valid = valid && fence->id < 32;
>>
>> Hardcode here. Assume always has 32 syncpts.
>> Change to a micro wrapped with tegra version ifdef will be better.
>>
>
> You are correct. I will fix this.
>
>>> +return valid;
>>> +}
>>> +
>> [...]
>>> +
>>> +/* Add fences */
>>> +if (num_fences) {
>>> +
>>> +tegra_stream_push(stream,
>>> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
>>> +host1x_uclass_wait_syncpt_r(), num_fences));
>>> +
>>> +for (; num_fences; num_fences--, fence++) {
>>> +assert(tegra_fence_is_valid(fence));
>>
>> This is useless. We already add "1 + num_fences" to num_words above. So
>> move this "assert" before adding "1 + num_fences" to num_words makes
>> sense.
>>
>
> The assertion checks the validity of a single fence - not if there is
> room in the command buffer.
>
> The goal is to prevent having invalid fences in the command stream. If
> this check were not here it would be possible to initialise a fence with
> tegra_fence_clear() and put that fence into the stream.
My idea is, if one fence is invalid, then we should not count this in
"num_words". In current code, if one fence is invalid, then this fence
will not be pushed into the command stream, and the "num_words" shows a
wrong command buffer size.
So I think we should:
- validate the fences, remove the invalid fence
- update num_words
- then you don't need to check fence here - I mean, before push a host1x
syncpt wait command into the active buffer of stream.
>
>>> +
>>> +tegra_stream_push(stream,
>>> nvhost_class_host_wait_syncpt(fence->id,
>>> +fence->value));
>>> +}
>>> +}
>>> +
>>> +if (class_id)
>>> +tegra_stream_push(stream, nvhost_opcode_setclass(class_id,
>>> 0, 0));
>>> +
>>> +return 0;
>>> +}
>>> +
>> [...]
>>> +
>>> +#endif /* TEGRA_DRMIF_H */
>>>
>
> - Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 08:47 AM, Mark Zhang wrote:
+int tegra_fence_is_valid(const struct tegra_fence *fence)
+{
+int valid = fence ? 1 : 0;
+valid = valid && fence->id != (uint32_t) -1;
+valid = valid && fence->id < 32;
Hardcode here. Assume always has 32 syncpts.
Change to a micro wrapped with tegra version ifdef will be better.
You are correct. I will fix this.
+return valid;
+}
+
[...]
+
+/* Add fences */
+if (num_fences) {
+
+tegra_stream_push(stream,
+nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
+host1x_uclass_wait_syncpt_r(), num_fences));
+
+for (; num_fences; num_fences--, fence++) {
+assert(tegra_fence_is_valid(fence));
This is useless. We already add "1 + num_fences" to num_words above. So
move this "assert" before adding "1 + num_fences" to num_words makes sense.
The assertion checks the validity of a single fence - not if there is
room in the command buffer.
The goal is to prevent having invalid fences in the command stream. If
this check were not here it would be possible to initialise a fence with
tegra_fence_clear() and put that fence into the stream.
+
+tegra_stream_push(stream, nvhost_class_host_wait_syncpt(fence->id,
+fence->value));
+}
+}
+
+if (class_id)
+tegra_stream_push(stream, nvhost_opcode_setclass(class_id, 0, 0));
+
+return 0;
+}
+
[...]
+
+#endif /* TEGRA_DRMIF_H */
- Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,libdrm 1/3] tegra: Add stream library
I just skimmed the code of libdrm while I'm trying to understand the
host1x driver. So below is what I found.
Mark
On 12/13/2012 10:01 PM, Arto Meriläinen wrote:
> From: Arto Merilainen
>
> This patch introduces tegra stream library. The library is used for
> buffer management, command stream construction and work
> synchronization.
>
[...]
> +
> +/*
> + * tegra_fence_is_valid(fence)
> + *
> + * Check validity of a fence. We just check that the fence range
> + * is valid w.r.t. host1x hardware.
> + */
> +
> +int tegra_fence_is_valid(const struct tegra_fence *fence)
> +{
> +int valid = fence ? 1 : 0;
> +valid = valid && fence->id != (uint32_t) -1;
> +valid = valid && fence->id < 32;
Hardcode here. Assume always has 32 syncpts.
Change to a micro wrapped with tegra version ifdef will be better.
> +return valid;
> +}
> +
[...]
> +
> +/* Add fences */
> +if (num_fences) {
> +
> +tegra_stream_push(stream,
> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
> +host1x_uclass_wait_syncpt_r(), num_fences));
> +
> +for (; num_fences; num_fences--, fence++) {
> +assert(tegra_fence_is_valid(fence));
This is useless. We already add "1 + num_fences" to num_words above. So
move this "assert" before adding "1 + num_fences" to num_words makes sense.
> +
> +tegra_stream_push(stream,
> nvhost_class_host_wait_syncpt(fence->id,
> +fence->value));
> +}
> +}
> +
> +if (class_id)
> +tegra_stream_push(stream, nvhost_opcode_setclass(class_id, 0, 0));
> +
> +return 0;
> +}
> +
[...]
> +
> +#endif /* TEGRA_DRMIF_H */
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC,libdrm 1/3] tegra: Add stream library
From: Arto Merilainen
This patch introduces tegra stream library. The library is used for
buffer management, command stream construction and work
synchronization.
Signed-off-by: Arto Merilainen
---
Makefile.am|6 +-
configure.ac | 13 +
tegra/Makefile.am | 25 ++
tegra/class_ids.h | 35 ++
tegra/host1x01_hardware.h | 122 ++
tegra/hw_host1x01_uclass.h | 143
tegra/libdrm_tegra.pc.in | 10 +
tegra/tegra_drm.c | 876
tegra/tegra_drm.h | 142 +++
tegra/tegra_drmif.h| 107 ++
10 files changed, 1478 insertions(+), 1 deletion(-)
create mode 100644 tegra/Makefile.am
create mode 100644 tegra/class_ids.h
create mode 100644 tegra/host1x01_hardware.h
create mode 100644 tegra/hw_host1x01_uclass.h
create mode 100644 tegra/libdrm_tegra.pc.in
create mode 100644 tegra/tegra_drm.c
create mode 100644 tegra/tegra_drm.h
create mode 100644 tegra/tegra_drmif.h
diff --git a/Makefile.am b/Makefile.am
index 8ecd9d9..e90ae43 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -49,7 +49,11 @@ if HAVE_EXYNOS
EXYNOS_SUBDIR = exynos
endif
-SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(NOUVEAU_SUBDIR)
$(RADEON_SUBDIR) $(OMAP_SUBDIR) $(EXYNOS_SUBDIR) tests include man
+if HAVE_TEGRA
+TEGRA_SUBDIR = tegra
+endif
+
+SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(NOUVEAU_SUBDIR)
$(RADEON_SUBDIR) $(OMAP_SUBDIR) $(EXYNOS_SUBDIR) $(TEGRA_SUBDIR) tests include
man
libdrm_la_LTLIBRARIES = libdrm.la
libdrm_ladir = $(libdir)
diff --git a/configure.ac b/configure.ac
index 0c19929..36c55c7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -114,6 +114,11 @@ AC_ARG_ENABLE(exynos-experimental-api,
[Enable support for EXYNOS's experimental API (default:
disabled)]),
[EXYNOS=$enableval], [EXYNOS=no])
+AC_ARG_ENABLE(tegra,
+ AS_HELP_STRING([--enable-tegra],
+ [Enable support for tegra's API (default: disabled)]),
+ [TEGRA=$enableval], [TEGRA=no])
+
dnl ===
dnl check compiler flags
AC_DEFUN([LIBDRM_CC_TRY_FLAG], [
@@ -222,6 +227,11 @@ if test "x$EXYNOS" = xyes; then
AC_DEFINE(HAVE_EXYNOS, 1, [Have EXYNOS support])
fi
+AM_CONDITIONAL(HAVE_TEGRA, [test "x$TEGRA" = xyes])
+if test "x$TEGRA" = xyes; then
+ AC_DEFINE(HAVE_TEGRA, 1, [Have TEGRA support])
+fi
+
AC_ARG_ENABLE([cairo-tests],
[AS_HELP_STRING([--enable-cairo-tests],
[Enable support for Cairo rendering in tests
(default: auto)])],
@@ -358,6 +368,8 @@ AC_CONFIG_FILES([
omap/libdrm_omap.pc
exynos/Makefile
exynos/libdrm_exynos.pc
+ tegra/Makefile
+ tegra/libdrm_tegra.pc
tests/Makefile
tests/modeprint/Makefile
tests/modetest/Makefile
@@ -380,4 +392,5 @@ echo " Radeon API $RADEON"
echo " Nouveau API$NOUVEAU"
echo " OMAP API $OMAP"
echo " EXYNOS API $EXYNOS"
+echo " TEGRA API $TEGRA"
echo ""
diff --git a/tegra/Makefile.am b/tegra/Makefile.am
new file mode 100644
index 000..72675e5
--- /dev/null
+++ b/tegra/Makefile.am
@@ -0,0 +1,25 @@
+AM_CFLAGS = \
+ $(WARN_CFLAGS) \
+ -I$(top_srcdir) \
+ -I$(top_srcdir)/tegra \
+ $(PTHREADSTUBS_CFLAGS) \
+ -I$(top_srcdir)/include/drm
+
+libdrm_tegra_la_LTLIBRARIES = libdrm_tegra.la
+libdrm_tegra_ladir = $(libdir)
+libdrm_tegra_la_LDFLAGS = -version-number 1:0:0 -no-undefined
+libdrm_tegra_la_LIBADD = ../libdrm.la @PTHREADSTUBS_LIBS@
+
+libdrm_tegra_la_SOURCES = \
+ tegra_drm.c
+
+libdrm_tegracommonincludedir = ${includedir}/tegra
+libdrm_tegracommoninclude_HEADERS = \
+ tegra_drm.h
+
+libdrm_tegraincludedir = ${includedir}/libdrm
+libdrm_tegrainclude_HEADERS = \
+ tegra_drmif.h
+
+pkgconfigdir = @pkgconfigdir@
+pkgconfig_DATA = libdrm_tegra.pc
diff --git a/tegra/class_ids.h b/tegra/class_ids.h
new file mode 100644
index 000..834e291
--- /dev/null
+++ b/tegra/class_ids.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2012 NVIDIA Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND
[RFC,libdrm 1/3] tegra: Add stream library
From: Arto Merilainen
This patch introduces tegra stream library. The library is used for
buffer management, command stream construction and work
synchronization.
Signed-off-by: Arto Merilainen
---
Makefile.am|6 +-
configure.ac | 13 +
tegra/Makefile.am | 25 ++
tegra/class_ids.h | 35 ++
tegra/host1x01_hardware.h | 122 ++
tegra/hw_host1x01_uclass.h | 143
tegra/libdrm_tegra.pc.in | 10 +
tegra/tegra_drm.c | 876
tegra/tegra_drm.h | 142 +++
tegra/tegra_drmif.h| 107 ++
10 files changed, 1478 insertions(+), 1 deletion(-)
create mode 100644 tegra/Makefile.am
create mode 100644 tegra/class_ids.h
create mode 100644 tegra/host1x01_hardware.h
create mode 100644 tegra/hw_host1x01_uclass.h
create mode 100644 tegra/libdrm_tegra.pc.in
create mode 100644 tegra/tegra_drm.c
create mode 100644 tegra/tegra_drm.h
create mode 100644 tegra/tegra_drmif.h
diff --git a/Makefile.am b/Makefile.am
index 8ecd9d9..e90ae43 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -49,7 +49,11 @@ if HAVE_EXYNOS
EXYNOS_SUBDIR = exynos
endif
-SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(NOUVEAU_SUBDIR)
$(RADEON_SUBDIR) $(OMAP_SUBDIR) $(EXYNOS_SUBDIR) tests include man
+if HAVE_TEGRA
+TEGRA_SUBDIR = tegra
+endif
+
+SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(NOUVEAU_SUBDIR)
$(RADEON_SUBDIR) $(OMAP_SUBDIR) $(EXYNOS_SUBDIR) $(TEGRA_SUBDIR) tests include
man
libdrm_la_LTLIBRARIES = libdrm.la
libdrm_ladir = $(libdir)
diff --git a/configure.ac b/configure.ac
index 0c19929..36c55c7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -114,6 +114,11 @@ AC_ARG_ENABLE(exynos-experimental-api,
[Enable support for EXYNOS's experimental API (default:
disabled)]),
[EXYNOS=$enableval], [EXYNOS=no])
+AC_ARG_ENABLE(tegra,
+ AS_HELP_STRING([--enable-tegra],
+ [Enable support for tegra's API (default: disabled)]),
+ [TEGRA=$enableval], [TEGRA=no])
+
dnl ===
dnl check compiler flags
AC_DEFUN([LIBDRM_CC_TRY_FLAG], [
@@ -222,6 +227,11 @@ if test "x$EXYNOS" = xyes; then
AC_DEFINE(HAVE_EXYNOS, 1, [Have EXYNOS support])
fi
+AM_CONDITIONAL(HAVE_TEGRA, [test "x$TEGRA" = xyes])
+if test "x$TEGRA" = xyes; then
+ AC_DEFINE(HAVE_TEGRA, 1, [Have TEGRA support])
+fi
+
AC_ARG_ENABLE([cairo-tests],
[AS_HELP_STRING([--enable-cairo-tests],
[Enable support for Cairo rendering in tests
(default: auto)])],
@@ -358,6 +368,8 @@ AC_CONFIG_FILES([
omap/libdrm_omap.pc
exynos/Makefile
exynos/libdrm_exynos.pc
+ tegra/Makefile
+ tegra/libdrm_tegra.pc
tests/Makefile
tests/modeprint/Makefile
tests/modetest/Makefile
@@ -380,4 +392,5 @@ echo " Radeon API $RADEON"
echo " Nouveau API$NOUVEAU"
echo " OMAP API $OMAP"
echo " EXYNOS API $EXYNOS"
+echo " TEGRA API $TEGRA"
echo ""
diff --git a/tegra/Makefile.am b/tegra/Makefile.am
new file mode 100644
index 000..72675e5
--- /dev/null
+++ b/tegra/Makefile.am
@@ -0,0 +1,25 @@
+AM_CFLAGS = \
+ $(WARN_CFLAGS) \
+ -I$(top_srcdir) \
+ -I$(top_srcdir)/tegra \
+ $(PTHREADSTUBS_CFLAGS) \
+ -I$(top_srcdir)/include/drm
+
+libdrm_tegra_la_LTLIBRARIES = libdrm_tegra.la
+libdrm_tegra_ladir = $(libdir)
+libdrm_tegra_la_LDFLAGS = -version-number 1:0:0 -no-undefined
+libdrm_tegra_la_LIBADD = ../libdrm.la @PTHREADSTUBS_LIBS@
+
+libdrm_tegra_la_SOURCES = \
+ tegra_drm.c
+
+libdrm_tegracommonincludedir = ${includedir}/tegra
+libdrm_tegracommoninclude_HEADERS = \
+ tegra_drm.h
+
+libdrm_tegraincludedir = ${includedir}/libdrm
+libdrm_tegrainclude_HEADERS = \
+ tegra_drmif.h
+
+pkgconfigdir = @pkgconfigdir@
+pkgconfig_DATA = libdrm_tegra.pc
diff --git a/tegra/class_ids.h b/tegra/class_ids.h
new file mode 100644
index 000..834e291
--- /dev/null
+++ b/tegra/class_ids.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2012 NVIDIA Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE
