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@xxxxxxxxxxxx>
>>> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
>>> Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
>>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>> Cc: Dave Airlie <airlied@xxxxxxxx>
>>> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
>>> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
>> 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=
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
