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

DmaConfig's DmaDeviceConfig flags and structure reversed in terms of src/dst #539

Open
xzn opened this issue Feb 5, 2024 · 3 comments
Open

Comments

@xzn
Copy link

xzn commented Feb 5, 2024

Test case source:

https://github.com/xzn/libctru_dma_test/blob/master/source/main.c

Interactive test:

https://github.com/xzn/libctru_dma_test/raw/master/libctru_dma_test.3dsx

Press any button to advance.

Compiled using official devkitPro's pacman installed Windows binaries in msys2.

Output from test:

image

Switched case:

image

(As suggested in #536)

Edit:

Not sure how to do a test case for this one but the current DMACFG_SRC_IS_DEVICE and DMACFG_USE_SRC_CONFIG do seem to correspond to the current srcCfg, so if the structs are to be reversed, the flags should be too.

@xzn
Copy link
Author

xzn commented Aug 16, 2024

@TuxSH

I've been staring at your decompiled k11 source https://gist.github.com/TuxSH/f1f270710ed84576823fd1da348acb38/

If we look at all the KDmaAddress::Initialize calls then the "is device" flags are indeed correct it seems.

However if we look at all the KDmaBufferSize::ComputeFromConfig calls, the src and dst config structs are seemingly reversed, in relation to dst and src dma addresses. (The KDmaAddress's usage of KDmaBufferSize is the key as it can't possibly be wrong.)

The way I understand it, then, only the src and dst struct and the "use respective cfg" flags are reversed.

The "is device" flags are NOT reversed.

So then it appears that actual behavior is that, after doing all the reversing, if "dst is device" flag is set, then src cfg is read, and the deviceId in that cfg is the device id of the dst, and vice versa. (i.e. The deviceId field of the struct signifies the device id of the other end of the dma)

I can only make sense of this by imagining that, since if dst is device, for instance, then the dst address remains the same during dma so burstSize and burstStride are all essentially zero, and transferSize and transferStride from the other end are used to determine the dma operation. Hence the dst cfg need not to be read. It seems to work even if both ends are devices.

(I'm happy to be proven wrong however, I just would like to make sense of this. The decompiled k11 source has been quite informative even if I can't make sense of most of it..)

Screenshot of relevant decompiled code from KDmaChannel::Start
image

@TuxSH
Copy link
Collaborator

TuxSH commented Nov 3, 2024

Hi again,

I've had another look at this issue

The way I understand it, then, only the src and dst struct and the "use respective cfg" flags are reversed.

They are not:

  if ( (dmaCfg->flags & (unsigned __int8)DMACFGFLAG_USE_DST_CONFIG) != 0 )
  {
    this->dmaConfig.dstUseCfg = 1;
    this->dmaConfig.dstCfg = dmaCfg->dstCfg;
    this->dmaConfig.dstCfg.deviceId = -1;
  }
  if ( (dmaCfg->flags & DMACFGFLAG_USE_SRC_CONFIG) != 0 )
  {
    this->dmaConfig.srcUseCfg = 1;
    this->dmaConfig.srcCfg = dmaCfg->srcCfg;
    this->dmaConfig.srcCfg.deviceId = -1;
  }

if dst is device, for instance, then the dst address remains the same

Correct.

then the dst address remains the same during dma so burstSize and burstStride are all essentially zero, and transferSize and transferStride from the other end are used to determine the dma operation

No that's plain wrong. For a device burst size is the size of the FIFO, transfer size is the amount of data between each WFP, and (stride - size) is how much data you skip. Total size (in svc args) is the total size of the buffer including stride.

For example, for LGY scaler -> mem, source burst size and stride = 64, transfer size = 8*bpp*width (subtex size), transfer stride = 8*bpp*textureWidth (texture size); and dst cfg = default.

@xzn
Copy link
Author

xzn commented Nov 4, 2024

Thank you for the clarification on burst/transfer size/stride. I still don't understand with regard to (stride - size) with devices, if that is skipped when reading/writing to a device, or discarded.. (this doesn't directly relate to the issue)

TLDR for this comment: the confusion is only when doing mem-to-mem transfer. When one end is device the current names make sense.

In any case, the long version:

Quoted code
  if ( (dmaCfg->flags & (unsigned __int8)DMACFGFLAG_USE_DST_CONFIG) != 0 )
  {
    this->dmaConfig.dstUseCfg = 1;
    this->dmaConfig.dstCfg = dmaCfg->dstCfg;
    this->dmaConfig.dstCfg.deviceId = -1;
  }
  if ( (dmaCfg->flags & DMACFGFLAG_USE_SRC_CONFIG) != 0 )
  {
    this->dmaConfig.srcUseCfg = 1;
    this->dmaConfig.srcCfg = dmaCfg->srcCfg;
    this->dmaConfig.srcCfg.deviceId = -1;
  }

The current srcCfg has address stride info for dst memory, but the current srcCfg.deviceId is still src device id. When "device is dst" is set, for example, the dstCfg.deviceId is indeed the dst device id, however dstCfg will actually have the memory stride information for the src address. Combine that with the code you showed, it says that when "use * config" is set, the opposite end is presumed to be memory (which overrides the device set by "* is device").

Quoted example

For example, for LGY scaler -> mem, source burst size and stride = 64, transfer size = 8bppwidth (subtex size), transfer stride = 8bpptextureWidth (texture size); and dst cfg = default.

So it's device to mem, yet dst cfg = default, which means the memory textureWidth (which is of the dst address) is actually in srcCfg. (I don't have LGY decompiled, but I did take a look at the dma usage of cam process, and it matches as well.)

More specifically

From KDmaManager::ValidateUserParameters

          KDmaAddress::Initialize(&dmaAddr, dstAddra, dstIsDevice, process);
          if ( useSrcCfg )
            KDmaBufferSize::ComputeFromConfig(&v22, size, &dmaCfg->srcCfg);
          else
            KDmaBufferSize::operator=(&v22, size);
          if ( !KDmaAddress::CheckBufferRangeValid(&dmaAddr, v22.minOffset, v22.maxOffset) )
            return 0xE0E01BF5;

Notice how srcCfg is checked against dstAddra.

Also KDmaChannel::Start

  KDmaAddress::Initialize(&this->dstDmaAddress, dstAddr, dstIsDevice, dstProcess);
  KDmaAddress::Initialize(&this->srcDmaAddress, srcAddr, this->dmaConfig.srcIsDevice, srcProcess);
  currentPa = this->dstDmaAddress.currentPa;
  v23 = this->srcDmaAddress.currentPa;
  this->dmaConfig.srcDmaAddressPtr = &this->srcDmaAddress;
  this->dmaConfig.dstDmaAddressPtr = &this->dstDmaAddress;
  this->dmaSize.size = size;
  this->dmaConfig.dstAddr = currentPa;
  this->dmaConfig.srcAddr = v23;
  this->dstProcess = dstProcess;
  this->srcProcess = srcProcess;
  KDmaBufferSize::ComputeFromConfig(&this->dmaSize.dstBufferSize, size, &this->dmaConfig.srcCfg);
  KDmaBufferSize::ComputeFromConfig(&this->dmaSize.srcBufferSize, size, &this->dmaConfig.dstCfg);
  this->dmaConfig.dstIsContiguous = KDmaAddress::CheckPhysicalMemContiguous(
                                      &this->dstDmaAddress,
                                      this->dmaSize.dstBufferSize.minOffset,
                                      this->dmaSize.dstBufferSize.maxOffset);
  this->dmaConfig.srcIsContiguous = KDmaAddress::CheckPhysicalMemContiguous(
                                      &this->srcDmaAddress,
                                      this->dmaSize.srcBufferSize.minOffset,
                                      this->dmaSize.srcBufferSize.maxOffset);

Again src/dst cfg is checked against dst/src addresses. (See the ComputeFromConfig lines)

Now that I think of it, this could be I'm unfamiliar with how dma's nomenclature are normally intended, so instead of changing the flags and fields name, update the doc from

Use the provided source device configuration even if the DMA source is not a device.

to something like

Source device is memory, destination memory stride information in source device configuration.

I don't have more to add on this issue except for maybe docs should be updated at least because of the case of mem-to-mem was confusing.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants