Re: [PATCH 3/3] drm/amd: validate user GEM object size

[kbuild test robot]

Hi Yu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.20-rc7 next-20181221]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yu-Zhao/drm-amd-fix-race-in-page-flip-job/20181222-072937
config: x86_64-randconfig-s5-12221153 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/cdev.h:8:0,
from include/drm/drmP.h:36,
from drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:26:
   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c: In function 
'amdgpu_display_user_framebuffer_create':
   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:556:28: warning: format '%d' 
expects argument of type 'int', but argument 4 has type 'size_t {aka long 
unsigned int}' [-Wformat=]
  dev_err(>pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
   ^
   include/linux/device.h:1370:22: note: in definition of macro 'dev_fmt'
#define dev_fmt(fmt) fmt
 ^~~
>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:556:3: note: in expansion of 
>> macro 'dev_err'
  dev_err(>pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
  ^~~

vim +/dev_err +556 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c

   521  
   522  struct drm_framebuffer *
   523  amdgpu_display_user_framebuffer_create(struct drm_device *dev,
   524 struct drm_file *file_priv,
   525 const struct drm_mode_fb_cmd2 
*mode_cmd)
   526  {
   527  struct drm_gem_object *obj;
   528  struct amdgpu_framebuffer *amdgpu_fb;
   529  int ret;
   530  int height;
   531  struct amdgpu_device *adev = dev->dev_private;
   532  int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
   533  int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, 
false);
   534  
   535  if (mode_cmd->pitches[0] != pitch) {
   536  dev_err(>pdev->dev, "Invalid pitch: expecting %d 
but got %d\n",
   537  pitch, mode_cmd->pitches[0]);
   538  return ERR_PTR(-EINVAL);
   539  }
   540  
   541  obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
   542  if (obj ==  NULL) {
   543  dev_err(>pdev->dev, "No GEM object associated to 
handle 0x%08X, "
   544  "can't create framebuffer\n", 
mode_cmd->handles[0]);
   545  return ERR_PTR(-ENOENT);
   546  }
   547  
   548  /* Handle is imported dma-buf, so cannot be migrated to VRAM 
for scanout */
   549  if (obj->import_attach) {
   550  DRM_DEBUG_KMS("Cannot create framebuffer from imported 
dma_buf\n");
   551  return ERR_PTR(-EINVAL);
   552  }
   553  
   554  height = ALIGN(mode_cmd->height, 8);
   555  if (obj->size < pitch * height) {
 > 556  dev_err(>pdev->dev, "Invalid GEM size: expecting 
 > %d but got %d\n",
   557  pitch * height, obj->size);
   558  return ERR_PTR(-EINVAL);
   559  }
   560  
   561  amdgpu_fb = kzalloc(sizeof(*amdgpu_fb), GFP_KERNEL);
   562  if (amdgpu_fb == NULL) {
   563  drm_gem_object_put_unlocked(obj);
   564  return ERR_PTR(-ENOMEM);
   565  }
   566  
   567  ret = amdgpu_display_framebuffer_init(dev, amdgpu_fb, mode_cmd, 
obj);
   568  if (ret) {
   569  kfree(amdgpu_fb);
   570  drm_gem_object_put_unlocked(obj);
   571  return ERR_PTR(ret);
   572  }
   573  
   574  return _fb->base;
   575  }
   576  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amd: validate user GEM object size

[kbuild test robot]

Hi Yu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.20-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Yu-Zhao/drm-amd-fix-race-in-page-flip-job/20181222-072937
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=ia64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/cdev.h:8,
from include/drm/drmP.h:36,
from drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:26:
   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c: In function 
'amdgpu_display_user_framebuffer_create':
>> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:556:28: warning: format '%d' 
>> expects argument of type 'int', but argument 4 has type 'size_t' {aka 'long 
>> unsigned int'} [-Wformat=]
  dev_err(>pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
   ^
   include/linux/device.h:1370:22: note: in definition of macro 'dev_fmt'
#define dev_fmt(fmt) fmt
 ^~~
   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c:556:3: note: in expansion of 
macro 'dev_err'
  dev_err(>pdev->dev, "Invalid GEM size: expecting %d but got %d\n",
  ^~~

vim +556 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c

   521  
   522  struct drm_framebuffer *
   523  amdgpu_display_user_framebuffer_create(struct drm_device *dev,
   524 struct drm_file *file_priv,
   525 const struct drm_mode_fb_cmd2 
*mode_cmd)
   526  {
   527  struct drm_gem_object *obj;
   528  struct amdgpu_framebuffer *amdgpu_fb;
   529  int ret;
   530  int height;
   531  struct amdgpu_device *adev = dev->dev_private;
   532  int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
   533  int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, 
false);
   534  
   535  if (mode_cmd->pitches[0] != pitch) {
   536  dev_err(>pdev->dev, "Invalid pitch: expecting %d 
but got %d\n",
   537  pitch, mode_cmd->pitches[0]);
   538  return ERR_PTR(-EINVAL);
   539  }
   540  
   541  obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
   542  if (obj ==  NULL) {
   543  dev_err(>pdev->dev, "No GEM object associated to 
handle 0x%08X, "
   544  "can't create framebuffer\n", 
mode_cmd->handles[0]);
   545  return ERR_PTR(-ENOENT);
   546  }
   547  
   548  /* Handle is imported dma-buf, so cannot be migrated to VRAM 
for scanout */
   549  if (obj->import_attach) {
   550  DRM_DEBUG_KMS("Cannot create framebuffer from imported 
dma_buf\n");
   551  return ERR_PTR(-EINVAL);
   552  }
   553  
   554  height = ALIGN(mode_cmd->height, 8);
   555  if (obj->size < pitch * height) {
 > 556  dev_err(>pdev->dev, "Invalid GEM size: expecting 
 > %d but got %d\n",
   557  pitch * height, obj->size);
   558  return ERR_PTR(-EINVAL);
   559  }
   560  
   561  amdgpu_fb = kzalloc(sizeof(*amdgpu_fb), GFP_KERNEL);
   562  if (amdgpu_fb == NULL) {
   563  drm_gem_object_put_unlocked(obj);
   564  return ERR_PTR(-ENOMEM);
   565  }
   566  
   567  ret = amdgpu_display_framebuffer_init(dev, amdgpu_fb, mode_cmd, 
obj);
   568  if (ret) {
   569  kfree(amdgpu_fb);
   570  drm_gem_object_put_unlocked(obj);
   571  return ERR_PTR(ret);
   572  }
   573  
   574  return _fb->base;
   575  }
   576  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.

[Grodzovsky, Andrey]

On 12/21/2018 01:37 PM, Christian König wrote:
> Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
>> Decauple sched threads stop and start and ring mirror
>> list handling from the policy of what to do about the
>> guilty jobs.
>> When stoppping the sched thread and detaching sched fences
>> from non signaled HW fenes wait for all signaled HW fences
>> to complete before rerunning the jobs.
>>
>> v2: Fix resubmission of guilty job into HW after refactoring.
>>
>> v4:
>> Full restart for all the jobs, not only from guilty ring.
>> Extract karma increase into standalone function.
>>
>> v5:
>> Rework waiting for signaled jobs without relying on the job
>> struct itself as those might already be freed for non 'guilty'
>> job's schedulers.
>> Expose karma increase to drivers.
>>
>> Suggested-by: Christian Koenig 
>> Signed-off-by: Andrey Grodzovsky 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  18 +--
>>   drivers/gpu/drm/etnaviv/etnaviv_sched.c    |  11 +-
>>   drivers/gpu/drm/scheduler/sched_main.c | 188 
>> +++--
>>   drivers/gpu/drm/v3d/v3d_sched.c    |  12 +-
>>   include/drm/gpu_scheduler.h    |  10 +-
>>   5 files changed, 151 insertions(+), 88 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 8a078f4..a4bd2d3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3298,12 +3298,10 @@ static int 
>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>   if (!ring || !ring->sched.thread)
>>   continue;
>>   -    kthread_park(ring->sched.thread);
>> +    drm_sched_stop(>sched, job ? >base : NULL);
>>   -    if (job && job->base.sched != >sched)
>> -    continue;
>> -
>> -    drm_sched_hw_job_reset(>sched, job ? >base : NULL);
>> +    if(job)
>> +    drm_sched_increase_karma(>base);
>
> Since we dropped the "job && job->base.sched != >sched" check 
> above this will now increase the jobs karma multiple times.
>
> Maybe just move that outside of the loop.
>
>>     /* after all hw jobs are reset, hw fence is meaningless, 
>> so force_completion */
>>   amdgpu_fence_driver_force_completion(ring);
>> @@ -3454,14 +3452,10 @@ static void 
>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
>>   if (!ring || !ring->sched.thread)
>>   continue;
>>   -    /* only need recovery sched of the given job's ring
>> - * or all rings (in the case @job is NULL)
>> - * after above amdgpu_reset accomplished
>> - */
>> -    if ((!job || job->base.sched == >sched) && 
>> !adev->asic_reset_res)
>> -    drm_sched_job_recovery(>sched);
>> +    if (!adev->asic_reset_res)
>> +    drm_sched_resubmit_jobs(>sched);
>>   -    kthread_unpark(ring->sched.thread);
>> +    drm_sched_start(>sched, !adev->asic_reset_res);
>>   }
>>     if (!amdgpu_device_has_dc_support(adev)) {
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c 
>> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> index 49a6763..6f1268f 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> @@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct 
>> drm_sched_job *sched_job)
>>   }
>>     /* block scheduler */
>> -    kthread_park(gpu->sched.thread);
>> -    drm_sched_hw_job_reset(>sched, sched_job);
>> +    drm_sched_stop(>sched, sched_job);
>> +
>> +    if(sched_job)
>> +    drm_sched_increase_karma(sched_job);
>>     /* get the GPU back into the init state */
>>   etnaviv_core_dump(gpu);
>>   etnaviv_gpu_recover_hang(gpu);
>>   +    drm_sched_resubmit_jobs(>sched);
>> +
>>   /* restart scheduler after GPU is usable again */
>> -    drm_sched_job_recovery(>sched);
>> -    kthread_unpark(gpu->sched.thread);
>> +    drm_sched_start(>sched, true);
>>   }
>>     static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index dbb6906..b5c5bee 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -60,8 +60,6 @@
>>     static void drm_sched_process_job(struct dma_fence *f, struct 
>> dma_fence_cb *cb);
>>   -static void drm_sched_expel_job_unlocked(struct drm_sched_job 
>> *s_job);
>> -
>>   /**
>>    * drm_sched_rq_init - initialize a given run queue struct
>>    *
>> @@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct 
>> work_struct *work)
>>   spin_unlock_irqrestore(>job_list_lock, flags);
>>   }
>
> Kernel doc here would be nice to have.
>
>> +void drm_sched_increase_karma(struct drm_sched_job *bad)
>> +{
>> +    int i;
>> +    struct drm_sched_entity *tmp;
>> +    struct drm_sched_entity *entity;
>> +    struct drm_gpu_scheduler 

[PATCH v2 2/2] drm/amd: validate user GEM object size

[Yu Zhao]

When creating frame buffer, userspace may request to attach to a
previously allocated GEM object that is smaller than what GPU
requires. Validation must be done to prevent out-of-bound DMA,
which could not only corrupt memory but also reveal sensitive data.

This fix is not done in a common code path because individual
driver might have different requirement.

Signed-off-by: Yu Zhao 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 883a4df2386d..098d6a816ee1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,7 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
*dev,
struct drm_gem_object *obj;
struct amdgpu_framebuffer *amdgpu_fb;
int ret;
+   int height;
struct amdgpu_device *adev = dev->dev_private;
int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
int pitch = amdgpu_align_pitch(adev, mode_cmd->pitches[0], cpp, false);
@@ -550,6 +551,13 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
*dev,
return ERR_PTR(-EINVAL);
}
 
+   height = ALIGN(mode_cmd->height, 8);
+   if (obj->size < pitch * height) {
+   DRM_DEBUG_KMS("Invalid GEM size: expecting >= %d but got %ld\n",
+ pitch * height, obj->size);
+   return ERR_PTR(-EINVAL);
+   }
+
amdgpu_fb = kzalloc(sizeof(*amdgpu_fb), GFP_KERNEL);
if (amdgpu_fb == NULL) {
drm_gem_object_put_unlocked(obj);
-- 
2.20.1.415.g653613c723-goog

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2 1/2] drm/amd: validate user pitch alignment

[Yu Zhao]

Userspace may request pitch alignment that is not supported by GPU.
Some requests 32, but GPU ignores it and uses default 64 when cpp is
4. If GEM object is allocated based on the smaller alignment, GPU
DMA will go out of bound.

For GPU that does frame buffer compression, DMA writing out of bound
memory will cause memory corruption.

Signed-off-by: Yu Zhao 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 686a26de50f9..883a4df2386d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
*dev,
struct drm_gem_object *obj;
struct amdgpu_framebuffer *amdgpu_fb;
int ret;
+   struct amdgpu_device *adev = dev->dev_private;
+   int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
+   int pitch = amdgpu_align_pitch(adev, mode_cmd->pitches[0], cpp, false);
+
+   if (mode_cmd->pitches[0] != pitch) {
+   DRM_DEBUG_KMS("Invalid pitch: expecting %d but got %d\n",
+ pitch, mode_cmd->pitches[0]);
+   return ERR_PTR(-EINVAL);
+   }
 
obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
if (obj ==  NULL) {
-- 
2.20.1.415.g653613c723-goog

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Yu Zhao]

On Fri, Dec 21, 2018 at 10:07:26AM +0100, Michel Dänzer wrote:
> On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> > Userspace may request pitch alignment that is not supported by GPU.
> > Some requests 32, but GPU ignores it and uses default 64 when cpp is
> > 4. If GEM object is allocated based on the smaller alignment, GPU
> > DMA will go out of bound.
> > 
> > For GPU that does frame buffer compression, DMA writing out of bound
> > memory will cause memory corruption.
> > 
> > Signed-off-by: Yu Zhao 
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index e309d26170db..755daa332f8a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct 
> > drm_device *dev,
> > struct drm_gem_object *obj;
> > struct amdgpu_framebuffer *amdgpu_fb;
> > int ret;
> > +   struct amdgpu_device *adev = dev->dev_private;
> > +   int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
> > +   int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false);
> 
> Also, this needs to use mode_cmd->pitches[0] instead of mode_cmd->width,
> otherwise it'll spuriously fail for larger but well-aligned pitches.

Good point, thanks.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.

[Christian König]

Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:

Decauple sched threads stop and start and ring mirror
list handling from the policy of what to do about the
guilty jobs.
When stoppping the sched thread and detaching sched fences
from non signaled HW fenes wait for all signaled HW fences
to complete before rerunning the jobs.

v2: Fix resubmission of guilty job into HW after refactoring.

v4:
Full restart for all the jobs, not only from guilty ring.
Extract karma increase into standalone function.

v5:
Rework waiting for signaled jobs without relying on the job
struct itself as those might already be freed for non 'guilty'
job's schedulers.
Expose karma increase to drivers.

Suggested-by: Christian Koenig 
Signed-off-by: Andrey Grodzovsky 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  18 +--
  drivers/gpu/drm/etnaviv/etnaviv_sched.c|  11 +-
  drivers/gpu/drm/scheduler/sched_main.c | 188 +++--
  drivers/gpu/drm/v3d/v3d_sched.c|  12 +-
  include/drm/gpu_scheduler.h|  10 +-
  5 files changed, 151 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 8a078f4..a4bd2d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3298,12 +3298,10 @@ static int amdgpu_device_pre_asic_reset(struct 
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
  
-		kthread_park(ring->sched.thread);

+   drm_sched_stop(>sched, job ? >base : NULL);
  
-		if (job && job->base.sched != >sched)

-   continue;
-
-   drm_sched_hw_job_reset(>sched, job ? >base : NULL);
+   if(job)
+   drm_sched_increase_karma(>base);


Since we dropped the "job && job->base.sched != >sched" check 
above this will now increase the jobs karma multiple times.


Maybe just move that outside of the loop.

  
  		/* after all hw jobs are reset, hw fence is meaningless, so force_completion */

amdgpu_fence_driver_force_completion(ring);
@@ -3454,14 +3452,10 @@ static void amdgpu_device_post_asic_reset(struct 
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
  
-		/* only need recovery sched of the given job's ring

-* or all rings (in the case @job is NULL)
-* after above amdgpu_reset accomplished
-*/
-   if ((!job || job->base.sched == >sched) && 
!adev->asic_reset_res)
-   drm_sched_job_recovery(>sched);
+   if (!adev->asic_reset_res)
+   drm_sched_resubmit_jobs(>sched);
  
-		kthread_unpark(ring->sched.thread);

+   drm_sched_start(>sched, !adev->asic_reset_res);
}
  
  	if (!amdgpu_device_has_dc_support(adev)) {

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c 
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 49a6763..6f1268f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct 
drm_sched_job *sched_job)
}
  
  	/* block scheduler */

-   kthread_park(gpu->sched.thread);
-   drm_sched_hw_job_reset(>sched, sched_job);
+   drm_sched_stop(>sched, sched_job);
+
+   if(sched_job)
+   drm_sched_increase_karma(sched_job);
  
  	/* get the GPU back into the init state */

etnaviv_core_dump(gpu);
etnaviv_gpu_recover_hang(gpu);
  
+	drm_sched_resubmit_jobs(>sched);

+
/* restart scheduler after GPU is usable again */
-   drm_sched_job_recovery(>sched);
-   kthread_unpark(gpu->sched.thread);
+   drm_sched_start(>sched, true);
  }
  
  static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index dbb6906..b5c5bee 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -60,8 +60,6 @@
  
  static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *cb);
  
-static void drm_sched_expel_job_unlocked(struct drm_sched_job *s_job);

-
  /**
   * drm_sched_rq_init - initialize a given run queue struct
   *
@@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct work_struct 
*work)
spin_unlock_irqrestore(>job_list_lock, flags);
  }
  


Kernel doc here would be nice to have.


+void drm_sched_increase_karma(struct drm_sched_job *bad)
+{
+   int i;
+   struct drm_sched_entity *tmp;
+   struct drm_sched_entity *entity;
+   struct drm_gpu_scheduler *sched = bad->sched;
+
+   /* don't increase @bad's karma if it's from KERNEL RQ,
+* because sometimes GPU hang would cause kernel jobs (like VM updating 
jobs)
+* corrupt but keep in mind that kernel jobs 

Re: [PATCH] drm/amd/powerplay/polaris10_smumgr: Remove duplicate

[Alex Deucher]

Already fixed here:
https://cgit.freedesktop.org/drm/drm/commit/?id=7e07834c12b96214e95a473f7b14fc03b20e2e7a

Thanks,

Alex

On Fri, Dec 21, 2018 at 9:59 AM Brajeswar Ghosh
 wrote:
>
> Remove ppatomctrl.h which is included more than once
>
> Signed-off-by: Brajeswar Ghosh 
> ---
>  drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c 
> b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
> index 872d3824337b..2b2c26616902 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
> @@ -44,7 +44,6 @@
>
>  #include "smu7_hwmgr.h"
>  #include "hardwaremanager.h"
> -#include "ppatomctrl.h"
>  #include "atombios.h"
>  #include "pppcielanes.h"
>
> --
> 2.17.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Koenig, Christian]

Am 21.12.18 um 19:19 schrieb Alex Deucher:
> On Fri, Dec 21, 2018 at 1:15 PM Christian König
>  wrote:
>> Am 21.12.18 um 10:09 schrieb Deng, Emily:
 -Original Message-
 From: Michel Dänzer 
 Sent: Friday, December 21, 2018 4:52 PM
 To: Deng, Emily 
 Cc: amd-gfx@lists.freedesktop.org
 Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

 On 2018-12-21 9:45 a.m., Deng, Emily wrote:
>> -Original Message-
>> From: Michel Dänzer 
>> Sent: Friday, December 21, 2018 4:38 PM
>> To: Deng, Emily 
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>
>> On 2018-12-21 8:26 a.m., Emily Deng wrote:
>>> When the bo is used to set mode, the bo need to be pinned.
>> On second thought, why does the BO need to be pinned? When using the
>> display hardware, the BO needs to be pinned to prevent it from being
>> moved while the hardware is scanning out from it, but that shouldn't be
 necessary here.
> The pin here is used for scan out the buffer by remote display app.
 I still don't understand why pinning is needed. What mechanism does the 
 remote
 display app use to access the BO contents?
>>> Sorry, I am not familiar with the remote display app. Maybe it will use drm 
>>> ioctl function to get the
>>> current crtc's fb's information, and get the content in the fb's buffer 
>>> object by mmap or translate the bo
>>> to an OpenGL texture for next rendering. Maybe don't need to pin the bo 
>>> here, as the use has no different with
>>> other normal bos. So please ignore the patch, and will send another patch 
>>> to remove the unpin the fb's bo code.
>> Opening the BO and doing something with it in OpenGL should result in
>> proper fencing of the BO, so this sounds like a workaround for a bug
>> somewhere else.
>>
>> As long as this isn't explained further this patch is certainly a NAK.
>>
>> Pinning for physical displays is allowed because they are limited in
>> number, but this is not necessary the case with a virtual output.
> Well practically, it's the same as real displays.  We limit the
> virtual displays to 6 just like most hw.  It really should overate the
> same.  We just don't need the pinning per se because there is no hw
> actively scanning out of it.

Yeah, but do we limit the number of pending flips like we do for real 
hardware as well?

For real hard we have at maximum two BOs pinned, the current one and the 
next one. For the virtual scanout I'm not sure about that.

Christian.

>
> Alex
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Alex Deucher]

On Fri, Dec 21, 2018 at 1:15 PM Christian König
 wrote:
>
> Am 21.12.18 um 10:09 schrieb Deng, Emily:
> >> -Original Message-
> >> From: Michel Dänzer 
> >> Sent: Friday, December 21, 2018 4:52 PM
> >> To: Deng, Emily 
> >> Cc: amd-gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
> >>
> >> On 2018-12-21 9:45 a.m., Deng, Emily wrote:
>  -Original Message-
>  From: Michel Dänzer 
>  Sent: Friday, December 21, 2018 4:38 PM
>  To: Deng, Emily 
>  Cc: amd-gfx@lists.freedesktop.org
>  Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
> 
>  On 2018-12-21 8:26 a.m., Emily Deng wrote:
> > When the bo is used to set mode, the bo need to be pinned.
>  On second thought, why does the BO need to be pinned? When using the
>  display hardware, the BO needs to be pinned to prevent it from being
>  moved while the hardware is scanning out from it, but that shouldn't be
> >> necessary here.
> >>> The pin here is used for scan out the buffer by remote display app.
> >> I still don't understand why pinning is needed. What mechanism does the 
> >> remote
> >> display app use to access the BO contents?
> > Sorry, I am not familiar with the remote display app. Maybe it will use drm 
> > ioctl function to get the
> > current crtc's fb's information, and get the content in the fb's buffer 
> > object by mmap or translate the bo
> > to an OpenGL texture for next rendering. Maybe don't need to pin the bo 
> > here, as the use has no different with
> > other normal bos. So please ignore the patch, and will send another patch 
> > to remove the unpin the fb's bo code.
>
> Opening the BO and doing something with it in OpenGL should result in
> proper fencing of the BO, so this sounds like a workaround for a bug
> somewhere else.
>
> As long as this isn't explained further this patch is certainly a NAK.
>
> Pinning for physical displays is allowed because they are limited in
> number, but this is not necessary the case with a virtual output.

Well practically, it's the same as real displays.  We limit the
virtual displays to 6 just like most hw.  It really should overate the
same.  We just don't need the pinning per se because there is no hw
actively scanning out of it.

Alex

>
> Regards,
> Christian.
>
> >>
> >> --
> >> Earthling Michel Dänzer   |   http://www.amd.com
> >> Libre software enthusiast | Mesa and X developer
> > ___
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Christian König]

Am 21.12.18 um 10:09 schrieb Deng, Emily:

-Original Message-
From: Michel Dänzer 
Sent: Friday, December 21, 2018 4:52 PM
To: Deng, Emily 
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

On 2018-12-21 9:45 a.m., Deng, Emily wrote:

-Original Message-
From: Michel Dänzer 
Sent: Friday, December 21, 2018 4:38 PM
To: Deng, Emily 
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

On 2018-12-21 8:26 a.m., Emily Deng wrote:

When the bo is used to set mode, the bo need to be pinned.

On second thought, why does the BO need to be pinned? When using the
display hardware, the BO needs to be pinned to prevent it from being
moved while the hardware is scanning out from it, but that shouldn't be

necessary here.

The pin here is used for scan out the buffer by remote display app.

I still don't understand why pinning is needed. What mechanism does the remote
display app use to access the BO contents?

Sorry, I am not familiar with the remote display app. Maybe it will use drm 
ioctl function to get the
current crtc's fb's information, and get the content in the fb's buffer object 
by mmap or translate the bo
to an OpenGL texture for next rendering. Maybe don't need to pin the bo here, 
as the use has no different with
other normal bos. So please ignore the patch, and will send another patch to 
remove the unpin the fb's bo code.


Opening the BO and doing something with it in OpenGL should result in 
proper fencing of the BO, so this sounds like a workaround for a bug 
somewhere else.


As long as this isn't explained further this patch is certainly a NAK.

Pinning for physical displays is allowed because they are limited in 
number, but this is not necessary the case with a virtual output.


Regards,
Christian.



--
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 06/13] Move deferred vblank events to separate drm_vblank_deferred list

[Michel Dänzer]

From: Michel Dänzer 

It was still possible for nested xorg_list_for_each_entry_safe loops
to occur over the drm_vblank_signalled list, which could mess up that
list. Moving deferred events to a separate list allows processing the
drm_vblank_signalled list without xorg_list_for_each_entry_safe.

Bugzilla: https://bugs.freedesktop.org/108600
(Ported from amdgpu commit 51ba6dddee40c3688d4c7b12eabeab516ed153b7)

Signed-off-by: Michel Dänzer 
---
 src/radeon_drm_queue.c | 57 ++
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/src/radeon_drm_queue.c b/src/radeon_drm_queue.c
index acfea3daf..d8a8243c5 100644
--- a/src/radeon_drm_queue.c
+++ b/src/radeon_drm_queue.c
@@ -56,6 +56,7 @@ static int radeon_drm_queue_refcnt;
 static struct xorg_list radeon_drm_queue;
 static struct xorg_list radeon_drm_flip_signalled;
 static struct xorg_list radeon_drm_vblank_signalled;
+static struct xorg_list radeon_drm_vblank_deferred;
 static uintptr_t radeon_drm_queue_seq;
 
 
@@ -111,6 +112,31 @@ radeon_drm_queue_handler(int fd, unsigned int frame, 
unsigned int sec,
 }
 }
 
+/*
+ * Handle signalled vblank events. If we're waiting for a flip event,
+ * put events for that CRTC in the vblank_deferred list.
+ */
+static void
+radeon_drm_handle_vblank_signalled(void)
+{
+drmmode_crtc_private_ptr drmmode_crtc;
+struct radeon_drm_queue_entry *e;
+
+while (!xorg_list_is_empty(_drm_vblank_signalled)) {
+   e = xorg_list_first_entry(_drm_vblank_signalled,
+ struct radeon_drm_queue_entry, list);
+   drmmode_crtc = e->crtc->driver_private;
+
+   if (drmmode_crtc->wait_flip_nesting_level == 0) {
+   radeon_drm_queue_handle_one(e);
+   continue;
+   }
+
+   xorg_list_del(>list);
+   xorg_list_append(>list, _drm_vblank_deferred);
+}
+}
+
 /*
  * Handle deferred DRM vblank events
  *
@@ -127,12 +153,18 @@ radeon_drm_queue_handle_deferred(xf86CrtcPtr crtc)
--drmmode_crtc->wait_flip_nesting_level > 0)
return;
 
-xorg_list_for_each_entry_safe(e, tmp, _drm_vblank_signalled, list) {
-   drmmode_crtc_private_ptr drmmode_crtc = e->crtc->driver_private;
+/* Put previously deferred vblank events for this CRTC back in the
+ * signalled queue
+ */
+xorg_list_for_each_entry_safe(e, tmp, _drm_vblank_deferred, list) {
+   if (e->crtc != crtc)
+   continue;
 
-   if (drmmode_crtc->wait_flip_nesting_level == 0)
-   radeon_drm_queue_handle_one(e);
+   xorg_list_del(>list);
+   xorg_list_append(>list, _drm_vblank_signalled);
 }
+
+radeon_drm_handle_vblank_signalled();
 }
 
 /*
@@ -205,6 +237,13 @@ radeon_drm_abort_entry(uintptr_t seq)
}
 }
 
+xorg_list_for_each_entry_safe(e, tmp, _drm_vblank_deferred, list) {
+   if (e->seq == seq) {
+   radeon_drm_abort_one(e);
+   return;
+   }
+}
+
 xorg_list_for_each_entry_safe(e, tmp, _drm_queue, list) {
if (e->seq == seq) {
radeon_drm_abort_one(e);
@@ -235,7 +274,7 @@ radeon_drm_abort_id(uint64_t id)
 int
 radeon_drm_handle_event(int fd, drmEventContext *event_context)
 {
-struct radeon_drm_queue_entry *e, *tmp;
+struct radeon_drm_queue_entry *e;
 int r;
 
 r = drmHandleEvent(fd, event_context);
@@ -246,12 +285,7 @@ radeon_drm_handle_event(int fd, drmEventContext 
*event_context)
radeon_drm_queue_handle_one(e);
 }
 
-xorg_list_for_each_entry_safe(e, tmp, _drm_vblank_signalled, list) {
-   drmmode_crtc_private_ptr drmmode_crtc = e->crtc->driver_private;
-
-   if (drmmode_crtc->wait_flip_nesting_level == 0)
-   radeon_drm_queue_handle_one(e);
-}
+radeon_drm_handle_vblank_signalled();
 
 return r;
 }
@@ -298,6 +332,7 @@ radeon_drm_queue_init(ScrnInfoPtr scrn)
 xorg_list_init(_drm_queue);
 xorg_list_init(_drm_flip_signalled);
 xorg_list_init(_drm_vblank_signalled);
+xorg_list_init(_drm_vblank_deferred);
 }
 
 /*
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 08/13] Cancel pending scanout update in drmmode_crtc_scanout_update

[Michel Dänzer]

From: Michel Dänzer 

drmmode_crtc_scanout_update does the equivalent of a scanout update,
so no need to do it again. This might also avoid issues if there's a
pending scanout update at this point.

(Ported from amdgpu commit 4e7a24ac5a64e402146953ec5850d13c05742116)

Signed-off-by: Michel Dänzer 
---
 src/drmmode_display.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 134b0f72b..34c88c8e6 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -781,11 +781,17 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, 
DisplayModePtr mode,
*fb = 
radeon_pixmap_get_fb(drmmode_crtc->scanout[scanout_id].pixmap);
*x = *y = 0;
 
-   radeon_scanout_do_update(crtc, scanout_id,
-screen->GetWindowPixmap(screen->root),
-extents);
-   RegionEmpty(DamageRegion(drmmode_crtc->scanout_damage));
-   radeon_finish(scrn, drmmode_crtc->scanout[scanout_id].bo);
+   if (radeon_scanout_do_update(crtc, scanout_id,
+
screen->GetWindowPixmap(screen->root),
+extents)) {
+   RegionEmpty(DamageRegion(drmmode_crtc->scanout_damage));
+   radeon_glamor_finish(scrn);
+
+   if (!drmmode_crtc->flip_pending) {
+   radeon_drm_abort_entry(drmmode_crtc->
+  scanout_update_pending);
+   }
+   }
}
 }
 
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 05/13] Explicitly keep track of whether a DRM event is for a flip or not

[Michel Dänzer]

From: Michel Dänzer 

When an async flip is performed, and TearFree is enabled on the CRTC
used for timing, we schedule a vblank event for completing the page
flip. The DRM event queuing code treated this event like a vblank event,
but it needs to be treated like a page flip event.

(Ported from amdgpu commit e2c7369cae65069aa93eed1c0b678f975ce5c274)

Signed-off-by: Michel Dänzer 
---
 src/drmmode_display.c  |  3 ++-
 src/radeon_dri2.c  |  4 ++--
 src/radeon_drm_queue.c | 39 +++
 src/radeon_drm_queue.h |  3 ++-
 src/radeon_kms.c   | 10 ++
 src/radeon_present.c   |  3 ++-
 6 files changed, 25 insertions(+), 37 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 831394d4c..134b0f72b 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -3397,7 +3397,8 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, ClientPtr 
client,
drm_queue_seq = radeon_drm_queue_alloc(crtc, client, id,
   flipdata,
   drmmode_flip_handler,
-  drmmode_flip_abort);
+  drmmode_flip_abort,
+  TRUE);
if (drm_queue_seq == RADEON_DRM_QUEUE_ERROR) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "Allocating DRM queue event entry 
failed.\n");
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index 4d12fc09a..922ed4fb6 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -1076,7 +1076,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr 
client, DrawablePtr draw,
 
 drm_queue_seq = radeon_drm_queue_alloc(crtc, client, 
RADEON_DRM_QUEUE_ID_DEFAULT,
   wait_info, 
radeon_dri2_frame_event_handler,
-  radeon_dri2_frame_event_abort);
+  radeon_dri2_frame_event_abort, 
FALSE);
 if (drm_queue_seq == RADEON_DRM_QUEUE_ERROR) {
 xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "Allocating DRM queue event entry failed.\n");
@@ -1215,7 +1215,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
DrawablePtr draw,
 
 drm_queue_seq = radeon_drm_queue_alloc(crtc, client, 
RADEON_DRM_QUEUE_ID_DEFAULT,
   swap_info, 
radeon_dri2_frame_event_handler,
-  radeon_dri2_frame_event_abort);
+  radeon_dri2_frame_event_abort, 
FALSE);
 if (drm_queue_seq == RADEON_DRM_QUEUE_ERROR) {
 xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "Allocating DRM queue entry failed.\n");
diff --git a/src/radeon_drm_queue.c b/src/radeon_drm_queue.c
index ebc6a5b64..acfea3daf 100644
--- a/src/radeon_drm_queue.c
+++ b/src/radeon_drm_queue.c
@@ -48,6 +48,7 @@ struct radeon_drm_queue_entry {
 xf86CrtcPtr crtc;
 radeon_drm_handler_proc handler;
 radeon_drm_abort_proc abort;
+Bool is_flip;
 unsigned int frame;
 };
 
@@ -86,8 +87,8 @@ radeon_drm_abort_one(struct radeon_drm_queue_entry *e)
 }
 
 static void
-radeon_drm_queue_handler(struct xorg_list *signalled, unsigned int frame,
-unsigned int sec, unsigned int usec, void *user_ptr)
+radeon_drm_queue_handler(int fd, unsigned int frame, unsigned int sec,
+unsigned int usec, void *user_ptr)
 {
 uintptr_t seq = (uintptr_t)user_ptr;
 struct radeon_drm_queue_entry *e, *tmp;
@@ -102,34 +103,14 @@ radeon_drm_queue_handler(struct xorg_list *signalled, 
unsigned int frame,
xorg_list_del(>list);
e->usec = (uint64_t)sec * 100 + usec;
e->frame = frame;
-   xorg_list_append(>list, signalled);
+   xorg_list_append(>list, e->is_flip ?
+_drm_flip_signalled :
+_drm_vblank_signalled);
break;
}
 }
 }
 
-/*
- * Signal a DRM page flip event
- */
-static void
-radeon_drm_page_flip_handler(int fd, unsigned int frame, unsigned int sec,
-unsigned int usec, void *user_ptr)
-{
-radeon_drm_queue_handler(_drm_flip_signalled, frame, sec, usec,
-user_ptr);
-}
-
-/*
- * Signal a DRM vblank event
- */
-static void
-radeon_drm_vblank_handler(int fd, unsigned int frame, unsigned int sec,
- unsigned int usec, void *user_ptr)
-{
-radeon_drm_queue_handler(_drm_vblank_signalled, frame, sec, usec,
-user_ptr);
-}
-
 /*
  * Handle deferred DRM vblank events
  *
@@ -162,7 +143,8 @@ uintptr_t
 radeon_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
   uint64_t id, void *data,
   radeon_drm_handler_proc 

[PATCH xf86-video-ati 01/13] Detect and fix up non-premultiplied cursor data

[Michel Dänzer]

From: Michel Dänzer 

X server >= 1.18 already had code for this, but it only caught cases
where some pixels have 0 for alpha and non-0 for a non-alpha component.
Turns out some apps (e.g. the Civilization VI game) use
non-premultiplied cursor data which doesn't have such pixels, but can
still result in visual artifacts.

This uses the method suggested by Kamil in
https://bugs.freedesktop.org/92309#c19: check for pixels where any
colour component value is larger than the alpha value, which isn't
possible with premultiplied alpha.

There can still be non-premultiplied data which won't be caught by this,
but that should result in slightly incorrect colours and/or blending at
the worst, not wildly incorrect colours such as shown in the bug report
below.

Bugzilla: https://bugs.freedesktop.org/108355
(Ported from amdgpu commits ad6dfb0124860cf67730bde85867f81d9258c84d &
 426f9a49655f01863cf4d898f525e5f95984e0c4)

Signed-off-by: Michel Dänzer 
---
 src/drmmode_display.c | 81 ---
 1 file changed, 61 insertions(+), 20 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 00d94449d..9a9e092a7 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1049,29 +1049,52 @@ drmmode_cursor_src_offset(Rotation rotation, int width, 
int height,
 
 #endif
 
-static uint32_t
-drmmode_cursor_gamma(xf86CrtcPtr crtc, uint32_t argb)
+static Bool
+drmmode_cursor_pixel(xf86CrtcPtr crtc, uint32_t *argb, Bool premultiplied,
+Bool apply_gamma)
 {
-   uint32_t alpha = argb >> 24;
+   uint32_t alpha = *argb >> 24;
uint32_t rgb[3];
int i;
 
-   if (!alpha)
-   return 0;
+   if (premultiplied) {
+#if XORG_VERSION_CURRENT < XORG_VERSION_NUMERIC(1, 18, 4, 0, 0)
+   if (alpha == 0 && (*argb & 0xff) != 0)
+   /* Doesn't look like premultiplied alpha */
+   return FALSE;
+#endif
 
-   if (crtc->scrn->depth != 24 && crtc->scrn->depth != 32)
-   return argb;
+   if (!apply_gamma)
+   return TRUE;
+   }
 
-   /* Un-premultiply alpha */
+   if (!alpha) {
+   *argb = 0;
+   return TRUE;
+   }
+
+   /* Extract RGB */
for (i = 0; i < 3; i++)
-   rgb[i] = ((argb >> (i * 8)) & 0xff) * 0xff / alpha;
+   rgb[i] = (*argb >> (i * 8)) & 0xff;
+
+   if (premultiplied) {
+   /* Un-premultiply alpha */
+   for (i = 0; i < 3; i++)
+   rgb[i] = rgb[i] * 0xff / alpha;
+   }
+
+   if (apply_gamma) {
+   rgb[0] = crtc->gamma_blue[rgb[0]] >> 8;
+   rgb[1] = crtc->gamma_green[rgb[1]] >> 8;
+   rgb[2] = crtc->gamma_red[rgb[2]] >> 8;
+   }
 
-   /* Apply gamma correction and pre-multiply alpha */
-   rgb[0] = (crtc->gamma_blue[rgb[0]] >> 8) * alpha / 0xff;
-   rgb[1] = (crtc->gamma_green[rgb[1]] >> 8) * alpha / 0xff;
-   rgb[2] = (crtc->gamma_red[rgb[2]] >> 8) * alpha / 0xff;
+   /* Premultiply alpha */
+   for (i = 0; i < 3; i++)
+   rgb[i] = rgb[i] * alpha / 0xff;
 
-   return alpha << 24 | rgb[2] << 16 | rgb[1] << 8 | rgb[0];
+   *argb = alpha << 24 | rgb[2] << 16 | rgb[1] << 8 | rgb[0];
+   return TRUE;
 }
 
 static void
@@ -1080,27 +1103,37 @@ drmmode_load_cursor_argb (xf86CrtcPtr crtc, CARD32 
*image)
ScrnInfoPtr pScrn = crtc->scrn;
RADEONInfoPtr info = RADEONPTR(pScrn);
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+   Bool premultiplied = TRUE;
+   Bool apply_gamma = TRUE;
+   uint32_t argb;
uint32_t *ptr;
 
/* cursor should be mapped already */
ptr = (uint32_t *)(drmmode_crtc->cursor_bo->ptr);
 
+   if (crtc->scrn->depth != 24 && crtc->scrn->depth != 32)
+   apply_gamma = FALSE;
+
 #if XF86_CRTC_VERSION < 7
if (crtc->driverIsPerformingTransform) {
uint32_t cursor_w = info->cursor_w, cursor_h = info->cursor_h;
int dstx, dsty;
int srcoffset;
 
+retry_transform:
for (dsty = 0; dsty < cursor_h; dsty++) {
for (dstx = 0; dstx < cursor_w; dstx++) {
srcoffset = 
drmmode_cursor_src_offset(crtc->rotation,
  cursor_w,
  cursor_h,
  dstx, 
dsty);
-
-   ptr[dsty * info->cursor_w + dstx] =
-   cpu_to_le32(drmmode_cursor_gamma(crtc,
-
image[srcoffset]));
+   argb = image[srcoffset];
+   if 

[PATCH xf86-video-ati 12/13] Update cursor position in drmmode_show_cursor if hotspot changed

[Michel Dänzer]

From: Michel Dänzer 

The cursor position is updated to be consistent with the new hotspot in
the same ioctl call.

(Ported from amdgpu commit b04697de5270e8e45744a7025c24df1f454a4cf0)

Signed-off-by: Michel Dänzer 
---
 src/drmmode_display.c | 75 +--
 src/drmmode_display.h |  5 +++
 2 files changed, 48 insertions(+), 32 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index c2a59da7b..bec309e5e 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1017,6 +1017,9 @@ drmmode_set_cursor_position (xf86CrtcPtr crtc, int x, int 
y)
}
 #endif
 
+   drmmode_crtc->cursor_x = x;
+   drmmode_crtc->cursor_y = y;
+
drmModeMoveCursor(pRADEONEnt->fd, drmmode_crtc->mode_crtc->crtc_id, x, 
y);
 }
 
@@ -1202,6 +1205,10 @@ drmmode_show_cursor (xf86CrtcPtr crtc)
RADEONInfoPtr info = RADEONPTR(pScrn);
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
RADEONEntPtr pRADEONEnt = RADEONEntPriv(pScrn);
+   xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
+   CursorPtr cursor = xf86_config->cursor;
+   int xhot = cursor->bits->xhot;
+   int yhot = cursor->bits->yhot;
static Bool use_set_cursor2 = TRUE;
struct drm_mode_cursor2 arg;
 
@@ -1213,41 +1220,45 @@ drmmode_show_cursor (xf86CrtcPtr crtc)
arg.width = info->cursor_w;
arg.height = info->cursor_h;
 
+   if (crtc->rotation != RR_Rotate_0 &&
+   crtc->rotation != (RR_Rotate_180 | RR_Reflect_X |
+  RR_Reflect_Y)) {
+   int t;
+
+   /* Reflect & rotate hotspot position */
+   if (crtc->rotation & RR_Reflect_X)
+   xhot = info->cursor_w - xhot - 1;
+   if (crtc->rotation & RR_Reflect_Y)
+   yhot = info->cursor_h - yhot - 1;
+
+   switch (crtc->rotation & 0xf) {
+   case RR_Rotate_90:
+   t = xhot;
+   xhot = yhot;
+   yhot = info->cursor_w - t - 1;
+   break;
+   case RR_Rotate_180:
+   xhot = info->cursor_w - xhot - 1;
+   yhot = info->cursor_h - yhot - 1;
+   break;
+   case RR_Rotate_270:
+   t = xhot;
+   xhot = info->cursor_h - yhot - 1;
+   yhot = t;
+   }
+   }
+
+   if (xhot != drmmode_crtc->cursor_xhot || yhot != 
drmmode_crtc->cursor_yhot) {
+   arg.flags |= DRM_MODE_CURSOR_MOVE;
+   arg.x = drmmode_crtc->cursor_x += drmmode_crtc->cursor_xhot - xhot;
+   arg.y = drmmode_crtc->cursor_y += drmmode_crtc->cursor_yhot - yhot;
+   drmmode_crtc->cursor_xhot = xhot;
+   drmmode_crtc->cursor_yhot = yhot;
+   }
+
if (use_set_cursor2) {
-   xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
-   CursorPtr cursor = xf86_config->cursor;
-   int xhot = cursor->bits->xhot;
-   int yhot = cursor->bits->yhot;
int ret;
 
-   if (crtc->rotation != RR_Rotate_0 &&
-   crtc->rotation != (RR_Rotate_180 | RR_Reflect_X |
-  RR_Reflect_Y)) {
-   int t;
-
-   /* Reflect & rotate hotspot position */
-   if (crtc->rotation & RR_Reflect_X)
-   xhot = info->cursor_w - xhot - 1;
-   if (crtc->rotation & RR_Reflect_Y)
-   yhot = info->cursor_h - yhot - 1;
-
-   switch (crtc->rotation & 0xf) {
-   case RR_Rotate_90:
-   t = xhot;
-   xhot = yhot;
-   yhot = info->cursor_w - t - 1;
-   break;
-   case RR_Rotate_180:
-   xhot = info->cursor_w - xhot - 1;
-   yhot = info->cursor_h - yhot - 1;
-   break;
-   case RR_Rotate_270:
-   t = xhot;
-   xhot = info->cursor_h - yhot - 1;
-   yhot = t;
-   }
-   }
-
arg.hot_x = xhot;
arg.hot_y = yhot;
 
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index d7ab9d7e9..49893ab04 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -87,6 +87,11 @@ typedef struct {
 drmmode_ptr drmmode;
 drmModeCrtcPtr mode_crtc;
 int hw_id;
+
+int cursor_x;
+int cursor_y;
+int cursor_xhot;
+int cursor_yhot;
 struct radeon_bo *cursor_bo;
 struct drmmode_scanout rotate;
 struct drmmode_scanout scanout[2];
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 10/13] Drop RADEONInfoRec::cursor_bo array

[Michel Dänzer]

From: Michel Dänzer 

Not needed or even useful for anything.

(Ported from amdgpu commit e95044e45350870fa7e237860e89ade91ac03550)

Signed-off-by: Michel Dänzer 
---
 src/drmmode_display.c |  9 -
 src/drmmode_display.h |  1 -
 src/radeon.h  |  1 -
 src/radeon_kms.c  | 21 +++--
 4 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 34c88c8e6..ef235bd21 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2967,15 +2967,6 @@ miPointerSpriteFuncRec drmmode_sprite_funcs = {
 };
 

-void drmmode_set_cursor(ScrnInfoPtr scrn, drmmode_ptr drmmode, int id, struct 
radeon_bo *bo)
-{
-   xf86CrtcConfigPtr   xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
-   xf86CrtcPtr crtc = xf86_config->crtc[id];
-   drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
-
-   drmmode_crtc->cursor_bo = bo;
-}
-
 void drmmode_adjust_frame(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int x, int y)
 {
xf86CrtcConfigPtr   config = XF86_CRTC_CONFIG_PTR(pScrn);
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index f56596644..d7ab9d7e9 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -213,7 +213,6 @@ extern Bool drmmode_pre_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, int cpp);
 extern void drmmode_init(ScrnInfoPtr pScrn, drmmode_ptr drmmode);
 extern void drmmode_fini(ScrnInfoPtr pScrn, drmmode_ptr drmmode);
 extern Bool drmmode_set_bufmgr(ScrnInfoPtr pScrn, drmmode_ptr drmmode, struct 
radeon_bo_manager *bufmgr);
-extern void drmmode_set_cursor(ScrnInfoPtr scrn, drmmode_ptr drmmode, int id, 
struct radeon_bo *bo);
 void drmmode_adjust_frame(ScrnInfoPtr pScrn, drmmode_ptr drmmode, int x, int 
y);
 extern Bool drmmode_set_desired_modes(ScrnInfoPtr pScrn, drmmode_ptr drmmode,
  Bool set_hw);
diff --git a/src/radeon.h b/src/radeon.h
index cde922c63..74454c307 100644
--- a/src/radeon.h
+++ b/src/radeon.h
@@ -572,7 +572,6 @@ typedef struct {
 struct radeon_cs_manager *csm;
 struct radeon_cs *cs;
 
-struct radeon_bo *cursor_bo[32];
 uint64_t vram_size;
 uint64_t gart_size;
 drmmode_rec drmmode;
diff --git a/src/radeon_kms.c b/src/radeon_kms.c
index dd9955da7..27a02109a 100644
--- a/src/radeon_kms.c
+++ b/src/radeon_kms.c
@@ -2760,21 +2760,20 @@ static Bool radeon_setup_kernel_mem(ScreenPtr pScreen)
cursor_size = info->cursor_w * info->cursor_h * 4;
cursor_size = RADEON_ALIGN(cursor_size, RADEON_GPU_PAGE_SIZE);
for (c = 0; c < xf86_config->num_crtc; c++) {
-   /* cursor objects */
-if (!info->cursor_bo[c]) {
-info->cursor_bo[c] = radeon_bo_open(info->bufmgr, 0,
-cursor_size, 0,
-RADEON_GEM_DOMAIN_VRAM, 0);
-if (!info->cursor_bo[c]) {
+   drmmode_crtc_private_ptr drmmode_crtc = 
xf86_config->crtc[c]->driver_private;
+
+   if (!drmmode_crtc->cursor_bo) {
+   drmmode_crtc->cursor_bo = radeon_bo_open(info->bufmgr, 0,
+cursor_size, 0,
+
RADEON_GEM_DOMAIN_VRAM, 0);
+   if (!(drmmode_crtc->cursor_bo)) {
 ErrorF("Failed to allocate cursor buffer memory\n");
 return FALSE;
 }
 
-if (radeon_bo_map(info->cursor_bo[c], 1)) {
+   if (radeon_bo_map(drmmode_crtc->cursor_bo, 1)) {
 ErrorF("Failed to map cursor buffer memory\n");
 }
-
-drmmode_set_cursor(pScrn, >drmmode, c, 
info->cursor_bo[c]);
 }
 }
 }
@@ -2840,7 +2839,9 @@ void radeon_kms_update_vram_limit(ScrnInfoPtr pScrn, 
uint32_t new_fb_size)
 int c;
 
 for (c = 0; c < xf86_config->num_crtc; c++) {
-   if (info->cursor_bo[c])
+   drmmode_crtc_private_ptr drmmode_crtc = 
xf86_config->crtc[c]->driver_private;
+
+   if (drmmode_crtc->cursor_bo)
new_fb_size += (64 * 4 * 64);
 }
 
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 07/13] Perform scanout buffer update immediately if drmmode_wait_vblank fails

[Michel Dänzer]

From: Michel Dänzer 

Otherwise the damaged screen contents may never be displayed in that
case.

(Ported from amdgpu commit 500fadb16285146e91f62fce3a0ce1360ca684ba)

Signed-off-by: Michel Dänzer 
---
 src/radeon_kms.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/radeon_kms.c b/src/radeon_kms.c
index a7aade702..796dac5f1 100644
--- a/src/radeon_kms.c
+++ b/src/radeon_kms.c
@@ -752,6 +752,7 @@ radeon_prime_scanout_update(PixmapDirtyUpdatePtr dirty)
 {
 ScreenPtr screen = dirty->slave_dst->drawable.pScreen;
 ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+RADEONEntPtr pRADEONEnt = RADEONEntPriv(scrn);
 xf86CrtcPtr xf86_crtc = radeon_prime_dirty_to_crtc(dirty);
 drmmode_crtc_private_ptr drmmode_crtc;
 uintptr_t drm_queue_seq;
@@ -774,19 +775,23 @@ radeon_prime_scanout_update(PixmapDirtyUpdatePtr dirty)
 if (drm_queue_seq == RADEON_DRM_QUEUE_ERROR) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "radeon_drm_queue_alloc failed for PRIME update\n");
+   radeon_prime_scanout_update_handler(xf86_crtc, 0, 0, NULL);
return;
 }
 
+drmmode_crtc->scanout_update_pending = drm_queue_seq;
+
 if (!drmmode_wait_vblank(xf86_crtc, DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT,
 1, drm_queue_seq, NULL, NULL)) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "drmmode_wait_vblank failed for PRIME update: %s\n",
   strerror(errno));
-   radeon_drm_abort_entry(drm_queue_seq);
-   return;
+   drmmode_crtc->drmmode->event_context.vblank_handler(pRADEONEnt->fd,
+   0, 0, 0,
+   
(void*)drm_queue_seq);
+   drmmode_crtc->wait_flip_nesting_level++;
+   radeon_drm_queue_handle_deferred(xf86_crtc);
 }
-
-drmmode_crtc->scanout_update_pending = drm_queue_seq;
 }
 
 static void
@@ -1022,8 +1027,9 @@ static void
 radeon_scanout_update(xf86CrtcPtr xf86_crtc)
 {
 drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
+ScrnInfoPtr scrn = xf86_crtc->scrn;
+RADEONEntPtr pRADEONEnt = RADEONEntPriv(scrn);
 uintptr_t drm_queue_seq;
-ScrnInfoPtr scrn;
 DamagePtr pDamage;
 RegionPtr pRegion;
 BoxRec extents;
@@ -1048,7 +1054,6 @@ radeon_scanout_update(xf86CrtcPtr xf86_crtc)
return;
 }
 
-scrn = xf86_crtc->scrn;
 drm_queue_seq = radeon_drm_queue_alloc(xf86_crtc,
   RADEON_DRM_QUEUE_CLIENT_DEFAULT,
   RADEON_DRM_QUEUE_ID_DEFAULT,
@@ -1059,19 +1064,23 @@ radeon_scanout_update(xf86CrtcPtr xf86_crtc)
 if (drm_queue_seq == RADEON_DRM_QUEUE_ERROR) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "radeon_drm_queue_alloc failed for scanout update\n");
+   radeon_scanout_update_handler(xf86_crtc, 0, 0, drmmode_crtc);
return;
 }
 
+drmmode_crtc->scanout_update_pending = drm_queue_seq;
+
 if (!drmmode_wait_vblank(xf86_crtc, DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT,
 1, drm_queue_seq, NULL, NULL)) {
xf86DrvMsg(scrn->scrnIndex, X_WARNING,
   "drmmode_wait_vblank failed for scanout update: %s\n",
   strerror(errno));
-   radeon_drm_abort_entry(drm_queue_seq);
-   return;
+   drmmode_crtc->drmmode->event_context.vblank_handler(pRADEONEnt->fd,
+   0, 0, 0,
+   
(void*)drm_queue_seq);
+   drmmode_crtc->wait_flip_nesting_level++;
+   radeon_drm_queue_handle_deferred(xf86_crtc);
 }
-
-drmmode_crtc->scanout_update_pending = drm_queue_seq;
 }
 
 static void
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 11/13] Use drmIoctl in drmmode_show_cursor

[Michel Dänzer]

From: Michel Dänzer 

This should be functionally equivalent to what drmModeSetCursor(2) do
behind the scenes, but allows for new tricks in following changes.

(Ported from amdgpu commit b344e1559e936046ef02c777fc4f6bcefa3830bc)

Signed-off-by: Michel Dänzer 
---
 src/drmmode_display.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index ef235bd21..c2a59da7b 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1202,8 +1202,16 @@ drmmode_show_cursor (xf86CrtcPtr crtc)
RADEONInfoPtr info = RADEONPTR(pScrn);
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
RADEONEntPtr pRADEONEnt = RADEONEntPriv(pScrn);
-   uint32_t handle = drmmode_crtc->cursor_bo->handle;
static Bool use_set_cursor2 = TRUE;
+   struct drm_mode_cursor2 arg;
+
+   memset(, 0, sizeof(arg));
+
+   arg.handle = drmmode_crtc->cursor_bo->handle;
+   arg.flags = DRM_MODE_CURSOR_BO;
+   arg.crtc_id = drmmode_crtc->mode_crtc->crtc_id;
+   arg.width = info->cursor_w;
+   arg.height = info->cursor_h;
 
if (use_set_cursor2) {
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
@@ -1240,18 +1248,17 @@ drmmode_show_cursor (xf86CrtcPtr crtc)
}
}
 
-   ret =
-   drmModeSetCursor2(pRADEONEnt->fd, 
drmmode_crtc->mode_crtc->crtc_id,
- handle, info->cursor_w, info->cursor_h,
- xhot, yhot);
+   arg.hot_x = xhot;
+   arg.hot_y = yhot;
+
+   ret = drmIoctl(pRADEONEnt->fd, DRM_IOCTL_MODE_CURSOR2, );
if (ret == -EINVAL)
use_set_cursor2 = FALSE;
else
return;
}
 
-   drmModeSetCursor(pRADEONEnt->fd, drmmode_crtc->mode_crtc->crtc_id, 
handle,
-info->cursor_w, info->cursor_h);
+   drmIoctl(pRADEONEnt->fd, DRM_IOCTL_MODE_CURSOR, );
 }
 
 /* Xorg expects a non-NULL return value from drmmode_crtc_shadow_allocate, and
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 02/13] Skip gamma correction of cursor data if premultiplied R/G/B > alpha

[Michel Dänzer]

From: Michel Dänzer 

The un-premultiplied R/G/B values would overflow the gamma LUT, so just
pass through the data unchanged, and leave it up to the HW how to
interpret such weird premultiplied alpha pixels.

Bugzilla: https://bugs.freedesktop.org/108355
(Ported from amdgpu commit 13c94a373b4858a2d2aa14c22b5f98d53c84c0d9)

Signed-off-by: Michel Dänzer 
---
 src/drmmode_display.c | 34 +-
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 9a9e092a7..831394d4c 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1050,8 +1050,8 @@ drmmode_cursor_src_offset(Rotation rotation, int width, 
int height,
 #endif
 
 static Bool
-drmmode_cursor_pixel(xf86CrtcPtr crtc, uint32_t *argb, Bool premultiplied,
-Bool apply_gamma)
+drmmode_cursor_pixel(xf86CrtcPtr crtc, uint32_t *argb, Bool *premultiplied,
+Bool *apply_gamma)
 {
uint32_t alpha = *argb >> 24;
uint32_t rgb[3];
@@ -1059,13 +1059,23 @@ drmmode_cursor_pixel(xf86CrtcPtr crtc, uint32_t *argb, 
Bool premultiplied,
 
if (premultiplied) {
 #if XORG_VERSION_CURRENT < XORG_VERSION_NUMERIC(1, 18, 4, 0, 0)
-   if (alpha == 0 && (*argb & 0xff) != 0)
+   if (alpha == 0 && (*argb & 0xff) != 0) {
/* Doesn't look like premultiplied alpha */
+   *premultiplied = FALSE;
return FALSE;
+   }
 #endif
 
-   if (!apply_gamma)
+   if (!(*apply_gamma))
return TRUE;
+
+   if (*argb > (alpha | alpha << 8 | alpha << 16 | alpha << 24)) {
+   /* Un-premultiplied R/G/B would overflow gamma LUT,
+* don't apply gamma correction
+*/
+   *apply_gamma = FALSE;
+   return FALSE;
+   }
}
 
if (!alpha) {
@@ -1083,7 +1093,7 @@ drmmode_cursor_pixel(xf86CrtcPtr crtc, uint32_t *argb, 
Bool premultiplied,
rgb[i] = rgb[i] * 0xff / alpha;
}
 
-   if (apply_gamma) {
+   if (*apply_gamma) {
rgb[0] = crtc->gamma_blue[rgb[0]] >> 8;
rgb[1] = crtc->gamma_green[rgb[1]] >> 8;
rgb[2] = crtc->gamma_red[rgb[2]] >> 8;
@@ -1128,11 +1138,10 @@ retry_transform:
  cursor_h,
  dstx, 
dsty);
argb = image[srcoffset];
-   if (!drmmode_cursor_pixel(crtc, , 
premultiplied,
- apply_gamma)) {
-   premultiplied = FALSE;
+   if (!drmmode_cursor_pixel(crtc, , 
,
+ _gamma))
goto retry_transform;
-   }
+
ptr[dsty * info->cursor_w + dstx] = 
cpu_to_le32(argb);
}
}
@@ -1145,11 +1154,10 @@ retry_transform:
 retry:
for (i = 0; i < cursor_size; i++) {
argb = image[i];
-   if (!drmmode_cursor_pixel(crtc, , premultiplied,
- apply_gamma)) {
-   premultiplied = FALSE;
+   if (!drmmode_cursor_pixel(crtc, , ,
+ _gamma))
goto retry;
-   }
+
ptr[i] = cpu_to_le32(argb);
}
}
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 04/13] Use drm_abort_one in drm_queue_handler

[Michel Dänzer]

From: Michel Dänzer 

At this point, we've already established that e->handler is NULL, no
need to check again in drm_queue_handle_one. This also makes it clearer
what's happening.

(Ported from amdgpu commit eda571222f5a6be47f8897e82d85199bb9d95251)

Signed-off-by: Michel Dänzer 
---
 src/radeon_drm_queue.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/radeon_drm_queue.c b/src/radeon_drm_queue.c
index ea78e8e2b..ebc6a5b64 100644
--- a/src/radeon_drm_queue.c
+++ b/src/radeon_drm_queue.c
@@ -72,6 +72,19 @@ radeon_drm_queue_handle_one(struct radeon_drm_queue_entry *e)
 free(e);
 }
 
+/*
+ * Abort one queued DRM entry, removing it
+ * from the list, calling the abort function and
+ * freeing the memory
+ */
+static void
+radeon_drm_abort_one(struct radeon_drm_queue_entry *e)
+{
+xorg_list_del(>list);
+e->abort(e->crtc, e->data);
+free(e);
+}
+
 static void
 radeon_drm_queue_handler(struct xorg_list *signalled, unsigned int frame,
 unsigned int sec, unsigned int usec, void *user_ptr)
@@ -82,7 +95,7 @@ radeon_drm_queue_handler(struct xorg_list *signalled, 
unsigned int frame,
 xorg_list_for_each_entry_safe(e, tmp, _drm_queue, list) {
if (e->seq == seq) {
if (!e->handler) {
-   radeon_drm_queue_handle_one(e);
+   radeon_drm_abort_one(e);
break;
}
 
@@ -173,19 +186,6 @@ radeon_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
 return e->seq;
 }
 
-/*
- * Abort one queued DRM entry, removing it
- * from the list, calling the abort function and
- * freeing the memory
- */
-static void
-radeon_drm_abort_one(struct radeon_drm_queue_entry *e)
-{
-xorg_list_del(>list);
-e->abort(e->crtc, e->data);
-free(e);
-}
-
 /*
  * Abort drm queue entries for a client
  *
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 00/13] Porting outstanding changes from amdgpu

[Michel Dänzer]

From: Michel Dänzer 

These are the outstanding applicable changes ported from
xf86-video-amdgpu.

Michel Dänzer (13):
  Detect and fix up non-premultiplied cursor data
  Skip gamma correction of cursor data if premultiplied R/G/B > alpha
  glamor: Can work at depth >= 15 with current xserver Git master
  Use drm_abort_one in drm_queue_handler
  Explicitly keep track of whether a DRM event is for a flip or not
  Move deferred vblank events to separate drm_vblank_deferred list
  Perform scanout buffer update immediately if drmmode_wait_vblank fails
  Cancel pending scanout update in drmmode_crtc_scanout_update
  Automatically try re-enabling TearFree after a flip failed
  Drop RADEONInfoRec::cursor_bo array
  Use drmIoctl in drmmode_show_cursor
  Update cursor position in drmmode_show_cursor if hotspot changed
  Use two HW cursor buffers per CRTC

 src/drmmode_display.c  | 225 +++--
 src/drmmode_display.h  |  18 +++-
 src/radeon.h   |   1 -
 src/radeon_bo_helper.c |   2 +
 src/radeon_dri2.c  |   4 +-
 src/radeon_drm_queue.c | 110 +++-
 src/radeon_drm_queue.h |   3 +-
 src/radeon_glamor.c|   9 +-
 src/radeon_kms.c   | 147 +++
 src/radeon_present.c   |   3 +-
 10 files changed, 353 insertions(+), 169 deletions(-)

-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 03/13] glamor: Can work at depth >= 15 with current xserver Git master

[Michel Dänzer]

From: Michel Dänzer 

(Ported from amdgpu commit 0734cdf544ffd3f2ac8749ad0e4bf43f8a5cea50)

Signed-off-by: Michel Dänzer 
---
 src/radeon_bo_helper.c | 2 ++
 src/radeon_glamor.c| 9 +++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/radeon_bo_helper.c b/src/radeon_bo_helper.c
index da5a484f2..8b3e57974 100644
--- a/src/radeon_bo_helper.c
+++ b/src/radeon_bo_helper.c
@@ -39,6 +39,8 @@ radeon_get_gbm_format(int depth, int bitsPerPixel)
 case 8:
return GBM_FORMAT_R8;
 #endif
+case 15:
+   return GBM_FORMAT_ARGB1555;
 case 16:
return GBM_FORMAT_RGB565;
 case 32:
diff --git a/src/radeon_glamor.c b/src/radeon_glamor.c
index bffc89ec6..68873cc41 100644
--- a/src/radeon_glamor.c
+++ b/src/radeon_glamor.c
@@ -109,9 +109,14 @@ radeon_glamor_pre_init(ScrnInfoPtr scrn)
   "glamor may not work (well) with GPUs < RV515.\n");
}
 
+#if XORG_VERSION_CURRENT < XORG_VERSION_NUMERIC(1,20,99,0,0)
if (scrn->depth < 24) {
-   xf86DrvMsg(scrn->scrnIndex, s ? X_ERROR : X_WARNING,
-  "glamor requires depth >= 24, disabling.\n");
+#else
+   if (scrn->depth < 15) {
+#endif
+   xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+  "Depth %d not supported with glamor, disabling\n",
+  scrn->depth);
return FALSE;
}
 
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 13/13] Use two HW cursor buffers per CRTC

[Michel Dänzer]

From: Michel Dänzer 

Switch to the other buffer when xf86_config->cursor changes. Avoids
these issues possible when re-using the same buffer:

* The HW may intermittently display a mix of the old and new cursor
  images.
* If the hotspot changes, the HW may intermittently display the new
  cursor image at the location corresponding to the old image's hotspot.

Bugzilla: https://bugs.freedesktop.org/108832
(Ported from amdgpu commit 0d60233d26ec70d4e1faa343b438e33829c6d5e4)

Signed-off-by: Michel Dänzer 
---
 src/drmmode_display.c | 18 +++---
 src/drmmode_display.h |  5 -
 src/radeon_kms.c  | 34 ++
 3 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index bec309e5e..d433e0611 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1122,13 +1122,18 @@ drmmode_load_cursor_argb (xf86CrtcPtr crtc, CARD32 
*image)
ScrnInfoPtr pScrn = crtc->scrn;
RADEONInfoPtr info = RADEONPTR(pScrn);
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+   unsigned id = drmmode_crtc->cursor_id;
Bool premultiplied = TRUE;
Bool apply_gamma = TRUE;
uint32_t argb;
uint32_t *ptr;
 
+   if (drmmode_crtc->cursor &&
+   XF86_CRTC_CONFIG_PTR(pScrn)->cursor != drmmode_crtc->cursor)
+   id ^= 1;
+
/* cursor should be mapped already */
-   ptr = (uint32_t *)(drmmode_crtc->cursor_bo->ptr);
+   ptr = (uint32_t *)(drmmode_crtc->cursor_bo[id]->ptr);
 
if (crtc->scrn->depth != 24 && crtc->scrn->depth != 32)
apply_gamma = FALSE;
@@ -1170,6 +1175,11 @@ retry:
ptr[i] = cpu_to_le32(argb);
}
}
+
+   if (id != drmmode_crtc->cursor_id) {
+   drmmode_crtc->cursor_id = id;
+   crtc->funcs->show_cursor(crtc);
+   }
 }
 
 #if XORG_VERSION_CURRENT >= XORG_VERSION_NUMERIC(1,15,99,903,0)
@@ -1195,7 +1205,7 @@ drmmode_hide_cursor (xf86CrtcPtr crtc)
 
drmModeSetCursor(pRADEONEnt->fd, drmmode_crtc->mode_crtc->crtc_id, 0,
 info->cursor_w, info->cursor_h);
-
+   drmmode_crtc->cursor = NULL;
 }
 
 static void
@@ -1212,9 +1222,11 @@ drmmode_show_cursor (xf86CrtcPtr crtc)
static Bool use_set_cursor2 = TRUE;
struct drm_mode_cursor2 arg;
 
+   drmmode_crtc->cursor = xf86_config->cursor;
+
memset(, 0, sizeof(arg));
 
-   arg.handle = drmmode_crtc->cursor_bo->handle;
+   arg.handle = drmmode_crtc->cursor_bo[drmmode_crtc->cursor_id]->handle;
arg.flags = DRM_MODE_CURSOR_BO;
arg.crtc_id = drmmode_crtc->mode_crtc->crtc_id;
arg.width = info->cursor_w;
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 49893ab04..2c2c3d57f 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -88,11 +88,14 @@ typedef struct {
 drmModeCrtcPtr mode_crtc;
 int hw_id;
 
+CursorPtr cursor;
 int cursor_x;
 int cursor_y;
 int cursor_xhot;
 int cursor_yhot;
-struct radeon_bo *cursor_bo;
+unsigned cursor_id;
+struct radeon_bo *cursor_bo[2];
+
 struct drmmode_scanout rotate;
 struct drmmode_scanout scanout[2];
 DamagePtr scanout_damage;
diff --git a/src/radeon_kms.c b/src/radeon_kms.c
index 27a02109a..bb6885fb9 100644
--- a/src/radeon_kms.c
+++ b/src/radeon_kms.c
@@ -2755,27 +2755,29 @@ static Bool radeon_setup_kernel_mem(ScreenPtr pScreen)
 
 {
int cursor_size;
-   int c;
+   int c, i;
 
cursor_size = info->cursor_w * info->cursor_h * 4;
cursor_size = RADEON_ALIGN(cursor_size, RADEON_GPU_PAGE_SIZE);
for (c = 0; c < xf86_config->num_crtc; c++) {
drmmode_crtc_private_ptr drmmode_crtc = 
xf86_config->crtc[c]->driver_private;
 
-   if (!drmmode_crtc->cursor_bo) {
-   drmmode_crtc->cursor_bo = radeon_bo_open(info->bufmgr, 0,
-cursor_size, 0,
-
RADEON_GEM_DOMAIN_VRAM, 0);
-   if (!(drmmode_crtc->cursor_bo)) {
-ErrorF("Failed to allocate cursor buffer memory\n");
-return FALSE;
-}
-
-   if (radeon_bo_map(drmmode_crtc->cursor_bo, 1)) {
-ErrorF("Failed to map cursor buffer memory\n");
-}
-}
-}
+   for (i = 0; i < 2; i++) {
+   if (!drmmode_crtc->cursor_bo[i]) {
+   drmmode_crtc->cursor_bo[i] =
+   radeon_bo_open(info->bufmgr, 0, cursor_size, 0,
+  RADEON_GEM_DOMAIN_VRAM, 0);
+
+   if (!(drmmode_crtc->cursor_bo[i])) {
+   ErrorF("Failed to allocate cursor buffer memory\n");
+   return FALSE;
+   }
+
+   if 

Re: [PATCH] drm/amdgpu: dma_fence finished signaled by unexpected callback

[Grodzovsky, Andrey]

I believe this issue would be resolved by my pending  in review patch 
set, specifically 'drm/sched: Refactor ring mirror list handling.' since 
already on the first TO handler it will go over all the rings including 
the second timed out ring and will remove all call backs including the 
bad job cb. In case by this time this bad job will signal for some 
reason it will be removed from the mirror list already during 
drm_sched_process_job (take a look at 'drm/sched: Rework HW fence 
processing.') and hence will not be rerun in drm_sched_job_recovery 
(drm_sched_resubmit_jobs under the new name).

Andrey


On 12/21/2018 03:25 AM, wentalou wrote:
> When 2 rings met timeout at same time, triggered job_timedout separately.
> Each job_timedout called gpu_recover, but one of gpu_recover locked by 
> another's mutex_lock.
> Bad jod’s callback should be removed by dma_fence_remove_callback but locked 
> inside mutex_lock.
> So dma_fence_remove_callback could not be called immediately.
> Then callback drm_sched_process_job triggered unexpectedly, and signaled 
> DMA_FENCE_FLAG_SIGNALED_BIT.
> After another's mutex_unlock, signaled bad job went through job_run inside 
> drm_sched_job_recovery.
> job_run would have WARN_ON and Call-Trace, when calling 
> kcl_dma_fence_set_error for signaled bad job.
>
> Change-Id: I6366add13f020476882b2b8b03330a58d072dd1a
> Signed-off-by: Wentao Lou 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 -
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 0a17fb1..fc1d3a0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -225,8 +225,11 @@ static struct dma_fence *amdgpu_job_run(struct 
> drm_sched_job *sched_job)
>   
>   trace_amdgpu_sched_run_job(job);
>   
> - if (job->vram_lost_counter != 
> atomic_read(>adev->vram_lost_counter))
> + if (job->vram_lost_counter != 
> atomic_read(>adev->vram_lost_counter)) {
> + /* flags might be signaled by unexpected callback, clear it */
> + test_and_clear_bit(DMA_FENCE_FLAG_SIGNALED_BIT, 
> >flags);
>   dma_fence_set_error(finished, -ECANCELED);/* skip IB as well if 
> VRAM lost */
> + }
>   
>   if (finished->error < 0) {
>   DRM_INFO("Skip scheduling IBs!\n");

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add new VegaM pci id

[Liu, Leo]

Reviewed-by: Leo Liu 

On 12/21/18 11:36 AM, Alex Deucher wrote:
> ping?
>
> On Thu, Dec 20, 2018 at 10:10 AM Alex Deucher  wrote:
>> Add a new pci id.
>>
>> Signed-off-by: Alex Deucher 
>> Cc: sta...@vger.kernel.org
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 9c77eaa45982..c806f984bcc5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -865,6 +865,7 @@ static const struct pci_device_id pciidlist[] = {
>>  /* VEGAM */
>>  {0x1002, 0x694C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_VEGAM},
>>  {0x1002, 0x694E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_VEGAM},
>> +   {0x1002, 0x694F, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_VEGAM},
>>  /* Vega 10 */
>>  {0x1002, 0x6860, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_VEGA10},
>>  {0x1002, 0x6861, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_VEGA10},
>> --
>> 2.13.6
>>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: return false in drm_arch_can_wc_memory() for ARM

[Eric Anholt]

Alex Deucher  writes:

> On Fri, Dec 21, 2018 at 9:16 AM Liviu Dudau  wrote:
>>
>> On Thu, Dec 20, 2018 at 04:36:19PM +0100, Daniel Vetter wrote:
>> > On Thu, Dec 20, 2018 at 09:56:57AM -0500, Alex Deucher wrote:
>> > > I'm not familiar enough with ARM to know if write combining
>> > > is actually an architectural limitation or if it's an issue
>> > > with the PCIe IPs used on various platforms, but so far
>> > > everyone that has tried to run radeon hardware on
>> > > ARM has had to disable it.  So let's just make it official.
>> >
>> > wc on arm is Really Complicated (tm) afaiui. There's issues with aliasing
>> > mappings and stuff, so you need to allocate your wc memory from special
>> > pools. So probably best to just disable it until we figure this out.
>>
>> I believe both of you are conflating different issues under the wrong
>> name. Write combining happens all the time with Arm, the ARMv8
>> architecture is a weakly-ordered model of memory so hardware is allowed
>> to re-order or combine memory access as they seem fit.
>>
>> A while ago I did run an AMD GPU card on my Juno dev board and it worked
>> (for a very limited definition of worked, I've only validated the fact
>> that I could get an fbcon and could run un-accelerated X11). So I would
>> be interested if Alex could share some of the scenarios where people are
>> seeing failures.
>
> Here's an example:
> https://bugs.freedesktop.org/show_bug.cgi?id=108625
> But there are probably 5 or 6 other cases where people have emailed me
> or our team directly with issues on ARM resolved by disabling WC.
> Generally the driver seems to load ok, but then hangs as soon as you
> try and use acceleration from userspace or we end up with page
> flipping timeouts.  Not really sure what the issue is.  Michel
> suggested maybe ARM has a cacheable kernel mapping of all "normal"
> system memory, and having
> both that mapping and another non-cacheable mapping of the same page
> can result in bad behaviour.
>
>>
>> As for aliasing, yeah, having multiple aliases to the same piece of
>> memory is a bad thing. The problem arises when devices on the PCI bus
>> have memory allocated as device memory (which on Arm is non-cacheable
>> and non-reorderable), but the PCI bus effectively acts as a write-combiner
>> which changes the order of transactions. Therefore, for devices that
>> have local memory associated with them (i.e. more than just register
>> accesses) one should allocate memory in the first place that is
>> Device-GRE (gathering, reordering and early-access). Otherwise, problems
>> will surface that are not visible on x86 as that is a strongly ordered
>> architecture.
>
> PCI framebuffer BARs are mapped on the CPU with WC.  We also use
> uncached WC mappings for system memory in cases where it's not likely
> we will be doing any CPU reads.  When accessing system memory, the GPU
> can either do a CPU cache snooped transaction or a non-snooped
> transaction.  The non-snooped transaction has lower latency and better
> throughput since it doesn't have to snoop the CPU cache.
>
>>
>> >
>> > > Signed-off-by: Alex Deucher 
>> >
>> > Reviewed-by: Daniel Vetter 
>>
>> Given that this API is only used by AMD I'm OK for now with the change,
>> but I think in general it is misleading and we should work towards
>> fixing radeon and amd drivers.
>
> Alternatively, we could just disable WC in the amdgpu driver on ARM.
> I'm not sure to what extent other drivers are using WC in general or
> have been tested on ARM.

FWIW, I use WC mappings of BOs on V3D (shmem) and VC4 (cma).  V3D is
totally stable.  VC4 I've heard reports of stability issues long-term
but I don't think it's related.  I don't do any cached mappings of my
BOs, though.


signature.asc
Description: PGP signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/powerplay: Remove duplicate header

[Brajeswar Ghosh]

Remove hwmgr_ppt.h which is included more than once

Signed-off-by: Brajeswar Ghosh 
---
 drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h 
b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
index e5a60aa44b5d..07d180ce4d18 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
@@ -28,7 +28,6 @@
 #include "hardwaremanager.h"
 #include "hwmgr_ppt.h"
 #include "ppatomctrl.h"
-#include "hwmgr_ppt.h"
 #include "power_state.h"
 #include "smu_helper.h"
 
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/powerplay: Remove duplicate header

[Souptick Joarder]

On Fri, Dec 21, 2018 at 2:49 PM Brajeswar Ghosh
 wrote:
>
> Remove hwmgr_ppt.h which is included more than once
>
> Signed-off-by: Brajeswar Ghosh 
> ---
Acked-by: Souptick Joarder 

>  drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h 
> b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> index e5a60aa44b5d..07d180ce4d18 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> @@ -28,7 +28,6 @@
>  #include "hardwaremanager.h"
>  #include "hwmgr_ppt.h"
>  #include "ppatomctrl.h"
> -#include "hwmgr_ppt.h"
>  #include "power_state.h"
>  #include "smu_helper.h"
>
> --
> 2.17.1
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/powerplay/polaris10_smumgr: Remove duplicate

[Brajeswar Ghosh]

Remove ppatomctrl.h which is included more than once

Signed-off-by: Brajeswar Ghosh 
---
 drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
index 872d3824337b..2b2c26616902 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
@@ -44,7 +44,6 @@
 
 #include "smu7_hwmgr.h"
 #include "hardwaremanager.h"
-#include "ppatomctrl.h"
 #include "atombios.h"
 #include "pppcielanes.h"
 
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: return false in drm_arch_can_wc_memory() for ARM

[Alex Deucher]

On Fri, Dec 21, 2018 at 9:16 AM Liviu Dudau  wrote:
>
> On Thu, Dec 20, 2018 at 04:36:19PM +0100, Daniel Vetter wrote:
> > On Thu, Dec 20, 2018 at 09:56:57AM -0500, Alex Deucher wrote:
> > > I'm not familiar enough with ARM to know if write combining
> > > is actually an architectural limitation or if it's an issue
> > > with the PCIe IPs used on various platforms, but so far
> > > everyone that has tried to run radeon hardware on
> > > ARM has had to disable it.  So let's just make it official.
> >
> > wc on arm is Really Complicated (tm) afaiui. There's issues with aliasing
> > mappings and stuff, so you need to allocate your wc memory from special
> > pools. So probably best to just disable it until we figure this out.
>
> I believe both of you are conflating different issues under the wrong
> name. Write combining happens all the time with Arm, the ARMv8
> architecture is a weakly-ordered model of memory so hardware is allowed
> to re-order or combine memory access as they seem fit.
>
> A while ago I did run an AMD GPU card on my Juno dev board and it worked
> (for a very limited definition of worked, I've only validated the fact
> that I could get an fbcon and could run un-accelerated X11). So I would
> be interested if Alex could share some of the scenarios where people are
> seeing failures.

Here's an example:
https://bugs.freedesktop.org/show_bug.cgi?id=108625
But there are probably 5 or 6 other cases where people have emailed me
or our team directly with issues on ARM resolved by disabling WC.
Generally the driver seems to load ok, but then hangs as soon as you
try and use acceleration from userspace or we end up with page
flipping timeouts.  Not really sure what the issue is.  Michel
suggested maybe ARM has a cacheable kernel mapping of all "normal"
system memory, and having
both that mapping and another non-cacheable mapping of the same page
can result in bad behaviour.

>
> As for aliasing, yeah, having multiple aliases to the same piece of
> memory is a bad thing. The problem arises when devices on the PCI bus
> have memory allocated as device memory (which on Arm is non-cacheable
> and non-reorderable), but the PCI bus effectively acts as a write-combiner
> which changes the order of transactions. Therefore, for devices that
> have local memory associated with them (i.e. more than just register
> accesses) one should allocate memory in the first place that is
> Device-GRE (gathering, reordering and early-access). Otherwise, problems
> will surface that are not visible on x86 as that is a strongly ordered
> architecture.

PCI framebuffer BARs are mapped on the CPU with WC.  We also use
uncached WC mappings for system memory in cases where it's not likely
we will be doing any CPU reads.  When accessing system memory, the GPU
can either do a CPU cache snooped transaction or a non-snooped
transaction.  The non-snooped transaction has lower latency and better
throughput since it doesn't have to snoop the CPU cache.

>
> >
> > > Signed-off-by: Alex Deucher 
> >
> > Reviewed-by: Daniel Vetter 
>
> Given that this API is only used by AMD I'm OK for now with the change,
> but I think in general it is misleading and we should work towards
> fixing radeon and amd drivers.

Alternatively, we could just disable WC in the amdgpu driver on ARM.
I'm not sure to what extent other drivers are using WC in general or
have been tested on ARM.

Alex

>
> Best regards,
> Liviu
>
> >
> > > ---
> > >  include/drm/drm_cache.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
> > > index bfe1639df02d..691b4c4b0587 100644
> > > --- a/include/drm/drm_cache.h
> > > +++ b/include/drm/drm_cache.h
> > > @@ -47,6 +47,8 @@ static inline bool drm_arch_can_wc_memory(void)
> > > return false;
> > >  #elif defined(CONFIG_MIPS) && defined(CONFIG_CPU_LOONGSON3)
> > > return false;
> > > +#elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > > +   return false;
> > >  #else
> > > return true;
> > >  #endif
> > > --
> > > 2.13.6
> > >
> > > ___
> > > dri-devel mailing list
> > > dri-de...@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> 
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---
> ¯\_(ツ)_/¯
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: return false in drm_arch_can_wc_memory() for ARM

[Liviu Dudau]

On Thu, Dec 20, 2018 at 04:36:19PM +0100, Daniel Vetter wrote:
> On Thu, Dec 20, 2018 at 09:56:57AM -0500, Alex Deucher wrote:
> > I'm not familiar enough with ARM to know if write combining
> > is actually an architectural limitation or if it's an issue
> > with the PCIe IPs used on various platforms, but so far
> > everyone that has tried to run radeon hardware on
> > ARM has had to disable it.  So let's just make it official.
> 
> wc on arm is Really Complicated (tm) afaiui. There's issues with aliasing
> mappings and stuff, so you need to allocate your wc memory from special
> pools. So probably best to just disable it until we figure this out.

I believe both of you are conflating different issues under the wrong
name. Write combining happens all the time with Arm, the ARMv8
architecture is a weakly-ordered model of memory so hardware is allowed
to re-order or combine memory access as they seem fit.

A while ago I did run an AMD GPU card on my Juno dev board and it worked
(for a very limited definition of worked, I've only validated the fact
that I could get an fbcon and could run un-accelerated X11). So I would
be interested if Alex could share some of the scenarios where people are
seeing failures.

As for aliasing, yeah, having multiple aliases to the same piece of
memory is a bad thing. The problem arises when devices on the PCI bus
have memory allocated as device memory (which on Arm is non-cacheable
and non-reorderable), but the PCI bus effectively acts as a write-combiner
which changes the order of transactions. Therefore, for devices that
have local memory associated with them (i.e. more than just register
accesses) one should allocate memory in the first place that is
Device-GRE (gathering, reordering and early-access). Otherwise, problems
will surface that are not visible on x86 as that is a strongly ordered
architecture.

>  
> > Signed-off-by: Alex Deucher 
> 
> Reviewed-by: Daniel Vetter 

Given that this API is only used by AMD I'm OK for now with the change,
but I think in general it is misleading and we should work towards
fixing radeon and amd drivers.

Best regards,
Liviu

> 
> > ---
> >  include/drm/drm_cache.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
> > index bfe1639df02d..691b4c4b0587 100644
> > --- a/include/drm/drm_cache.h
> > +++ b/include/drm/drm_cache.h
> > @@ -47,6 +47,8 @@ static inline bool drm_arch_can_wc_memory(void)
> > return false;
> >  #elif defined(CONFIG_MIPS) && defined(CONFIG_CPU_LOONGSON3)
> > return false;
> > +#elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +   return false;
> >  #else
> > return true;
> >  #endif
> > -- 
> > 2.13.6
> > 
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/gmc: fix compiler errors [-Werror,-Wmissing-braces]

[Koenig, Christian]

Am 21.12.18 um 12:18 schrieb Michel Dänzer:
> On 2018-12-20 3:01 p.m., S, Shirish wrote:
>> Initializing structures with { } is known to be problematic since
>> it doesn't necessararily initializes all bytes, in case of padding,
>> causing random failures when structures are memcmp().
>>
>> This patch fixes the structure initialisation compiler error by memsetting
>> the entire structure elements instead of only the first one.
>>
>> Signed-off-by: Shirish S 
>>
>> [...]
>>   
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index bacdaef..2bfddce 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -320,8 +320,9 @@ static int gmc_v9_0_process_interrupt(struct 
>> amdgpu_device *adev,
>>  }
>>   
>>  if (printk_ratelimit()) {
>> -struct amdgpu_task_info task_info = { 0 };
>> +struct amdgpu_task_info task_info;
>>   
>> +memset(_info, 0, sizeof(amdgpu_task_info));
> This doesn't compile:
>
> drivers/gpu/drm//amd/amdgpu/gmc_v9_0.c: In function 
> ‘gmc_v9_0_process_interrupt’:
> drivers/gpu/drm//amd/amdgpu/gmc_v9_0.c:325:32: error: ‘amdgpu_task_info’ 
> undeclared (first use in this function)
> memset(_info, 0, sizeof(amdgpu_task_info));
>  ^~~~
>
> (Needs to be "sizeof(struct amdgpu_task_info)")

The coding style guide lines actually document that you should avoid 
types with sizeof and instead use the variables directly.

E.g. use "memset(_info, 0, sizeof(task_info));"

> I've reverted this on the internal amd-staging-drm-next branch.
>
> Patches must be tested appropriately (against the target branch),
> ideally before submitting them for review. Reviewers are not expected to
> look for things which trivially break the build.

Completely agree.

I mean I break things more often than not myself because of our limited 
QA resources, but at least such trivial things should not happen any more.

Regards,
Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/gmc: fix compiler errors [-Werror,-Wmissing-braces]

[Michel Dänzer]

On 2018-12-20 3:01 p.m., S, Shirish wrote:
> Initializing structures with { } is known to be problematic since
> it doesn't necessararily initializes all bytes, in case of padding,
> causing random failures when structures are memcmp().
> 
> This patch fixes the structure initialisation compiler error by memsetting
> the entire structure elements instead of only the first one.
> 
> Signed-off-by: Shirish S 
> 
> [...]
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index bacdaef..2bfddce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -320,8 +320,9 @@ static int gmc_v9_0_process_interrupt(struct 
> amdgpu_device *adev,
>   }
>  
>   if (printk_ratelimit()) {
> - struct amdgpu_task_info task_info = { 0 };
> + struct amdgpu_task_info task_info;
>  
> + memset(_info, 0, sizeof(amdgpu_task_info));

This doesn't compile:

drivers/gpu/drm//amd/amdgpu/gmc_v9_0.c: In function 
‘gmc_v9_0_process_interrupt’:
drivers/gpu/drm//amd/amdgpu/gmc_v9_0.c:325:32: error: ‘amdgpu_task_info’ 
undeclared (first use in this function)
   memset(_info, 0, sizeof(amdgpu_task_info));
^~~~

(Needs to be "sizeof(struct amdgpu_task_info)")


I've reverted this on the internal amd-staging-drm-next branch.

Patches must be tested appropriately (against the target branch),
ideally before submitting them for review. Reviewers are not expected to
look for things which trivially break the build.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Michel Dänzer]

On 2018-12-21 11:15 a.m., Deng, Emily wrote:
>> -Original Message-
>> From: Michel Dänzer 
>> Sent: Friday, December 21, 2018 6:08 PM
>> To: Deng, Emily 
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>
>> On 2018-12-21 10:55 a.m., Deng, Emily wrote:
 -Original Message-
 From: amd-gfx  On Behalf Of
 Deng, Emily
 Sent: Friday, December 21, 2018 5:41 PM
 To: Michel Dänzer 
 Cc: amd-gfx@lists.freedesktop.org
 Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

> -Original Message-
> From: Michel Dänzer 
> Sent: Friday, December 21, 2018 5:37 PM
> To: Deng, Emily 
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
> On 2018-12-21 10:32 a.m., Deng, Emily wrote:
>>> -Original Message-
>>> From: Michel Dänzer 
>>> Sent: Friday, December 21, 2018 5:28 PM
>>> To: Deng, Emily 
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's
>>> bo
>>>
>>> On 2018-12-21 10:17 a.m., Deng, Emily wrote:
> From: amd-gfx  On Behalf
> Of Deng, Emily
>> From: Michel Dänzer  On 2018-12-21 9:45
>> a.m., Deng, Emily wrote:
 From: Michel Dänzer  On 2018-12-21 8:26
 a.m., Emily Deng wrote:
> When the bo is used to set mode, the bo need to be pinned.

 On second thought, why does the BO need to be pinned? When
 using the display hardware, the BO needs to be pinned to
 prevent it from being moved while the hardware is scanning
 out from it, but that shouldn't be
>> necessary here.
>>> The pin here is used for scan out the buffer by remote display app.
>>
>> I still don't understand why pinning is needed. What mechanism
>> does the remote display app use to access the BO contents?
> Sorry, I am not familiar with the remote display app. Maybe it
> will use drm ioctl function to get the current crtc's fb's
> information, and get the content in the fb's buffer object by
> mmap or translate the bo to an OpenGL texture for next
> rendering. Maybe don't need to pin the bo here, as the use has
> no different with other
 normal bos.
> So please ignore the patch, and will send another patch to
> remove the unpin
>>> the fb's bo code.
 It seems to be hard to remove all the pin for virtual_dce, as it
 uses some
>>> common code in amdgpu_display.c.
>>>
>>> Because of amdgpu_display_unpin_work_func? That might be as simple
>>> as replacing
>>>
>>> schedule_work(>unpin_work);
>>>
>>> with
>>>
>>> kfree(works->shared);
>>> kfree(works);
>>>
>>> in dce_virtual_pageflip.
>> But the amdgpu_display_crtc_page_flip_target will pin the new_bo,
>> then we don't need to unpin it?
>
> Ah, right, but then dce_virtual_pageflip could just unpin it? Not
> ideal, but better than leaving it pinned unnecessarily.
 Yes, it is not a good idea to leave it pinned. Then will need lots of
 "if (amdgpu_virtual_display)", don't know whether it could be accept?
>>
>> Should rather be if (adev->enable_virtual_display). If you want to never 
>> pin, it's
>> probably worth giving this a shot and seeing how bad it gets.
>> BTW, amdgpu_ttm_alloc_gart can probably also be skipped for virtual display,
>> maybe more.
> Ok, then I will try, but the code may be ugly.
>
> [...]
>
>> But none of that is necessary if dce_virtual_pageflip simply unpins the BO 
>> and
>> skips unpin_work.
> Yes, but it will still have the issue that it won't unpin the bo which is 
> pinned by amdgpu_display_crtc_page_flip_target.

I mean dce_virtual_pageflip unpinning precisely the BO pinned by
amdgpu_display_crtc_page_flip_target.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Deng, Emily]

>-Original Message-
>From: Michel Dänzer 
>Sent: Friday, December 21, 2018 6:08 PM
>To: Deng, Emily 
>Cc: amd-gfx@lists.freedesktop.org
>Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
>On 2018-12-21 10:55 a.m., Deng, Emily wrote:
>>> -Original Message-
>>> From: amd-gfx  On Behalf Of
>>> Deng, Emily
>>> Sent: Friday, December 21, 2018 5:41 PM
>>> To: Michel Dänzer 
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>>
 -Original Message-
 From: Michel Dänzer 
 Sent: Friday, December 21, 2018 5:37 PM
 To: Deng, Emily 
 Cc: amd-gfx@lists.freedesktop.org
 Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

 On 2018-12-21 10:32 a.m., Deng, Emily wrote:
>> -Original Message-
>> From: Michel Dänzer 
>> Sent: Friday, December 21, 2018 5:28 PM
>> To: Deng, Emily 
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's
>> bo
>>
>> On 2018-12-21 10:17 a.m., Deng, Emily wrote:
 From: amd-gfx  On Behalf
 Of Deng, Emily
> From: Michel Dänzer  On 2018-12-21 9:45
> a.m., Deng, Emily wrote:
>>> From: Michel Dänzer  On 2018-12-21 8:26
>>> a.m., Emily Deng wrote:
 When the bo is used to set mode, the bo need to be pinned.
>>>
>>> On second thought, why does the BO need to be pinned? When
>>> using the display hardware, the BO needs to be pinned to
>>> prevent it from being moved while the hardware is scanning
>>> out from it, but that shouldn't be
> necessary here.
>> The pin here is used for scan out the buffer by remote display app.
>
> I still don't understand why pinning is needed. What mechanism
> does the remote display app use to access the BO contents?
 Sorry, I am not familiar with the remote display app. Maybe it
 will use drm ioctl function to get the current crtc's fb's
 information, and get the content in the fb's buffer object by
 mmap or translate the bo to an OpenGL texture for next
 rendering. Maybe don't need to pin the bo here, as the use has
 no different with other
>>> normal bos.
 So please ignore the patch, and will send another patch to
 remove the unpin
>> the fb's bo code.
>>> It seems to be hard to remove all the pin for virtual_dce, as it
>>> uses some
>> common code in amdgpu_display.c.
>>
>> Because of amdgpu_display_unpin_work_func? That might be as simple
>> as replacing
>>
>>  schedule_work(>unpin_work);
>>
>> with
>>
>>  kfree(works->shared);
>>  kfree(works);
>>
>> in dce_virtual_pageflip.
> But the amdgpu_display_crtc_page_flip_target will pin the new_bo,
> then we don't need to unpin it?

 Ah, right, but then dce_virtual_pageflip could just unpin it? Not
 ideal, but better than leaving it pinned unnecessarily.
>>> Yes, it is not a good idea to leave it pinned. Then will need lots of
>>> "if (amdgpu_virtual_display)", don't know whether it could be accept?
>
>Should rather be if (adev->enable_virtual_display). If you want to never pin, 
>it's
>probably worth giving this a shot and seeing how bad it gets.
>BTW, amdgpu_ttm_alloc_gart can probably also be skipped for virtual display,
>maybe more.
Ok, then I will try, but the code may be ugly.
>
>> Another method is let the logical stay no change, just use if
>> (!amdgpu_virtual_display) before WARN_ON_ONCE of amdgpu_bo_unpin to
>remove the virtual_dce's call trace.
>
>That would be ugly IMHO.
>
>
>But none of that is necessary if dce_virtual_pageflip simply unpins the BO and
>skips unpin_work.
Yes, but it will still have the issue that it won't unpin the bo which is 
pinned by amdgpu_display_crtc_page_flip_target.

Best wishes
Emily Deng


>
>
>--
>Earthling Michel Dänzer   |   http://www.amd.com
>Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Michel Dänzer]

On 2018-12-21 10:55 a.m., Deng, Emily wrote:
>> -Original Message-
>> From: amd-gfx  On Behalf Of Deng,
>> Emily
>> Sent: Friday, December 21, 2018 5:41 PM
>> To: Michel Dänzer 
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>
>>> -Original Message-
>>> From: Michel Dänzer 
>>> Sent: Friday, December 21, 2018 5:37 PM
>>> To: Deng, Emily 
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>>
>>> On 2018-12-21 10:32 a.m., Deng, Emily wrote:
> -Original Message-
> From: Michel Dänzer 
> Sent: Friday, December 21, 2018 5:28 PM
> To: Deng, Emily 
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
> On 2018-12-21 10:17 a.m., Deng, Emily wrote:
>>> From: amd-gfx  On Behalf Of
>>> Deng, Emily
 From: Michel Dänzer  On 2018-12-21 9:45 a.m.,
 Deng, Emily wrote:
>> From: Michel Dänzer  On 2018-12-21 8:26
>> a.m., Emily Deng wrote:
>>> When the bo is used to set mode, the bo need to be pinned.
>>
>> On second thought, why does the BO need to be pinned? When
>> using the display hardware, the BO needs to be pinned to
>> prevent it from being moved while the hardware is scanning out
>> from it, but that shouldn't be
 necessary here.
> The pin here is used for scan out the buffer by remote display app.

 I still don't understand why pinning is needed. What mechanism
 does the remote display app use to access the BO contents?
>>> Sorry, I am not familiar with the remote display app. Maybe it
>>> will use drm ioctl function to get the current crtc's fb's
>>> information, and get the content in the fb's buffer object by mmap
>>> or translate the bo to an OpenGL texture for next rendering. Maybe
>>> don't need to pin the bo here, as the use has no different with other
>> normal bos.
>>> So please ignore the patch, and will send another patch to remove
>>> the unpin
> the fb's bo code.
>> It seems to be hard to remove all the pin for virtual_dce, as it
>> uses some
> common code in amdgpu_display.c.
>
> Because of amdgpu_display_unpin_work_func? That might be as simple
> as replacing
>
>   schedule_work(>unpin_work);
>
> with
>
>   kfree(works->shared);
>   kfree(works);
>
> in dce_virtual_pageflip.
 But the amdgpu_display_crtc_page_flip_target will pin the new_bo,
 then we don't need to unpin it?
>>>
>>> Ah, right, but then dce_virtual_pageflip could just unpin it? Not
>>> ideal, but better than leaving it pinned unnecessarily.
>> Yes, it is not a good idea to leave it pinned. Then will need lots of "if
>> (amdgpu_virtual_display)", don't know whether it could be accept?

Should rather be if (adev->enable_virtual_display). If you want to never
pin, it's probably worth giving this a shot and seeing how bad it gets.
BTW, amdgpu_ttm_alloc_gart can probably also be skipped for virtual
display, maybe more.


> Another method is let the logical stay no change, just use if 
> (!amdgpu_virtual_display)
> before WARN_ON_ONCE of amdgpu_bo_unpin to remove the virtual_dce's call trace.

That would be ugly IMHO.


But none of that is necessary if dce_virtual_pageflip simply unpins the
BO and skips unpin_work.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Deng, Emily]

>-Original Message-
>From: amd-gfx  On Behalf Of Deng,
>Emily
>Sent: Friday, December 21, 2018 5:41 PM
>To: Michel Dänzer 
>Cc: amd-gfx@lists.freedesktop.org
>Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
>>-Original Message-
>>From: Michel Dänzer 
>>Sent: Friday, December 21, 2018 5:37 PM
>>To: Deng, Emily 
>>Cc: amd-gfx@lists.freedesktop.org
>>Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>
>>On 2018-12-21 10:32 a.m., Deng, Emily wrote:
 -Original Message-
 From: Michel Dänzer 
 Sent: Friday, December 21, 2018 5:28 PM
 To: Deng, Emily 
 Cc: amd-gfx@lists.freedesktop.org
 Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

 On 2018-12-21 10:17 a.m., Deng, Emily wrote:
>> From: amd-gfx  On Behalf Of
>> Deng, Emily
>>> From: Michel Dänzer  On 2018-12-21 9:45 a.m.,
>>> Deng, Emily wrote:
> From: Michel Dänzer  On 2018-12-21 8:26
> a.m., Emily Deng wrote:
>> When the bo is used to set mode, the bo need to be pinned.
>
> On second thought, why does the BO need to be pinned? When
> using the display hardware, the BO needs to be pinned to
> prevent it from being moved while the hardware is scanning out
> from it, but that shouldn't be
>>> necessary here.
 The pin here is used for scan out the buffer by remote display app.
>>>
>>> I still don't understand why pinning is needed. What mechanism
>>> does the remote display app use to access the BO contents?
>> Sorry, I am not familiar with the remote display app. Maybe it
>> will use drm ioctl function to get the current crtc's fb's
>> information, and get the content in the fb's buffer object by mmap
>> or translate the bo to an OpenGL texture for next rendering. Maybe
>> don't need to pin the bo here, as the use has no different with other
>normal bos.
>> So please ignore the patch, and will send another patch to remove
>> the unpin
 the fb's bo code.
> It seems to be hard to remove all the pin for virtual_dce, as it
> uses some
 common code in amdgpu_display.c.

 Because of amdgpu_display_unpin_work_func? That might be as simple
 as replacing

schedule_work(>unpin_work);

 with

kfree(works->shared);
kfree(works);

 in dce_virtual_pageflip.
>>> But the amdgpu_display_crtc_page_flip_target will pin the new_bo,
>>> then we don't need to unpin it?
>>
>>Ah, right, but then dce_virtual_pageflip could just unpin it? Not
>>ideal, but better than leaving it pinned unnecessarily.
>Yes, it is not a good idea to leave it pinned. Then will need lots of "if
>(amdgpu_virtual_display)", don't know whether it could be accept?
Another method is let the logical stay no change, just use if 
(!amdgpu_virtual_display)
before WARN_ON_ONCE of amdgpu_bo_unpin to remove the virtual_dce's call trace.
Which method do you think is better?
>>
>>
>>--
>>Earthling Michel Dänzer   |   http://www.amd.com
>>Libre software enthusiast | Mesa and X developer
>___
>amd-gfx mailing list
>amd-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Deng, Emily]

>-Original Message-
>From: Michel Dänzer 
>Sent: Friday, December 21, 2018 5:37 PM
>To: Deng, Emily 
>Cc: amd-gfx@lists.freedesktop.org
>Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
>On 2018-12-21 10:32 a.m., Deng, Emily wrote:
>>> -Original Message-
>>> From: Michel Dänzer 
>>> Sent: Friday, December 21, 2018 5:28 PM
>>> To: Deng, Emily 
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>>
>>> On 2018-12-21 10:17 a.m., Deng, Emily wrote:
> From: amd-gfx  On Behalf Of
> Deng, Emily
>> From: Michel Dänzer  On 2018-12-21 9:45 a.m.,
>> Deng, Emily wrote:
 From: Michel Dänzer  On 2018-12-21 8:26
 a.m., Emily Deng wrote:
> When the bo is used to set mode, the bo need to be pinned.

 On second thought, why does the BO need to be pinned? When using
 the display hardware, the BO needs to be pinned to prevent it
 from being moved while the hardware is scanning out from it, but
 that shouldn't be
>> necessary here.
>>> The pin here is used for scan out the buffer by remote display app.
>>
>> I still don't understand why pinning is needed. What mechanism
>> does the remote display app use to access the BO contents?
> Sorry, I am not familiar with the remote display app. Maybe it will
> use drm ioctl function to get the current crtc's fb's information,
> and get the content in the fb's buffer object by mmap or translate
> the bo to an OpenGL texture for next rendering. Maybe don't need to
> pin the bo here, as the use has no different with other normal bos.
> So please ignore the patch, and will send another patch to remove
> the unpin
>>> the fb's bo code.
 It seems to be hard to remove all the pin for virtual_dce, as it
 uses some
>>> common code in amdgpu_display.c.
>>>
>>> Because of amdgpu_display_unpin_work_func? That might be as simple as
>>> replacing
>>>
>>> schedule_work(>unpin_work);
>>>
>>> with
>>>
>>> kfree(works->shared);
>>> kfree(works);
>>>
>>> in dce_virtual_pageflip.
>> But the amdgpu_display_crtc_page_flip_target will pin the new_bo, then
>> we don't need to unpin it?
>
>Ah, right, but then dce_virtual_pageflip could just unpin it? Not ideal, but 
>better
>than leaving it pinned unnecessarily.
Yes, it is not a good idea to leave it pinned. Then will need lots of "if 
(amdgpu_virtual_display)", don't know
whether it could be accept?
>
>
>--
>Earthling Michel Dänzer   |   http://www.amd.com
>Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Michel Dänzer]

On 2018-12-21 10:32 a.m., Deng, Emily wrote:
>> -Original Message-
>> From: Michel Dänzer 
>> Sent: Friday, December 21, 2018 5:28 PM
>> To: Deng, Emily 
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>
>> On 2018-12-21 10:17 a.m., Deng, Emily wrote:
 From: amd-gfx  On Behalf Of
 Deng, Emily
> From: Michel Dänzer  On 2018-12-21 9:45 a.m.,
> Deng, Emily wrote:
>>> From: Michel Dänzer  On 2018-12-21 8:26 a.m.,
>>> Emily Deng wrote:
 When the bo is used to set mode, the bo need to be pinned.
>>>
>>> On second thought, why does the BO need to be pinned? When using
>>> the display hardware, the BO needs to be pinned to prevent it from
>>> being moved while the hardware is scanning out from it, but that
>>> shouldn't be
> necessary here.
>> The pin here is used for scan out the buffer by remote display app.
>
> I still don't understand why pinning is needed. What mechanism does
> the remote display app use to access the BO contents?
 Sorry, I am not familiar with the remote display app. Maybe it will
 use drm ioctl function to get the current crtc's fb's information,
 and get the content in the fb's buffer object by mmap or translate
 the bo to an OpenGL texture for next rendering. Maybe don't need to
 pin the bo here, as the use has no different with other normal bos.
 So please ignore the patch, and will send another patch to remove the unpin
>> the fb's bo code.
>>> It seems to be hard to remove all the pin for virtual_dce, as it uses some
>> common code in amdgpu_display.c.
>>
>> Because of amdgpu_display_unpin_work_func? That might be as simple as
>> replacing
>>
>>  schedule_work(>unpin_work);
>>
>> with
>>
>>  kfree(works->shared);
>>  kfree(works);
>>
>> in dce_virtual_pageflip.
> But the amdgpu_display_crtc_page_flip_target will pin the new_bo, then
> we don't need to unpin it?

Ah, right, but then dce_virtual_pageflip could just unpin it? Not ideal,
but better than leaving it pinned unnecessarily.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Deng, Emily]

>-Original Message-
>From: Michel Dänzer 
>Sent: Friday, December 21, 2018 5:28 PM
>To: Deng, Emily 
>Cc: amd-gfx@lists.freedesktop.org
>Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
>On 2018-12-21 10:17 a.m., Deng, Emily wrote:
>>> From: amd-gfx  On Behalf Of
>>> Deng, Emily
 From: Michel Dänzer  On 2018-12-21 9:45 a.m.,
 Deng, Emily wrote:
>> From: Michel Dänzer  On 2018-12-21 8:26 a.m.,
>> Emily Deng wrote:
>>> When the bo is used to set mode, the bo need to be pinned.
>>
>> On second thought, why does the BO need to be pinned? When using
>> the display hardware, the BO needs to be pinned to prevent it from
>> being moved while the hardware is scanning out from it, but that
>> shouldn't be
 necessary here.
> The pin here is used for scan out the buffer by remote display app.

 I still don't understand why pinning is needed. What mechanism does
 the remote display app use to access the BO contents?
>>> Sorry, I am not familiar with the remote display app. Maybe it will
>>> use drm ioctl function to get the current crtc's fb's information,
>>> and get the content in the fb's buffer object by mmap or translate
>>> the bo to an OpenGL texture for next rendering. Maybe don't need to
>>> pin the bo here, as the use has no different with other normal bos.
>>> So please ignore the patch, and will send another patch to remove the unpin
>the fb's bo code.
>> It seems to be hard to remove all the pin for virtual_dce, as it uses some
>common code in amdgpu_display.c.
>
>Because of amdgpu_display_unpin_work_func? That might be as simple as
>replacing
>
>   schedule_work(>unpin_work);
>
>with
>
>   kfree(works->shared);
>   kfree(works);
>
>in dce_virtual_pageflip.
But the amdgpu_display_crtc_page_flip_target will pin the new_bo, then we don't 
need to unpin it?

Best wishes
Emily Deng


>
>
>--
>Earthling Michel Dänzer   |   http://www.amd.com
>Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Michel Dänzer]

On 2018-12-21 10:17 a.m., Deng, Emily wrote:
>> From: amd-gfx  On Behalf Of Deng,
>> Emily
>>> From: Michel Dänzer 
>>> On 2018-12-21 9:45 a.m., Deng, Emily wrote:
> From: Michel Dänzer 
> On 2018-12-21 8:26 a.m., Emily Deng wrote:
>> When the bo is used to set mode, the bo need to be pinned.
>
> On second thought, why does the BO need to be pinned? When using the
> display hardware, the BO needs to be pinned to prevent it from being
> moved while the hardware is scanning out from it, but that shouldn't
> be
>>> necessary here.
 The pin here is used for scan out the buffer by remote display app.
>>>
>>> I still don't understand why pinning is needed. What mechanism does the
>>> remote display app use to access the BO contents?
>> Sorry, I am not familiar with the remote display app. Maybe it will use drm 
>> ioctl
>> function to get the current crtc's fb's information, and get the content in 
>> the fb's
>> buffer object by mmap or translate the bo to an OpenGL texture for next
>> rendering. Maybe don't need to pin the bo here, as the use has no different 
>> with
>> other normal bos. So please ignore the patch, and will send another patch to
>> remove the unpin the fb's bo code.
> It seems to be hard to remove all the pin for virtual_dce, as it uses some 
> common code in amdgpu_display.c.

Because of amdgpu_display_unpin_work_func? That might be as simple as
replacing

schedule_work(>unpin_work);

with

kfree(works->shared);
kfree(works);

in dce_virtual_pageflip.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Deng, Emily]

