| Message ID | 1469107503-2491-1-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Philipp, This patch's headline doesn't exactly reflect what the patch actually does - retrieve lvds bus format from imx_crtc_state during encoder mode_set. On Thu, Jul 21, 2016 at 9:25 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > The code in imx_ldb_encoder_mode_set crashes trying to access the > crtc->state->state drm_atomic_state pointer if that was previously > cleared by drm_atomic_helper_swap_state. Nit: Providing a crash log might be good. > Instead of trying to walk all connectors during encoder mode_set to > determine the LVDS bus format from panel or external bridge connector > display info, store it in imx_crtc_state during encoder atomic_check, > where it is already known, and just retrieve it from imx_crtc_state > during encoder mode_set. > > Fixes: 49f98bc4d44a4 ("drm/imx: store internal bus configuration in crtc state") > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/gpu/drm/imx/imx-drm.h | 1 + > drivers/gpu/drm/imx/imx-ldb.c | 22 +++++----------------- > 2 files changed, 6 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h > index bdaa381..5e2f68a 100644 > --- a/drivers/gpu/drm/imx/imx-drm.h > +++ b/drivers/gpu/drm/imx/imx-drm.h > @@ -19,6 +19,7 @@ struct imx_crtc_state { > u32 bus_flags; > int di_hsync_pin; > int di_vsync_pin; > + u32 ldb_bus_format; Pretty much a hack here, I think. Comparing to the other members of imx_crtc_state, ldb_bus_format is less likely to be a member, since crtc just doesn't like to know the LVDS bus format... > }; > > static inline struct imx_crtc_state *to_imx_crtc_state(struct drm_crtc_state *s) > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c > index dc2b420..871dff9 100644 > --- a/drivers/gpu/drm/imx/imx-ldb.c > +++ b/drivers/gpu/drm/imx/imx-ldb.c > @@ -262,7 +262,10 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder, > unsigned long serial_clk; > unsigned long di_clk = mode->clock * 1000; > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder); > - u32 bus_format = imx_ldb_ch->bus_format; > + struct drm_crtc_state *crtc_state = encoder->crtc->state; > + struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); > + u32 bus_format = imx_ldb_ch->bus_format ?: > + imx_crtc_state->ldb_bus_format; Instead of introducing imx_crtc_state->ldb_bus_format to get the lvds bus format, you may get the connector via crtc_state->connector_mask(verify connector->encoder to be the encoder we are talking about). connector_mask is maintained well by the atomic core I assume. Regards, Liu Ying > > if (mode->clock > 170000) { > dev_warn(ldb->dev, > @@ -297,22 +300,6 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder, > ldb->ldb_ctrl &= ~LDB_DI1_VS_POL_ACT_LOW; > } > > - if (!bus_format) { > - struct drm_connector_state *conn_state; > - struct drm_connector *connector; > - int i; > - > - for_each_connector_in_state(encoder->crtc->state->state, > - connector, conn_state, i) { > - struct drm_display_info *di = &connector->display_info; > - > - if (conn_state->crtc == encoder->crtc && > - di->num_bus_formats) { > - bus_format = di->bus_formats[0]; > - break; > - } > - } > - } > imx_ldb_ch_set_bus_format(imx_ldb_ch, bus_format); > } > > @@ -404,6 +391,7 @@ static int imx_ldb_encoder_atomic_check(struct drm_encoder *encoder, > > imx_crtc_state->di_hsync_pin = 2; > imx_crtc_state->di_vsync_pin = 3; > + imx_crtc_state->ldb_bus_format = bus_format; > > return 0; > } > -- > 2.8.1 >
Hi Liu, thank you for your comments. Am Freitag, den 22.07.2016, 12:01 +0800 schrieb Ying Liu: > Hi Philipp, > > This patch's headline doesn't exactly reflect what the patch actually > does - retrieve lvds bus format from imx_crtc_state during encoder > mode_set. Yes. I initially stored ldb_bus_format in imx_ldb_ch, then fixed that, and forgot to change the subject. > On Thu, Jul 21, 2016 at 9:25 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > The code in imx_ldb_encoder_mode_set crashes trying to access the > > crtc->state->state drm_atomic_state pointer if that was previously > > cleared by drm_atomic_helper_swap_state. > > Nit: Providing a crash log might be good. Will do. > > Instead of trying to walk all connectors during encoder mode_set to > > determine the LVDS bus format from panel or external bridge connector > > display info, store it in imx_crtc_state during encoder atomic_check, > > where it is already known, and just retrieve it from imx_crtc_state > > during encoder mode_set. > > > > Fixes: 49f98bc4d44a4 ("drm/imx: store internal bus configuration in crtc state") > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/gpu/drm/imx/imx-drm.h | 1 + > > drivers/gpu/drm/imx/imx-ldb.c | 22 +++++----------------- > > 2 files changed, 6 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h > > index bdaa381..5e2f68a 100644 > > --- a/drivers/gpu/drm/imx/imx-drm.h > > +++ b/drivers/gpu/drm/imx/imx-drm.h > > @@ -19,6 +19,7 @@ struct imx_crtc_state { > > u32 bus_flags; > > int di_hsync_pin; > > int di_vsync_pin; > > + u32 ldb_bus_format; > > Pretty much a hack here, I think. > Comparing to the other members of imx_crtc_state, > ldb_bus_format is less likely to be a member, since > crtc just doesn't like to know the LVDS bus format... I suppose I should keep the connector walk then. I'll send a new version. [...] > Instead of introducing imx_crtc_state->ldb_bus_format to get the > lvds bus format, you may get the connector via > crtc_state->connector_mask(verify connector->encoder to be > the encoder we are talking about). connector_mask is maintained > well by the atomic core I assume. I could also use drm_for_each_connector and just keep the (connector->state->crtc == encoder->crtc) check in the loop. regards Philipp
Hi Philipp, On Fri, Jul 22, 2016 at 4:59 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Hi Liu, > > thank you for your comments. > > Am Freitag, den 22.07.2016, 12:01 +0800 schrieb Ying Liu: >> Hi Philipp, >> >> This patch's headline doesn't exactly reflect what the patch actually >> does - retrieve lvds bus format from imx_crtc_state during encoder >> mode_set. > > Yes. I initially stored ldb_bus_format in imx_ldb_ch, then fixed that, > and forgot to change the subject. > >> On Thu, Jul 21, 2016 at 9:25 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > The code in imx_ldb_encoder_mode_set crashes trying to access the >> > crtc->state->state drm_atomic_state pointer if that was previously >> > cleared by drm_atomic_helper_swap_state. >> >> Nit: Providing a crash log might be good. > > Will do. > >> > Instead of trying to walk all connectors during encoder mode_set to >> > determine the LVDS bus format from panel or external bridge connector >> > display info, store it in imx_crtc_state during encoder atomic_check, >> > where it is already known, and just retrieve it from imx_crtc_state >> > during encoder mode_set. >> > >> > Fixes: 49f98bc4d44a4 ("drm/imx: store internal bus configuration in crtc state") >> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >> > --- >> > drivers/gpu/drm/imx/imx-drm.h | 1 + >> > drivers/gpu/drm/imx/imx-ldb.c | 22 +++++----------------- >> > 2 files changed, 6 insertions(+), 17 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h >> > index bdaa381..5e2f68a 100644 >> > --- a/drivers/gpu/drm/imx/imx-drm.h >> > +++ b/drivers/gpu/drm/imx/imx-drm.h >> > @@ -19,6 +19,7 @@ struct imx_crtc_state { >> > u32 bus_flags; >> > int di_hsync_pin; >> > int di_vsync_pin; >> > + u32 ldb_bus_format; >> >> Pretty much a hack here, I think. >> Comparing to the other members of imx_crtc_state, >> ldb_bus_format is less likely to be a member, since >> crtc just doesn't like to know the LVDS bus format... > > I suppose I should keep the connector walk then. I'll send a new > version. > > [...] >> Instead of introducing imx_crtc_state->ldb_bus_format to get the >> lvds bus format, you may get the connector via >> crtc_state->connector_mask(verify connector->encoder to be >> the encoder we are talking about). connector_mask is maintained >> well by the atomic core I assume. > > I could also use drm_for_each_connector and just keep the > (connector->state->crtc == encoder->crtc) check in the loop. It looks (connector->encoder == encoder) is simpler. Regards, Liu Ying > > regards > Philipp >
diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h index bdaa381..5e2f68a 100644 --- a/drivers/gpu/drm/imx/imx-drm.h +++ b/drivers/gpu/drm/imx/imx-drm.h @@ -19,6 +19,7 @@ struct imx_crtc_state { u32 bus_flags; int di_hsync_pin; int di_vsync_pin; + u32 ldb_bus_format; }; static inline struct imx_crtc_state *to_imx_crtc_state(struct drm_crtc_state *s) diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index dc2b420..871dff9 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -262,7 +262,10 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder, unsigned long serial_clk; unsigned long di_clk = mode->clock * 1000; int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder); - u32 bus_format = imx_ldb_ch->bus_format; + struct drm_crtc_state *crtc_state = encoder->crtc->state; + struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); + u32 bus_format = imx_ldb_ch->bus_format ?: + imx_crtc_state->ldb_bus_format; if (mode->clock > 170000) { dev_warn(ldb->dev, @@ -297,22 +300,6 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder, ldb->ldb_ctrl &= ~LDB_DI1_VS_POL_ACT_LOW; } - if (!bus_format) { - struct drm_connector_state *conn_state; - struct drm_connector *connector; - int i; - - for_each_connector_in_state(encoder->crtc->state->state, - connector, conn_state, i) { - struct drm_display_info *di = &connector->display_info; - - if (conn_state->crtc == encoder->crtc && - di->num_bus_formats) { - bus_format = di->bus_formats[0]; - break; - } - } - } imx_ldb_ch_set_bus_format(imx_ldb_ch, bus_format); } @@ -404,6 +391,7 @@ static int imx_ldb_encoder_atomic_check(struct drm_encoder *encoder, imx_crtc_state->di_hsync_pin = 2; imx_crtc_state->di_vsync_pin = 3; + imx_crtc_state->ldb_bus_format = bus_format; return 0; }
The code in imx_ldb_encoder_mode_set crashes trying to access the crtc->state->state drm_atomic_state pointer if that was previously cleared by drm_atomic_helper_swap_state. Instead of trying to walk all connectors during encoder mode_set to determine the LVDS bus format from panel or external bridge connector display info, store it in imx_crtc_state during encoder atomic_check, where it is already known, and just retrieve it from imx_crtc_state during encoder mode_set. Fixes: 49f98bc4d44a4 ("drm/imx: store internal bus configuration in crtc state") Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/gpu/drm/imx/imx-drm.h | 1 + drivers/gpu/drm/imx/imx-ldb.c | 22 +++++----------------- 2 files changed, 6 insertions(+), 17 deletions(-)