[PATCH] drm/atomic: Handling the case when setting old crtc for plane

[PATCH] drm/atomic: Handling the case when setting old crtc for plane

[Satendra Singh Thakur]

In the func drm_atomic_set_crtc_for_plane, with the current code,
if crtc of the plane_state and crtc passed as argument to the func
are same, entire func will executed in vein.
It will get state of crtc and clear and set the bits in plane_mask.
All these steps are not required for same old crtc.
Ideally, we should do nothing in this case, this patch handles the same,
and causes the program to return without doing anything in such scenario.

Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
Cc: Madhur Verma <madhur.verma@samsung.com>
Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
---
 drivers/gpu/drm/drm_atomic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..5bd3365 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1421,7 +1421,9 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
 {
 	struct drm_plane *plane = plane_state->plane;
 	struct drm_crtc_state *crtc_state;
-
+	/* Nothing to do for same crtc*/
+	if (plane_state->crtc == crtc)
+		return 0;
 	if (plane_state->crtc) {
 		crtc_state = drm_atomic_get_crtc_state(plane_state->state,
 						       plane_state->crtc);
-- 
2.7.4

Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane

[Daniel Vetter]

On Thu, May 03, 2018 at 11:19:32AM +0530, Satendra Singh Thakur wrote:
> In the func drm_atomic_set_crtc_for_plane, with the current code,
> if crtc of the plane_state and crtc passed as argument to the func
> are same, entire func will executed in vein.
> It will get state of crtc and clear and set the bits in plane_mask.
> All these steps are not required for same old crtc.
> Ideally, we should do nothing in this case, this patch handles the same,
> and causes the program to return without doing anything in such scenario.
> 
> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> Cc: Madhur Verma <madhur.verma@samsung.com>
> Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42..5bd3365 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1421,7 +1421,9 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>  {
>  	struct drm_plane *plane = plane_state->plane;
>  	struct drm_crtc_state *crtc_state;
> -
> +	/* Nothing to do for same crtc*/
> +	if (plane_state->crtc == crtc)
> +		return 0;

I didn't do this (both here and in the set_crtc_for_connector functions)
because the overhead is probably way down in the noise compared to the
overall atomic commit machinery. Do you really see this as a hotpath?
-Daniel

>  	if (plane_state->crtc) {
>  		crtc_state = drm_atomic_get_crtc_state(plane_state->state,
>  						       plane_state->crtc);
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@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-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane

[Daniel Vetter]

On Thu, May 03, 2018 at 11:19:32AM +0530, Satendra Singh Thakur wrote:
> In the func drm_atomic_set_crtc_for_plane, with the current code,
> if crtc of the plane_state and crtc passed as argument to the func
> are same, entire func will executed in vein.
> It will get state of crtc and clear and set the bits in plane_mask.
> All these steps are not required for same old crtc.
> Ideally, we should do nothing in this case, this patch handles the same,
> and causes the program to return without doing anything in such scenario.
> 
> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> Cc: Madhur Verma <madhur.verma@samsung.com>
> Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42..5bd3365 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1421,7 +1421,9 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>  {
>  	struct drm_plane *plane = plane_state->plane;
>  	struct drm_crtc_state *crtc_state;
> -
> +	/* Nothing to do for same crtc*/
> +	if (plane_state->crtc == crtc)
> +		return 0;

I didn't do this (both here and in the set_crtc_for_connector functions)
because the overhead is probably way down in the noise compared to the
overall atomic commit machinery. Do you really see this as a hotpath?
-Daniel

>  	if (plane_state->crtc) {
>  		crtc_state = drm_atomic_get_crtc_state(plane_state->state,
>  						       plane_state->crtc);
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane

[Ville Syrjälä]

On Thu, May 03, 2018 at 11:24:59AM +0200, Daniel Vetter wrote:
> On Thu, May 03, 2018 at 11:19:32AM +0530, Satendra Singh Thakur wrote:
> > In the func drm_atomic_set_crtc_for_plane, with the current code,
> > if crtc of the plane_state and crtc passed as argument to the func
> > are same, entire func will executed in vein.
> > It will get state of crtc and clear and set the bits in plane_mask.
> > All these steps are not required for same old crtc.
> > Ideally, we should do nothing in this case, this patch handles the same,
> > and causes the program to return without doing anything in such scenario.
> > 
> > Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> > Cc: Madhur Verma <madhur.verma@samsung.com>
> > Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42..5bd3365 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1421,7 +1421,9 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> >  {
> >  	struct drm_plane *plane = plane_state->plane;
> >  	struct drm_crtc_state *crtc_state;
> > -
> > +	/* Nothing to do for same crtc*/
> > +	if (plane_state->crtc == crtc)
> > +		return 0;
> 
> I didn't do this (both here and in the set_crtc_for_connector functions)
> because the overhead is probably way down in the noise compared to the
> overall atomic commit machinery. Do you really see this as a hotpath?

drm_atomic_set_crtc_for_connector() actually has this since commit
e2d800a3ce1b ("drm: Avoid connector reference imbalance on error path")

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane

[Ville Syrjälä]

On Thu, May 03, 2018 at 11:24:59AM +0200, Daniel Vetter wrote:
> On Thu, May 03, 2018 at 11:19:32AM +0530, Satendra Singh Thakur wrote:
> > In the func drm_atomic_set_crtc_for_plane, with the current code,
> > if crtc of the plane_state and crtc passed as argument to the func
> > are same, entire func will executed in vein.
> > It will get state of crtc and clear and set the bits in plane_mask.
> > All these steps are not required for same old crtc.
> > Ideally, we should do nothing in this case, this patch handles the same,
> > and causes the program to return without doing anything in such scenario.
> > 
> > Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> > Cc: Madhur Verma <madhur.verma@samsung.com>
> > Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42..5bd3365 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1421,7 +1421,9 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> >  {
> >  	struct drm_plane *plane = plane_state->plane;
> >  	struct drm_crtc_state *crtc_state;
> > -
> > +	/* Nothing to do for same crtc*/
> > +	if (plane_state->crtc == crtc)
> > +		return 0;
> 
> I didn't do this (both here and in the set_crtc_for_connector functions)
> because the overhead is probably way down in the noise compared to the
> overall atomic commit machinery. Do you really see this as a hotpath?

drm_atomic_set_crtc_for_connector() actually has this since commit
e2d800a3ce1b ("drm: Avoid connector reference imbalance on error path")

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane

[Daniel Vetter]

On Thu, May 03, 2018 at 01:55:03PM +0300, Ville Syrjälä wrote:
> On Thu, May 03, 2018 at 11:24:59AM +0200, Daniel Vetter wrote:
> > On Thu, May 03, 2018 at 11:19:32AM +0530, Satendra Singh Thakur wrote:
> > > In the func drm_atomic_set_crtc_for_plane, with the current code,
> > > if crtc of the plane_state and crtc passed as argument to the func
> > > are same, entire func will executed in vein.
> > > It will get state of crtc and clear and set the bits in plane_mask.
> > > All these steps are not required for same old crtc.
> > > Ideally, we should do nothing in this case, this patch handles the same,
> > > and causes the program to return without doing anything in such scenario.
> > > 
> > > Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> > > Cc: Madhur Verma <madhur.verma@samsung.com>
> > > Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42..5bd3365 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1421,7 +1421,9 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> > >  {
> > >  	struct drm_plane *plane = plane_state->plane;
> > >  	struct drm_crtc_state *crtc_state;
> > > -
> > > +	/* Nothing to do for same crtc*/
> > > +	if (plane_state->crtc == crtc)
> > > +		return 0;
> > 
> > I didn't do this (both here and in the set_crtc_for_connector functions)
> > because the overhead is probably way down in the noise compared to the
> > overall atomic commit machinery. Do you really see this as a hotpath?
> 
> drm_atomic_set_crtc_for_connector() actually has this since commit
> e2d800a3ce1b ("drm: Avoid connector reference imbalance on error path")

Ok, applied for consistency. Thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane

[Daniel Vetter]

On Thu, May 03, 2018 at 01:55:03PM +0300, Ville Syrjälä wrote:
> On Thu, May 03, 2018 at 11:24:59AM +0200, Daniel Vetter wrote:
> > On Thu, May 03, 2018 at 11:19:32AM +0530, Satendra Singh Thakur wrote:
> > > In the func drm_atomic_set_crtc_for_plane, with the current code,
> > > if crtc of the plane_state and crtc passed as argument to the func
> > > are same, entire func will executed in vein.
> > > It will get state of crtc and clear and set the bits in plane_mask.
> > > All these steps are not required for same old crtc.
> > > Ideally, we should do nothing in this case, this patch handles the same,
> > > and causes the program to return without doing anything in such scenario.
> > > 
> > > Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> > > Cc: Madhur Verma <madhur.verma@samsung.com>
> > > Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42..5bd3365 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1421,7 +1421,9 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> > >  {
> > >  	struct drm_plane *plane = plane_state->plane;
> > >  	struct drm_crtc_state *crtc_state;
> > > -
> > > +	/* Nothing to do for same crtc*/
> > > +	if (plane_state->crtc == crtc)
> > > +		return 0;
> > 
> > I didn't do this (both here and in the set_crtc_for_connector functions)
> > because the overhead is probably way down in the noise compared to the
> > overall atomic commit machinery. Do you really see this as a hotpath?
> 
> drm_atomic_set_crtc_for_connector() actually has this since commit
> e2d800a3ce1b ("drm: Avoid connector reference imbalance on error path")

Ok, applied for consistency. Thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch