| Message ID | c4314b22644ed311d246d5aa97b63b3be04212c6.1494347165.git.joabreu@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: > Introduce a new helper function which calls mode_valid() callback > for all bridges in an encoder chain. > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > Cc: Carlos Palminha <palminha@synopsys.com> > Cc: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Dave Airlie <airlied@linux.ie> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Archit Taneja <architt@codeaurora.org> > --- > drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ > include/drm/drm_bridge.h | 2 ++ > 2 files changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 86a7637..dc8cdfe 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, > EXPORT_SYMBOL(drm_bridge_mode_fixup); > > /** > + * drm_bridge_mode_valid - validate the mode against all bridges in the > + * encoder chain. > + * @bridge: bridge control structure > + * @mode: desired mode to be validated > + * > + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder > + * chain, starting from the first bridge to the last. If at least one bridge > + * does not accept the mode the function returns the error code. > + * > + * Note: the bridge passed should be the one closest to the encoder. > + * > + * RETURNS: > + * MODE_OK on success, drm_mode_status Enum error code on failure > + */ > +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, > + const struct drm_display_mode *mode) > +{ > + enum drm_mode_status ret = MODE_OK; > + > + if (!bridge) > + return ret; > + > + if (bridge->funcs->mode_valid) > + ret = bridge->funcs->mode_valid(bridge, mode); > + > + if (ret != MODE_OK) > + return ret; > + > + return drm_bridge_mode_valid(bridge->next, mode); Looks like it should be pretty trivial to avoid the recursion. Am I correct in interpreting this that bridges have some kind of a hand rolled linked list implementation? Reusing the standard linked lists would allow you to use list_for_each() etc. > +} > +EXPORT_SYMBOL(drm_bridge_mode_valid); > + > +/** > * drm_bridge_disable - disables all bridges in the encoder chain > * @bridge: bridge control structure > * > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 00c6c36..8358eb3 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > bool drm_bridge_mode_fixup(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, > + const struct drm_display_mode *mode); > void drm_bridge_disable(struct drm_bridge *bridge); > void drm_bridge_post_disable(struct drm_bridge *bridge); > void drm_bridge_mode_set(struct drm_bridge *bridge, > -- > 1.9.1 >
Hi Ville, On 10-05-2017 14:41, Ville Syrjälä wrote: > On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: >> Introduce a new helper function which calls mode_valid() callback >> for all bridges in an encoder chain. >> >> Signed-off-by: Jose Abreu <joabreu@synopsys.com> >> Cc: Carlos Palminha <palminha@synopsys.com> >> Cc: Alexey Brodkin <abrodkin@synopsys.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Dave Airlie <airlied@linux.ie> >> Cc: Andrzej Hajda <a.hajda@samsung.com> >> Cc: Archit Taneja <architt@codeaurora.org> >> --- >> drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ >> include/drm/drm_bridge.h | 2 ++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >> index 86a7637..dc8cdfe 100644 >> --- a/drivers/gpu/drm/drm_bridge.c >> +++ b/drivers/gpu/drm/drm_bridge.c >> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, >> EXPORT_SYMBOL(drm_bridge_mode_fixup); >> >> /** >> + * drm_bridge_mode_valid - validate the mode against all bridges in the >> + * encoder chain. >> + * @bridge: bridge control structure >> + * @mode: desired mode to be validated >> + * >> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder >> + * chain, starting from the first bridge to the last. If at least one bridge >> + * does not accept the mode the function returns the error code. >> + * >> + * Note: the bridge passed should be the one closest to the encoder. >> + * >> + * RETURNS: >> + * MODE_OK on success, drm_mode_status Enum error code on failure >> + */ >> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, >> + const struct drm_display_mode *mode) >> +{ >> + enum drm_mode_status ret = MODE_OK; >> + >> + if (!bridge) >> + return ret; >> + >> + if (bridge->funcs->mode_valid) >> + ret = bridge->funcs->mode_valid(bridge, mode); >> + >> + if (ret != MODE_OK) >> + return ret; >> + >> + return drm_bridge_mode_valid(bridge->next, mode); > Looks like it should be pretty trivial to avoid the recursion. > > Am I correct in interpreting this that bridges have some kind of > a hand rolled linked list implementation? Reusing the standard > linked lists would allow you to use list_for_each() etc. I reused the drm_bridge_mode_fixup but now I see how its done like that: so that the fixup is propagated in the correct order. As for mode_valid we just need to check if ret != MODE_OK then I think we can use the list_for_each_entry(bridge->list). Best regards, Jose Miguel Abreu > >> +} >> +EXPORT_SYMBOL(drm_bridge_mode_valid); >> + >> +/** >> * drm_bridge_disable - disables all bridges in the encoder chain >> * @bridge: bridge control structure >> * >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >> index 00c6c36..8358eb3 100644 >> --- a/include/drm/drm_bridge.h >> +++ b/include/drm/drm_bridge.h >> @@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >> bool drm_bridge_mode_fixup(struct drm_bridge *bridge, >> const struct drm_display_mode *mode, >> struct drm_display_mode *adjusted_mode); >> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, >> + const struct drm_display_mode *mode); >> void drm_bridge_disable(struct drm_bridge *bridge); >> void drm_bridge_post_disable(struct drm_bridge *bridge); >> void drm_bridge_mode_set(struct drm_bridge *bridge, >> -- >> 1.9.1 >>
Hi Ville, On 10-05-2017 15:01, Jose Abreu wrote: > Hi Ville, > > > On 10-05-2017 14:41, Ville Syrjälä wrote: >> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: >>> Introduce a new helper function which calls mode_valid() callback >>> for all bridges in an encoder chain. >>> >>> Signed-off-by: Jose Abreu <joabreu@synopsys.com> >>> Cc: Carlos Palminha <palminha@synopsys.com> >>> Cc: Alexey Brodkin <abrodkin@synopsys.com> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Dave Airlie <airlied@linux.ie> >>> Cc: Andrzej Hajda <a.hajda@samsung.com> >>> Cc: Archit Taneja <architt@codeaurora.org> >>> --- >>> drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ >>> include/drm/drm_bridge.h | 2 ++ >>> 2 files changed, 35 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >>> index 86a7637..dc8cdfe 100644 >>> --- a/drivers/gpu/drm/drm_bridge.c >>> +++ b/drivers/gpu/drm/drm_bridge.c >>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, >>> EXPORT_SYMBOL(drm_bridge_mode_fixup); >>> >>> /** >>> + * drm_bridge_mode_valid - validate the mode against all bridges in the >>> + * encoder chain. >>> + * @bridge: bridge control structure >>> + * @mode: desired mode to be validated >>> + * >>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder >>> + * chain, starting from the first bridge to the last. If at least one bridge >>> + * does not accept the mode the function returns the error code. >>> + * >>> + * Note: the bridge passed should be the one closest to the encoder. >>> + * >>> + * RETURNS: >>> + * MODE_OK on success, drm_mode_status Enum error code on failure >>> + */ >>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, >>> + const struct drm_display_mode *mode) >>> +{ >>> + enum drm_mode_status ret = MODE_OK; >>> + >>> + if (!bridge) >>> + return ret; >>> + >>> + if (bridge->funcs->mode_valid) >>> + ret = bridge->funcs->mode_valid(bridge, mode); >>> + >>> + if (ret != MODE_OK) >>> + return ret; >>> + >>> + return drm_bridge_mode_valid(bridge->next, mode); >> Looks like it should be pretty trivial to avoid the recursion. >> >> Am I correct in interpreting this that bridges have some kind of >> a hand rolled linked list implementation? Reusing the standard >> linked lists would allow you to use list_for_each() etc. > I reused the drm_bridge_mode_fixup but now I see how its done > like that: so that the fixup is propagated in the correct order. > As for mode_valid we just need to check if ret != MODE_OK then I > think we can use the list_for_each_entry(bridge->list). Oops, I got this wrong sorry. I meant there is a list but its for all the system bridges. This is a "custom" linked list yeah. Best regards, Jose Miguel Abreu > > Best regards, > Jose Miguel Abreu > >>> +} >>> +EXPORT_SYMBOL(drm_bridge_mode_valid); >>> + >>> +/** >>> * drm_bridge_disable - disables all bridges in the encoder chain >>> * @bridge: bridge control structure >>> * >>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >>> index 00c6c36..8358eb3 100644 >>> --- a/include/drm/drm_bridge.h >>> +++ b/include/drm/drm_bridge.h >>> @@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, >>> bool drm_bridge_mode_fixup(struct drm_bridge *bridge, >>> const struct drm_display_mode *mode, >>> struct drm_display_mode *adjusted_mode); >>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, >>> + const struct drm_display_mode *mode); >>> void drm_bridge_disable(struct drm_bridge *bridge); >>> void drm_bridge_post_disable(struct drm_bridge *bridge); >>> void drm_bridge_mode_set(struct drm_bridge *bridge, >>> -- >>> 1.9.1 >>>
On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote: > On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: > > Introduce a new helper function which calls mode_valid() callback > > for all bridges in an encoder chain. > > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > > Cc: Carlos Palminha <palminha@synopsys.com> > > Cc: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Dave Airlie <airlied@linux.ie> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Archit Taneja <architt@codeaurora.org> > > --- > > drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ > > include/drm/drm_bridge.h | 2 ++ > > 2 files changed, 35 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index 86a7637..dc8cdfe 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, > > EXPORT_SYMBOL(drm_bridge_mode_fixup); > > > > /** > > + * drm_bridge_mode_valid - validate the mode against all bridges in the > > + * encoder chain. > > + * @bridge: bridge control structure > > + * @mode: desired mode to be validated > > + * > > + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder > > + * chain, starting from the first bridge to the last. If at least one bridge > > + * does not accept the mode the function returns the error code. > > + * > > + * Note: the bridge passed should be the one closest to the encoder. > > + * > > + * RETURNS: > > + * MODE_OK on success, drm_mode_status Enum error code on failure > > + */ > > +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, > > + const struct drm_display_mode *mode) > > +{ > > + enum drm_mode_status ret = MODE_OK; > > + > > + if (!bridge) > > + return ret; > > + > > + if (bridge->funcs->mode_valid) > > + ret = bridge->funcs->mode_valid(bridge, mode); > > + > > + if (ret != MODE_OK) > > + return ret; > > + > > + return drm_bridge_mode_valid(bridge->next, mode); > > Looks like it should be pretty trivial to avoid the recursion. > > Am I correct in interpreting this that bridges have some kind of > a hand rolled linked list implementation? Reusing the standard > linked lists would allow you to use list_for_each() etc. Yeah it's a hand-rolled list, but current hw also has a bridge nesting depth of 2, so it really doesn't matter. I guess once we have real long chains of bridges we can fix this (and just using list_head sounds like a great idea). -Daniel
Hi Daniel, On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote: > On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote: > > On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: > > > Introduce a new helper function which calls mode_valid() callback > > > for all bridges in an encoder chain. > > > > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > > > Cc: Carlos Palminha <palminha@synopsys.com> > > > Cc: Alexey Brodkin <abrodkin@synopsys.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Dave Airlie <airlied@linux.ie> > > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > > Cc: Archit Taneja <architt@codeaurora.org> > > > --- > > > > > > drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ > > > include/drm/drm_bridge.h | 2 ++ > > > 2 files changed, 35 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > > index 86a7637..dc8cdfe 100644 > > > --- a/drivers/gpu/drm/drm_bridge.c > > > +++ b/drivers/gpu/drm/drm_bridge.c > > > @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge > > > *bridge, > > > > > > EXPORT_SYMBOL(drm_bridge_mode_fixup); > > > > > > /** > > > > > > + * drm_bridge_mode_valid - validate the mode against all bridges in the > > > + * encoder chain. > > > + * @bridge: bridge control structure > > > + * @mode: desired mode to be validated > > > + * > > > + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the > > > encoder > > > + * chain, starting from the first bridge to the last. If at least one > > > bridge + * does not accept the mode the function returns the error > > > code. > > > + * > > > + * Note: the bridge passed should be the one closest to the encoder. > > > + * > > > + * RETURNS: > > > + * MODE_OK on success, drm_mode_status Enum error code on failure > > > + */ > > > +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, > > > + const struct drm_display_mode *mode) > > > +{ > > > + enum drm_mode_status ret = MODE_OK; > > > + > > > + if (!bridge) > > > + return ret; > > > + > > > + if (bridge->funcs->mode_valid) > > > + ret = bridge->funcs->mode_valid(bridge, mode); > > > + > > > + if (ret != MODE_OK) > > > + return ret; > > > + > > > + return drm_bridge_mode_valid(bridge->next, mode); > > > > Looks like it should be pretty trivial to avoid the recursion. > > > > Am I correct in interpreting this that bridges have some kind of > > a hand rolled linked list implementation? Reusing the standard > > linked lists would allow you to use list_for_each() etc. > > Yeah it's a hand-rolled list, but current hw also has a bridge nesting > depth of 2, so it really doesn't matter. I guess once we have real long > chains of bridges we can fix this (and just using list_head sounds like a > great idea). Even if not really needed right now, it's a pretty easy cleanup, if Jose has time to handle it in v3 of this series let's not postpone it ;-)
On 05/12/2017 03:08 PM, Laurent Pinchart wrote: > Hi Daniel, > > On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote: >> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote: >>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: >>>> Introduce a new helper function which calls mode_valid() callback >>>> for all bridges in an encoder chain. >>>> >>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com> >>>> Cc: Carlos Palminha <palminha@synopsys.com> >>>> Cc: Alexey Brodkin <abrodkin@synopsys.com> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Cc: Dave Airlie <airlied@linux.ie> >>>> Cc: Andrzej Hajda <a.hajda@samsung.com> >>>> Cc: Archit Taneja <architt@codeaurora.org> >>>> --- >>>> >>>> drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ >>>> include/drm/drm_bridge.h | 2 ++ >>>> 2 files changed, 35 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >>>> index 86a7637..dc8cdfe 100644 >>>> --- a/drivers/gpu/drm/drm_bridge.c >>>> +++ b/drivers/gpu/drm/drm_bridge.c >>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge >>>> *bridge, >>>> >>>> EXPORT_SYMBOL(drm_bridge_mode_fixup); >>>> >>>> /** >>>> >>>> + * drm_bridge_mode_valid - validate the mode against all bridges in the >>>> + * encoder chain. >>>> + * @bridge: bridge control structure >>>> + * @mode: desired mode to be validated >>>> + * >>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the >>>> encoder >>>> + * chain, starting from the first bridge to the last. If at least one >>>> bridge + * does not accept the mode the function returns the error >>>> code. >>>> + * >>>> + * Note: the bridge passed should be the one closest to the encoder. >>>> + * >>>> + * RETURNS: >>>> + * MODE_OK on success, drm_mode_status Enum error code on failure >>>> + */ >>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, >>>> + const struct drm_display_mode > *mode) >>>> +{ >>>> + enum drm_mode_status ret = MODE_OK; >>>> + >>>> + if (!bridge) >>>> + return ret; >>>> + >>>> + if (bridge->funcs->mode_valid) >>>> + ret = bridge->funcs->mode_valid(bridge, mode); >>>> + >>>> + if (ret != MODE_OK) >>>> + return ret; >>>> + >>>> + return drm_bridge_mode_valid(bridge->next, mode); >>> >>> Looks like it should be pretty trivial to avoid the recursion. >>> >>> Am I correct in interpreting this that bridges have some kind of >>> a hand rolled linked list implementation? Reusing the standard >>> linked lists would allow you to use list_for_each() etc. >> >> Yeah it's a hand-rolled list, but current hw also has a bridge nesting >> depth of 2, so it really doesn't matter. I guess once we have real long >> chains of bridges we can fix this (and just using list_head sounds like a >> great idea). > > Even if not really needed right now, it's a pretty easy cleanup, if Jose has > time to handle it in v3 of this series let's not postpone it ;-) jfyi, some of the bridge functions call the ops from the last bridge in the chain to first, so we'd need to use list_for_each_entry_prev() (or something like that) for them. Archit
Hi Archit, On Friday 12 May 2017 16:20:07 Archit Taneja wrote: > On 05/12/2017 03:08 PM, Laurent Pinchart wrote: > > On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote: > >> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote: > >>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: > >>>> Introduce a new helper function which calls mode_valid() callback > >>>> for all bridges in an encoder chain. > >>>> > >>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com> > >>>> Cc: Carlos Palminha <palminha@synopsys.com> > >>>> Cc: Alexey Brodkin <abrodkin@synopsys.com> > >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>> Cc: Dave Airlie <airlied@linux.ie> > >>>> Cc: Andrzej Hajda <a.hajda@samsung.com> > >>>> Cc: Archit Taneja <architt@codeaurora.org> > >>>> --- > >>>> > >>>> drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ > >>>> include/drm/drm_bridge.h | 2 ++ > >>>> 2 files changed, 35 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_bridge.c > >>>> b/drivers/gpu/drm/drm_bridge.c > >>>> index 86a7637..dc8cdfe 100644 > >>>> --- a/drivers/gpu/drm/drm_bridge.c > >>>> +++ b/drivers/gpu/drm/drm_bridge.c > >>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge > >>>> *bridge, > >>>> > >>>> EXPORT_SYMBOL(drm_bridge_mode_fixup); > >>>> > >>>> /** > >>>> > >>>> + * drm_bridge_mode_valid - validate the mode against all bridges in > >>>> the > >>>> + * encoder chain. > >>>> + * @bridge: bridge control structure > >>>> + * @mode: desired mode to be validated > >>>> + * > >>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the > >>>> encoder > >>>> + * chain, starting from the first bridge to the last. If at least one > >>>> bridge + * does not accept the mode the function returns the error > >>>> code. > >>>> + * > >>>> + * Note: the bridge passed should be the one closest to the encoder. > >>>> + * > >>>> + * RETURNS: > >>>> + * MODE_OK on success, drm_mode_status Enum error code on failure > >>>> + */ > >>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, > >>>> + const struct drm_display_mode > > > > *mode) > > > >>>> +{ > >>>> + enum drm_mode_status ret = MODE_OK; > >>>> + > >>>> + if (!bridge) > >>>> + return ret; > >>>> + > >>>> + if (bridge->funcs->mode_valid) > >>>> + ret = bridge->funcs->mode_valid(bridge, mode); > >>>> + > >>>> + if (ret != MODE_OK) > >>>> + return ret; > >>>> + > >>>> + return drm_bridge_mode_valid(bridge->next, mode); > >>> > >>> Looks like it should be pretty trivial to avoid the recursion. > >>> > >>> Am I correct in interpreting this that bridges have some kind of > >>> a hand rolled linked list implementation? Reusing the standard > >>> linked lists would allow you to use list_for_each() etc. > >> > >> Yeah it's a hand-rolled list, but current hw also has a bridge nesting > >> depth of 2, so it really doesn't matter. I guess once we have real long > >> chains of bridges we can fix this (and just using list_head sounds like a > >> great idea). > > > > Even if not really needed right now, it's a pretty easy cleanup, if Jose > > has time to handle it in v3 of this series let's not postpone it ;-) > > jfyi, some of the bridge functions call the ops from the last bridge in the > chain to first, so we'd need to use list_for_each_entry_prev() (or something > like that) for them. And now that I think about it, for some of the operations (especially enable/disable) I believe that the bridge should be able to decide whether to call the next/previous bridge first or to configure its hardware first. I can image bridges that need the previous bridge in the chain to provide a valid clock before they get started, as well as bridges that need to be started with the incoming video signal stopped.
On 05/12/2017 04:31 PM, Laurent Pinchart wrote: > Hi Archit, > > On Friday 12 May 2017 16:20:07 Archit Taneja wrote: >> On 05/12/2017 03:08 PM, Laurent Pinchart wrote: >>> On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote: >>>> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote: >>>>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: >>>>>> Introduce a new helper function which calls mode_valid() callback >>>>>> for all bridges in an encoder chain. >>>>>> >>>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com> >>>>>> Cc: Carlos Palminha <palminha@synopsys.com> >>>>>> Cc: Alexey Brodkin <abrodkin@synopsys.com> >>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>> Cc: Dave Airlie <airlied@linux.ie> >>>>>> Cc: Andrzej Hajda <a.hajda@samsung.com> >>>>>> Cc: Archit Taneja <architt@codeaurora.org> >>>>>> --- >>>>>> >>>>>> drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ >>>>>> include/drm/drm_bridge.h | 2 ++ >>>>>> 2 files changed, 35 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c >>>>>> b/drivers/gpu/drm/drm_bridge.c >>>>>> index 86a7637..dc8cdfe 100644 >>>>>> --- a/drivers/gpu/drm/drm_bridge.c >>>>>> +++ b/drivers/gpu/drm/drm_bridge.c >>>>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge >>>>>> *bridge, >>>>>> >>>>>> EXPORT_SYMBOL(drm_bridge_mode_fixup); >>>>>> >>>>>> /** >>>>>> >>>>>> + * drm_bridge_mode_valid - validate the mode against all bridges in >>>>>> the >>>>>> + * encoder chain. >>>>>> + * @bridge: bridge control structure >>>>>> + * @mode: desired mode to be validated >>>>>> + * >>>>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the >>>>>> encoder >>>>>> + * chain, starting from the first bridge to the last. If at least one >>>>>> bridge + * does not accept the mode the function returns the error >>>>>> code. >>>>>> + * >>>>>> + * Note: the bridge passed should be the one closest to the encoder. >>>>>> + * >>>>>> + * RETURNS: >>>>>> + * MODE_OK on success, drm_mode_status Enum error code on failure >>>>>> + */ >>>>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, >>>>>> + const struct > drm_display_mode >>> >>> *mode) >>> >>>>>> +{ >>>>>> + enum drm_mode_status ret = MODE_OK; >>>>>> + >>>>>> + if (!bridge) >>>>>> + return ret; >>>>>> + >>>>>> + if (bridge->funcs->mode_valid) >>>>>> + ret = bridge->funcs->mode_valid(bridge, mode); >>>>>> + >>>>>> + if (ret != MODE_OK) >>>>>> + return ret; >>>>>> + >>>>>> + return drm_bridge_mode_valid(bridge->next, mode); >>>>> >>>>> Looks like it should be pretty trivial to avoid the recursion. >>>>> >>>>> Am I correct in interpreting this that bridges have some kind of >>>>> a hand rolled linked list implementation? Reusing the standard >>>>> linked lists would allow you to use list_for_each() etc. >>>> >>>> Yeah it's a hand-rolled list, but current hw also has a bridge nesting >>>> depth of 2, so it really doesn't matter. I guess once we have real long >>>> chains of bridges we can fix this (and just using list_head sounds like a >>>> great idea). >>> >>> Even if not really needed right now, it's a pretty easy cleanup, if Jose >>> has time to handle it in v3 of this series let's not postpone it ;-) >> >> jfyi, some of the bridge functions call the ops from the last bridge in the >> chain to first, so we'd need to use list_for_each_entry_prev() (or something >> like that) for them. > > And now that I think about it, for some of the operations (especially > enable/disable) I believe that the bridge should be able to decide whether to > call the next/previous bridge first or to configure its hardware first. I can > image bridges that need the previous bridge in the chain to provide a valid > clock before they get started, as well as bridges that need to be started with > the incoming video signal stopped. I guess converting into list would be a good start to achieve this. We'd probably need to extend/redo the drm_bridge_attach() API to tweak the order in the which the ops are called. Thanks, Archit
On Fri, May 12, 2017 at 02:01:49PM +0300, Laurent Pinchart wrote: > Hi Archit, > > On Friday 12 May 2017 16:20:07 Archit Taneja wrote: > > On 05/12/2017 03:08 PM, Laurent Pinchart wrote: > > > On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote: > > >> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote: > > >>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote: > > >>>> Introduce a new helper function which calls mode_valid() callback > > >>>> for all bridges in an encoder chain. > > >>>> > > >>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com> > > >>>> Cc: Carlos Palminha <palminha@synopsys.com> > > >>>> Cc: Alexey Brodkin <abrodkin@synopsys.com> > > >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > >>>> Cc: Dave Airlie <airlied@linux.ie> > > >>>> Cc: Andrzej Hajda <a.hajda@samsung.com> > > >>>> Cc: Archit Taneja <architt@codeaurora.org> > > >>>> --- > > >>>> > > >>>> drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ > > >>>> include/drm/drm_bridge.h | 2 ++ > > >>>> 2 files changed, 35 insertions(+) > > >>>> > > >>>> diff --git a/drivers/gpu/drm/drm_bridge.c > > >>>> b/drivers/gpu/drm/drm_bridge.c > > >>>> index 86a7637..dc8cdfe 100644 > > >>>> --- a/drivers/gpu/drm/drm_bridge.c > > >>>> +++ b/drivers/gpu/drm/drm_bridge.c > > >>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge > > >>>> *bridge, > > >>>> > > >>>> EXPORT_SYMBOL(drm_bridge_mode_fixup); > > >>>> > > >>>> /** > > >>>> > > >>>> + * drm_bridge_mode_valid - validate the mode against all bridges in > > >>>> the > > >>>> + * encoder chain. > > >>>> + * @bridge: bridge control structure > > >>>> + * @mode: desired mode to be validated > > >>>> + * > > >>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the > > >>>> encoder > > >>>> + * chain, starting from the first bridge to the last. If at least one > > >>>> bridge + * does not accept the mode the function returns the error > > >>>> code. > > >>>> + * > > >>>> + * Note: the bridge passed should be the one closest to the encoder. > > >>>> + * > > >>>> + * RETURNS: > > >>>> + * MODE_OK on success, drm_mode_status Enum error code on failure > > >>>> + */ > > >>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, > > >>>> + const struct > drm_display_mode > > > > > > *mode) > > > > > >>>> +{ > > >>>> + enum drm_mode_status ret = MODE_OK; > > >>>> + > > >>>> + if (!bridge) > > >>>> + return ret; > > >>>> + > > >>>> + if (bridge->funcs->mode_valid) > > >>>> + ret = bridge->funcs->mode_valid(bridge, mode); > > >>>> + > > >>>> + if (ret != MODE_OK) > > >>>> + return ret; > > >>>> + > > >>>> + return drm_bridge_mode_valid(bridge->next, mode); > > >>> > > >>> Looks like it should be pretty trivial to avoid the recursion. > > >>> > > >>> Am I correct in interpreting this that bridges have some kind of > > >>> a hand rolled linked list implementation? Reusing the standard > > >>> linked lists would allow you to use list_for_each() etc. > > >> > > >> Yeah it's a hand-rolled list, but current hw also has a bridge nesting > > >> depth of 2, so it really doesn't matter. I guess once we have real long > > >> chains of bridges we can fix this (and just using list_head sounds like a > > >> great idea). > > > > > > Even if not really needed right now, it's a pretty easy cleanup, if Jose > > > has time to handle it in v3 of this series let's not postpone it ;-) > > > > jfyi, some of the bridge functions call the ops from the last bridge in the > > chain to first, so we'd need to use list_for_each_entry_prev() (or something > > like that) for them. > > And now that I think about it, for some of the operations (especially > enable/disable) I believe that the bridge should be able to decide whether to > call the next/previous bridge first or to configure its hardware first. I can > image bridges that need the previous bridge in the chain to provide a valid > clock before they get started, as well as bridges that need to be started with > the incoming video signal stopped. That's why we have pre_/post_ hooks ... -Daniel
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 86a7637..dc8cdfe 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_bridge_mode_fixup); /** + * drm_bridge_mode_valid - validate the mode against all bridges in the + * encoder chain. + * @bridge: bridge control structure + * @mode: desired mode to be validated + * + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder + * chain, starting from the first bridge to the last. If at least one bridge + * does not accept the mode the function returns the error code. + * + * Note: the bridge passed should be the one closest to the encoder. + * + * RETURNS: + * MODE_OK on success, drm_mode_status Enum error code on failure + */ +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, + const struct drm_display_mode *mode) +{ + enum drm_mode_status ret = MODE_OK; + + if (!bridge) + return ret; + + if (bridge->funcs->mode_valid) + ret = bridge->funcs->mode_valid(bridge, mode); + + if (ret != MODE_OK) + return ret; + + return drm_bridge_mode_valid(bridge->next, mode); +} +EXPORT_SYMBOL(drm_bridge_mode_valid); + +/** * drm_bridge_disable - disables all bridges in the encoder chain * @bridge: bridge control structure * diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 00c6c36..8358eb3 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bool drm_bridge_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, + const struct drm_display_mode *mode); void drm_bridge_disable(struct drm_bridge *bridge); void drm_bridge_post_disable(struct drm_bridge *bridge); void drm_bridge_mode_set(struct drm_bridge *bridge,
Introduce a new helper function which calls mode_valid() callback for all bridges in an encoder chain. Signed-off-by: Jose Abreu <joabreu@synopsys.com> Cc: Carlos Palminha <palminha@synopsys.com> Cc: Alexey Brodkin <abrodkin@synopsys.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Dave Airlie <airlied@linux.ie> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Archit Taneja <architt@codeaurora.org> --- drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 2 ++ 2 files changed, 35 insertions(+)