LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] drm/atomic: Handling the case when setting old crtc for plane
       [not found] <CGME20180503055008epcas5p2e98aff943c031a90a14df1a85e359d6c@epcas5p2.samsung.com>
@ 2018-05-03  5:49 ` Satendra Singh Thakur
  2018-05-03  9:24   ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Satendra Singh Thakur @ 2018-05-03  5:49 UTC (permalink / raw)
  To: Gustavo Padovan, Maarten Lankhorst, Sean Paul, David Airlie,
	dri-devel, linux-kernel
  Cc: Satendra Singh Thakur, Madhur Verma, Hemanshu Srivastava

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane
  2018-05-03  5:49 ` [PATCH] drm/atomic: Handling the case when setting old crtc for plane Satendra Singh Thakur
@ 2018-05-03  9:24   ` Daniel Vetter
  2018-05-03 10:55     ` Ville Syrjälä
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2018-05-03  9:24 UTC (permalink / raw)
  To: Satendra Singh Thakur
  Cc: Gustavo Padovan, Maarten Lankhorst, Sean Paul, David Airlie,
	dri-devel, linux-kernel, Hemanshu Srivastava, Madhur Verma

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane
  2018-05-03  9:24   ` Daniel Vetter
@ 2018-05-03 10:55     ` Ville Syrjälä
  2018-05-03 13:33       ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Ville Syrjälä @ 2018-05-03 10:55 UTC (permalink / raw)
  To: Satendra Singh Thakur, Gustavo Padovan, Maarten Lankhorst,
	Sean Paul, David Airlie, dri-devel, linux-kernel,
	Hemanshu Srivastava, Madhur Verma

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/atomic: Handling the case when setting old crtc for plane
  2018-05-03 10:55     ` Ville Syrjälä
@ 2018-05-03 13:33       ` Daniel Vetter
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2018-05-03 13:33 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: Satendra Singh Thakur, Gustavo Padovan, Maarten Lankhorst,
	Sean Paul, David Airlie, dri-devel, linux-kernel,
	Hemanshu Srivastava, Madhur Verma

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-05-03 13:33 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20180503055008epcas5p2e98aff943c031a90a14df1a85e359d6c@epcas5p2.samsung.com>
2018-05-03  5:49 ` [PATCH] drm/atomic: Handling the case when setting old crtc for plane Satendra Singh Thakur
2018-05-03  9:24   ` Daniel Vetter
2018-05-03 10:55     ` Ville Syrjälä
2018-05-03 13:33       ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).