>-Original Message-
>From: amd-gfx  On Behalf Of Deng,
>Emily
>Sent: Friday, December 21, 2018 5:10 PM
>To: Michel Dänzer 
>Cc: amd-gfx@lists.freedesktop.org
>Subject: RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
>>-Original Message-
>>From: Michel Dänzer 
>>Sent: Friday, December 21, 2018 4:52 PM
>>To: Deng, Emily 
>>Cc: amd-gfx@lists.freedesktop.org
>>Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>
>>On 2018-12-21 9:45 a.m., Deng, Emily wrote:
 -Original Message-
 From: Michel Dänzer 
 Sent: Friday, December 21, 2018 4:38 PM
 To: Deng, Emily 
 Cc: amd-gfx@lists.freedesktop.org
 Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

 On 2018-12-21 8:26 a.m., Emily Deng wrote:
> When the bo is used to set mode, the bo need to be pinned.

 On second thought, why does the BO need to be pinned? When using the
 display hardware, the BO needs to be pinned to prevent it from being
 moved while the hardware is scanning out from it, but that shouldn't
 be
>>necessary here.
>>> The pin here is used for scan out the buffer by remote display app.
>>
>>I still don't understand why pinning is needed. What mechanism does the
>>remote display app use to access the BO contents?
>Sorry, I am not familiar with the remote display app. Maybe it will use drm 
>ioctl
>function to get the current crtc's fb's information, and get the content in 
>the fb's
>buffer object by mmap or translate the bo to an OpenGL texture for next
>rendering. Maybe don't need to pin the bo here, as the use has no different 
>with
>other normal bos. So please ignore the patch, and will send another patch to
>remove the unpin the fb's bo code.
It seems to be hard to remove all the pin for virtual_dce, as it uses some 
common code in amdgpu_display.c.
So for code consistency, maybe still need to add the pin here.

Best wishes
Emily Deng
>>
>>
>>--
>>Earthling Michel Dänzer   |   http://www.amd.com
>>Libre software enthusiast | Mesa and X developer
>___
>amd-gfx mailing list
>amd-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Deng, Emily]

>-Original Message-
>From: Michel Dänzer 
>Sent: Friday, December 21, 2018 4:52 PM
>To: Deng, Emily 
>Cc: amd-gfx@lists.freedesktop.org
>Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
>On 2018-12-21 9:45 a.m., Deng, Emily wrote:
>>> -Original Message-
>>> From: Michel Dänzer 
>>> Sent: Friday, December 21, 2018 4:38 PM
>>> To: Deng, Emily 
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>>
>>> On 2018-12-21 8:26 a.m., Emily Deng wrote:
 When the bo is used to set mode, the bo need to be pinned.
>>>
>>> On second thought, why does the BO need to be pinned? When using the
>>> display hardware, the BO needs to be pinned to prevent it from being
>>> moved while the hardware is scanning out from it, but that shouldn't be
>necessary here.
>> The pin here is used for scan out the buffer by remote display app.
>
>I still don't understand why pinning is needed. What mechanism does the remote
>display app use to access the BO contents?
Sorry, I am not familiar with the remote display app. Maybe it will use drm 
ioctl function to get the 
current crtc's fb's information, and get the content in the fb's buffer object 
by mmap or translate the bo
to an OpenGL texture for next rendering. Maybe don't need to pin the bo here, 
as the use has no different with
other normal bos. So please ignore the patch, and will send another patch to 
remove the unpin the fb's bo code. 
>
>
>--
>Earthling Michel Dänzer   |   http://www.amd.com
>Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amd: validate user GEM object size

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> When creating frame buffer, userspace may request to attach to a
> previously allocated GEM object that is smaller than what GPU
> requires. Validation must be done to prevent out-of-bound DMA,
> which could not only corrupt memory but also reveal sensitive data.
> 
> This fix is not done in a common code path because individual
> driver might have different requirement.
> 
> Signed-off-by: Yu Zhao 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 755daa332f8a..bb48b016cc68 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,7 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
> *dev,
>   struct drm_gem_object *obj;
>   struct amdgpu_framebuffer *amdgpu_fb;
>   int ret;
> + int height;
>   struct amdgpu_device *adev = dev->dev_private;
>   int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
>   int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false);
> @@ -550,6 +551,13 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
> *dev,
>   return ERR_PTR(-EINVAL);
>   }
>  
> + height = ALIGN(mode_cmd->height, 8);
> + if (obj->size < pitch * height) {
> + dev_err(>pdev->dev, "Invalid GEM size: expecting %d but 
> got %d\n",
> + pitch * height, obj->size);

Same comment as patch 2, and maybe write "expecting >= %d but got %d"
for clarity.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> For GPU that does frame buffer compression, DMA writing out of bound
> memory will cause memory corruption.
> 
> Signed-off-by: Yu Zhao 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index e309d26170db..755daa332f8a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
> *dev,
>   struct drm_gem_object *obj;
>   struct amdgpu_framebuffer *amdgpu_fb;
>   int ret;
> + struct amdgpu_device *adev = dev->dev_private;
> + int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
> + int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false);

Also, this needs to use mode_cmd->pitches[0] instead of mode_cmd->width,
otherwise it'll spuriously fail for larger but well-aligned pitches.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd: validate user pitch alignment

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Userspace may request pitch alignment that is not supported by GPU.
> Some requests 32, but GPU ignores it and uses default 64 when cpp is
> 4. If GEM object is allocated based on the smaller alignment, GPU
> DMA will go out of bound.
> 
> For GPU that does frame buffer compression, DMA writing out of bound
> memory will cause memory corruption.
> 
> Signed-off-by: Yu Zhao 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index e309d26170db..755daa332f8a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
> *dev,
>   struct drm_gem_object *obj;
>   struct amdgpu_framebuffer *amdgpu_fb;
>   int ret;
> + struct amdgpu_device *adev = dev->dev_private;
> + int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
> + int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false);
> +
> + if (mode_cmd->pitches[0] != pitch) {
> + dev_err(>pdev->dev, "Invalid pitch: expecting %d but got 
> %d\n",
> + pitch, mode_cmd->pitches[0]);

This message shouldn't be printed by default, or buggy / malicious
userspace can spam dmesg. I suggest using DRM_DEBUG_KMS or DRM_DEBUG_DRIVER.


> + return ERR_PTR(-EINVAL);
> + }
>  
>   obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
>   if (obj ==  NULL) {
> 

Other than that, looks good to me.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/amd: fix race in page flip job

[Michel Dänzer]

On 2018-12-21 4:10 a.m., Yu Zhao wrote:
> Fix race between page flip job submission and completion. We invoke
> page_flip callback to submit page flip job to GPU first and then set
> pflip_status. If GPU fires page flip done irq in between, its handler
> won't see the correct pflip_status thus will refuse to notify the job
> completion. The job will eventually times out.
> 
> Reverse the order of calling page_flip and setting pflip_status to
> fix the race.

There is no race, amdgpu_crtc->pflip_status is protected by dev->event_lock.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Michel Dänzer]

On 2018-12-21 9:45 a.m., Deng, Emily wrote:
>> -Original Message-
>> From: Michel Dänzer 
>> Sent: Friday, December 21, 2018 4:38 PM
>> To: Deng, Emily 
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>>
>> On 2018-12-21 8:26 a.m., Emily Deng wrote:
>>> When the bo is used to set mode, the bo need to be pinned.
>>
>> On second thought, why does the BO need to be pinned? When using the display
>> hardware, the BO needs to be pinned to prevent it from being moved while the
>> hardware is scanning out from it, but that shouldn't be necessary here.
> The pin here is used for scan out the buffer by remote display app.

I still don't understand why pinning is needed. What mechanism does the
remote display app use to access the BO contents?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/3] drm/amd: validate user GEM object size

[Yu Zhao]

When creating frame buffer, userspace may request to attach to a
previously allocated GEM object that is smaller than what GPU
requires. Validation must be done to prevent out-of-bound DMA,
which could not only corrupt memory but also reveal sensitive data.

This fix is not done in a common code path because individual
driver might have different requirement.

Signed-off-by: Yu Zhao 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 755daa332f8a..bb48b016cc68 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,7 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
*dev,
struct drm_gem_object *obj;
struct amdgpu_framebuffer *amdgpu_fb;
int ret;
+   int height;
struct amdgpu_device *adev = dev->dev_private;
int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false);
@@ -550,6 +551,13 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
*dev,
return ERR_PTR(-EINVAL);
}
 
+   height = ALIGN(mode_cmd->height, 8);
+   if (obj->size < pitch * height) {
+   dev_err(>pdev->dev, "Invalid GEM size: expecting %d but 
got %d\n",
+   pitch * height, obj->size);
+   return ERR_PTR(-EINVAL);
+   }
+
amdgpu_fb = kzalloc(sizeof(*amdgpu_fb), GFP_KERNEL);
if (amdgpu_fb == NULL) {
drm_gem_object_put_unlocked(obj);
-- 
2.20.1.415.g653613c723-goog

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/3] drm/amd: fix race in page flip job

[Yu Zhao]

Fix race between page flip job submission and completion. We invoke
page_flip callback to submit page flip job to GPU first and then set
pflip_status. If GPU fires page flip done irq in between, its handler
won't see the correct pflip_status thus will refuse to notify the job
completion. The job will eventually times out.

Reverse the order of calling page_flip and setting pflip_status to
fix the race.

Signed-off-by: Yu Zhao 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 686a26de50f9..e309d26170db 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -105,11 +105,11 @@ static void amdgpu_display_flip_work_func(struct 
work_struct *__work)
/* We borrow the event spin lock for protecting flip_status */
spin_lock_irqsave(>dev->event_lock, flags);
 
+   /* Set the flip status */
+   amdgpu_crtc->pflip_status = AMDGPU_FLIP_SUBMITTED;
/* Do the flip (mmio) */
adev->mode_info.funcs->page_flip(adev, work->crtc_id, work->base, 
work->async);
 
-   /* Set the flip status */
-   amdgpu_crtc->pflip_status = AMDGPU_FLIP_SUBMITTED;
spin_unlock_irqrestore(>dev->event_lock, flags);
 
 
-- 
2.20.1.415.g653613c723-goog

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/amd: validate user pitch alignment

[Yu Zhao]

Userspace may request pitch alignment that is not supported by GPU.
Some requests 32, but GPU ignores it and uses default 64 when cpp is
4. If GEM object is allocated based on the smaller alignment, GPU
DMA will go out of bound.

For GPU that does frame buffer compression, DMA writing out of bound
memory will cause memory corruption.

Signed-off-by: Yu Zhao 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index e309d26170db..755daa332f8a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -527,6 +527,15 @@ amdgpu_display_user_framebuffer_create(struct drm_device 
*dev,
struct drm_gem_object *obj;
struct amdgpu_framebuffer *amdgpu_fb;
int ret;
+   struct amdgpu_device *adev = dev->dev_private;
+   int cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
+   int pitch = amdgpu_align_pitch(adev, mode_cmd->width, cpp, false);
+
+   if (mode_cmd->pitches[0] != pitch) {
+   dev_err(>pdev->dev, "Invalid pitch: expecting %d but got 
%d\n",
+   pitch, mode_cmd->pitches[0]);
+   return ERR_PTR(-EINVAL);
+   }
 
obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
if (obj ==  NULL) {
-- 
2.20.1.415.g653613c723-goog

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Deng, Emily]

>-Original Message-
>From: Michel Dänzer 
>Sent: Friday, December 21, 2018 4:38 PM
>To: Deng, Emily 
>Cc: amd-gfx@lists.freedesktop.org
>Subject: Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo
>
>On 2018-12-21 8:26 a.m., Emily Deng wrote:
>> When the bo is used to set mode, the bo need to be pinned.
>
>On second thought, why does the BO need to be pinned? When using the display
>hardware, the BO needs to be pinned to prevent it from being moved while the
>hardware is scanning out from it, but that shouldn't be necessary here.
The pin here is used for scan out the buffer by remote display app.

Best wishes
Emily Deng


>
>--
>Earthling Michel Dänzer   |   http://www.amd.com
>Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Michel Dänzer]

On 2018-12-21 8:26 a.m., Emily Deng wrote:
> When the bo is used to set mode, the bo need to be pinned.

On second thought, why does the BO need to be pinned? When using the
display hardware, the BO needs to be pinned to prevent it from being
moved while the hardware is scanning out from it, but that shouldn't be
necessary here.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/virtual_dce: Need to pin the fb's bo

[Michel Dänzer]

On 2018-12-21 8:26 a.m., Emily Deng wrote:
> When the bo is used to set mode, the bo need to be pinned.
> 
> Signed-off-by: Emily Deng 
> 
> [...]
>  
> @@ -235,6 +222,47 @@ static bool dce_virtual_crtc_mode_fixup(struct drm_crtc 
> *crtc,
>  static int dce_virtual_crtc_set_base(struct drm_crtc *crtc, int x, int y,
> struct drm_framebuffer *old_fb)
>  {
> + struct drm_framebuffer *target_fb;
> + struct drm_gem_object *obj;
> + struct amdgpu_bo *abo;
> + int r;
> +
> + /* no fb bound */
> + if (!crtc->primary->fb) {
> + DRM_DEBUG_KMS("No FB bound\n");
> + return 0;
> + }
> +
> + target_fb = crtc->primary->fb;
> +
> + obj = kcl_drm_fb_get_gem_obj(target_fb, 0);
> + abo = gem_to_amdgpu_bo(obj);
> + r = amdgpu_bo_reserve(abo, false);
> + if (unlikely(r != 0))
> + return r;
> +
> + r = amdgpu_bo_pin(abo, AMDGPU_GEM_DOMAIN_VRAM);
> + if (unlikely(r != 0)) {
> + amdgpu_bo_unreserve(abo);
> + return -EINVAL;
> + }
> +
> + amdgpu_bo_unreserve(abo);
> + return 0;
> +}

Good catch, but old_fb also needs to be unpinned, otherwise the pinning
is unbalanced and a BO will always be pinned after it's scanned out once.


> +static int dce_virtual_crtc_mode_set(struct drm_crtc *crtc,
> +   struct drm_display_mode *mode,
> +   struct drm_display_mode *adjusted_mode,
> +   int x, int y, struct drm_framebuffer *old_fb)

Indentation of the continuation lines here looks wrong (needs 3 more
spaces if I'm counting correctly). Would be good to fix that up while
you're moving the function. :)


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: dma_fence finished signaled by unexpected callback

[wentalou]

When 2 rings met timeout at same time, triggered job_timedout separately.
Each job_timedout called gpu_recover, but one of gpu_recover locked by 
another's mutex_lock.
Bad jod’s callback should be removed by dma_fence_remove_callback but locked 
inside mutex_lock.
So dma_fence_remove_callback could not be called immediately.
Then callback drm_sched_process_job triggered unexpectedly, and signaled 
DMA_FENCE_FLAG_SIGNALED_BIT.
After another's mutex_unlock, signaled bad job went through job_run inside 
drm_sched_job_recovery.
job_run would have WARN_ON and Call-Trace, when calling kcl_dma_fence_set_error 
for signaled bad job.

Change-Id: I6366add13f020476882b2b8b03330a58d072dd1a
Signed-off-by: Wentao Lou 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 0a17fb1..fc1d3a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -225,8 +225,11 @@ static struct dma_fence *amdgpu_job_run(struct 
drm_sched_job *sched_job)
 
trace_amdgpu_sched_run_job(job);
 
-   if (job->vram_lost_counter != 
atomic_read(>adev->vram_lost_counter))
+   if (job->vram_lost_counter != 
atomic_read(>adev->vram_lost_counter)) {
+   /* flags might be signaled by unexpected callback, clear it */
+   test_and_clear_bit(DMA_FENCE_FLAG_SIGNALED_BIT, 
>flags);
dma_fence_set_error(finished, -ECANCELED);/* skip IB as well if 
VRAM lost */
+   }
 
if (finished->error < 0) {
DRM_INFO("Skip scheduling IBs!\n");
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx