| Message ID | 20230408082014.235425-1-nikita@trvn.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Implement wait_hpd_asserted | expand |
Hi, On Sat, Apr 8, 2023 at 1:20 AM Nikita Travkin <nikita@trvn.ru> wrote: > > This bridge doesn't actually implement HPD due to it being way too slow > but instead expects the panel driver to wait enough to assume HPD is > asserted. However some panels (such as the generic 'edp-panel') expect > the bridge to deal with the delay and pass maximum delay to the aux > instead. > > In order to support such panels, add a dummy implementation of wait > that would just sleep the maximum delay and assume no failure has > happened. > > Signed-off-by: Nikita Travkin <nikita@trvn.ru> > --- > This was suggested in [1] to make sure DT users can be semantically > correct (not adding no-hpd when the line is actually there) while > still using a hard delay to be faster than waiting the long debounce > time. > > [1] - https://lore.kernel.org/all/CAD=FV=VR7sKsquE25eF7joc7gPApu-vqwduZzjE=wFCoXjMYnQ@mail.gmail.com/ > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 7a748785c545..260cad1fd1da 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -618,6 +618,24 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, > return len; > } > > +static int ti_sn_aux_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us) > +{ > + /* > + * The HPD in this chip is a bit useless (See comment in > + * ti_sn65dsi86_enable_comms) so if our driver is expected to wait > + * for HPD, we just assume it's asserted after the wait_us delay. > + * > + * In case we are asked to wait forever (wait_us=0) take conservative > + * 500ms delay. > + */ > + if (wait_us == 0) > + wait_us = 500000; > + > + usleep_range(wait_us, wait_us + 1000); > + > + return 0; > +} > + > static int ti_sn_aux_probe(struct auxiliary_device *adev, > const struct auxiliary_device_id *id) > { > @@ -627,6 +645,7 @@ static int ti_sn_aux_probe(struct auxiliary_device *adev, > pdata->aux.name = "ti-sn65dsi86-aux"; > pdata->aux.dev = &adev->dev; > pdata->aux.transfer = ti_sn_aux_transfer; > + pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; This looks reasonable to me, but I think you only want this implementation if the "no-hpd" property _isn't_ present. In other words: if (!of_property_read_bool(np, "no-hpd")) pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; Essentially: * If "no-hpd" is present in ti-sn65dsi86 then we'll assume that HPD is handled by the panel driver via a GPIO or a "no-hpd" there (which will cause the panel driver to wait the maximum duration). * If "no-hpd" isn't present in ti-sn65dsi86 then HPD is actually hooked up and thus the panel driver _won't_ handle it. Does that seem right? Presumably this should be explained by comments. -Doug
Doug Anderson писал(а) 13.04.2023 01:22: > Hi, > > On Sat, Apr 8, 2023 at 1:20 AM Nikita Travkin <nikita@trvn.ru> wrote: >> >> This bridge doesn't actually implement HPD due to it being way too slow >> but instead expects the panel driver to wait enough to assume HPD is >> asserted. However some panels (such as the generic 'edp-panel') expect >> the bridge to deal with the delay and pass maximum delay to the aux >> instead. >> >> In order to support such panels, add a dummy implementation of wait >> that would just sleep the maximum delay and assume no failure has >> happened. >> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >> --- >> This was suggested in [1] to make sure DT users can be semantically >> correct (not adding no-hpd when the line is actually there) while >> still using a hard delay to be faster than waiting the long debounce >> time. >> >> [1] - https://lore.kernel.org/all/CAD=FV=VR7sKsquE25eF7joc7gPApu-vqwduZzjE=wFCoXjMYnQ@mail.gmail.com/ >> --- >> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c >> index 7a748785c545..260cad1fd1da 100644 >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c >> @@ -618,6 +618,24 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, >> return len; >> } >> >> +static int ti_sn_aux_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us) >> +{ >> + /* >> + * The HPD in this chip is a bit useless (See comment in >> + * ti_sn65dsi86_enable_comms) so if our driver is expected to wait >> + * for HPD, we just assume it's asserted after the wait_us delay. >> + * >> + * In case we are asked to wait forever (wait_us=0) take conservative >> + * 500ms delay. >> + */ >> + if (wait_us == 0) >> + wait_us = 500000; >> + >> + usleep_range(wait_us, wait_us + 1000); >> + >> + return 0; >> +} >> + >> static int ti_sn_aux_probe(struct auxiliary_device *adev, >> const struct auxiliary_device_id *id) >> { >> @@ -627,6 +645,7 @@ static int ti_sn_aux_probe(struct auxiliary_device *adev, >> pdata->aux.name = "ti-sn65dsi86-aux"; >> pdata->aux.dev = &adev->dev; >> pdata->aux.transfer = ti_sn_aux_transfer; >> + pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; > > This looks reasonable to me, but I think you only want this > implementation if the "no-hpd" property _isn't_ present. In other > words: > > if (!of_property_read_bool(np, "no-hpd")) > pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; > > Essentially: > > * If "no-hpd" is present in ti-sn65dsi86 then we'll assume that HPD is > handled by the panel driver via a GPIO or a "no-hpd" there (which will > cause the panel driver to wait the maximum duration). > > * If "no-hpd" isn't present in ti-sn65dsi86 then HPD is actually > hooked up and thus the panel driver _won't_ handle it. > > Does that seem right? Presumably this should be explained by comments. > This does sound reasonable indeed, I didn't think to add it conditionally because, looking at the current users of wait_hpd_asserted, they will first try the "no-hpd" paths and will only call the bridge when they think it's on the bridge to wait. Thus, if DT is modeled properly - Panel has no-hpd or a gpio, wait_hpd_asserted will never be called anyway. Other bridges seem to also unconditionally enable the method. For this to be a trouble, a panel driver has to be "broken" with some form of calling wait_hpd_asserted despite knowing the HPD line is not hooked up... So I feel like guarding the wait_hpd_asserted for no-hpd users should not actually change much, but if you think I should add the check anyway, please let me know. Thanks for taking a look! Nikita > -Doug
Hi, On Wed, Apr 12, 2023 at 9:19 PM Nikita Travkin <nikita@trvn.ru> wrote: > > Doug Anderson писал(а) 13.04.2023 01:22: > > Hi, > > > > On Sat, Apr 8, 2023 at 1:20 AM Nikita Travkin <nikita@trvn.ru> wrote: > >> > >> This bridge doesn't actually implement HPD due to it being way too slow > >> but instead expects the panel driver to wait enough to assume HPD is > >> asserted. However some panels (such as the generic 'edp-panel') expect > >> the bridge to deal with the delay and pass maximum delay to the aux > >> instead. > >> > >> In order to support such panels, add a dummy implementation of wait > >> that would just sleep the maximum delay and assume no failure has > >> happened. > >> > >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> > >> --- > >> This was suggested in [1] to make sure DT users can be semantically > >> correct (not adding no-hpd when the line is actually there) while > >> still using a hard delay to be faster than waiting the long debounce > >> time. > >> > >> [1] - https://lore.kernel.org/all/CAD=FV=VR7sKsquE25eF7joc7gPApu-vqwduZzjE=wFCoXjMYnQ@mail.gmail.com/ > >> --- > >> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 19 +++++++++++++++++++ > >> 1 file changed, 19 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > >> index 7a748785c545..260cad1fd1da 100644 > >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > >> @@ -618,6 +618,24 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, > >> return len; > >> } > >> > >> +static int ti_sn_aux_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us) > >> +{ > >> + /* > >> + * The HPD in this chip is a bit useless (See comment in > >> + * ti_sn65dsi86_enable_comms) so if our driver is expected to wait > >> + * for HPD, we just assume it's asserted after the wait_us delay. > >> + * > >> + * In case we are asked to wait forever (wait_us=0) take conservative > >> + * 500ms delay. > >> + */ > >> + if (wait_us == 0) > >> + wait_us = 500000; > >> + > >> + usleep_range(wait_us, wait_us + 1000); > >> + > >> + return 0; > >> +} > >> + > >> static int ti_sn_aux_probe(struct auxiliary_device *adev, > >> const struct auxiliary_device_id *id) > >> { > >> @@ -627,6 +645,7 @@ static int ti_sn_aux_probe(struct auxiliary_device *adev, > >> pdata->aux.name = "ti-sn65dsi86-aux"; > >> pdata->aux.dev = &adev->dev; > >> pdata->aux.transfer = ti_sn_aux_transfer; > >> + pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; > > > > This looks reasonable to me, but I think you only want this > > implementation if the "no-hpd" property _isn't_ present. In other > > words: > > > > if (!of_property_read_bool(np, "no-hpd")) > > pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; > > > > Essentially: > > > > * If "no-hpd" is present in ti-sn65dsi86 then we'll assume that HPD is > > handled by the panel driver via a GPIO or a "no-hpd" there (which will > > cause the panel driver to wait the maximum duration). > > > > * If "no-hpd" isn't present in ti-sn65dsi86 then HPD is actually > > hooked up and thus the panel driver _won't_ handle it. > > > > Does that seem right? Presumably this should be explained by comments. > > > > This does sound reasonable indeed, I didn't think to add it > conditionally because, looking at the current users of > wait_hpd_asserted, they will first try the "no-hpd" paths > and will only call the bridge when they think it's on the > bridge to wait. > > Thus, if DT is modeled properly - Panel has no-hpd or a gpio, > wait_hpd_asserted will never be called anyway. Other bridges > seem to also unconditionally enable the method. > > For this to be a trouble, a panel driver has to be "broken" > with some form of calling wait_hpd_asserted despite knowing > the HPD line is not hooked up... > > So I feel like guarding the wait_hpd_asserted for no-hpd > users should not actually change much, but if you think > I should add the check anyway, please let me know. Ah, true, it shouldn't actually matter. I guess I still like it slightly better with the extra check but not enough that I'll insist on it. Thus: Reviewed-by: Douglas Anderson <dianders@chromium.org> I can commit this to drm-misc-next, but I'll plan to wait ~1 week to see if anyone else has any comments about it. -Doug
Hi, On Thu, Apr 13, 2023 at 12:10 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Apr 12, 2023 at 9:19 PM Nikita Travkin <nikita@trvn.ru> wrote: > > > > Doug Anderson писал(а) 13.04.2023 01:22: > > > Hi, > > > > > > On Sat, Apr 8, 2023 at 1:20 AM Nikita Travkin <nikita@trvn.ru> wrote: > > >> > > >> This bridge doesn't actually implement HPD due to it being way too slow > > >> but instead expects the panel driver to wait enough to assume HPD is > > >> asserted. However some panels (such as the generic 'edp-panel') expect > > >> the bridge to deal with the delay and pass maximum delay to the aux > > >> instead. > > >> > > >> In order to support such panels, add a dummy implementation of wait > > >> that would just sleep the maximum delay and assume no failure has > > >> happened. > > >> > > >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> > > >> --- > > >> This was suggested in [1] to make sure DT users can be semantically > > >> correct (not adding no-hpd when the line is actually there) while > > >> still using a hard delay to be faster than waiting the long debounce > > >> time. > > >> > > >> [1] - https://lore.kernel.org/all/CAD=FV=VR7sKsquE25eF7joc7gPApu-vqwduZzjE=wFCoXjMYnQ@mail.gmail.com/ > > >> --- > > >> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 19 +++++++++++++++++++ > > >> 1 file changed, 19 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > >> index 7a748785c545..260cad1fd1da 100644 > > >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > >> @@ -618,6 +618,24 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, > > >> return len; > > >> } > > >> > > >> +static int ti_sn_aux_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us) > > >> +{ > > >> + /* > > >> + * The HPD in this chip is a bit useless (See comment in > > >> + * ti_sn65dsi86_enable_comms) so if our driver is expected to wait > > >> + * for HPD, we just assume it's asserted after the wait_us delay. > > >> + * > > >> + * In case we are asked to wait forever (wait_us=0) take conservative > > >> + * 500ms delay. > > >> + */ > > >> + if (wait_us == 0) > > >> + wait_us = 500000; > > >> + > > >> + usleep_range(wait_us, wait_us + 1000); > > >> + > > >> + return 0; > > >> +} > > >> + > > >> static int ti_sn_aux_probe(struct auxiliary_device *adev, > > >> const struct auxiliary_device_id *id) > > >> { > > >> @@ -627,6 +645,7 @@ static int ti_sn_aux_probe(struct auxiliary_device *adev, > > >> pdata->aux.name = "ti-sn65dsi86-aux"; > > >> pdata->aux.dev = &adev->dev; > > >> pdata->aux.transfer = ti_sn_aux_transfer; > > >> + pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; > > > > > > This looks reasonable to me, but I think you only want this > > > implementation if the "no-hpd" property _isn't_ present. In other > > > words: > > > > > > if (!of_property_read_bool(np, "no-hpd")) > > > pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; > > > > > > Essentially: > > > > > > * If "no-hpd" is present in ti-sn65dsi86 then we'll assume that HPD is > > > handled by the panel driver via a GPIO or a "no-hpd" there (which will > > > cause the panel driver to wait the maximum duration). > > > > > > * If "no-hpd" isn't present in ti-sn65dsi86 then HPD is actually > > > hooked up and thus the panel driver _won't_ handle it. > > > > > > Does that seem right? Presumably this should be explained by comments. > > > > > > > This does sound reasonable indeed, I didn't think to add it > > conditionally because, looking at the current users of > > wait_hpd_asserted, they will first try the "no-hpd" paths > > and will only call the bridge when they think it's on the > > bridge to wait. > > > > Thus, if DT is modeled properly - Panel has no-hpd or a gpio, > > wait_hpd_asserted will never be called anyway. Other bridges > > seem to also unconditionally enable the method. > > > > For this to be a trouble, a panel driver has to be "broken" > > with some form of calling wait_hpd_asserted despite knowing > > the HPD line is not hooked up... > > > > So I feel like guarding the wait_hpd_asserted for no-hpd > > users should not actually change much, but if you think > > I should add the check anyway, please let me know. > > Ah, true, it shouldn't actually matter. I guess I still like it > slightly better with the extra check but not enough that I'll insist > on it. Thus: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > I can commit this to drm-misc-next, but I'll plan to wait ~1 week to > see if anyone else has any comments about it. Landed to drm-misc-next: 34c1aeb579dd drm/bridge: ti-sn65dsi86: Implement wait_hpd_asserted
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 7a748785c545..260cad1fd1da 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -618,6 +618,24 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, return len; } +static int ti_sn_aux_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us) +{ + /* + * The HPD in this chip is a bit useless (See comment in + * ti_sn65dsi86_enable_comms) so if our driver is expected to wait + * for HPD, we just assume it's asserted after the wait_us delay. + * + * In case we are asked to wait forever (wait_us=0) take conservative + * 500ms delay. + */ + if (wait_us == 0) + wait_us = 500000; + + usleep_range(wait_us, wait_us + 1000); + + return 0; +} + static int ti_sn_aux_probe(struct auxiliary_device *adev, const struct auxiliary_device_id *id) { @@ -627,6 +645,7 @@ static int ti_sn_aux_probe(struct auxiliary_device *adev, pdata->aux.name = "ti-sn65dsi86-aux"; pdata->aux.dev = &adev->dev; pdata->aux.transfer = ti_sn_aux_transfer; + pdata->aux.wait_hpd_asserted = ti_sn_aux_wait_hpd_asserted; drm_dp_aux_init(&pdata->aux); ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
This bridge doesn't actually implement HPD due to it being way too slow but instead expects the panel driver to wait enough to assume HPD is asserted. However some panels (such as the generic 'edp-panel') expect the bridge to deal with the delay and pass maximum delay to the aux instead. In order to support such panels, add a dummy implementation of wait that would just sleep the maximum delay and assume no failure has happened. Signed-off-by: Nikita Travkin <nikita@trvn.ru> --- This was suggested in [1] to make sure DT users can be semantically correct (not adding no-hpd when the line is actually there) while still using a hard delay to be faster than waiting the long debounce time. [1] - https://lore.kernel.org/all/CAD=FV=VR7sKsquE25eF7joc7gPApu-vqwduZzjE=wFCoXjMYnQ@mail.gmail.com/ --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)