On Thu, Dec 13, 2018 at 10:51:03AM -0800, Jeykumar Sankaran wrote: > Bail out KMS hw init on display initialization failures with > proper error logging. > > changes in v3: > - introduced in the series > > Signed-off-by: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 39 +++++++++++++++++++++++---------- > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 685686e..39c8549 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -405,33 +405,36 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms, > } > } > > -static void _dpu_kms_initialize_dsi(struct drm_device *dev, > +static int _dpu_kms_initialize_dsi(struct drm_device *dev, > struct msm_drm_private *priv, > struct dpu_kms *dpu_kms) > { > struct drm_encoder *encoder = NULL; > - int i, rc; > + int i, rc = 0; > + > + if (!(priv->dsi[0] || priv->dsi[1])) It'd be nice to not have to support both blocks if only one is hooked up. Perhaps move the TODO below above this check. > + return rc; > > /*TODO: Support two independent DSI connectors */ > encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI); > - if (IS_ERR_OR_NULL(encoder)) { > + if (IS_ERR(encoder)) { > DPU_ERROR("encoder init failed for dsi display\n"); > - return; > + return PTR_ERR(encoder); > } > > for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { > - if (!priv->dsi[i]) { > - DPU_DEBUG("invalid msm_dsi for ctrl %d\n", i); > - return; > - } > + if (!priv->dsi[i]) I don't think this is possible given the check at the beginning of the function > + continue; > > rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder); > if (rc) { > DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n", > i, rc); > - continue; > + return rc; > } > } > + > + return rc; nit: You can keep rc uninitialized above and just return 0 here. > } > > /** > @@ -442,16 +445,24 @@ static void _dpu_kms_initialize_dsi(struct drm_device *dev, > * @dpu_kms: Pointer to dpu kms structure > * Returns: Zero on success > */ > -static void _dpu_kms_setup_displays(struct drm_device *dev, > +static int _dpu_kms_setup_displays(struct drm_device *dev, > struct msm_drm_private *priv, > struct dpu_kms *dpu_kms) > { > - _dpu_kms_initialize_dsi(dev, priv, dpu_kms); > + int rc = 0; nit: No need to initialize to 0 > + > + rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms); > + if (rc) { > + DPU_ERROR("failed to initialize dsi, rc = %d\n", rc); > + return rc; > + } > > /** > * Extend this function to initialize other > * types of displays > */ > + > + return rc; > } > > static void _dpu_kms_drm_obj_destroy(struct dpu_kms *dpu_kms) > @@ -517,7 +528,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > * Create encoder and query display drivers to create > * bridges and connectors > */ > - _dpu_kms_setup_displays(dev, priv, dpu_kms); > + ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); > + if (ret) { > + DPU_ERROR("failed to setup display, rc = %d\n", ret); > + goto fail; > + } > > max_crtc_count = min(catalog->mixer_count, > (u32)dev->mode_config.num_encoder); > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > > _______________________________________________ > Freedreno mailing list > Freedreno@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/freedreno -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel