[PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
Introduce a new helper function which calls mode_valid() callback for all bridges in an encoder chain. Signed-off-by: Jose Abreu Cc: Carlos Palminha Cc: Alexey Brodkin Cc: Ville Syrjälä Cc: Daniel Vetter Cc: Dave Airlie Cc: Andrzej Hajda Cc: Archit Taneja --- 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); +} +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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 > Cc: Carlos Palminha > Cc: Alexey Brodkin > Cc: Ville Syrjälä > Cc: Daniel Vetter > Cc: Dave Airlie > Cc: Andrzej Hajda > Cc: Archit Taneja > --- > 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 > -- Ville Syrjälä Intel OTC ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 > > Cc: Carlos Palminha > > Cc: Alexey Brodkin > > Cc: Ville Syrjälä > > Cc: Daniel Vetter > > Cc: Dave Airlie > > Cc: Andrzej Hajda > > Cc: Archit Taneja > > --- > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 >> Cc: Carlos Palminha >> Cc: Alexey Brodkin >> Cc: Ville Syrjälä >> Cc: Daniel Vetter >> Cc: Dave Airlie >> Cc: Andrzej Hajda >> Cc: Archit Taneja >> --- >> 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 >> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 >>> Cc: Carlos Palminha >>> Cc: Alexey Brodkin >>> Cc: Ville Syrjälä >>> Cc: Daniel Vetter >>> Cc: Dave Airlie >>> Cc: Andrzej Hajda >>> Cc: Archit Taneja >>> --- >>> 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 >>> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 > > > Cc: Carlos Palminha > > > Cc: Alexey Brodkin > > > Cc: Ville Syrjälä > > > Cc: Daniel Vetter > > > Cc: Dave Airlie > > > Cc: Andrzej Hajda > > > Cc: Archit Taneja > > > --- > > > > > > 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 ;-) -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 Cc: Carlos Palminha Cc: Alexey Brodkin Cc: Ville Syrjälä Cc: Daniel Vetter Cc: Dave Airlie Cc: Andrzej Hajda Cc: Archit Taneja --- 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 -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 > Cc: Carlos Palminha > Cc: Alexey Brodkin > Cc: Ville Syrjälä > Cc: Daniel Vetter > Cc: Dave Airlie > Cc: Andrzej Hajda > Cc: Archit Taneja > --- > > 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. -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 Cc: Carlos Palminha Cc: Alexey Brodkin Cc: Ville Syrjälä Cc: Daniel Vetter Cc: Dave Airlie Cc: Andrzej Hajda Cc: Archit Taneja --- 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 -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()
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 > > Cc: Carlos Palminha > > Cc: Alexey Brodkin > > Cc: Ville Syrjälä > > Cc: Daniel Vetter > > Cc: Dave Airlie > > Cc: Andrzej Hajda > > Cc: Archit Taneja > > --- > > > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel