On Mon, Nov 28, 2016 at 10:46 PM, Laurent Pinchart
<laurent.pinch...@ideasonboard.com> wrote:
> Hi John,
>
> Thank you for the patch.
>
> On Monday 28 Nov 2016 21:04:40 John Stultz wrote:
>> I was recently seeing issues with EDID probing, where
>> the logic to wait for the EDID read bit to be set by the
>> IRQ wasn't happening and the code would time out and fail.
>>
>> Digging deeper, I found this was due to the fact that
>> IRQs were disabled as we were running in IRQ context from
>> the HPD signal.
>>
>> Thus this patch changes the logic to handle the HPD signal
>> via a work_struct so we can be out of irq context.
>>
>> With this patch, the EDID probing on hotplug does not time
>> out.
>>
>> Cc: David Airlie <airl...@linux.ie>
>> Cc: Archit Taneja <arch...@codeaurora.org>
>> Cc: Wolfram Sang <wsa+rene...@sang-engineering.com>
>> Cc: Lars-Peter Clausen <l...@metafoo.de>
>> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>> Cc: dri-de...@lists.freedesktop.org
>> Signed-off-by: John Stultz <john.stu...@linaro.org>
>> ---
>> v2: Reworked to properly fix the issue rather then
>> just delaying for 200ms
>>
>> drivers/gpu/drm/bridge/adv7511/adv7511.h | 2 ++
>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++++-
>> 2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 992d76c..2a1e722 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -317,6 +317,8 @@ struct adv7511 {
>> bool edid_read;
>>
>> wait_queue_head_t wq;
>> + struct work_struct irq_work;
>> +
>
> I'd call this hpd_work.
>
>> struct drm_bridge bridge;
>> struct drm_connector connector;
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b38e743
>> 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -402,6 +402,14 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
>> return false;
>> }
>>
>> +static void adv7511_irq_work(struct work_struct *work)
>> +{
>> + struct adv7511 *adv7511 = container_of(work, struct adv7511,
> irq_work);
>> +
>> + drm_helper_hpd_irq_event(adv7511->connector.dev);
>> +}
>> +
>> +
>
> One blank line is enough.
>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Thanks so much for the review! I've made your suggested changes!
-john
