[PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Thomas Zimmermann]

In drm_atomic_helper_check_crtc_state(), do not add a new plane state
to the global state if it does not exist already. Adding a new plane
state will results in overhead for the plane during the atomic-commit
step.

For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
important that the CRTC has an enabled primary plane after the commit.
This can be a newly enabled plane or an already enabled plane. So if a
plane is not part of the commit, use the plane's existing state. The new
helper drm_atomic_get_next_plane_state() returns the correct instance.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_atomic_helper.c |  4 +---
 include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 98cc3137c062..463d4f3fa443 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
 
 			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
 				continue;
-			plane_state = drm_atomic_get_plane_state(state, plane);
-			if (IS_ERR(plane_state))
-				return PTR_ERR(plane_state);
+			plane_state = drm_atomic_get_next_plane_state(state, plane);
 			if (plane_state->fb && plane_state->crtc) {
 				has_primary_plane = true;
 				break;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 10b1990bc1f6..4006f2d459e3 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
 	return state->planes[drm_plane_index(plane)].new_state;
 }
 
+/**
+ * drm_atomic_get_next_plane_state - get plane state after atomic commit
+ * @state: global atomic state object
+ * @plane: plane to grab
+ *
+ * This function returns the plane state that the given plane will have
+ * after the atomic commit. This will be the new plane state if the plane
+ * is part of the global atomic state, or the current state otherwise.
+ */
+static inline struct drm_plane_state *
+drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
+				struct drm_plane *plane)
+{
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+
+	if (plane_state)
+		return plane_state;
+	return plane->state;
+}
+
 /**
  * drm_atomic_get_existing_connector_state - get connector state, if it exists
  * @state: global atomic state object
-- 
2.37.3

Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Ville Syrjälä]

On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
> to the global state if it does not exist already. Adding a new plane
> state will results in overhead for the plane during the atomic-commit
> step.
> 
> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
> important that the CRTC has an enabled primary plane after the commit.
> This can be a newly enabled plane or an already enabled plane. So if a
> plane is not part of the commit, use the plane's existing state. The new
> helper drm_atomic_get_next_plane_state() returns the correct instance.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Jocelyn Falempe <jfalempe@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  4 +---
>  include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 98cc3137c062..463d4f3fa443 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
>  
>  			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>  				continue;
> -			plane_state = drm_atomic_get_plane_state(state, plane);
> -			if (IS_ERR(plane_state))
> -				return PTR_ERR(plane_state);
> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
>  			if (plane_state->fb && plane_state->crtc) {

Hmm. Why this is even looking at these. If the plane is in the crtc's
plane_mask then surely plane_state->crtc must already point to this
crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
if one is set then surely the other one must be as well or we'd
return an error at some point somewhere?

>  				has_primary_plane = true;
>  				break;
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 10b1990bc1f6..4006f2d459e3 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
>  	return state->planes[drm_plane_index(plane)].new_state;
>  }
>  
> +/**
> + * drm_atomic_get_next_plane_state - get plane state after atomic commit
> + * @state: global atomic state object
> + * @plane: plane to grab
> + *
> + * This function returns the plane state that the given plane will have
> + * after the atomic commit. This will be the new plane state if the plane
> + * is part of the global atomic state, or the current state otherwise.
> + */
> +static inline struct drm_plane_state *
> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
> +				struct drm_plane *plane)
> +{
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +
> +	if (plane_state)
> +		return plane_state;
> +	return plane->state;

You're not holding the appropriate lock so that does not look safe. Even
if you know somewhere, somehow this sort of thing to be safe I don't
think it's a good idea to promote this with any kind of official
function because someone will shoot themselves in the foot with it.

> +}
> +
>  /**
>   * drm_atomic_get_existing_connector_state - get connector state, if it exists
>   * @state: global atomic state object
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4502 bytes --]

Hi,

thanks for reviewing.

Am 29.09.22 um 21:03 schrieb Ville Syrjälä:
> On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
>> to the global state if it does not exist already. Adding a new plane
>> state will results in overhead for the plane during the atomic-commit
>> step.
>>
>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
>> important that the CRTC has an enabled primary plane after the commit.
>> This can be a newly enabled plane or an already enabled plane. So if a
>> plane is not part of the commit, use the plane's existing state. The new
>> helper drm_atomic_get_next_plane_state() returns the correct instance.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Jocelyn Falempe <jfalempe@redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_atomic_helper.c |  4 +---
>>   include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 98cc3137c062..463d4f3fa443 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
>>   
>>   			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>>   				continue;
>> -			plane_state = drm_atomic_get_plane_state(state, plane);
>> -			if (IS_ERR(plane_state))
>> -				return PTR_ERR(plane_state);
>> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
>>   			if (plane_state->fb && plane_state->crtc) {
> 
> Hmm. Why this is even looking at these. If the plane is in the crtc's
> plane_mask then surely plane_state->crtc must already point to this
> crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
> if one is set then surely the other one must be as well or we'd
> return an error at some point somewhere?

Yeah, the crtc test is done for keeping consistency. Other places also 
sometimes validate that these fields don't disagree. I'll remove the 
crtc test in the next version. The fb test is the important one.

> 
>>   				has_primary_plane = true;
>>   				break;
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 10b1990bc1f6..4006f2d459e3 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
>>   	return state->planes[drm_plane_index(plane)].new_state;
>>   }
>>   
>> +/**
>> + * drm_atomic_get_next_plane_state - get plane state after atomic commit
>> + * @state: global atomic state object
>> + * @plane: plane to grab
>> + *
>> + * This function returns the plane state that the given plane will have
>> + * after the atomic commit. This will be the new plane state if the plane
>> + * is part of the global atomic state, or the current state otherwise.
>> + */
>> +static inline struct drm_plane_state *
>> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
>> +				struct drm_plane *plane)
>> +{
>> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>> +
>> +	if (plane_state)
>> +		return plane_state;
>> +	return plane->state;
> 
> You're not holding the appropriate lock so that does not look safe. Even
> if you know somewhere, somehow this sort of thing to be safe I don't
> think it's a good idea to promote this with any kind of official
> function because someone will shoot themselves in the foot with it.

OK. It'll be inlined with locking added.

Best regards
Thomas

> 
>> +}
>> +
>>   /**
>>    * drm_atomic_get_existing_connector_state - get connector state, if it exists
>>    * @state: global atomic state object
>> -- 
>> 2.37.3
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote:
> Hi,
> 
> thanks for reviewing.
> 
> Am 29.09.22 um 21:03 schrieb Ville Syrjälä:
> > On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
> >> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
> >> to the global state if it does not exist already. Adding a new plane
> >> state will results in overhead for the plane during the atomic-commit
> >> step.
> >>
> >> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
> >> important that the CRTC has an enabled primary plane after the commit.
> >> This can be a newly enabled plane or an already enabled plane. So if a
> >> plane is not part of the commit, use the plane's existing state. The new
> >> helper drm_atomic_get_next_plane_state() returns the correct instance.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> Cc: Jocelyn Falempe <jfalempe@redhat.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Maxime Ripard <mripard@kernel.org>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Cc: dri-devel@lists.freedesktop.org
> >> ---
> >>   drivers/gpu/drm/drm_atomic_helper.c |  4 +---
> >>   include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
> >>   2 files changed, 21 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 98cc3137c062..463d4f3fa443 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
> >>   
> >>   			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> >>   				continue;
> >> -			plane_state = drm_atomic_get_plane_state(state, plane);
> >> -			if (IS_ERR(plane_state))
> >> -				return PTR_ERR(plane_state);
> >> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
> >>   			if (plane_state->fb && plane_state->crtc) {
> > 
> > Hmm. Why this is even looking at these. If the plane is in the crtc's
> > plane_mask then surely plane_state->crtc must already point to this
> > crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
> > if one is set then surely the other one must be as well or we'd
> > return an error at some point somewhere?
> 
> Yeah, the crtc test is done for keeping consistency. Other places also 
> sometimes validate that these fields don't disagree. I'll remove the 
> crtc test in the next version. The fb test is the important one.

What I'm asking how can you have crtc!=NULL && fb==NULL at all here?
Some other plane state check function (can't remember which one
specifically) should have rejected that. So either you're checking
for impossible things, or there is a bug somewhere else. 

So to me it looks like it should be enough to just check the plane_mask
and not even bother looking at the plane state at all.


> 
> > 
> >>   				has_primary_plane = true;
> >>   				break;
> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >> index 10b1990bc1f6..4006f2d459e3 100644
> >> --- a/include/drm/drm_atomic.h
> >> +++ b/include/drm/drm_atomic.h
> >> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
> >>   	return state->planes[drm_plane_index(plane)].new_state;
> >>   }
> >>   
> >> +/**
> >> + * drm_atomic_get_next_plane_state - get plane state after atomic commit
> >> + * @state: global atomic state object
> >> + * @plane: plane to grab
> >> + *
> >> + * This function returns the plane state that the given plane will have
> >> + * after the atomic commit. This will be the new plane state if the plane
> >> + * is part of the global atomic state, or the current state otherwise.
> >> + */
> >> +static inline struct drm_plane_state *
> >> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
> >> +				struct drm_plane *plane)
> >> +{
> >> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> >> +
> >> +	if (plane_state)
> >> +		return plane_state;
> >> +	return plane->state;
> > 
> > You're not holding the appropriate lock so that does not look safe. Even
> > if you know somewhere, somehow this sort of thing to be safe I don't
> > think it's a good idea to promote this with any kind of official
> > function because someone will shoot themselves in the foot with it.
> 
> OK. It'll be inlined with locking added.
> 
> Best regards
> Thomas
> 
> > 
> >> +}
> >> +
> >>   /**
> >>    * drm_atomic_get_existing_connector_state - get connector state, if it exists
> >>    * @state: global atomic state object
> >> -- 
> >> 2.37.3
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 12:12:09PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote:
> > Hi,
> > 
> > thanks for reviewing.
> > 
> > Am 29.09.22 um 21:03 schrieb Ville Syrjälä:
> > > On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
> > >> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
> > >> to the global state if it does not exist already. Adding a new plane
> > >> state will results in overhead for the plane during the atomic-commit
> > >> step.
> > >>
> > >> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
> > >> important that the CRTC has an enabled primary plane after the commit.
> > >> This can be a newly enabled plane or an already enabled plane. So if a
> > >> plane is not part of the commit, use the plane's existing state. The new
> > >> helper drm_atomic_get_next_plane_state() returns the correct instance.
> > >>
> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > >> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
> > >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > >> Cc: Jocelyn Falempe <jfalempe@redhat.com>
> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> Cc: Maxime Ripard <mripard@kernel.org>
> > >> Cc: David Airlie <airlied@linux.ie>
> > >> Cc: Daniel Vetter <daniel@ffwll.ch>
> > >> Cc: dri-devel@lists.freedesktop.org
> > >> ---
> > >>   drivers/gpu/drm/drm_atomic_helper.c |  4 +---
> > >>   include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
> > >>   2 files changed, 21 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > >> index 98cc3137c062..463d4f3fa443 100644
> > >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > >> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
> > >>   
> > >>   			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> > >>   				continue;
> > >> -			plane_state = drm_atomic_get_plane_state(state, plane);
> > >> -			if (IS_ERR(plane_state))
> > >> -				return PTR_ERR(plane_state);
> > >> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
> > >>   			if (plane_state->fb && plane_state->crtc) {
> > > 
> > > Hmm. Why this is even looking at these. If the plane is in the crtc's
> > > plane_mask then surely plane_state->crtc must already point to this
> > > crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
> > > if one is set then surely the other one must be as well or we'd
> > > return an error at some point somewhere?
> > 
> > Yeah, the crtc test is done for keeping consistency. Other places also 
> > sometimes validate that these fields don't disagree. I'll remove the 
> > crtc test in the next version. The fb test is the important one.
> 
> What I'm asking how can you have crtc!=NULL && fb==NULL at all here?
> Some other plane state check function (can't remember which one
> specifically) should have rejected that. So either you're checking
> for impossible things, or there is a bug somewhere else. 

Oh and btw, fb != NULL doesn't guarantee the plane is actually
visible (could have been fully clipped), if that is what you're
trying to check here.

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5599 bytes --]

Hi

Am 30.09.22 um 11:12 schrieb Ville Syrjälä:
> On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote:
>> Hi,
>>
>> thanks for reviewing.
>>
>> Am 29.09.22 um 21:03 schrieb Ville Syrjälä:
>>> On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
>>>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
>>>> to the global state if it does not exist already. Adding a new plane
>>>> state will results in overhead for the plane during the atomic-commit
>>>> step.
>>>>
>>>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
>>>> important that the CRTC has an enabled primary plane after the commit.
>>>> This can be a newly enabled plane or an already enabled plane. So if a
>>>> plane is not part of the commit, use the plane's existing state. The new
>>>> helper drm_atomic_get_next_plane_state() returns the correct instance.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Cc: Jocelyn Falempe <jfalempe@redhat.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>> Cc: David Airlie <airlied@linux.ie>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> ---
>>>>    drivers/gpu/drm/drm_atomic_helper.c |  4 +---
>>>>    include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
>>>>    2 files changed, 21 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 98cc3137c062..463d4f3fa443 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
>>>>    
>>>>    			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>>>>    				continue;
>>>> -			plane_state = drm_atomic_get_plane_state(state, plane);
>>>> -			if (IS_ERR(plane_state))
>>>> -				return PTR_ERR(plane_state);
>>>> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
>>>>    			if (plane_state->fb && plane_state->crtc) {
>>>
>>> Hmm. Why this is even looking at these. If the plane is in the crtc's
>>> plane_mask then surely plane_state->crtc must already point to this
>>> crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
>>> if one is set then surely the other one must be as well or we'd
>>> return an error at some point somewhere?
>>
>> Yeah, the crtc test is done for keeping consistency. Other places also
>> sometimes validate that these fields don't disagree. I'll remove the
>> crtc test in the next version. The fb test is the important one.
> 
> What I'm asking how can you have crtc!=NULL && fb==NULL at all here?
> Some other plane state check function (can't remember which one
> specifically) should have rejected that. So either you're checking
> for impossible things, or there is a bug somewhere else.
> 
> So to me it looks like it should be enough to just check the plane_mask
> and not even bother looking at the plane state at all.

Indeed. Somehow I confused plane_mask with 'mask of all planes possible'.

Best regards
Thomas

> 
> 
>>
>>>
>>>>    				has_primary_plane = true;
>>>>    				break;
>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>>> index 10b1990bc1f6..4006f2d459e3 100644
>>>> --- a/include/drm/drm_atomic.h
>>>> +++ b/include/drm/drm_atomic.h
>>>> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
>>>>    	return state->planes[drm_plane_index(plane)].new_state;
>>>>    }
>>>>    
>>>> +/**
>>>> + * drm_atomic_get_next_plane_state - get plane state after atomic commit
>>>> + * @state: global atomic state object
>>>> + * @plane: plane to grab
>>>> + *
>>>> + * This function returns the plane state that the given plane will have
>>>> + * after the atomic commit. This will be the new plane state if the plane
>>>> + * is part of the global atomic state, or the current state otherwise.
>>>> + */
>>>> +static inline struct drm_plane_state *
>>>> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
>>>> +				struct drm_plane *plane)
>>>> +{
>>>> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>>>> +
>>>> +	if (plane_state)
>>>> +		return plane_state;
>>>> +	return plane->state;
>>>
>>> You're not holding the appropriate lock so that does not look safe. Even
>>> if you know somewhere, somehow this sort of thing to be safe I don't
>>> think it's a good idea to promote this with any kind of official
>>> function because someone will shoot themselves in the foot with it.
>>
>> OK. It'll be inlined with locking added.
>>
>> Best regards
>> Thomas
>>
>>>
>>>> +}
>>>> +
>>>>    /**
>>>>     * drm_atomic_get_existing_connector_state - get connector state, if it exists
>>>>     * @state: global atomic state object
>>>> -- 
>>>> 2.37.3
>>>
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3723 bytes --]

Hi

Am 30.09.22 um 12:26 schrieb Ville Syrjälä:
> On Fri, Sep 30, 2022 at 12:12:09PM +0300, Ville Syrjälä wrote:
>> On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote:
>>> Hi,
>>>
>>> thanks for reviewing.
>>>
>>> Am 29.09.22 um 21:03 schrieb Ville Syrjälä:
>>>> On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
>>>>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
>>>>> to the global state if it does not exist already. Adding a new plane
>>>>> state will results in overhead for the plane during the atomic-commit
>>>>> step.
>>>>>
>>>>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
>>>>> important that the CRTC has an enabled primary plane after the commit.
>>>>> This can be a newly enabled plane or an already enabled plane. So if a
>>>>> plane is not part of the commit, use the plane's existing state. The new
>>>>> helper drm_atomic_get_next_plane_state() returns the correct instance.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Cc: Jocelyn Falempe <jfalempe@redhat.com>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>> Cc: David Airlie <airlied@linux.ie>
>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>> ---
>>>>>    drivers/gpu/drm/drm_atomic_helper.c |  4 +---
>>>>>    include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
>>>>>    2 files changed, 21 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> index 98cc3137c062..463d4f3fa443 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
>>>>>    
>>>>>    			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>>>>>    				continue;
>>>>> -			plane_state = drm_atomic_get_plane_state(state, plane);
>>>>> -			if (IS_ERR(plane_state))
>>>>> -				return PTR_ERR(plane_state);
>>>>> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
>>>>>    			if (plane_state->fb && plane_state->crtc) {
>>>>
>>>> Hmm. Why this is even looking at these. If the plane is in the crtc's
>>>> plane_mask then surely plane_state->crtc must already point to this
>>>> crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
>>>> if one is set then surely the other one must be as well or we'd
>>>> return an error at some point somewhere?
>>>
>>> Yeah, the crtc test is done for keeping consistency. Other places also
>>> sometimes validate that these fields don't disagree. I'll remove the
>>> crtc test in the next version. The fb test is the important one.
>>
>> What I'm asking how can you have crtc!=NULL && fb==NULL at all here?
>> Some other plane state check function (can't remember which one
>> specifically) should have rejected that. So either you're checking
>> for impossible things, or there is a bug somewhere else.
> 
> Oh and btw, fb != NULL doesn't guarantee the plane is actually
> visible (could have been fully clipped), if that is what you're
> trying to check here.
> 

No, it's really just about having a primary plane enabled on the CRTC.

Best regards
Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 12:36:45PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 30.09.22 um 12:26 schrieb Ville Syrjälä:
> > On Fri, Sep 30, 2022 at 12:12:09PM +0300, Ville Syrjälä wrote:
> >> On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote:
> >>> Hi,
> >>>
> >>> thanks for reviewing.
> >>>
> >>> Am 29.09.22 um 21:03 schrieb Ville Syrjälä:
> >>>> On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
> >>>>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
> >>>>> to the global state if it does not exist already. Adding a new plane
> >>>>> state will results in overhead for the plane during the atomic-commit
> >>>>> step.
> >>>>>
> >>>>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
> >>>>> important that the CRTC has an enabled primary plane after the commit.
> >>>>> This can be a newly enabled plane or an already enabled plane. So if a
> >>>>> plane is not part of the commit, use the plane's existing state. The new
> >>>>> helper drm_atomic_get_next_plane_state() returns the correct instance.
> >>>>>
> >>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Cc: Jocelyn Falempe <jfalempe@redhat.com>
> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> Cc: Maxime Ripard <mripard@kernel.org>
> >>>>> Cc: David Airlie <airlied@linux.ie>
> >>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
> >>>>> Cc: dri-devel@lists.freedesktop.org
> >>>>> ---
> >>>>>    drivers/gpu/drm/drm_atomic_helper.c |  4 +---
> >>>>>    include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
> >>>>>    2 files changed, 21 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>> index 98cc3137c062..463d4f3fa443 100644
> >>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
> >>>>>    
> >>>>>    			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> >>>>>    				continue;
> >>>>> -			plane_state = drm_atomic_get_plane_state(state, plane);
> >>>>> -			if (IS_ERR(plane_state))
> >>>>> -				return PTR_ERR(plane_state);
> >>>>> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
> >>>>>    			if (plane_state->fb && plane_state->crtc) {
> >>>>
> >>>> Hmm. Why this is even looking at these. If the plane is in the crtc's
> >>>> plane_mask then surely plane_state->crtc must already point to this
> >>>> crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
> >>>> if one is set then surely the other one must be as well or we'd
> >>>> return an error at some point somewhere?
> >>>
> >>> Yeah, the crtc test is done for keeping consistency. Other places also
> >>> sometimes validate that these fields don't disagree. I'll remove the
> >>> crtc test in the next version. The fb test is the important one.
> >>
> >> What I'm asking how can you have crtc!=NULL && fb==NULL at all here?
> >> Some other plane state check function (can't remember which one
> >> specifically) should have rejected that. So either you're checking
> >> for impossible things, or there is a bug somewhere else.
> > 
> > Oh and btw, fb != NULL doesn't guarantee the plane is actually
> > visible (could have been fully clipped), if that is what you're
> > trying to check here.
> > 
> 
> No, it's really just about having a primary plane enabled on the CRTC.

Not sure what you mean by enabled. Usually a fully clipped plane
will actually be disabled in hardware.

-- 
Ville Syrjälä
Intel