| Message ID | f4dbd80323f567cef785b250daf6e50760160ecd.1494492646.git.joabreu@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 11, 2017 at 10:06:02AM +0100, Jose Abreu wrote: > Now that we have a callback to check if crtc supports a given mode > we can use it in arcpgu so that we restrict the number of probbed > modes to the ones we can actually display. > > This is specially useful because arcpgu crtc is responsible to set > a clock value in the commit() stage but unfortunatelly this clock > does not support all the needed ranges. > > Also, remove the atomic_check() callback as mode_valid() callback > will be called before. > > 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> Btw for justifying your patch series a bit more, would be real sweet to roll ->mode_valid out to more drivers. I see two easy cases: bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a mode_valid callback bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we could move the connector_mode valid to the bridge and done. Care to type these 2 patches on top, just to make this a bit more useful and a clearer case for merging? Thanks, Daniel > --- > drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++--------------- > 1 file changed, 24 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c > index ad9a959..01cae0a 100644 > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c > @@ -32,6 +32,18 @@ > { "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, > }; > > +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu, > + const struct drm_display_mode *mode) > +{ > + long rate, clk_rate = mode->clock * 1000; > + > + rate = clk_round_rate(arcpgu->clk, clk_rate); > + if (rate != clk_rate) > + return false; > + > + return true; > +} > + > static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) > { > struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > }; > > +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc, > + const struct drm_display_mode *mode) > +{ > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > + > + if (!arc_pgu_is_mode_valid(arcpgu, mode)) > + return MODE_NOCLOCK; > + > + return MODE_OK; > +} > + > static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc) > { > struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc) > ~ARCPGU_CTRL_ENABLE_MASK); > } > > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, > - struct drm_crtc_state *state) > -{ > - struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > - struct drm_display_mode *mode = &state->adjusted_mode; > - long rate, clk_rate = mode->clock * 1000; > - > - rate = clk_round_rate(arcpgu->clk, clk_rate); > - if (rate != clk_rate) > - return -EINVAL; > - > - return 0; > -} > - > static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > struct drm_crtc_state *state) > { > @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > } > > static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { > + .mode_valid = arc_pgu_crtc_mode_valid, > .mode_set = drm_helper_crtc_mode_set, > .mode_set_base = drm_helper_crtc_mode_set_base, > .mode_set_nofb = arc_pgu_crtc_mode_set_nofb, > @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > .disable = arc_pgu_crtc_disable, > .prepare = arc_pgu_crtc_disable, > .commit = arc_pgu_crtc_enable, > - .atomic_check = arc_pgu_crtc_atomic_check, > .atomic_begin = arc_pgu_crtc_atomic_begin, > }; > > -- > 1.9.1 > >
On 11.05.2017 11:06, Jose Abreu wrote: > Now that we have a callback to check if crtc supports a given mode > we can use it in arcpgu so that we restrict the number of probbed > modes to the ones we can actually display. > > This is specially useful because arcpgu crtc is responsible to set > a clock value in the commit() stage but unfortunatelly this clock > does not support all the needed ranges. > > Also, remove the atomic_check() callback as mode_valid() callback > will be called before. > > 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> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej
On Mon, May 15, 2017 at 10:53:25AM +0200, Daniel Vetter wrote: > On Thu, May 11, 2017 at 10:06:02AM +0100, Jose Abreu wrote: > > Now that we have a callback to check if crtc supports a given mode > > we can use it in arcpgu so that we restrict the number of probbed > > modes to the ones we can actually display. > > > > This is specially useful because arcpgu crtc is responsible to set > > a clock value in the commit() stage but unfortunatelly this clock > > does not support all the needed ranges. > > > > Also, remove the atomic_check() callback as mode_valid() callback > > will be called before. > > > > 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> > > Btw for justifying your patch series a bit more, would be real sweet to > roll ->mode_valid out to more drivers. I see two easy cases: > > bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a > mode_valid callback > > bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we > could move the connector_mode valid to the bridge and done. > > Care to type these 2 patches on top, just to make this a bit more useful > and a clearer case for merging? Aside: There's a pile more drivers (encoders and crtc drivers) which would similarly benefit and dont really look like they'd be hard to patch up: - malidp_crtc_mode_fixup - armada_drm_crtc_mode_fixup (armada isn't atomic, so forget this one) - atmel_hlcdc_crtc_mode_fixup - hdmi_mode_fixup in exynos_hdmi.c - vc4_crtc_mode_fixup and vc4_dpi_encoder_mode_fixup (they both just check for interlaced modes, we could probably dump the same check in connector->mode_valid). Plus the 2 bridge drivers. Everyone else does either something more complex, or something that's not quite correct. -Daniel > > Thanks, Daniel > > > --- > > drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++--------------- > > 1 file changed, 24 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c > > index ad9a959..01cae0a 100644 > > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c > > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c > > @@ -32,6 +32,18 @@ > > { "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, > > }; > > > > +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu, > > + const struct drm_display_mode *mode) > > +{ > > + long rate, clk_rate = mode->clock * 1000; > > + > > + rate = clk_round_rate(arcpgu->clk, clk_rate); > > + if (rate != clk_rate) > > + return false; > > + > > + return true; > > +} > > + > > static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) > > { > > struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) > > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > > }; > > > > +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc, > > + const struct drm_display_mode *mode) > > +{ > > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > + > > + if (!arc_pgu_is_mode_valid(arcpgu, mode)) > > + return MODE_NOCLOCK; > > + > > + return MODE_OK; > > +} > > + > > static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc) > > { > > struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc) > > ~ARCPGU_CTRL_ENABLE_MASK); > > } > > > > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, > > - struct drm_crtc_state *state) > > -{ > > - struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > - struct drm_display_mode *mode = &state->adjusted_mode; > > - long rate, clk_rate = mode->clock * 1000; > > - > > - rate = clk_round_rate(arcpgu->clk, clk_rate); > > - if (rate != clk_rate) > > - return -EINVAL; > > - > > - return 0; > > -} > > - > > static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > > struct drm_crtc_state *state) > > { > > @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > > } > > > > static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { > > + .mode_valid = arc_pgu_crtc_mode_valid, > > .mode_set = drm_helper_crtc_mode_set, > > .mode_set_base = drm_helper_crtc_mode_set_base, > > .mode_set_nofb = arc_pgu_crtc_mode_set_nofb, > > @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > > .disable = arc_pgu_crtc_disable, > > .prepare = arc_pgu_crtc_disable, > > .commit = arc_pgu_crtc_enable, > > - .atomic_check = arc_pgu_crtc_atomic_check, > > .atomic_begin = arc_pgu_crtc_atomic_begin, > > }; > > > > -- > > 1.9.1 > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Hi Daniel, On 15-05-2017 16:52, Daniel Vetter wrote: > On Mon, May 15, 2017 at 10:53:25AM +0200, Daniel Vetter wrote: >> On Thu, May 11, 2017 at 10:06:02AM +0100, Jose Abreu wrote: >>> Now that we have a callback to check if crtc supports a given mode >>> we can use it in arcpgu so that we restrict the number of probbed >>> modes to the ones we can actually display. >>> >>> This is specially useful because arcpgu crtc is responsible to set >>> a clock value in the commit() stage but unfortunatelly this clock >>> does not support all the needed ranges. >>> >>> Also, remove the atomic_check() callback as mode_valid() callback >>> will be called before. >>> >>> 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> >> Btw for justifying your patch series a bit more, would be real sweet to >> roll ->mode_valid out to more drivers. I see two easy cases: >> >> bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a >> mode_valid callback >> >> bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we >> could move the connector_mode valid to the bridge and done. >> >> Care to type these 2 patches on top, just to make this a bit more useful >> and a clearer case for merging? > Aside: There's a pile more drivers (encoders and crtc drivers) which would > similarly benefit and dont really look like they'd be hard to patch up: > - malidp_crtc_mode_fixup > - armada_drm_crtc_mode_fixup (armada isn't atomic, so forget this one) > - atmel_hlcdc_crtc_mode_fixup > - hdmi_mode_fixup in exynos_hdmi.c > - vc4_crtc_mode_fixup and vc4_dpi_encoder_mode_fixup (they both just check > for interlaced modes, we could probably dump the same check in > connector->mode_valid). > > Plus the 2 bridge drivers. Everyone else does either something more > complex, or something that's not quite correct. > -Daniel Sorry for being silent but I am away from office this week. I will try to evaluate and add this in next version. In the meantime I will review the documentation patches you sent. Thanks! Best regards, Jose Miguel Abreu > >> Thanks, Daniel >> >>> --- >>> drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++--------------- >>> 1 file changed, 24 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c >>> index ad9a959..01cae0a 100644 >>> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c >>> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c >>> @@ -32,6 +32,18 @@ >>> { "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, >>> }; >>> >>> +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu, >>> + const struct drm_display_mode *mode) >>> +{ >>> + long rate, clk_rate = mode->clock * 1000; >>> + >>> + rate = clk_round_rate(arcpgu->clk, clk_rate); >>> + if (rate != clk_rate) >>> + return false; >>> + >>> + return true; >>> +} >>> + >>> static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) >>> { >>> struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); >>> @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) >>> .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, >>> }; >>> >>> +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc, >>> + const struct drm_display_mode *mode) >>> +{ >>> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); >>> + >>> + if (!arc_pgu_is_mode_valid(arcpgu, mode)) >>> + return MODE_NOCLOCK; >>> + >>> + return MODE_OK; >>> +} >>> + >>> static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc) >>> { >>> struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); >>> @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc) >>> ~ARCPGU_CTRL_ENABLE_MASK); >>> } >>> >>> -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, >>> - struct drm_crtc_state *state) >>> -{ >>> - struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); >>> - struct drm_display_mode *mode = &state->adjusted_mode; >>> - long rate, clk_rate = mode->clock * 1000; >>> - >>> - rate = clk_round_rate(arcpgu->clk, clk_rate); >>> - if (rate != clk_rate) >>> - return -EINVAL; >>> - >>> - return 0; >>> -} >>> - >>> static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, >>> struct drm_crtc_state *state) >>> { >>> @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, >>> } >>> >>> static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { >>> + .mode_valid = arc_pgu_crtc_mode_valid, >>> .mode_set = drm_helper_crtc_mode_set, >>> .mode_set_base = drm_helper_crtc_mode_set_base, >>> .mode_set_nofb = arc_pgu_crtc_mode_set_nofb, >>> @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, >>> .disable = arc_pgu_crtc_disable, >>> .prepare = arc_pgu_crtc_disable, >>> .commit = arc_pgu_crtc_enable, >>> - .atomic_check = arc_pgu_crtc_atomic_check, >>> .atomic_begin = arc_pgu_crtc_atomic_begin, >>> }; >>> >>> -- >>> 1.9.1 >>> >>> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> https://urldefense.proofpoint.com/v2/url?u=http-3A__blog.ffwll.ch&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=yaVFU4TjGY0gVF8El1uKcisy6TPsyCl9uN7Wsis-qhY&m=v9tZNfdrkm8mrAlJyEpm71Te7tDReAFXijS23BkDSTo&s=Y6DG3RpFLs0nomQ09FA3hqCqq6MQ3dvG81s_2XlVytM&e=
diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c index ad9a959..01cae0a 100644 --- a/drivers/gpu/drm/arc/arcpgu_crtc.c +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c @@ -32,6 +32,18 @@ { "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, }; +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu, + const struct drm_display_mode *mode) +{ + long rate, clk_rate = mode->clock * 1000; + + rate = clk_round_rate(arcpgu->clk, clk_rate); + if (rate != clk_rate) + return false; + + return true; +} + static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) { struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, }; +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc, + const struct drm_display_mode *mode) +{ + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); + + if (!arc_pgu_is_mode_valid(arcpgu, mode)) + return MODE_NOCLOCK; + + return MODE_OK; +} + static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc) { struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc) ~ARCPGU_CTRL_ENABLE_MASK); } -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, - struct drm_crtc_state *state) -{ - struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); - struct drm_display_mode *mode = &state->adjusted_mode; - long rate, clk_rate = mode->clock * 1000; - - rate = clk_round_rate(arcpgu->clk, clk_rate); - if (rate != clk_rate) - return -EINVAL; - - return 0; -} - static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { + .mode_valid = arc_pgu_crtc_mode_valid, .mode_set = drm_helper_crtc_mode_set, .mode_set_base = drm_helper_crtc_mode_set_base, .mode_set_nofb = arc_pgu_crtc_mode_set_nofb, @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, .disable = arc_pgu_crtc_disable, .prepare = arc_pgu_crtc_disable, .commit = arc_pgu_crtc_enable, - .atomic_check = arc_pgu_crtc_atomic_check, .atomic_begin = arc_pgu_crtc_atomic_begin, };
Now that we have a callback to check if crtc supports a given mode we can use it in arcpgu so that we restrict the number of probbed modes to the ones we can actually display. This is specially useful because arcpgu crtc is responsible to set a clock value in the commit() stage but unfortunatelly this clock does not support all the needed ranges. Also, remove the atomic_check() callback as mode_valid() callback will be called before. 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/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)