Messages in this thread | | | Date | Tue, 21 Jun 2022 21:59:05 +0300 | Subject | Re: [v3 3/5] drm/bridge: add psr support during panel bridge enable & disable sequence | From | Dmitry Baryshkov <> |
| |
On 21/06/2022 13:53, Vinod Polimera wrote: > This change avoids panel prepare/unprepare based on self-refresh > state. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > --- > drivers/gpu/drm/bridge/panel.c | 102 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 98 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 59a3496..6b09ae0 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -41,6 +41,40 @@ static int panel_bridge_connector_get_modes(struct drm_connector *connector) > return drm_panel_get_modes(panel_bridge->panel, connector); > } > > +static struct drm_crtc *bridge_drm_get_old_connector_crtc(struct drm_encoder *encoder, > + struct drm_atomic_state *state) > +{ > + struct drm_connector *connector; > + struct drm_connector_state *conn_state; > + > + connector = drm_atomic_get_old_connector_for_encoder(state, encoder); > + if (!connector) > + return NULL; > + > + conn_state = drm_atomic_get_old_connector_state(state, connector); > + if (!conn_state) > + return NULL; > + > + return conn_state->crtc; > +} > + > +static struct drm_crtc *bridge_drm_get_new_connector_crtc(struct drm_encoder *encoder, > + struct drm_atomic_state *state) > +{ > + struct drm_connector *connector; > + struct drm_connector_state *conn_state; > + > + connector = drm_atomic_get_new_connector_for_encoder(state, encoder); > + if (!connector) > + return NULL; > + > + conn_state = drm_atomic_get_new_connector_state(state, connector); > + if (!conn_state) > + return NULL; > + > + return conn_state->crtc; > +}
As I wrote earlier, this should become generic drm helpers.
> + > static const struct drm_connector_helper_funcs > panel_bridge_connector_helper_funcs = { > .get_modes = panel_bridge_connector_get_modes, > @@ -108,30 +142,90 @@ static void panel_bridge_detach(struct drm_bridge *bridge) > drm_connector_cleanup(connector); > } > > -static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge) > +static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state)
This must be a part of the previous patch?
> { > struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + struct drm_atomic_state *old_state = old_bridge_state->base.state; > + struct drm_encoder *encoder = bridge->encoder; > + struct drm_crtc *crtc; > + struct drm_crtc_state *old_crtc_state; > + > + crtc = bridge_drm_get_new_connector_crtc(encoder, old_state); > + if (!crtc) > + return;
Why? And why do you ask for the new crtc from the old state?
> + > + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc); > + > + /* Don't touch the panel if we're coming back from self refresh state */ > + if (old_crtc_state && old_crtc_state->self_refresh_active) > + return; > > drm_panel_prepare(panel_bridge->panel); > } > > -static void panel_bridge_atomic_enable(struct drm_bridge *bridge) > +static void panel_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > { > struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + struct drm_atomic_state *old_state = old_bridge_state->base.state; > + struct drm_encoder *encoder = bridge->encoder; > + struct drm_crtc *crtc; > + struct drm_crtc_state *old_crtc_state; > + > + crtc = bridge_drm_get_new_connector_crtc(encoder, old_state); > + if (!crtc) > + return; > + > + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc); > + > + /* Don't touch the panel if we're coming back from self refresh state */ > + if (old_crtc_state && old_crtc_state->self_refresh_active) > + return; > > drm_panel_enable(panel_bridge->panel); > } > > -static void panel_bridge_atomic_disable(struct drm_bridge *bridge) > +static void panel_bridge_atomic_disable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > { > struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + struct drm_atomic_state *old_state = old_bridge_state->base.state; > + struct drm_encoder *encoder = bridge->encoder; > + struct drm_crtc *crtc; > + struct drm_crtc_state *new_crtc_state; > + > + crtc = bridge_drm_get_old_connector_crtc(encoder, old_state); > + if (!crtc) > + return; > + > + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
This doesn't sound right too.
> + > + /* Don't do a full disable on PSR transitions if new state is self refresh state */ > + if (new_crtc_state && new_crtc_state->self_refresh_active) > + return; > > drm_panel_disable(panel_bridge->panel); > } > > -static void panel_bridge_atomic_post_disable(struct drm_bridge *bridge) > +static void panel_bridge_atomic_post_disable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state) > { > struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + struct drm_atomic_state *old_state = old_bridge_state->base.state; > + struct drm_encoder *encoder = bridge->encoder; > + struct drm_crtc *crtc; > + struct drm_crtc_state *new_crtc_state; > + > + crtc = bridge_drm_get_old_connector_crtc(encoder, old_state); > + if (!crtc) > + return; > + > + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc); > + > + /* Don't do unprepare on PSR transitions if new state is self refresh state */ > + if (new_crtc_state && new_crtc_state->self_refresh_active) > + return; > > drm_panel_unprepare(panel_bridge->panel); > }
-- With best wishes Dmitry
| |