Re: [PATCH] drm/tilcdc: If CRTC is enabled at init phase, disable it

[Jyri Sarha <jsarha@ti.com>]

On 04/11/17 10:01, Laurent Pinchart wrote:
> 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@ti.com>
>> Reported-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
>> ---
>> 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 ?
> 

Oh, it crossed my mind that there may be a proper call back for this
purpose, but I forgot it while implementing this. I'll check if the
reset callback suits the purpose.

>> +		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. 
> 

If there is, I have not figured that out yet. The main reset bit toggle
does not reset do a full reset but, for instance leaves raster on, and
probably pretty much all other registers intact too.

The driver currently toggles the main reset in CRTC enable, before
setting the mode, which in turn overwrites pretty much every LCDC
register (there are not many). Unfortunately that does not appear to be
enough if the raster was on before the CRTC enable.

>>  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);
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel