[PULL] drm-amdkfd-next
Hi Dave, Here are a few amdkfd patches for 4.6. These patches defer radeon/amdgpu loading in case amdkfd is not yet loaded, by returning -EPROBE_DEFER during their probing stage. Thanks, Oded The following changes since commit 44ab4042178bd596275927ea050980aa4257581b: Merge branch 'for-next' of http://git.agner.ch/git/linux-drm-fsl-dcu into drm-next (2016-02-26 13:02:57 +1000) are available in the git repository at: git://people.freedesktop.org/~gabbayo/linux tags/drm-amdkfd-next-2016-02-27 for you to fetch changes up to efb1c6582e5cd291a6b9e6dde55fd31ce6f606a1: drm/amdgpu: Return -EPROBE_DEFER when amdkfd not loaded (2016-02-27 22:52:40 +0200) Oded Gabbay (3): drm/amdkfd: Track when module's init is complete drm/radeon: Return -EPROBE_DEFER when amdkfd not loaded drm/amdgpu: Return -EPROBE_DEFER when amdkfd not loaded drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 57 + drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 10 - drivers/gpu/drm/amd/amdkfd/kfd_module.c | 15 +-- drivers/gpu/drm/amd/include/kgd_kfd_interface.h | 2 +- drivers/gpu/drm/radeon/radeon_drv.c | 10 - drivers/gpu/drm/radeon/radeon_kfd.c | 25 ++- drivers/gpu/drm/radeon/radeon_kfd.h | 2 +- 8 files changed, 64 insertions(+), 59 deletions(-)
[Bug 60879] [radeonsi] X11 can't start with acceleration enabled
https://bugs.freedesktop.org/show_bug.cgi?id=60879 --- Comment #133 from madmalkav --- I've set up a bounty for fixing this bug. Quantity is quite low, sorry. If anyone else affected by this bug can throw some bucks into this, I think it can help to get a solution sooner. https://www.bountysource.com/issues/5643054-radeonsi-x11-can-t-start-with-acceleration-enabled -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160227/721ebcc9/attachment.html>
[Bug 113341] New: GPU Lockup on AMD Kaveri
https://bugzilla.kernel.org/show_bug.cgi?id=113341 Bug ID: 113341 Summary: GPU Lockup on AMD Kaveri Product: Drivers Version: 2.5 Kernel Version: 4.4.2 Hardware: x86-64 OS: Linux Tree: Mainline Status: NEW Severity: normal Priority: P1 Component: Video(DRI - non Intel) Assignee: drivers_video-dri at kernel-bugs.osdl.org Reporter: linux at bernd-steinhauser.de Regression: No Created attachment 206321 --> https://bugzilla.kernel.org/attachment.cgi?id=206321&action=edit journal log from the time of the gpu lockup. GPU is an AMD Kaveri: 1002:130f Happened when I started an application (way before the application actually showen) on KDE Plasma 5.5. The kernel version was 4.4.2 with an additional vblank fix applied (see [1]). Since I wasn't prepared for this and ssh wasn't activated and I couldn't save dmesg output. Instead I'll attach what I found in the journal. [1] https://bugs.freedesktop.org/show_bug.cgi?id=93746 -- You are receiving this mail because: You are watching the assignee of the bug.
[Bug 112491] Radeon: HD 7400G / A4-4355M System overheats with 3D graphics active.
https://bugzilla.kernel.org/show_bug.cgi?id=112491 --- Comment #14 from Dionisus Torimens --- Created attachment 206311 --> https://bugzilla.kernel.org/attachment.cgi?id=206311&action=edit Temperature Graph with nomodeset radeon.modeset=1 without redshift -- You are receiving this mail because: You are watching the assignee of the bug.
[Bug 112491] Radeon: HD 7400G / A4-4355M System overheats with 3D graphics active.
https://bugzilla.kernel.org/show_bug.cgi?id=112491 --- Comment #13 from Dionisus Torimens --- Created attachment 206301 --> https://bugzilla.kernel.org/attachment.cgi?id=206301&action=edit Temperature Graph with nomodeset radeon.modeset=1 and redshift -- You are receiving this mail because: You are watching the assignee of the bug.
[Bug 112491] Radeon: HD 7400G / A4-4355M System overheats with 3D graphics active.
https://bugzilla.kernel.org/show_bug.cgi?id=112491 Dionisus Torimens changed: What|Removed |Added Attachment #203781|0 |1 is obsolete|| Attachment #203781|Temperature Graph 1 |Temperature Graph 1 (crash description||not heat related in this ||case) -- You are receiving this mail because: You are watching the assignee of the bug.
[Bug 112491] Radeon: HD 7400G / A4-4355M System overheats with 3D graphics active.
https://bugzilla.kernel.org/show_bug.cgi?id=112491 --- Comment #12 from Dionisus Torimens --- [Wonderful, fglrx doesn't work at all with kernel 4.2... ...] The fastest way to get the system to overheat is to - enable redshift (or probably xgamma) - disable vsync - set dpm to preformance* echo performance | sudo tee /sys/class/drm/card0/device/power_dpm_state - activate cpu turbo mode* echo 1 | sudo tee /sys/devices/system/cpu/cpufreq/boost - activate BAPM - activate DRI3 - stay in the game menu (tested with blazrush, kotor) * Here the effect is not that certain/serious. Things that help to avoid overheating: - boot with nomodeset radeon.modeset=1 #vblank_mode=0 glmark2 --run-forever does not cause a hang, some of the tests seem less demanding, so the temperature does up and down. GALLIUM_HUD=temperature is helpful to watch how fast the temperature clims. -- You are receiving this mail because: You are watching the assignee of the bug.
[Bug 93826] 144Hz graphic glitches and bad refresh rate
https://bugs.freedesktop.org/show_bug.cgi?id=93826 --- Comment #17 from Alex Deucher --- The pll divider combination that is getting selected doesn't seem to agree with that monitor. You can try adjusting the algorithm in radeon_compute_pll_avivo() or the pll flags passed to radeon_compute_pll_avivo() in atombios_adjust_pll(). -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160227/65a1eb54/attachment.html>
[Bug 94315] AMDGPU 290X Refresh rate issue at 144Hz
https://bugs.freedesktop.org/show_bug.cgi?id=94315 Alex Deucher changed: What|Removed |Added Resolution|--- |DUPLICATE Status|NEW |RESOLVED --- Comment #7 from Alex Deucher --- *** This bug has been marked as a duplicate of bug 93826 *** -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160227/0a73dbf5/attachment.html>
[Bug 93826] 144Hz graphic glitches and bad refresh rate
https://bugs.freedesktop.org/show_bug.cgi?id=93826 --- Comment #16 from Alex Deucher --- *** Bug 94315 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160227/1620b00b/attachment.html>
[patch] drm/amd: cleanup get_mfd_cell_dev()
On Sat, Feb 27, 2016 at 10:50:40AM +0100, walter harms wrote:
>
>
> Am 25.02.2016 08:47, schrieb Dan Carpenter:
> > It's simpler to just use snprintf() to print this to one buffer instead
> > of using strcpy() and strcat(). Also using snprintf() is slightly safer
> > than using sprintf().
> >
> > Signed-off-by: Dan Carpenter
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> > index 9f8cfaa..d6b0bff 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> > @@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain
> > *genpd)
> > static struct device *get_mfd_cell_dev(const char *device_name, int r)
> > {
> > char auto_dev_name[25];
> > - char buf[8];
> > struct device *dev;
> >
> > - sprintf(buf, ".%d.auto", r);
> > - strcpy(auto_dev_name, device_name);
> > - strcat(auto_dev_name, buf);
> > + snprintf(auto_dev_name, sizeof(auto_dev_name),
> > +"%s.%d.auto", device_name, r);
> > dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name);
> > dev_info(dev, "device %s added to pm domain\n", auto_dev_name);
> >
>
> hi,
> i tried to understand what is the base for char auto_dev_name[25]. It is not
> clear
> from these snipped if that is large or small.
>
> (To be aware i assume that
> get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will never
> happen
> but i could find no reason)
>
> A small comment that explains the magic 25 would be nice.
I have no idea, either of course. For example,
mc13xxx_add_subdevice_pdata() assumes device_name by itself can be 30
characters. Hence the change to snprintf.
regards,
dan carpenter
[PATCH v4 5/5] staging/android: add flags member to sync ioctl structs
Hi Emil,
2016-02-27 Emil Velikov :
> Hi Gustavo,
>
> On 26 February 2016 at 18:31, Gustavo Padovan wrote:
> > From: Gustavo Padovan
> >
> > Play safe and add flags member to all structs. So we don't need to
> > break API or create new IOCTL in the future if new features that requires
> > flags arises.
> >
> > v2: check if flags are valid (zero, in this case)
> >
> > Signed-off-by: Gustavo Padovan
> > ---
> > drivers/staging/android/sync.c | 7 ++-
> > drivers/staging/android/uapi/sync.h | 6 ++
> > 2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> > index 837cff5..54fd5ab 100644
> > --- a/drivers/staging/android/sync.c
> > +++ b/drivers/staging/android/sync.c
> > @@ -445,6 +445,11 @@ static long sync_file_ioctl_merge(struct sync_file
> > *sync_file,
> > goto err_put_fd;
> > }
> >
> > + if (data.flags) {
> > + err = -EFAULT;
> -EINVAL ?
>
> > + goto err_put_fd;
> > + }
> > +
> > fence2 = sync_file_fdget(data.fd2);
> > if (!fence2) {
> > err = -ENOENT;
> > @@ -511,7 +516,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
> > *sync_file,
> > if (copy_from_user(&in, (void __user *)arg, sizeof(*info)))
> > return -EFAULT;
> >
> > - if (in.status || strcmp(in.name, "\0"))
> > + if (in.status || in.flags || strcmp(in.name, "\0"))
> > return -EFAULT;
> -EINVAL ?
>
> >
> > if (in.num_fences && !in.sync_fence_info)
> > diff --git a/drivers/staging/android/uapi/sync.h
> > b/drivers/staging/android/uapi/sync.h
> > index 9aad623..f56a6c2 100644
> > --- a/drivers/staging/android/uapi/sync.h
> > +++ b/drivers/staging/android/uapi/sync.h
> > @@ -19,11 +19,13 @@
> > * @fd2: file descriptor of second fence
> > * @name: name of new fence
> > * @fence: returns the fd of the new fence to userspace
> > + * @flags: merge_data flags
> > */
> > struct sync_merge_data {
> > __s32 fd2;
> > charname[32];
> > __s32 fence;
> > + __u32 flags;
> The overall size of the struct is not multiple of 64bit, so things
> will end up badly if we decide to extend it in the future. Even if
> there's a small chance that update will be needed, we might as well
> pad it now (and check the padding for zero, returning -EINVAL).
I think name could be the first field here.
>
> > };
> >
> > /**
> > @@ -31,12 +33,14 @@ struct sync_merge_data {
> > * @obj_name: name of parent sync_timeline
> > * @driver_name: name of driver implementing the parent
> > * @status:status of the fence 0:active 1:signaled <0:error
> > + * @flags: fence_info flags
> > * @timestamp_ns: timestamp of status change in nanoseconds
> > */
> > struct sync_fence_info {
> > charobj_name[32];
> > chardriver_name[32];
> > __s32 status;
> > + __u32 flags;
> > __u64 timestamp_ns;
> Should we be doing some form of validation in sync_fill_fence_info()
> of 'flags' ?
Do you think it is necessary? The kernel allocates a zero'ed buffer to
fill sync_fence_info array.
Gustavo
[PATCH] staging/android: refactor SYNC_IOC_FILE_INFO
Hi Emil,
2016-02-27 Emil Velikov :
> Hi Gustavo,
>
> On 26 February 2016 at 21:00, Gustavo Padovan wrote:
> > From: Gustavo Padovan
> >
> > Change SYNC_IOC_FILE_INFO behaviour to avoid future API breaks and
> > optimize buffer allocation. In the new approach the ioctl needs to be called
> > twice to retrieve the array of fence_infos pointed by info->sync_fence_info.
> >
> I might have misunderstood things but I no one says you "need" to call
> it twice - you can just request a "random" amount of fences_info. Upon
> return (if num_fences was non zero) it will report how many fence_info
> were retrieved.
Right, I don't see any problem doing it in one request, I just didn't
think about that in the new proposal. I'll update the code and commit
message accordinly.
>
> > The first call should pass num_fences = 0, the kernel will then fill
> > info->num_fences. Userspace receives back the number of fences and
> > allocates a buffer size num_fences * sizeof(struct sync_fence_info) on
> > info->sync_fence_info.
> >
> > It then call the ioctl again passing num_fences received in
> > info->num_fences.
> "calls"
>
> > The kernel checks if info->num_fences > 0 and if yes it fill
> > info->sync_fence_info with an array containing all fence_infos.
> >
> The above sentence sounds a bit strange. I believe you meant to say
> something like "Then the kernel fills the fence_infos array with data.
> One should read back the actual number from info->num_fences." ?
>
> > info->len now represents the length of the buffer sync_fence_info points
> > to.
> Now that I think about it, I'm wondering if there'll be a case where
> len != info->num_fences * sizeof(struct sync_file_info). If that's not
> possible one could just drop len and nicely simplify things.
>
> > Also, info->sync_fence_info was converted to __u64 pointer.
> >
> ... pointer to prevent 32bit compatibility issues.
>
> > An example userspace code would be:
> >
> > struct sync_file_info *info;
> > int err, size, num_fences;
> >
> > info = malloc(sizeof(*info));
> >
> > memset(info, 0, sizeof(*info));
> >
> > err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> > num_fences = info->num_fences;
> >
> > if (num_fences) {
> > memset(info, 0, sizeof(*info));
> > size = sizeof(struct sync_fence_info) * num_fences;
> > info->len = size;
> > info->num_fences = num_fences;
> > info->sync_fence_info = (uint64_t) calloc(num_fences,
> > sizeof(struct
> > sync_fence_info));
> >
> > err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> > }
> >
> > v2: fix fence_info memory leak
> >
> > Signed-off-by: Gustavo Padovan
> > ---
> > drivers/staging/android/sync.c | 52
> > +
> > drivers/staging/android/uapi/sync.h | 9 +++
> > 2 files changed, 45 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> > index dc5f382..2379f23 100644
> > --- a/drivers/staging/android/sync.c
> > +++ b/drivers/staging/android/sync.c
> > @@ -502,21 +502,22 @@ static int sync_fill_fence_info(struct fence *fence,
> > void *data, int size)
> > static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> > unsigned long arg)
> > {
> > - struct sync_file_info *info;
> > + struct sync_file_info in, *info;
> > + struct sync_fence_info *fence_info = NULL;
> > __u32 size;
> > __u32 len = 0;
> > int ret, i;
> >
> > - if (copy_from_user(&size, (void __user *)arg, sizeof(size)))
> > + if (copy_from_user(&in, (void __user *)arg, sizeof(*info)))
> s/*info/in/
>
> > return -EFAULT;
> >
> > - if (size < sizeof(struct sync_file_info))
> > - return -EINVAL;
> > + if (in.status || strcmp(in.name, "\0"))
> Afaict these two are outputs, so we should be checking them ?
Hmm. Maybe not.
>
> > + return -EFAULT;
> >
> As originally, input validation should return -EINVAL on error.
>
>
> > - if (size > 4096)
> > - size = 4096;
> > + if (in.num_fences && !in.sync_fence_info)
> > + return -EFAULT;
> >
> Ditto.
>
> > - info = kzalloc(size, GFP_KERNEL);
> > + info = kzalloc(sizeof(*info), GFP_KERNEL);
> > if (!info)
> > return -ENOMEM;
> >
> > @@ -525,14 +526,33 @@ static long sync_file_ioctl_fence_info(struct
> > sync_file *sync_file,
> > if (info->status >= 0)
> > info->status = !info->status;
> >
> > - info->num_fences = sync_file->num_fences;
> > + /*
> > +* Passing num_fences = 0 means that userspace want to know how
> > +* many fences are in the sync_file to be able to allocate a buffer
> > to
> > +* fit all sync
[patch] drm/amd: cleanup get_mfd_cell_dev()
Am 27.02.2016 11:40, schrieb Dan Carpenter:
> On Sat, Feb 27, 2016 at 10:50:40AM +0100, walter harms wrote:
>>
>>
>> Am 25.02.2016 08:47, schrieb Dan Carpenter:
>>> It's simpler to just use snprintf() to print this to one buffer instead
>>> of using strcpy() and strcat(). Also using snprintf() is slightly safer
>>> than using sprintf().
>>>
>>> Signed-off-by: Dan Carpenter
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> index 9f8cfaa..d6b0bff 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> @@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain
>>> *genpd)
>>> static struct device *get_mfd_cell_dev(const char *device_name, int r)
>>> {
>>> char auto_dev_name[25];
>>> - char buf[8];
>>> struct device *dev;
>>>
>>> - sprintf(buf, ".%d.auto", r);
>>> - strcpy(auto_dev_name, device_name);
>>> - strcat(auto_dev_name, buf);
>>> + snprintf(auto_dev_name, sizeof(auto_dev_name),
>>> +"%s.%d.auto", device_name, r);
>>> dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name);
>>> dev_info(dev, "device %s added to pm domain\n", auto_dev_name);
>>>
>>
>> hi,
>> i tried to understand what is the base for char auto_dev_name[25]. It is not
>> clear
>> from these snipped if that is large or small.
>>
>> (To be aware i assume that
>> get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will
>> never happen
>> but i could find no reason)
>>
>> A small comment that explains the magic 25 would be nice.
>
> I have no idea, either of course. For example,
> mc13xxx_add_subdevice_pdata() assumes device_name by itself can be 30
> characters. Hence the change to snprintf.
>
Hi Dan,
i also think that this limit is artificial, one of those "it works" things.
The problem is that i have seen changes in the naming als ready done, like the
shift from /dev/sda to things like
/dev/disk/by-id/scsi-SATA_WDC_WD5000AAKS-_WD-WCASY5869245-part1
and you are out of the game here. snprintf will cut the tail and the %d.auto
stuff is dead in the water.
To make it clear, I do not thing that is security related issue but it could
result in annoying
failures. (the solution is obviously to use asprintf() and free the mem later
:) ).
re,
wh
[Bug 112921] Lock-up after screensaver on radeon
https://bugzilla.kernel.org/show_bug.cgi?id=112921 Jean Delvare changed: What|Removed |Added Status|NEEDINFO|ASSIGNED --- Comment #6 from Jean Delvare --- Unfortunately, Mario's patch does not fix my problem. -- You are receiving this mail because: You are watching the assignee of the bug.
[patch] drm/amd: cleanup get_mfd_cell_dev()
Am 25.02.2016 08:47, schrieb Dan Carpenter:
> It's simpler to just use snprintf() to print this to one buffer instead
> of using strcpy() and strcat(). Also using snprintf() is slightly safer
> than using sprintf().
>
> Signed-off-by: Dan Carpenter
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> index 9f8cfaa..d6b0bff 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> @@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd)
> static struct device *get_mfd_cell_dev(const char *device_name, int r)
> {
> char auto_dev_name[25];
> - char buf[8];
> struct device *dev;
>
> - sprintf(buf, ".%d.auto", r);
> - strcpy(auto_dev_name, device_name);
> - strcat(auto_dev_name, buf);
> + snprintf(auto_dev_name, sizeof(auto_dev_name),
> + "%s.%d.auto", device_name, r);
> dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name);
> dev_info(dev, "device %s added to pm domain\n", auto_dev_name);
>
hi,
i tried to understand what is the base for char auto_dev_name[25]. It is not
clear
from these snipped if that is large or small.
(To be aware i assume that
get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will never
happen
but i could find no reason)
A small comment that explains the magic 25 would be nice.
re,
wh
[PATCH v4 5/5] staging/android: add flags member to sync ioctl structs
Hi Gustavo,
On 26 February 2016 at 18:31, Gustavo Padovan wrote:
> From: Gustavo Padovan
>
> Play safe and add flags member to all structs. So we don't need to
> break API or create new IOCTL in the future if new features that requires
> flags arises.
>
> v2: check if flags are valid (zero, in this case)
>
> Signed-off-by: Gustavo Padovan
> ---
> drivers/staging/android/sync.c | 7 ++-
> drivers/staging/android/uapi/sync.h | 6 ++
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> index 837cff5..54fd5ab 100644
> --- a/drivers/staging/android/sync.c
> +++ b/drivers/staging/android/sync.c
> @@ -445,6 +445,11 @@ static long sync_file_ioctl_merge(struct sync_file
> *sync_file,
> goto err_put_fd;
> }
>
> + if (data.flags) {
> + err = -EFAULT;
-EINVAL ?
> + goto err_put_fd;
> + }
> +
> fence2 = sync_file_fdget(data.fd2);
> if (!fence2) {
> err = -ENOENT;
> @@ -511,7 +516,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
> *sync_file,
> if (copy_from_user(&in, (void __user *)arg, sizeof(*info)))
> return -EFAULT;
>
> - if (in.status || strcmp(in.name, "\0"))
> + if (in.status || in.flags || strcmp(in.name, "\0"))
> return -EFAULT;
-EINVAL ?
>
> if (in.num_fences && !in.sync_fence_info)
> diff --git a/drivers/staging/android/uapi/sync.h
> b/drivers/staging/android/uapi/sync.h
> index 9aad623..f56a6c2 100644
> --- a/drivers/staging/android/uapi/sync.h
> +++ b/drivers/staging/android/uapi/sync.h
> @@ -19,11 +19,13 @@
> * @fd2: file descriptor of second fence
> * @name: name of new fence
> * @fence: returns the fd of the new fence to userspace
> + * @flags: merge_data flags
> */
> struct sync_merge_data {
> __s32 fd2;
> charname[32];
> __s32 fence;
> + __u32 flags;
The overall size of the struct is not multiple of 64bit, so things
will end up badly if we decide to extend it in the future. Even if
there's a small chance that update will be needed, we might as well
pad it now (and check the padding for zero, returning -EINVAL).
> };
>
> /**
> @@ -31,12 +33,14 @@ struct sync_merge_data {
> * @obj_name: name of parent sync_timeline
> * @driver_name: name of driver implementing the parent
> * @status:status of the fence 0:active 1:signaled <0:error
> + * @flags: fence_info flags
> * @timestamp_ns: timestamp of status change in nanoseconds
> */
> struct sync_fence_info {
> charobj_name[32];
> chardriver_name[32];
> __s32 status;
> + __u32 flags;
> __u64 timestamp_ns;
Should we be doing some form of validation in sync_fill_fence_info()
of 'flags' ?
Regards,
Emil
[PATCH] staging/android: refactor SYNC_IOC_FILE_INFO
Hi Gustavo,
On 26 February 2016 at 21:00, Gustavo Padovan wrote:
> From: Gustavo Padovan
>
> Change SYNC_IOC_FILE_INFO behaviour to avoid future API breaks and
> optimize buffer allocation. In the new approach the ioctl needs to be called
> twice to retrieve the array of fence_infos pointed by info->sync_fence_info.
>
I might have misunderstood things but I no one says you "need" to call
it twice - you can just request a "random" amount of fences_info. Upon
return (if num_fences was non zero) it will report how many fence_info
were retrieved.
> The first call should pass num_fences = 0, the kernel will then fill
> info->num_fences. Userspace receives back the number of fences and
> allocates a buffer size num_fences * sizeof(struct sync_fence_info) on
> info->sync_fence_info.
>
> It then call the ioctl again passing num_fences received in info->num_fences.
"calls"
> The kernel checks if info->num_fences > 0 and if yes it fill
> info->sync_fence_info with an array containing all fence_infos.
>
The above sentence sounds a bit strange. I believe you meant to say
something like "Then the kernel fills the fence_infos array with data.
One should read back the actual number from info->num_fences." ?
> info->len now represents the length of the buffer sync_fence_info points
> to.
Now that I think about it, I'm wondering if there'll be a case where
len != info->num_fences * sizeof(struct sync_file_info). If that's not
possible one could just drop len and nicely simplify things.
> Also, info->sync_fence_info was converted to __u64 pointer.
>
... pointer to prevent 32bit compatibility issues.
> An example userspace code would be:
>
> struct sync_file_info *info;
> int err, size, num_fences;
>
> info = malloc(sizeof(*info));
>
> memset(info, 0, sizeof(*info));
>
> err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> num_fences = info->num_fences;
>
> if (num_fences) {
> memset(info, 0, sizeof(*info));
> size = sizeof(struct sync_fence_info) * num_fences;
> info->len = size;
> info->num_fences = num_fences;
> info->sync_fence_info = (uint64_t) calloc(num_fences,
> sizeof(struct
> sync_fence_info));
>
> err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> }
>
> v2: fix fence_info memory leak
>
> Signed-off-by: Gustavo Padovan
> ---
> drivers/staging/android/sync.c | 52
> +
> drivers/staging/android/uapi/sync.h | 9 +++
> 2 files changed, 45 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> index dc5f382..2379f23 100644
> --- a/drivers/staging/android/sync.c
> +++ b/drivers/staging/android/sync.c
> @@ -502,21 +502,22 @@ static int sync_fill_fence_info(struct fence *fence,
> void *data, int size)
> static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> unsigned long arg)
> {
> - struct sync_file_info *info;
> + struct sync_file_info in, *info;
> + struct sync_fence_info *fence_info = NULL;
> __u32 size;
> __u32 len = 0;
> int ret, i;
>
> - if (copy_from_user(&size, (void __user *)arg, sizeof(size)))
> + if (copy_from_user(&in, (void __user *)arg, sizeof(*info)))
s/*info/in/
> return -EFAULT;
>
> - if (size < sizeof(struct sync_file_info))
> - return -EINVAL;
> + if (in.status || strcmp(in.name, "\0"))
Afaict these two are outputs, so we should be checking them ?
> + return -EFAULT;
>
As originally, input validation should return -EINVAL on error.
> - if (size > 4096)
> - size = 4096;
> + if (in.num_fences && !in.sync_fence_info)
> + return -EFAULT;
>
Ditto.
> - info = kzalloc(size, GFP_KERNEL);
> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> if (!info)
> return -ENOMEM;
>
> @@ -525,14 +526,33 @@ static long sync_file_ioctl_fence_info(struct sync_file
> *sync_file,
> if (info->status >= 0)
> info->status = !info->status;
>
> - info->num_fences = sync_file->num_fences;
> + /*
> +* Passing num_fences = 0 means that userspace want to know how
> +* many fences are in the sync_file to be able to allocate a buffer to
> +* fit all sync_fence_infos and call the ioctl again with the buffer
> +* assigned to info->sync_fence_info. The second call pass the
> +* num_fences value received in the first call.
> +*/
> + if (!in.num_fences)
> + goto no_fences;
> +
We should clamp in.num_fences to min2(in.num_fences,
sync_file->num_fences) and use it over sync_file->num_fences though
the rest of the function. Or just bail out when the two are not the
same.
Depends on what t
[PATCH 3/5] drm: introduce pipe color correction properties
On 26 February 2016 at 15:43, Lionel Landwerlin
wrote:
> On 26/02/16 00:36, Emil Velikov wrote:
>>
>> Hi Lionel,
>>
>> A bunch of suggestions - feel free to take or ignore them :-)
>>
>> On 25 February 2016 at 10:58, Lionel Landwerlin
>> wrote:
> I'm not sure it matters as the drm_crtc_state you're set properties on will
> be discarded if there is an error.
> The current drm_crtc_state that has been applied onto the hardware should be
> untouched.
>
That's the thing - the current drm_crts_state (mode_blob) is being
discarded, as opposed to the newly setup one. Although I could have
misunderstood something.
>
> This is because we accept more than one size of degamma/gamma LUT (legacy ->
> 256 elements, new LUT -> (de)gamma_lut_size elements).
> It's up for the driver the check the size and raise an error in its
> atomic_check() vfunc.
>
Ahh yes the legacy vs atomic difference.
>
> Moving the if (blob == NULL) right after the blob allocation to make it
> simpler.
>
> What about completeness? Is there something inherently wrong here?
The suggestion to reorder things is mostly to keep the setup/teardown
order in reverse order - create_blob, create_state and drop_state,
drop_blob. As you've noticed, I'm kind of a sucker for those :-) There
are no inter-dependencies that would require it here so it's not
required.
>>
>>
>>> + state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
>>> +retry:
>>> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>> + if (IS_ERR(crtc_state)) {
>>> + ret = PTR_ERR(crtc_state);
>>> + goto fail;
>>> + }
>>> +
>>> + /* Reset DEGAMMA_LUT and CTM properties. */
>>> + ret = drm_atomic_crtc_set_property(crtc, crtc_state,
>>> + config->degamma_lut_property, 0);
>>> + if (ret)
>>> + goto fail;
>>
>> Add new blank line please.
>
>
> Sure.
>>
>>
>>> + ret = drm_atomic_crtc_set_property(crtc, crtc_state,
>>> + config->ctm_property, 0);
>>> + if (ret)
>>> + goto fail;
>>> +
>>> + /* Set GAMMA_LUT with legacy values. */
>>> + if (blob == NULL) {
>>> + ret = -ENOMEM;
>>> + goto fail;
>>> + }
>>> +
>>> + blob_data = (struct drm_color_lut *) blob->data;
>>> + for (i = 0; i < size; i++) {
>>> + blob_data[i].red = red[i];
>>> + blob_data[i].green = green[i];
>>> + blob_data[i].blue = blue[i];
>>> + }
>>> +
>>
>> Move this loop after create_blob()
>
> Thanks, indeed no need to refill it in case of retry.
>
>>
>>> + ret = drm_atomic_crtc_set_property(crtc, crtc_state,
>>> + config->gamma_lut_property, blob->base.id);
>>> + if (ret)
>>> + goto fail;
>>> +
>>> + ret = drm_atomic_commit(state);
>>> + if (ret != 0)
>>
>> Please check in a consistent way. Currently we have ret != 0 vs ret
>> and foo == NULL vs !foo.
>
>
> Sure.
>
>>
>>> + goto fail;
>>> +
>>> + drm_property_unreference_blob(blob);
>>> +
>>> + /* Driver takes ownership of state on successful commit. */
>>
>> Move the comment before unreference_blob(), so that it's closer to
>> atomic_commit() ?
>
>
> Sure.
>>
>>
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -1554,6 +1554,41 @@ static int
>>> drm_mode_create_standard_properties(struct drm_device *dev)
>>> return -ENOMEM;
>>> dev->mode_config.prop_mode_id = prop;
>>>
>>> + prop = drm_property_create(dev,
>>> + DRM_MODE_PROP_BLOB,
>>> + "DEGAMMA_LUT", 0);
>>
>> Just wondering - don't we want this and the remaining properties to
>> be atomic only ? I doubt we have userspace that [will be updated to]
>> handle these, yet lacks atomic.
>
> This was pointed out by Matt already. Here is Daniel Stone's response :
> https://lists.freedesktop.org/archives/intel-gfx/2016-January/086120.html
>
> I think it's fine to have these properties not atomic because it's not
> really something you update very often (maybe just when starting your UI).
> That's actually how we would like to use them in ChromiumOS as a first step,
> until eventually ChromiumOS switches to atomic.
>
It wasn't a question of "can it be used" but more of "would it make
sense to not switch to atomics once we're here". As is, userspace will
need to have two slightly different code paths. Put the question "how
to deal with if compositor crashes and (re)applying gamma/etc.
multiple times" by Daniel Vettel, on top of it all and things get
extra messy. In both usespace in kernel.
My line of thought is - if there is a high demand for
non-atomic(legacy) degamma/etc. one can easily add it. On the other
hand, once it lands one cannot remove the code from the kernel, ever.
It's up-to you guys really. Just thought I mentioned it.
-Emil
