Re: [RFC][PATCH 1/5 v2] drm/bridge: adv7511: Use work_struct to defer hotplug handing to out of irq context

[John Stultz]

On Mon, Nov 28, 2016 at 10:46 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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 <airlied@xxxxxxxx>
>> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
>> Cc: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>> ---
>> 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.pinchart@xxxxxxxxxxxxxxxx>

Thanks so much for the review! I've made your suggested changes!
-john