Re: [PATCH v3 0/6] Introduce new mode validation callbacks

[Andrzej Hajda]

On 12.05.2017 09:32, Daniel Vetter wrote:
> On Thu, May 11, 2017 at 10:05:56AM +0100, Jose Abreu wrote:
>> This series is a follow up from the discussion at [1]. We start by
>> introducing crtc->mode_valid(), encoder->mode_valid() and
>> bridge->mode_valid() callbacks which will be used in followup
>> patches and also by cleaning the documentation a little bit.
>>
>> We proceed by introducing new helpers to call this new callbacks
>> at 2/6.
>>
>> At 3/6 a helper function is introduced that calls all mode_valid()
>> from a set of bridges.
>>
>> Next, at 4/6 we modify the connector probe helper so that only modes
>> which are supported by a given bridge+encoder+crtc combination are
>> probbed.
>>
>> At 5/6 we call all the mode_valid() callbacks for a given pipeline,
>> except the connector->mode_valid one, so that the mode is validated.
>> This is done before calling mode_fixup().
>>
>> Finally, at 6/6 we use the new crtc->mode_valid() callback in arcpgu
>> and remove the atomic_check() callback.
>>
>> [1] https://patchwork.kernel.org/patch/9702233/
>>
>> Jose Abreu (6):
>>   drm: Add crtc/encoder/bridge->mode_valid() callbacks
>>   drm: Add drm_{crtc/encoder/connector}_mode_valid()
>>   drm: Introduce drm_bridge_mode_valid()
>>   drm: Use new mode_valid() helpers in connector probe helper
>>   drm: Use mode_valid() in atomic modeset
>>   drm: arc: Use crtc->mode_valid() callback
>>
>> 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>
> Commented with an entire patch on patch 1, patches 2-5 are all
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> But I think some more acks/r-bs would be really good, since this is quite
> a bit change in the helper infrastructure. But otherwise ready for merging
> imo. Can you pls also review my proposal for patch 1?
>
> Thanks, Daniel

As the patchset improves many things, I would like to point here that
there are still issues with mode probing at least in case of panels.
Panels in general does not provide discrete list of supported modes, but
they provide list of supported mode ranges (named display_timings), at
the moment this is only addressed in drm_panel_funcs::get_timings, but
drm core does not use it at all.
Currently most of the panel drivers advertises only fixed list of
arbitrary chosen modes, and if bridge/connector/encoder/crtc does not
support it pipeline does not work, even if slightly different
configuration could work. Of course there are workarounds but maybe it
would be good to replace drm_connector::modes with drm_connector::timings.
This way set of valid timings of the whole pipeline will be intersection
of sets of valid timings of every component of the pipeline - quite
straightforward and simple construct.

As this is just an idea which came to me during patchset review, it is
not backed by any code, but maybe it can be interesting for quick
brainstorm.

Regards
Andrzej


>
>>  drivers/gpu/drm/arc/arcpgu_crtc.c          |  39 ++++++----
>>  drivers/gpu/drm/drm_atomic_helper.c        |  76 +++++++++++++++++++-
>>  drivers/gpu/drm/drm_bridge.c               |  33 +++++++++
>>  drivers/gpu/drm/drm_crtc_helper_internal.h |  13 ++++
>>  drivers/gpu/drm/drm_probe_helper.c         | 103 ++++++++++++++++++++++++++-
>>  include/drm/drm_bridge.h                   |  22 ++++++
>>  include/drm/drm_modeset_helper_vtables.h   | 110 ++++++++++++++++++++++-------
>>  7 files changed, 348 insertions(+), 48 deletions(-)
>>
>> -- 
>> 1.9.1
>>
>>