Hi Jyri, Thank you for the patch. On Monday 10 Apr 2017 14:16:54 Jyri Sarha wrote: > If the LCDC is already enabled (e.g. by the bootloader) at the > initialization phase, disable it before returning from the probe. > > Signed-off-by: Jyri Sarha <jsarha@xxxxxx> > Reported-by: Emiliano Ingrassia <ingrassia@xxxxxxxxxxxxxx> > --- > This patch should fix the same issus as this patch does: > https://lists.freedesktop.org/archives/dri-devel/2017-March/137091.html > > I do not like the above patch duplicating the already existing code > for CRTC disable. Another issue is making the check every time the > CRTC is turned on. > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 21 +++++++++++++++++++++ > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 3 +++ > drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 + > 3 files changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index afd2a7b..540378a 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -566,6 +566,26 @@ void tilcdc_crtc_shutdown(struct drm_crtc *crtc) > tilcdc_crtc_off(crtc, true); > } > > +void tilcdc_crtc_disable_init(struct drm_crtc *crtc) > +{ > + struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); > + struct drm_device *dev = crtc->dev; > + > + /* > + * If the LCDC was already enabled (e.g. by the bootloader) > + * disable the CRTC before finishing the probe. > + */ > + pm_runtime_get_sync(dev->dev); > + if (tilcdc_read(dev, LCDC_RASTER_CTRL_REG) & LCDC_RASTER_ENABLE) { > + tilcdc_crtc->enabled = true; > + tilcdc_crtc_enable_irqs(dev); > + /* To match pm_runtime_put_sync() in tilcdc_crtc_off() */ > + pm_runtime_get_sync(dev->dev); Doesn't all this belong to the CRTC's ->reset() operation, whose role is to create an initial software state that corresponds to the hardware state ? > + tilcdc_crtc_off(crtc, false); > + } > + pm_runtime_put_sync(dev->dev); > +} Is there a way to reset the LCDC completely ? Sure, stopping the CRTC is one step towards reconciling the hardware and software states, but I'm concerned that the driver may hardcode other assumptions regarding the hardware state that don't correspond to what the boot loader may have programmed. > static bool tilcdc_crtc_is_on(struct drm_crtc *crtc) > { > return crtc->state && crtc->state->enable && crtc->state->active; > @@ -1054,6 +1074,7 @@ int tilcdc_crtc_create(struct drm_device *dev) > } > > priv->crtc = crtc; > + > return 0; > > fail: > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c > b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index d7ae5be..7dabe55 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > @@ -415,6 +415,9 @@ static int tilcdc_init(struct drm_driver *ddrv, struct > device *dev) goto init_failed; > > priv->is_registered = true; > + > + tilcdc_crtc_disable_init(priv->crtc); > + > return 0; > > init_failed: > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h > b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index 8caa11b..adde1e4 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h > @@ -179,6 +179,7 @@ void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc > *crtc, int tilcdc_crtc_update_fb(struct drm_crtc *crtc, > struct drm_framebuffer *fb, > struct drm_pending_vblank_event *event); > +void tilcdc_crtc_disable_init(struct drm_crtc *crtc); > > int tilcdc_plane_init(struct drm_device *dev, struct drm_plane *plane); -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
