Re: [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection

[Peter Ujfalusi]

On 2017-05-23 12:45, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patch.
> 
> On Monday 15 May 2017 12:03:11 Peter Ujfalusi wrote:
>> If the hpd_gpio is valid, use interrupt handler to react to HPD changes.
>> In case the hpd_gpio is not valid, try to enable hpd detection on the
>> encoder if it supports it.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
>> ---
>>  drivers/gpu/drm/omapdrm/displays/connector-hdmi.c | 104 +++++++++++++++++++
>>  1 file changed, 104 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c index
>> 1ef130641bae..3a90f89ada77 100644
>> --- a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> +++ b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/of.h>
>>  #include <linux/of_gpio.h>
>> +#include <linux/spinlock.h>
> 
> Did you mean linux/mutex.h ?

Oops, yes I mean linux/mutex.h.

>> +static irqreturn_t hdmic_hpd_isr(int irq, void *data)
>> +{
>> +	struct panel_drv_data *ddata = data;
>> +
>> +	mutex_lock(&ddata->hpd_lock);
>> +	if (ddata->hpd_enabled && ddata->hpd_cb) {
>> +		enum drm_connector_status status;
>> +
>> +		if (hdmic_detect(&ddata->dssdev))
>> +			status = connector_status_connected;
>> +		else
>> +			status = connector_status_disconnected;
>> +
>> +		ddata->hpd_cb(ddata->hpd_cb_data, status);
>> +	}
>> +	mutex_unlock(&ddata->hpd_lock);
> 
> Shouldn't ddata->hpd_cb() be called without the mutex held ? I don't think 
> core code should rely on callers to handle locking in this case.

the mutex is for protecting the internal data of the driver (hpd_cb
mainly). W/o keeping the mutex there might be a chance that we are going
to get an unregister call (unlikely, but in theory it can happen) which
would NULL out the hpd_cb, if we save the hpd_cb and data while holding
the mux, it is possible that the  omap DRM driver was removed already
causing other type of crash.

> 
> 
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int hdmic_probe_of(struct platform_device *pdev)
>>  {
>>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> @@ -249,11 +342,22 @@ static int hdmic_probe(struct platform_device *pdev)
>>  	if (r)
>>  		return r;
>>
>> +	mutex_init(&ddata->hpd_lock);
>> +
>>  	if (gpio_is_valid(ddata->hpd_gpio)) {
>>  		r = devm_gpio_request_one(&pdev->dev, ddata->hpd_gpio,
>>  				GPIOF_DIR_IN, "hdmi_hpd");
>>  		if (r)
>>  			goto err_reg;
>> +
>> +		r = devm_request_threaded_irq(&pdev->dev,
>> +				gpio_to_irq(ddata->hpd_gpio),
> 
> What is hpd_gpio is valid but has no IRQ support ?
> 
>> +				NULL, hdmic_hpd_isr,
>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
>> +				IRQF_ONESHOT,
>> +				"hdmic hpd", ddata);
>> +		if (r)
>> +			goto err_reg;
>>  	}
>>
>>  	ddata->vm = hdmic_default_vm;
> 

- Péter