| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a382b75d-4254-419c-aab5-a01eed845df1@igalia.com>
Date: Thu, 1 Feb 2024 15:42:49 -0300
From: André Almeida <andrealmeid@...lia.com>
To: Pekka Paalanen <pekka.paalanen@...oniitty.fi>
Cc: dri-devel@...ts.freedesktop.org, amd-gfx@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, kernel-dev@...lia.com,
alexander.deucher@....com, christian.koenig@....com,
Simon Ser <contact@...rsion.fr>, daniel@...ll.ch,
Daniel Stone <daniel@...ishbar.org>, 'Marek Olšák'
<maraeo@...il.com>, Dave Airlie <airlied@...il.com>,
ville.syrjala@...ux.intel.com, Xaver Hugl <xaver.hugl@...il.com>,
Joshua Ashton <joshua@...ggi.es>, Michel Dänzer
<michel.daenzer@...lbox.org>
Subject: Re: [PATCH v3 1/3] drm/atomic: Allow drivers to write their own plane
check for async flips
Hi Pekka,
Em 29/01/2024 05:49, Pekka Paalanen escreveu:
> On Sun, 28 Jan 2024 18:25:13 -0300
> André Almeida <andrealmeid@...lia.com> wrote:
>
>> Some hardware are more flexible on what they can flip asynchronously, so
>> rework the plane check so drivers can implement their own check, lifting
>> up some of the restrictions.
>>
>> Signed-off-by: André Almeida <andrealmeid@...lia.com>
>> ---
>> v3: no changes
>>
>> drivers/gpu/drm/drm_atomic_uapi.c | 62 ++++++++++++++++++++++---------
>> include/drm/drm_atomic_uapi.h | 12 ++++++
>> include/drm/drm_plane.h | 5 +++
>> 3 files changed, 62 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index aee4a65d4959..6d5b9fec90c7 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -620,7 +620,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>> return 0;
>> }
>>
>> -static int
>> +int
>> drm_atomic_plane_get_property(struct drm_plane *plane,
>> const struct drm_plane_state *state,
>> struct drm_property *property, uint64_t *val)
>> @@ -683,6 +683,7 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL(drm_atomic_plane_get_property);
>>
>> static int drm_atomic_set_writeback_fb_for_connector(
>> struct drm_connector_state *conn_state,
>> @@ -1026,18 +1027,54 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>> return ret;
>> }
>>
>> -static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
>> +int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
>
> Hi,
>
> should the word "async" be somewhere in the function name?
>
>> struct drm_property *prop)
>> {
>> if (ret != 0 || old_val != prop_value) {
>> drm_dbg_atomic(prop->dev,
>> - "[PROP:%d:%s] No prop can be changed during async flip\n",
>> + "[PROP:%d:%s] This prop cannot be changed during async flip\n",
>> prop->base.id, prop->name);
>> return -EINVAL;
>> }
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL(drm_atomic_check_prop_changes);
>> +
>> +/* plane changes may have exceptions, so we have a special function for them */
>> +static int drm_atomic_check_plane_changes(struct drm_property *prop,
>> + struct drm_plane *plane,
>> + struct drm_plane_state *plane_state,
>> + struct drm_mode_object *obj,
>> + u64 prop_value, u64 old_val)
>> +{
>> + struct drm_mode_config *config = &plane->dev->mode_config;
>> + int ret;
>> +
>> + if (plane->funcs->check_async_props)
>> + return plane->funcs->check_async_props(prop, plane, plane_state,
>> + obj, prop_value, old_val);
>
> Is it really ok to allow drivers to opt-in to also *reject* the FB_ID
> and IN_FENCE_FD props on async commits?
>
> Either intentionally or by accident.
>
Right, perhaps I should write this function in a way that you can only
lift restrictions, and not add new ones.
>> +
>> + /*
>> + * if you are trying to change something other than the FB ID, your
>> + * change will be either rejected or ignored, so we can stop the check
>> + * here
>> + */
>> + if (prop != config->prop_fb_id) {
>> + ret = drm_atomic_plane_get_property(plane, plane_state,
>> + prop, &old_val);
>> + return drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
>
> When I first read this code, it seemed to say: "If the prop is not
> FB_ID, then do the usual prop change checking that happens on all
> changes, not only async.". Reading this patch a few more times over, I
> finally realized drm_atomic_check_prop_changes() is about async
> specifically.
>
I see that the lack of the async word is giving some confusion, so I'll
add it to the functions.
Thanks,
André
>> + }
>> +
>> + if (plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> + drm_dbg_atomic(prop->dev,
>> + "[OBJECT:%d] Only primary planes can be changed during async flip\n",
>> + obj->id);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>>
>> int drm_atomic_set_property(struct drm_atomic_state *state,
>> struct drm_file *file_priv,
>> @@ -1100,7 +1137,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>> case DRM_MODE_OBJECT_PLANE: {
>> struct drm_plane *plane = obj_to_plane(obj);
>> struct drm_plane_state *plane_state;
>> - struct drm_mode_config *config = &plane->dev->mode_config;
>>
>> plane_state = drm_atomic_get_plane_state(state, plane);
>> if (IS_ERR(plane_state)) {
>> @@ -1108,19 +1144,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>> break;
>> }
>>
>> - if (async_flip && prop != config->prop_fb_id) {
>> - ret = drm_atomic_plane_get_property(plane, plane_state,
>> - prop, &old_val);
>> - ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
>> - break;
>> - }
>> -
>> - if (async_flip && plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> - drm_dbg_atomic(prop->dev,
>> - "[OBJECT:%d] Only primary planes can be changed during async flip\n",
>> - obj->id);
>> - ret = -EINVAL;
>> - break;
>> + if (async_flip) {
>> + ret = drm_atomic_check_plane_changes(prop, plane, plane_state,
>
> Should there be "async" somewhere in the name of
> drm_atomic_check_plane_changes()?
>
>
> Thanks,
> pq
>
>> + obj, prop_value, old_val);
>> + if (ret)
>> + break;
>> }
>>
>> ret = drm_atomic_plane_set_property(plane,
>> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
>> index 4c6d39d7bdb2..d65fa8fbbca0 100644
>> --- a/include/drm/drm_atomic_uapi.h
>> +++ b/include/drm/drm_atomic_uapi.h
>> @@ -29,6 +29,8 @@
>> #ifndef DRM_ATOMIC_UAPI_H_
>> #define DRM_ATOMIC_UAPI_H_
>>
>> +#include <linux/types.h>
>> +
>> struct drm_crtc_state;
>> struct drm_display_mode;
>> struct drm_property_blob;
>> @@ -37,6 +39,9 @@ struct drm_crtc;
>> struct drm_connector_state;
>> struct dma_fence;
>> struct drm_framebuffer;
>> +struct drm_mode_object;
>> +struct drm_property;
>> +struct drm_plane;
>>
>> int __must_check
>> drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>> @@ -53,4 +58,11 @@ int __must_check
>> drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>> struct drm_crtc *crtc);
>>
>> +int drm_atomic_plane_get_property(struct drm_plane *plane,
>> + const struct drm_plane_state *state,
>> + struct drm_property *property, uint64_t *val);
>> +
>> +int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
>> + struct drm_property *prop);
>> +
>> #endif
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index c6565a6f9324..73dac8d76831 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -540,6 +540,11 @@ struct drm_plane_funcs {
>> */
>> bool (*format_mod_supported)(struct drm_plane *plane, uint32_t format,
>> uint64_t modifier);
>> +
>> + int (*check_async_props)(struct drm_property *prop, struct drm_plane *plane,
>> + struct drm_plane_state *plane_state,
>> + struct drm_mode_object *obj,
>> + u64 prop_value, u64 old_val);
>> };
>>
>> /**
>
Powered by blists - more mailing lists