Re: [PATCH v2 08/16] drm/exynos: add host_ops callback for platform drivers

[Andrzej Hajda <a.hajda@samsung.com>]

W dniu 15.09.2020 o 20:02, Michael Tretter pisze:
> On Tue, 15 Sep 2020 19:07:59 +0200, Andrzej Hajda wrote:
>> W dniu 11.09.2020 o 15:54, Michael Tretter pisze:
>>> Platform drivers need to be aware if a mipi dsi device attaches or
>>> detaches. Add host_ops to the driver_data for attach and detach
>>> callbacks and move the i80 mode selection and the hotplug handling into
>>> the callback, because these depend on the drm driver.
>>>
>>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>>> ---
>>> v2:
>>> - new patch
>>> ---
>>>    drivers/gpu/drm/exynos/exynos_drm_dsi.c | 64 ++++++++++++++++++++-----
>>>    1 file changed, 53 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index 1a15ae71205d..684a2fbef08a 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -239,6 +239,12 @@ struct exynos_dsi_transfer {
>>>    #define DSIM_STATE_CMD_LPM		BIT(2)
>>>    #define DSIM_STATE_VIDOUT_AVAILABLE	BIT(3)
>>>    
>>> +struct exynos_dsi;
>>> +struct exynos_dsi_host_ops {
>>> +	int (*attach)(struct device *dev, struct mipi_dsi_device *device);
>>> +	int (*detach)(struct device *dev, struct mipi_dsi_device *device);
>>> +};
>>> +
>>>    enum exynos_reg_offset {
>>>    	EXYNOS_REG_OFS,
>>>    	EXYNOS5433_REG_OFS
>>> @@ -254,6 +260,7 @@ struct exynos_dsi_driver_data {
>>>    	unsigned int wait_for_reset;
>>>    	unsigned int num_bits_resol;
>>>    	const unsigned int *reg_values;
>>> +	const struct exynos_dsi_host_ops *host_ops;
>>>    };
>>>    
>>>    struct exynos_dsi {
>>> @@ -467,6 +474,41 @@ static const unsigned int exynos5433_reg_values[] = {
>>>    	[PHYTIMING_HS_TRAIL] = 0x0c,
>>>    };
>>>    
>>> +static int __exynos_dsi_host_attach(struct device *dev,
>>> +				    struct mipi_dsi_device *device)
>>> +{
>>> +	struct exynos_dsi *dsi = dev_get_drvdata(dev);
>>> +	struct drm_device *drm = dsi->encoder.dev;
>>
>> As I wrote yesterday drm device was guaranteed to be present here only
>> if mipi dsi host registration was performed in component bind. In case
>> it is performed in probe it will be always NULL, and the code does not
>> make sense.
> Correct, but if the driver is used as a drm bridge, there won't be an encoder
> until bridge_attach. Mipi dsi devices defer their probe until the mipi dsi
> host is available. If I move the mipi dsi host registration into
> bridge_attach, the driver does not know if the next device is another bridge
> or a panel during bridge_attach, but the driver cannot successfully return
> from bridge_attach, because then the drm driver expects a connector.


Hmm, I am not sure why do you think driver expects connector on return 
of briddge_attach.


>
> I guess that the encoder should be initialized before registering the mipi dsi
> host during probe instead of bind.


But you should have already drm dev on encoder init which in probe is 
unavailable.



> The probe won't be rolled back on
> PROBE_DEFER during bind and the encoder will be available in host_attach.
> When splitting the driver into the exynos platform specific code and the more
> generic code, there won't be an encoder during host_attach in the generic
> code, but the host_ops callback could (and will) use the platform specific
> encoder, which is available before bridge_attach.
>
> Does this make sense to you?


Nope :) But maybe I am missing sth.

Generally I see two ways (which I have already described in different 
e-mail, in different words):

A. Static - we wait for every part of display stack to be probed, then 
create drm_dev - typical approach, but slow (deferred probe causes late 
drm creation), and racy - only(?) component framework and DSI bus have 
possibility to signal driver unbind, so we can react on it properly.

B. Dynamic - drm framework requires only crtcs and encoders to be 
attached to drm on init, connectors, and hidden parts (drm_bridges, 
drm_panels) can be created/destroyed and attached/detached at any time 
(almost), so lets take advantage of it - create drm_dev ASAP and attach 
other parts when they become available, the only issue is that there is 
no generic way to be notified when given parts becomes available - in 
interesting area only mipi devices have such notifications via attach 
callbacks.


So either we convert exynos_dsi to A either we continue B approach. In 
second case we should assure mipi_dsi host is created if drm_dev is 
available.


Regards

Andrzej



>
> Michael
>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>
>>> +	struct exynos_drm_crtc *crtc;
>>> +
>>> +	mutex_lock(&drm->mode_config.mutex);
>>> +	crtc = exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD);
>>> +	crtc->i80_mode = !(device->mode_flags & MIPI_DSI_MODE_VIDEO);
>>> +	mutex_unlock(&drm->mode_config.mutex);
>>> +
>>> +	if (drm->mode_config.poll_enabled)
>>> +		drm_kms_helper_hotplug_event(drm);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int __exynos_dsi_host_detach(struct device *dev,
>>> +				     struct mipi_dsi_device *device)
>>> +{
>>> +	struct exynos_dsi *dsi = dev_get_drvdata(dev);
>>> +	struct drm_device *drm = dsi->encoder.dev;
>>> +
>>> +	if (drm->mode_config.poll_enabled)
>>> +		drm_kms_helper_hotplug_event(drm);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct exynos_dsi_host_ops exynos_dsi_host_ops = {
>>> +	.attach = __exynos_dsi_host_attach,
>>> +	.detach = __exynos_dsi_host_detach,
>>> +};
>>> +
>>>    static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
>>>    	.reg_ofs = EXYNOS_REG_OFS,
>>>    	.plltmr_reg = 0x50,
>>> @@ -477,6 +519,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
>>>    	.wait_for_reset = 1,
>>>    	.num_bits_resol = 11,
>>>    	.reg_values = reg_values,
>>> +	.host_ops = &exynos_dsi_host_ops,
>>>    };
>>>    
>>>    static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
>>> @@ -489,6 +532,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
>>>    	.wait_for_reset = 1,
>>>    	.num_bits_resol = 11,
>>>    	.reg_values = reg_values,
>>> +	.host_ops = &exynos_dsi_host_ops,
>>>    };
>>>    
>>>    static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
>>> @@ -499,6 +543,7 @@ static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
>>>    	.wait_for_reset = 1,
>>>    	.num_bits_resol = 11,
>>>    	.reg_values = reg_values,
>>> +	.host_ops = &exynos_dsi_host_ops,
>>>    };
>>>    
>>>    static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
>>> @@ -510,6 +555,7 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
>>>    	.wait_for_reset = 0,
>>>    	.num_bits_resol = 12,
>>>    	.reg_values = exynos5433_reg_values,
>>> +	.host_ops = &exynos_dsi_host_ops,
>>>    };
>>>    
>>>    static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
>>> @@ -521,6 +567,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
>>>    	.wait_for_reset = 1,
>>>    	.num_bits_resol = 12,
>>>    	.reg_values = exynos5422_reg_values,
>>> +	.host_ops = &exynos_dsi_host_ops,
>>>    };
>>>    
>>>    static const struct of_device_id exynos_dsi_of_match[] = {
>>> @@ -1551,8 +1598,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>    				  struct mipi_dsi_device *device)
>>>    {
>>>    	struct exynos_dsi *dsi = host_to_dsi(host);
>>> +	const struct exynos_dsi_host_ops *ops = dsi->driver_data->host_ops;
>>>    	struct drm_encoder *encoder = &dsi->encoder;
>>> -	struct drm_device *drm = encoder->dev;
>>>    	struct drm_bridge *out_bridge;
>>>    
>>>    	out_bridge  = of_drm_find_bridge(device->dev.of_node);
>>> @@ -1590,18 +1637,12 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>    			return ret;
>>>    	}
>>>    
>>> -	mutex_lock(&drm->mode_config.mutex);
>>> -
>>>    	dsi->lanes = device->lanes;
>>>    	dsi->format = device->format;
>>>    	dsi->mode_flags = device->mode_flags;
>>> -	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
>>> -			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
>>>    
>>> -	mutex_unlock(&drm->mode_config.mutex);
>>> -
>>> -	if (drm->mode_config.poll_enabled)
>>> -		drm_kms_helper_hotplug_event(drm);
>>> +	if (ops && ops->attach)
>>> +		ops->attach(dsi->dsi_host.dev, device);
>>>    
>>>    	return 0;
>>>    }
>>> @@ -1610,6 +1651,7 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>    				  struct mipi_dsi_device *device)
>>>    {
>>>    	struct exynos_dsi *dsi = host_to_dsi(host);
>>> +	const struct exynos_dsi_host_ops *ops = dsi->driver_data->host_ops;
>>>    	struct drm_device *drm = dsi->encoder.dev;
>>>    
>>>    	if (dsi->panel) {
>>> @@ -1625,8 +1667,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>    		INIT_LIST_HEAD(&dsi->bridge_chain);
>>>    	}
>>>    
>>> -	if (drm->mode_config.poll_enabled)
>>> -		drm_kms_helper_hotplug_event(drm);
>>> +	if (ops && ops->detach)
>>> +		ops->detach(dsi->dsi_host.dev, device);
>>>    
>>>    	exynos_dsi_unregister_te_irq(dsi);
>>>    
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/v1/url?k=80c78954-dd15920c-80c6021b-0cc47a31c8b4-c39fad41cb70b194&q=1&e=b51d6682-ba72-48c0-b0c9-013866ba39ab&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel