* [PATCH] drm/kms/atomic: Improved the func drm_atomic_set_crtc_for_connector [not found] <CGME20180730061450epcas5p137fd022ec7fda83eeadea2102a62ae4f@epcas5p1.samsung.com> @ 2018-07-30 6:14 ` Satendra Singh Thakur 2018-07-30 8:00 ` Maarten Lankhorst 0 siblings, 1 reply; 3+ messages in thread From: Satendra Singh Thakur @ 2018-07-30 6:14 UTC (permalink / raw) To: Gustavo Padovan, Maarten Lankhorst, Sean Paul, David Airlie, dri-devel, linux-kernel Cc: vineet.j, hemanshu.s, sst2005, Satendra Singh Thakur a. Used drm_atomic_get_crtc_state instead of drm_atomic_get_new_crtc_state. Anyway new_state and state are same, this should make no difference. This change will make the code of this func and the func drm_atomic_set_crtc_for_plane similar. b. Added error checking after this func call c. Currently conn_state->crtc is getting assigned a value at two places We can just reduce this assignment to one Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com> --- drivers/gpu/drm/drm_atomic.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e..907fb2d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1552,16 +1552,16 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, return 0; if (conn_state->crtc) { - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, + crtc_state = drm_atomic_get_crtc_state(conn_state->state, conn_state->crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); crtc_state->connector_mask &= ~(1 << drm_connector_index(conn_state->connector)); drm_connector_put(conn_state->connector); - conn_state->crtc = NULL; } - if (crtc) { crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc); if (IS_ERR(crtc_state)) @@ -1571,7 +1571,6 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, 1 << drm_connector_index(conn_state->connector); drm_connector_get(conn_state->connector); - conn_state->crtc = crtc; DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n", conn_state, crtc->base.id, crtc->name); @@ -1579,7 +1578,7 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, DRM_DEBUG_ATOMIC("Link connector state %p to [NOCRTC]\n", conn_state); } - + conn_state->crtc = crtc; return 0; } EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector); -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/kms/atomic: Improved the func drm_atomic_set_crtc_for_connector 2018-07-30 6:14 ` [PATCH] drm/kms/atomic: Improved the func drm_atomic_set_crtc_for_connector Satendra Singh Thakur @ 2018-07-30 8:00 ` Maarten Lankhorst [not found] ` <CGME20180803115013epcas5p2abd0af8d1ada31e66d5c6ae38ec1a9a8@epcas5p2.samsung.com> 0 siblings, 1 reply; 3+ messages in thread From: Maarten Lankhorst @ 2018-07-30 8:00 UTC (permalink / raw) To: Satendra Singh Thakur, Gustavo Padovan, Sean Paul, David Airlie, dri-devel, linux-kernel Cc: vineet.j, hemanshu.s, sst2005 Op 30-07-18 om 08:14 schreef Satendra Singh Thakur: > a. Used drm_atomic_get_crtc_state instead of drm_atomic_get_new_crtc_state. > Anyway new_state and state are same, this should make no difference. > This change will make the code of this func and the func > drm_atomic_set_crtc_for_plane similar. > b. Added error checking after this func call > c. Currently conn_state->crtc is getting assigned a value at two places > We can just reduce this assignment to one > > Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com> > --- > drivers/gpu/drm/drm_atomic.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 895741e..907fb2d 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1552,16 +1552,16 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, > return 0; > > if (conn_state->crtc) { > - crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, > + crtc_state = drm_atomic_get_crtc_state(conn_state->state, > conn_state->crtc); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); The error checking is not required when using drm_atomic_get_new_crtc_state, because the end of drm_atomic_get_connector_state() will fetch it for us. The other place where conn_state->crtc is assigned is in drm_atomic_set_crtc_for_connector(), which also makes sure we have the crtc_state part of the commit. It would be a bug if we would have a conn_state without conn_state->crtc's state. Did you hit a crash here? > crtc_state->connector_mask &= > ~(1 << drm_connector_index(conn_state->connector)); > > drm_connector_put(conn_state->connector); > - conn_state->crtc = NULL; > } > - > if (crtc) { > crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc); > if (IS_ERR(crtc_state)) > @@ -1571,7 +1571,6 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, > 1 << drm_connector_index(conn_state->connector); > > drm_connector_get(conn_state->connector); > - conn_state->crtc = crtc; > > DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n", > conn_state, crtc->base.id, crtc->name); > @@ -1579,7 +1578,7 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, > DRM_DEBUG_ATOMIC("Link connector state %p to [NOCRTC]\n", > conn_state); > } > - > + conn_state->crtc = crtc; > return 0; > } > EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector); ~Maarten ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <CGME20180803115013epcas5p2abd0af8d1ada31e66d5c6ae38ec1a9a8@epcas5p2.samsung.com>]
* [PATCH v1] drm/kms/atomic: Improved the func drm_atomic_set_crtc_for_connector [not found] ` <CGME20180803115013epcas5p2abd0af8d1ada31e66d5c6ae38ec1a9a8@epcas5p2.samsung.com> @ 2018-08-03 11:48 ` Satendra Singh Thakur 0 siblings, 0 replies; 3+ messages in thread From: Satendra Singh Thakur @ 2018-08-03 11:48 UTC (permalink / raw) To: Gustavo Padovan, Maarten Lankhorst, Sean Paul, David Airlie, dri-devel, linux-kernel Cc: vineet.j, hemanshu.s, sst2005, Satendra Singh Thakur 1. Currently conn_state->crtc is getting assigned a value at two places We can just reduce this assignment to one Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com> --- v1: Hi Mr Maarten, Thanks for the comments. I have modified the patch suitably. Please review. drivers/gpu/drm/drm_atomic.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e..560b01c 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1559,7 +1559,6 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, ~(1 << drm_connector_index(conn_state->connector)); drm_connector_put(conn_state->connector); - conn_state->crtc = NULL; } if (crtc) { @@ -1571,7 +1570,6 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, 1 << drm_connector_index(conn_state->connector); drm_connector_get(conn_state->connector); - conn_state->crtc = crtc; DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n", conn_state, crtc->base.id, crtc->name); @@ -1579,6 +1577,7 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, DRM_DEBUG_ATOMIC("Link connector state %p to [NOCRTC]\n", conn_state); } + conn_state->crtc = crtc; return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-03 11:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20180730061450epcas5p137fd022ec7fda83eeadea2102a62ae4f@epcas5p1.samsung.com>
2018-07-30 6:14 ` [PATCH] drm/kms/atomic: Improved the func drm_atomic_set_crtc_for_connector Satendra Singh Thakur
2018-07-30 8:00 ` Maarten Lankhorst
[not found] ` <CGME20180803115013epcas5p2abd0af8d1ada31e66d5c6ae38ec1a9a8@epcas5p2.samsung.com>
2018-08-03 11:48 ` [PATCH v1] " Satendra Singh Thakur
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).