Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vkCmdBlitImage on itself. Resource conflicts issue #1441

Closed
qnope opened this issue Nov 23, 2020 · 9 comments
Closed

vkCmdBlitImage on itself. Resource conflicts issue #1441

qnope opened this issue Nov 23, 2020 · 9 comments

Comments

@qnope
Copy link
Contributor

qnope commented Nov 23, 2020

Template

I am beginning in Rust, but I have some experiences with Vulkan in Cpp.
I am trying to implement the mipmap generation into ImmutableImage. (As I see it is in Todo list)

I was inspired from #989, siince it seems to work (even if there was some layout transition issues that was apparently solved now)

So what I tried is that :

pub fn from_buffer<B, P>(
        source: B,
        dimensions: Dimensions,
        mipmaps: MipmapsCount,
        format: F,
        queue: Arc<Queue>,
    ) -> Result<
        (
            Arc<Self>,
            CommandBufferExecFuture<NowFuture, AutoCommandBuffer>,
        ),
        ImageCreationError,
    >
    where
        B: BufferAccess + TypedBufferAccess<Content = [P]> + 'static + Clone + Send + Sync,
        P: Send + Sync + Clone + 'static,
        F: FormatDesc + AcceptsPixels<P> + 'static + Send + Sync,
        Format: AcceptsPixels<P>,
    {
        let usage = ImageUsage {
            transfer_destination: true,
            transfer_source: true,
            sampled: true,
            ..ImageUsage::none()
        };
        let layout = ImageLayout::ShaderReadOnlyOptimal;

        let (buffer, init) = ImmutableImage::uninitialized(
            source.device().clone(),
            dimensions,
            format,
            mipmaps,
            usage,
            layout,
            source.device().active_queue_families(),
        )?;

        let mut cbb = AutoCommandBufferBuilder::new(source.device().clone(), queue.family())?;
        cbb.copy_buffer_to_image_dimensions(
            source,
            init,
            [0, 0, 0],
            dimensions.width_height_depth(),
            0,
            dimensions.array_layers_with_cube(),
            0,
        )
        .unwrap();

        let img_dim = buffer.dimensions.to_image_dimensions();
        for level in 1..buffer.mipmap_levels() {
            let [xs, ys, ds] = img_dim.mipmap_dimensions(level - 1).unwrap().width_height_depth();
            let [xd, yd, dd] = img_dim.mipmap_dimensions(level).unwrap().width_height_depth();

            cbb.blit_image(
                buffer.clone(), //source
                [0, 0, 0], //source_top_left
                [xs as i32, ys as i32, ds as i32], //source_bottom_right
                0, //source_base_array_layer
                level - 1, //source_mip_level
                buffer.clone(), //destination
                [0, 0, 0], //destination_top_left
                [xd as i32, yd as i32, dd as i32], //destination_bottom_right
                0, //destination_base_array_layer
                level, //destination_mip_level
                1, //layer_count
                Filter::Linear //filter
            ).expect("failed to blit a mip map to image!");
        }

        let cb = cbb.build().unwrap();

        let future = match cb.execute(queue) {
            Ok(f) => f,
            Err(_) => unreachable!(),
        };

        Ok((buffer, future))
    }

I am just blitting the image successively, but I get this runtime-error

thread 'main' panicked at 'failed to blit a mip map to image!: SyncCommandBufferBuilderError(Conflict { command1_name: "vkCmdBlitImage", command1_param: "source", command1_offset: 1, command2_name: "vkCmdBlitImage", command2_param: "destination", command2_offset: 1 })

As I understand the error, it seems that the VkCmdBlitImage does not understand that we don't use the same part of the image.
I am using MipmapCounts::Log2
It is a non power of 2 image : 1920x1080p.

Maybe I miss something :)

@qnope qnope changed the title Mipmap generation and resources conflicts vkCmdBlitImage on itself. Resource conflicts issue Nov 23, 2020
@Eliah-Lakhin
Copy link
Contributor

@qnope Your code looks fine to my understanding. Bliting between different levels of the same image is safe Vulkan operation and is an efficient way to generate mipmaps. So it seems to be a bug in Vulkano of false-positive checking of resource occupation between different commands in command buffer builder, and is probably happening somewhere here: https://github.com/vulkano-rs/vulkano/blob/master/vulkano/src/command_buffer/synced/base.rs#L514

I don't have enough time to investigate it deeper at this moment, but if you want to invest some time in debugging it I will be very appreciated! In my project I generate mipmaps using two independent image resources to bypasses this issue, but it is memory/performance lesser efficient.

@qnope
Copy link
Contributor Author

qnope commented Nov 25, 2020

@Eliah-Lakhin
I am going to try to debug it this week. However, since I am a rust beginner, I don't know if it will be easy ahaha

Thanks

@qnope
Copy link
Contributor Author

qnope commented Nov 27, 2020

I begin to try to fix the issue.

I notice that the conflict* functions does not take care about the mip levels or the layer level of a resource. (It was the case in an old version, like 0.6, but there was a change for another version).

I also notice that a ImageAccesstrait does not have any information about the current access about the mip level or the layer level.

So I added two functions in the trait :

    /// Returns the current mip level that is accessed by the gpu
    fn current_miplevel_access(&self) -> u32 {
        0
    }

    /// Returns the current layer level that is accessed by the gpu
    fn current_layer_level_access(&self) -> u32 {
        0        
    }

The default behaviour will be changed lated, but it is just for the test purpose...

I created the following structure that represents an image accessed by one command and which mip level are used by the command

pub struct ImageResourceAccess<Img> {
    image: Arc<Img>,
    mip_level: u32,
    layer_level: u32
}

It implements the ImageAccess trait and forward the call to the image object

unsafe impl<Img> ImageAccess for ImageResourceAccess<Img> where
    Img : ImageAccess {
        fn inner(&self) -> ImageInner {
            self.image.inner()
        }
    
        #[inline]
        fn initial_layout_requirement(&self) -> ImageLayout {
            self.image.initial_layout_requirement()
        }
// ......

         fn conflicts_image(&self, other: &dyn ImageAccess) -> bool {
            self.conflict_key() == other.conflict_key() &&
            self.current_miplevel_access() == other.current_miplevel_access() &&
            self.current_layer_level_access() == other.current_layer_level_access()
        }

        fn current_miplevel_access(&self) -> u32 {
            self.mip_level
        }
        fn current_layer_level_access(&self) -> u32 {
            self.layer_level
        }
    
        #[inline]
        fn conflict_key(&self) -> u64 {
            self.image.conflict_key() + self.mip_level as u64 + self.layer_level as u64
        }
}

For the conflict_key, I will do a better version later with a proper combine_hash

Here is the way I use it to transfer from one level to another.

let [xs, ys, ds] = img_dim.mipmap_dimensions(level - 1).unwrap().width_height_depth();
            let [xd, yd, dd] = img_dim.mipmap_dimensions(level).unwrap().width_height_depth();

            println!("{}, {}, {}", xs, xd, level);

            let src = Arc::new(ImageResourceAccess {
                image: buffer.clone(),
                mip_level: level - 1,
                layer_level: 0
            });

            let dst = Arc::new(ImageResourceAccess {
                image: buffer.clone(),
                mip_level: level,
                layer_level: 0
            });

            cbb.blit_image(
                src, //source
                [0, 0, 0], //source_top_left
                [xs as i32, ys as i32, ds as i32], //source_bottom_right
                0, //source_base_array_layer
                level - 1, //source_mip_level
                dst, //destination
                [0, 0, 0], //destination_top_left
                [xd as i32, yd as i32, dd as i32], //destination_bottom_right
                0, //destination_base_array_layer
                level, //destination_mip_level
                1, //layer_count
                Filter::Linear //filter
            ).expect("failed to blit a mip map to image!");

When I build all the mip levels, I finish on unreachable part.

thread 'main' panicked at 'internal error: entered unreachable code', A:\Programmation\rust\vulkano\vulkano\src\image\immutable.rs:349:23

       let future = match cb.execute(queue) {
            Ok(f) => f,
            Err(_) => unreachable!(),
        };

When I build only the level 1 (transfer from 0 to 1), I got this error

  left: `None`,
 right: `Some(ShaderReadOnlyOptimal)`', A:\Programmation\rust\vulkano\vulkano\src\image\immutable.rs:643:9

here

unsafe impl<F, A> ImageAccess for ImmutableImageInitialization<F, A>
where
    F: 'static + Send + Sync,
{
    unsafe fn increase_gpu_lock(&self) {
        debug_assert!(self.used.load(Ordering::Relaxed));
    }

    #[inline]
    unsafe fn unlock(&self, new_layout: Option<ImageLayout>) {
        assert_eq!(new_layout, Some(self.image.layout));
        self.image.initialized.store(true, Ordering::Relaxed);
    }
}

Do you have any ideas?

@Eliah-Lakhin
Copy link
Contributor

Eliah-Lakhin commented Nov 27, 2020

@qnope Resource access control is probably one of the most difficult part of Vulkano. And I don't have a mindset on it's internal implementation architecture, unfortunately. So, the only way to fix or implement something inside it is to make such mindset by digging inside the internals.

For the first error message I would log what came inside Err(_) variant. Maybe there is useful metadata that could shed light on what has actually failed and on which stage. Otherwise you can try to follow cb.execute control flow from inside.

For the second error it seems like there is a mechanism on the lower level of architecture that implicitly prevents(or more likely not designed for) the desired behavior. Maybe it will require certain modifications to achieve this goal.

My apology for such generic answer :) I really don't have any expertise about it, but that's are steps I would do next if I work on this feature.

@jpteb
Copy link
Contributor

jpteb commented Dec 2, 2020

The internal panic here is the same as in #1447.

If the CommandBuffer encounters an error in the lock_submit function, it will unlock all the previously locked resources. It then tries to unlock every image with a None optional. The above assertion in the unlock method of the image fails.

jpteb added a commit to jpteb/vulkano that referenced this issue Dec 2, 2020
If the CommandBuffer encountered an error in the lock_submit function,
it would unlock all the previously locked resources and return the
error. It would unlock all the images with a None value which would
panic in an assertion in the unlock method of the images. Now it checks
for the final_layout and unlocks the image this way.
This change should help with the panics mentioned in vulkano-rs#1441 and vulkano-rs#1447.
With this fix it should return the error that is occuring instead of
panicking.
Eliah-Lakhin pushed a commit that referenced this issue Dec 4, 2020
* Unlock images correctly on Err

If the CommandBuffer encountered an error in the lock_submit function,
it would unlock all the previously locked resources and return the
error. It would unlock all the images with a None value which would
panic in an assertion in the unlock method of the images. Now it checks
for the final_layout and unlocks the image this way.
This change should help with the panics mentioned in #1441 and #1447.
With this fix it should return the error that is occuring instead of
panicking.

* Add fix to the change log to inform users.
@Eliah-Lakhin
Copy link
Contributor

@qnope The core issue with cross-layers bliting is probably fixed in #1449 thanks to @Gosuo .

Can you confirm please if the bliting between layers works fine on your end, and you can continue working on this feature?

@qnope
Copy link
Contributor Author

qnope commented Dec 5, 2020

I am going to try to continue the dev during the weekend!

Thanks for you all

@qnope
Copy link
Contributor Author

qnope commented Dec 5, 2020

Good news,

I succeed to generate mipmaps !
I have to clean the code and I hope I can submit a pull request later this night or tomorrow

@Rua
Copy link
Contributor

Rua commented May 12, 2022

This should be solved by the various changes made to SyncCommandBufferBuilder in 0.30.

@Rua Rua closed this as completed May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants