| Message ID | 1449263345-4938-1-git-send-email-oded.gabbay@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Patch #1 & #2 are Reviewed-by: Christian König <christian.koenig@amd.com> For patch #3: Couldn't we just in a loop go over all the dw in the IB and swap them after writing them? That would simplify the patch massively. And line like the one below just look a bit odd to me: > for (i = ib.length_dw; i < ib_size_dw; ++i) > - ib.ptr[i] = 0x0; > + ib.ptr[i] = cpu_to_le32(0x0); Alternatively a helper function adding DW to an IB with swapping could do it as well. Regards, Christian. On 04.12.2015 22:09, Oded Gabbay wrote: > This patch makes the IB test on the GFX ring pass for CI-based cards > installed in Big-Endian machines. > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/radeon/cik.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c > index 248953d..05d43a0 100644 > --- a/drivers/gpu/drm/radeon/cik.c > +++ b/drivers/gpu/drm/radeon/cik.c > @@ -4173,11 +4173,7 @@ void cik_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib) > control |= ib->length_dw | (vm_id << 24); > > radeon_ring_write(ring, header); > - radeon_ring_write(ring, > -#ifdef __BIG_ENDIAN > - (2 << 0) | > -#endif > - (ib->gpu_addr & 0xFFFFFFFC)); > + radeon_ring_write(ring, (ib->gpu_addr & 0xFFFFFFFC)); > radeon_ring_write(ring, upper_32_bits(ib->gpu_addr) & 0xFFFF); > radeon_ring_write(ring, control); > }
On Sat, Dec 5, 2015 at 12:23 PM, Christian König <deathsimple@vodafone.de> wrote: > Patch #1 & #2 are Reviewed-by: Christian König <christian.koenig@amd.com> > > For patch #3: > > Couldn't we just in a loop go over all the dw in the IB and swap them after > writing them? That would simplify the patch massively. I guess we could do that, however I made the fix similar to the same code in the uvd module (See radeon_uvd_get_create_msg() for example) > > And line like the one below just look a bit odd to me: >> >> for (i = ib.length_dw; i < ib_size_dw; ++i) >> - ib.ptr[i] = 0x0; >> + ib.ptr[i] = cpu_to_le32(0x0); > Same remark as above. I added it in the last second before sending the patch just to be similar to uvd code. I guess maybe it is to remind people to do it if they ever change that zero value to something else ??? > > Alternatively a helper function adding DW to an IB with swapping could do it > as well. > Don't you think its an overkill ? It's just a few places in the code. > Regards, > Christian. > > > On 04.12.2015 22:09, Oded Gabbay wrote: >> >> This patch makes the IB test on the GFX ring pass for CI-based cards >> installed in Big-Endian machines. >> >> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >> Cc: stable@vger.kernel.org >> --- >> drivers/gpu/drm/radeon/cik.c | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c >> index 248953d..05d43a0 100644 >> --- a/drivers/gpu/drm/radeon/cik.c >> +++ b/drivers/gpu/drm/radeon/cik.c >> @@ -4173,11 +4173,7 @@ void cik_ring_ib_execute(struct radeon_device >> *rdev, struct radeon_ib *ib) >> control |= ib->length_dw | (vm_id << 24); >> radeon_ring_write(ring, header); >> - radeon_ring_write(ring, >> -#ifdef __BIG_ENDIAN >> - (2 << 0) | >> -#endif >> - (ib->gpu_addr & 0xFFFFFFFC)); >> + radeon_ring_write(ring, (ib->gpu_addr & 0xFFFFFFFC)); >> radeon_ring_write(ring, upper_32_bits(ib->gpu_addr) & 0xFFFF); >> radeon_ring_write(ring, control); >> } > >
On 06.12.2015 08:29, Oded Gabbay wrote: > On Sat, Dec 5, 2015 at 12:23 PM, Christian König > <deathsimple@vodafone.de> wrote: >> Patch #1 & #2 are Reviewed-by: Christian König <christian.koenig@amd.com> >> >> For patch #3: >> >> Couldn't we just in a loop go over all the dw in the IB and swap them after >> writing them? That would simplify the patch massively. > I guess we could do that, however I made the fix similar to the same > code in the uvd module (See radeon_uvd_get_create_msg() for example) > >> And line like the one below just look a bit odd to me: >>> for (i = ib.length_dw; i < ib_size_dw; ++i) >>> - ib.ptr[i] = 0x0; >>> + ib.ptr[i] = cpu_to_le32(0x0); > Same remark as above. I added it in the last second before sending the > patch just to be similar to uvd code. I guess maybe it is to remind > people to do it if they ever change that zero value to something else > ??? > >> Alternatively a helper function adding DW to an IB with swapping could do it >> as well. >> > Don't you think its an overkill ? It's just a few places in the code. Yeah, true indeed. Ok then let's just stick with the coding style like it is in radeon_uvd_get_create_msg(). So the patch is Reviewed-by: Christian König <christian.koenig@amd.com>. It sound like you have hardware to test this combination, so could you try to get it working on for amdgpu as well? Shouldn't be to much of a hassle, since amdgpu only supports CIK and VI generation for now and I think we never enabled the hardware swapping in amdgpu. Thanks in advance, Christian. > >> Regards, >> Christian. >> >> >> On 04.12.2015 22:09, Oded Gabbay wrote: >>> This patch makes the IB test on the GFX ring pass for CI-based cards >>> installed in Big-Endian machines. >>> >>> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >>> Cc: stable@vger.kernel.org >>> --- >>> drivers/gpu/drm/radeon/cik.c | 6 +----- >>> 1 file changed, 1 insertion(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c >>> index 248953d..05d43a0 100644 >>> --- a/drivers/gpu/drm/radeon/cik.c >>> +++ b/drivers/gpu/drm/radeon/cik.c >>> @@ -4173,11 +4173,7 @@ void cik_ring_ib_execute(struct radeon_device >>> *rdev, struct radeon_ib *ib) >>> control |= ib->length_dw | (vm_id << 24); >>> radeon_ring_write(ring, header); >>> - radeon_ring_write(ring, >>> -#ifdef __BIG_ENDIAN >>> - (2 << 0) | >>> -#endif >>> - (ib->gpu_addr & 0xFFFFFFFC)); >>> + radeon_ring_write(ring, (ib->gpu_addr & 0xFFFFFFFC)); >>> radeon_ring_write(ring, upper_32_bits(ib->gpu_addr) & 0xFFFF); >>> radeon_ring_write(ring, control); >>> } >>
On Sun, Dec 6, 2015 at 8:45 PM, Christian König <deathsimple@vodafone.de> wrote: > On 06.12.2015 08:29, Oded Gabbay wrote: >> >> On Sat, Dec 5, 2015 at 12:23 PM, Christian König >> <deathsimple@vodafone.de> wrote: >>> >>> Patch #1 & #2 are Reviewed-by: Christian König <christian.koenig@amd.com> >>> >>> For patch #3: >>> >>> Couldn't we just in a loop go over all the dw in the IB and swap them >>> after >>> writing them? That would simplify the patch massively. >> >> I guess we could do that, however I made the fix similar to the same >> code in the uvd module (See radeon_uvd_get_create_msg() for example) >> >>> And line like the one below just look a bit odd to me: >>>> >>>> for (i = ib.length_dw; i < ib_size_dw; ++i) >>>> - ib.ptr[i] = 0x0; >>>> + ib.ptr[i] = cpu_to_le32(0x0); >> >> Same remark as above. I added it in the last second before sending the >> patch just to be similar to uvd code. I guess maybe it is to remind >> people to do it if they ever change that zero value to something else >> ??? >> >>> Alternatively a helper function adding DW to an IB with swapping could do >>> it >>> as well. >>> >> Don't you think its an overkill ? It's just a few places in the code. > > > Yeah, true indeed. Ok then let's just stick with the coding style like it is > in radeon_uvd_get_create_msg(). > > So the patch is Reviewed-by: Christian König <christian.koenig@amd.com>. Thanks! > > It sound like you have hardware to test this combination, so could you try > to get it working on for amdgpu as well? Yes, I have a BE machine that I can test amdgpu with. However, I only have a Bonaire card. Is it worth doing it for CIK ? It's only for debug support, no ? > > Shouldn't be to much of a hassle, since amdgpu only supports CIK and VI > generation for now and I think we never enabled the hardware swapping in > amdgpu. > > Thanks in advance, > Christian. > > >> >>> Regards, >>> Christian. >>> >>> >>> On 04.12.2015 22:09, Oded Gabbay wrote: >>>> >>>> This patch makes the IB test on the GFX ring pass for CI-based cards >>>> installed in Big-Endian machines. >>>> >>>> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >>>> Cc: stable@vger.kernel.org >>>> --- >>>> drivers/gpu/drm/radeon/cik.c | 6 +----- >>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c >>>> index 248953d..05d43a0 100644 >>>> --- a/drivers/gpu/drm/radeon/cik.c >>>> +++ b/drivers/gpu/drm/radeon/cik.c >>>> @@ -4173,11 +4173,7 @@ void cik_ring_ib_execute(struct radeon_device >>>> *rdev, struct radeon_ib *ib) >>>> control |= ib->length_dw | (vm_id << 24); >>>> radeon_ring_write(ring, header); >>>> - radeon_ring_write(ring, >>>> -#ifdef __BIG_ENDIAN >>>> - (2 << 0) | >>>> -#endif >>>> - (ib->gpu_addr & 0xFFFFFFFC)); >>>> + radeon_ring_write(ring, (ib->gpu_addr & 0xFFFFFFFC)); >>>> radeon_ring_write(ring, upper_32_bits(ib->gpu_addr) & 0xFFFF); >>>> radeon_ring_write(ring, control); >>>> } >>> >>> >
On 06.12.2015 20:00, Oded Gabbay wrote: > On Sun, Dec 6, 2015 at 8:45 PM, Christian König <deathsimple@vodafone.de> wrote: >> On 06.12.2015 08:29, Oded Gabbay wrote: >>> On Sat, Dec 5, 2015 at 12:23 PM, Christian König >>> <deathsimple@vodafone.de> wrote: >>>> Patch #1 & #2 are Reviewed-by: Christian König <christian.koenig@amd.com> >>>> >>>> For patch #3: >>>> >>>> Couldn't we just in a loop go over all the dw in the IB and swap them >>>> after >>>> writing them? That would simplify the patch massively. >>> I guess we could do that, however I made the fix similar to the same >>> code in the uvd module (See radeon_uvd_get_create_msg() for example) >>> >>>> And line like the one below just look a bit odd to me: >>>>> for (i = ib.length_dw; i < ib_size_dw; ++i) >>>>> - ib.ptr[i] = 0x0; >>>>> + ib.ptr[i] = cpu_to_le32(0x0); >>> Same remark as above. I added it in the last second before sending the >>> patch just to be similar to uvd code. I guess maybe it is to remind >>> people to do it if they ever change that zero value to something else >>> ??? >>> >>>> Alternatively a helper function adding DW to an IB with swapping could do >>>> it >>>> as well. >>>> >>> Don't you think its an overkill ? It's just a few places in the code. >> >> Yeah, true indeed. Ok then let's just stick with the coding style like it is >> in radeon_uvd_get_create_msg(). >> >> So the patch is Reviewed-by: Christian König <christian.koenig@amd.com>. > Thanks! > >> It sound like you have hardware to test this combination, so could you try >> to get it working on for amdgpu as well? > Yes, I have a BE machine that I can test amdgpu with. > However, I only have a Bonaire card. Is it worth doing it for CIK ? > It's only for debug support, no ? Yeah, CIK support in amdgpu was only for debugging and bringup. But since it's only shared code you touch when it works with CIK it should work with VI as well and that's rather interesting to us. Regards, Christian. > >> Shouldn't be to much of a hassle, since amdgpu only supports CIK and VI >> generation for now and I think we never enabled the hardware swapping in >> amdgpu. >> >> Thanks in advance, >> Christian. >> >> >>>> Regards, >>>> Christian. >>>> >>>> >>>> On 04.12.2015 22:09, Oded Gabbay wrote: >>>>> This patch makes the IB test on the GFX ring pass for CI-based cards >>>>> installed in Big-Endian machines. >>>>> >>>>> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >>>>> Cc: stable@vger.kernel.org >>>>> --- >>>>> drivers/gpu/drm/radeon/cik.c | 6 +----- >>>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c >>>>> index 248953d..05d43a0 100644 >>>>> --- a/drivers/gpu/drm/radeon/cik.c >>>>> +++ b/drivers/gpu/drm/radeon/cik.c >>>>> @@ -4173,11 +4173,7 @@ void cik_ring_ib_execute(struct radeon_device >>>>> *rdev, struct radeon_ib *ib) >>>>> control |= ib->length_dw | (vm_id << 24); >>>>> radeon_ring_write(ring, header); >>>>> - radeon_ring_write(ring, >>>>> -#ifdef __BIG_ENDIAN >>>>> - (2 << 0) | >>>>> -#endif >>>>> - (ib->gpu_addr & 0xFFFFFFFC)); >>>>> + radeon_ring_write(ring, (ib->gpu_addr & 0xFFFFFFFC)); >>>>> radeon_ring_write(ring, upper_32_bits(ib->gpu_addr) & 0xFFFF); >>>>> radeon_ring_write(ring, control); >>>>> } >>>>
OK, I'll take a look
Oded
> Yeah, CIK support in amdgpu was only for debugging and bringup. But since it's only shared code you touch when it works with CIK it should work with VI as well and that's rather interesting to us. Regards, Christian.
On Sun, Dec 6, 2015 at 1:45 PM, Christian König <deathsimple@vodafone.de> wrote: > On 06.12.2015 08:29, Oded Gabbay wrote: >> >> On Sat, Dec 5, 2015 at 12:23 PM, Christian König >> <deathsimple@vodafone.de> wrote: >>> >>> Patch #1 & #2 are Reviewed-by: Christian König <christian.koenig@amd.com> >>> >>> For patch #3: >>> >>> Couldn't we just in a loop go over all the dw in the IB and swap them >>> after >>> writing them? That would simplify the patch massively. >> >> I guess we could do that, however I made the fix similar to the same >> code in the uvd module (See radeon_uvd_get_create_msg() for example) >> >>> And line like the one below just look a bit odd to me: >>>> >>>> for (i = ib.length_dw; i < ib_size_dw; ++i) >>>> - ib.ptr[i] = 0x0; >>>> + ib.ptr[i] = cpu_to_le32(0x0); >> >> Same remark as above. I added it in the last second before sending the >> patch just to be similar to uvd code. I guess maybe it is to remind >> people to do it if they ever change that zero value to something else >> ??? >> >>> Alternatively a helper function adding DW to an IB with swapping could do >>> it >>> as well. >>> >> Don't you think its an overkill ? It's just a few places in the code. > > > Yeah, true indeed. Ok then let's just stick with the coding style like it is > in radeon_uvd_get_create_msg(). > > So the patch is Reviewed-by: Christian König <christian.koenig@amd.com>. Applied the series. Thanks! Alex > > It sound like you have hardware to test this combination, so could you try > to get it working on for amdgpu as well? > > Shouldn't be to much of a hassle, since amdgpu only supports CIK and VI > generation for now and I think we never enabled the hardware swapping in > amdgpu. > > Thanks in advance, > Christian. > > >> >>> Regards, >>> Christian. >>> >>> >>> On 04.12.2015 22:09, Oded Gabbay wrote: >>>> >>>> This patch makes the IB test on the GFX ring pass for CI-based cards >>>> installed in Big-Endian machines. >>>> >>>> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >>>> Cc: stable@vger.kernel.org >>>> --- >>>> drivers/gpu/drm/radeon/cik.c | 6 +----- >>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c >>>> index 248953d..05d43a0 100644 >>>> --- a/drivers/gpu/drm/radeon/cik.c >>>> +++ b/drivers/gpu/drm/radeon/cik.c >>>> @@ -4173,11 +4173,7 @@ void cik_ring_ib_execute(struct radeon_device >>>> *rdev, struct radeon_ib *ib) >>>> control |= ib->length_dw | (vm_id << 24); >>>> radeon_ring_write(ring, header); >>>> - radeon_ring_write(ring, >>>> -#ifdef __BIG_ENDIAN >>>> - (2 << 0) | >>>> -#endif >>>> - (ib->gpu_addr & 0xFFFFFFFC)); >>>> + radeon_ring_write(ring, (ib->gpu_addr & 0xFFFFFFFC)); >>>> radeon_ring_write(ring, upper_32_bits(ib->gpu_addr) & 0xFFFF); >>>> radeon_ring_write(ring, control); >>>> } >>> >>> >
diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c index 248953d..05d43a0 100644 --- a/drivers/gpu/drm/radeon/cik.c +++ b/drivers/gpu/drm/radeon/cik.c @@ -4173,11 +4173,7 @@ void cik_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib) control |= ib->length_dw | (vm_id << 24); radeon_ring_write(ring, header); - radeon_ring_write(ring, -#ifdef __BIG_ENDIAN - (2 << 0) | -#endif - (ib->gpu_addr & 0xFFFFFFFC)); + radeon_ring_write(ring, (ib->gpu_addr & 0xFFFFFFFC)); radeon_ring_write(ring, upper_32_bits(ib->gpu_addr) & 0xFFFF); radeon_ring_write(ring, control); }
This patch makes the IB test on the GFX ring pass for CI-based cards installed in Big-Endian machines. Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/radeon/cik.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)