| Message ID | 20240524093509.127189-2-j-choudhary@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add mode_valid hook for sii902x bridge | expand |
On Fri, May 24, 2024 at 03:05:08PM +0530, Jayesh Choudhary wrote: > Currently, mode_valid hook returns all mode as valid and it is > defined only in drm_connector_helper_funcs. With the introduction of > 'DRM_BRIDGE_ATTACH_NO_CONNECTOR', connector is not initialized in > bridge_attach call for cases when the encoder has this flag enabled. > So add the mode_valid hook in drm_bridge_funcs as well with proper > clock checks for maximum and minimum pixel clock supported by the > bridge. > > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> > --- > drivers/gpu/drm/bridge/sii902x.c | 37 ++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c > index 2fbeda9025bf..bae551e107f9 100644 > --- a/drivers/gpu/drm/bridge/sii902x.c > +++ b/drivers/gpu/drm/bridge/sii902x.c > @@ -163,6 +163,14 @@ > > #define SII902X_AUDIO_PORT_INDEX 3 > > +/* > + * The maximum resolution supported by the HDMI bridge is 1080p@60Hz > + * and 1920x1200 requiring a pixel clock of 165MHz and the minimum > + * resolution supported is 480p@60Hz requiring a pixel clock of 25MHz > + */ > +#define SII902X_MIN_PIXEL_CLOCK_KHZ 25000 > +#define SII902X_MAX_PIXEL_CLOCK_KHZ 165000 > + > struct sii902x { > struct i2c_client *i2c; > struct regmap *regmap; > @@ -310,12 +318,26 @@ static int sii902x_get_modes(struct drm_connector *connector) > return num; > } > > +static enum > +drm_mode_status sii902x_validate(struct sii902x *sii902x, > + const struct drm_display_mode *mode) > +{ > + if (mode->clock < SII902X_MIN_PIXEL_CLOCK_KHZ) > + return MODE_CLOCK_LOW; > + > + if (mode->clock > SII902X_MAX_PIXEL_CLOCK_KHZ) > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} > + > static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector, > struct drm_display_mode *mode) > { > - /* TODO: check mode */ > + struct sii902x *sii902x = connector_to_sii902x(connector); > + const struct drm_display_mode *mod = mode; > > - return MODE_OK; > + return sii902x_validate(sii902x, mod); There is no need to. The drm_bridge_chain_mode_valid() should take care of calling bridge's mode_valid callback and rejecting the mode if it is not accepted. > } > > static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs = { > @@ -504,6 +526,16 @@ static int sii902x_bridge_atomic_check(struct drm_bridge *bridge, > return 0; > } > > +static enum drm_mode_status > +sii902x_bridge_mode_valid(struct drm_bridge *bridge, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + struct sii902x *sii902x = bridge_to_sii902x(bridge); > + > + return sii902x_validate(sii902x, mode); > +} > + > static const struct drm_bridge_funcs sii902x_bridge_funcs = { > .attach = sii902x_bridge_attach, > .mode_set = sii902x_bridge_mode_set, > @@ -516,6 +548,7 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = { > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > .atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts, > .atomic_check = sii902x_bridge_atomic_check, > + .mode_valid = sii902x_bridge_mode_valid, > }; > > static int sii902x_mute(struct sii902x *sii902x, bool mute) > -- > 2.25.1 >
Hello Dmitry, On 24/05/24 15:11, Dmitry Baryshkov wrote: > On Fri, May 24, 2024 at 03:05:08PM +0530, Jayesh Choudhary wrote: >> Currently, mode_valid hook returns all mode as valid and it is >> defined only in drm_connector_helper_funcs. With the introduction of >> 'DRM_BRIDGE_ATTACH_NO_CONNECTOR', connector is not initialized in >> bridge_attach call for cases when the encoder has this flag enabled. >> So add the mode_valid hook in drm_bridge_funcs as well with proper >> clock checks for maximum and minimum pixel clock supported by the >> bridge. >> >> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> [...] >> + >> static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector, >> struct drm_display_mode *mode) >> { >> - /* TODO: check mode */ >> + struct sii902x *sii902x = connector_to_sii902x(connector); >> + const struct drm_display_mode *mod = mode; >> >> - return MODE_OK; >> + return sii902x_validate(sii902x, mod); > > There is no need to. The drm_bridge_chain_mode_valid() should take care > of calling bridge's mode_valid callback and rejecting the mode if it is > not accepted. I need some clarity here. IIRC, if the bridge does initialize the connector in case where the encoder does not attach the bridge with the DRM_BRIDGE_ATTACH_NO_CONNECTOR (DBANC) flag (referring to tidss encoder before we implemented the DBANC feature), then drm_connector_helper_func are called and drm_bridge_funcs are NOT called (atleast from what I have seen in detect hook for cdns-mhdp-8546 driver which is there in both structures). I do not have any platform to test non-DBANC encoders. And I did not want to break any platform that were still using bridge_attach without DBANC flag. That is why I kept mode_valid hook in both structures. Are you implying that if connector_helper_funcs are not there then there will be some sort of fallback to bridge_funcs instead of passthrough for mode_valid check? Because that goes against my previous observations. Thanks, Jayesh
On Fri, May 24, 2024 at 05:54:02PM +0530, Jayesh Choudhary wrote: > Hello Dmitry, > > On 24/05/24 15:11, Dmitry Baryshkov wrote: > > On Fri, May 24, 2024 at 03:05:08PM +0530, Jayesh Choudhary wrote: > > > Currently, mode_valid hook returns all mode as valid and it is > > > defined only in drm_connector_helper_funcs. With the introduction of > > > 'DRM_BRIDGE_ATTACH_NO_CONNECTOR', connector is not initialized in > > > bridge_attach call for cases when the encoder has this flag enabled. > > > So add the mode_valid hook in drm_bridge_funcs as well with proper > > > clock checks for maximum and minimum pixel clock supported by the > > > bridge. > > > > > > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> > > [...] > > > > + > > > static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector, > > > struct drm_display_mode *mode) > > > { > > > - /* TODO: check mode */ > > > + struct sii902x *sii902x = connector_to_sii902x(connector); > > > + const struct drm_display_mode *mod = mode; > > > - return MODE_OK; > > > + return sii902x_validate(sii902x, mod); > > > > There is no need to. The drm_bridge_chain_mode_valid() should take care > > of calling bridge's mode_valid callback and rejecting the mode if it is > > not accepted. > > I need some clarity here. > > IIRC, if the bridge does initialize the connector in case > where the encoder does not attach the bridge with the > DRM_BRIDGE_ATTACH_NO_CONNECTOR (DBANC) flag (referring to tidss > encoder before we implemented the DBANC feature), then > drm_connector_helper_func are called and drm_bridge_funcs > are NOT called (atleast from what I have seen in detect > hook for cdns-mhdp-8546 driver which is there in both > structures). There are different kinds of bridge_funcs. detect is a part of the connector-related interface, so it is not called by the drm core. On the other hand functions like mode_valid, enable/disable, etc. are called for all bridges. > I do not have any platform to test non-DBANC encoders. > And I did not want to break any platform that were still using > bridge_attach without DBANC flag. > That is why I kept mode_valid hook in both structures. > > Are you implying that if connector_helper_funcs are not there > then there will be some sort of fallback to bridge_funcs instead > of passthrough for mode_valid check? Because that goes against my > previous observations. Not quite. See how drm_atomic_heler uses bridge_funcs.
On 25/05/24 01:16, Dmitry Baryshkov wrote: > On Fri, May 24, 2024 at 05:54:02PM +0530, Jayesh Choudhary wrote: >> Hello Dmitry, >> >> On 24/05/24 15:11, Dmitry Baryshkov wrote: >>> On Fri, May 24, 2024 at 03:05:08PM +0530, Jayesh Choudhary wrote: >>>> Currently, mode_valid hook returns all mode as valid and it is >>>> defined only in drm_connector_helper_funcs. With the introduction of >>>> 'DRM_BRIDGE_ATTACH_NO_CONNECTOR', connector is not initialized in >>>> bridge_attach call for cases when the encoder has this flag enabled. >>>> So add the mode_valid hook in drm_bridge_funcs as well with proper >>>> clock checks for maximum and minimum pixel clock supported by the >>>> bridge. >>>> >>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> >> >> [...] >> >>>> + >>>> static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector, >>>> struct drm_display_mode *mode) >>>> { >>>> - /* TODO: check mode */ >>>> + struct sii902x *sii902x = connector_to_sii902x(connector); >>>> + const struct drm_display_mode *mod = mode; >>>> - return MODE_OK; >>>> + return sii902x_validate(sii902x, mod); >>> >>> There is no need to. The drm_bridge_chain_mode_valid() should take care >>> of calling bridge's mode_valid callback and rejecting the mode if it is >>> not accepted. >> >> I need some clarity here. >> >> IIRC, if the bridge does initialize the connector in case >> where the encoder does not attach the bridge with the >> DRM_BRIDGE_ATTACH_NO_CONNECTOR (DBANC) flag (referring to tidss >> encoder before we implemented the DBANC feature), then >> drm_connector_helper_func are called and drm_bridge_funcs >> are NOT called (atleast from what I have seen in detect >> hook for cdns-mhdp-8546 driver which is there in both >> structures). > > There are different kinds of bridge_funcs. detect is a part of the > connector-related interface, so it is not called by the drm core. On the > other hand functions like mode_valid, enable/disable, etc. are called > for all bridges. > Oh okay! Thanks for clarifying. >> I do not have any platform to test non-DBANC encoders. >> And I did not want to break any platform that were still using >> bridge_attach without DBANC flag. >> That is why I kept mode_valid hook in both structures. >> >> Are you implying that if connector_helper_funcs are not there >> then there will be some sort of fallback to bridge_funcs instead >> of passthrough for mode_valid check? Because that goes against my >> previous observations. > > Not quite. See how drm_atomic_heler uses bridge_funcs. > I will do that and spin another revision with the suggested changes. Warm Regards, Jayesh
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index 2fbeda9025bf..bae551e107f9 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -163,6 +163,14 @@ #define SII902X_AUDIO_PORT_INDEX 3 +/* + * The maximum resolution supported by the HDMI bridge is 1080p@60Hz + * and 1920x1200 requiring a pixel clock of 165MHz and the minimum + * resolution supported is 480p@60Hz requiring a pixel clock of 25MHz + */ +#define SII902X_MIN_PIXEL_CLOCK_KHZ 25000 +#define SII902X_MAX_PIXEL_CLOCK_KHZ 165000 + struct sii902x { struct i2c_client *i2c; struct regmap *regmap; @@ -310,12 +318,26 @@ static int sii902x_get_modes(struct drm_connector *connector) return num; } +static enum +drm_mode_status sii902x_validate(struct sii902x *sii902x, + const struct drm_display_mode *mode) +{ + if (mode->clock < SII902X_MIN_PIXEL_CLOCK_KHZ) + return MODE_CLOCK_LOW; + + if (mode->clock > SII902X_MAX_PIXEL_CLOCK_KHZ) + return MODE_CLOCK_HIGH; + + return MODE_OK; +} + static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) { - /* TODO: check mode */ + struct sii902x *sii902x = connector_to_sii902x(connector); + const struct drm_display_mode *mod = mode; - return MODE_OK; + return sii902x_validate(sii902x, mod); } static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs = { @@ -504,6 +526,16 @@ static int sii902x_bridge_atomic_check(struct drm_bridge *bridge, return 0; } +static enum drm_mode_status +sii902x_bridge_mode_valid(struct drm_bridge *bridge, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + struct sii902x *sii902x = bridge_to_sii902x(bridge); + + return sii902x_validate(sii902x, mode); +} + static const struct drm_bridge_funcs sii902x_bridge_funcs = { .attach = sii902x_bridge_attach, .mode_set = sii902x_bridge_mode_set, @@ -516,6 +548,7 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = { .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts, .atomic_check = sii902x_bridge_atomic_check, + .mode_valid = sii902x_bridge_mode_valid, }; static int sii902x_mute(struct sii902x *sii902x, bool mute)
Currently, mode_valid hook returns all mode as valid and it is defined only in drm_connector_helper_funcs. With the introduction of 'DRM_BRIDGE_ATTACH_NO_CONNECTOR', connector is not initialized in bridge_attach call for cases when the encoder has this flag enabled. So add the mode_valid hook in drm_bridge_funcs as well with proper clock checks for maximum and minimum pixel clock supported by the bridge. Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com> --- drivers/gpu/drm/bridge/sii902x.c | 37 ++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)