Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback

[Jose Abreu <Jose.Abreu@synopsys.com>]

Hi Andrzej,


Thanks for your answer!


On 27-04-2017 11:05, Andrzej Hajda wrote:
> Hi Jose,
>
> On 26.04.2017 12:48, Jose Abreu wrote:
>> Some crtc's may have restrictions in the mode they can display. In
>> this patch a new callback (crtc->mode_valid()) is introduced that
>> is called at the same stage of connector->mode_valid() callback.
>>
>> This shall be implemented if the crtc has some sort of restriction
>> so that we don't probe modes that will fail in the commit() stage.
>> For example: A given crtc may be responsible to set a clock value.
>> If the clock can not produce all the values for the available
>> modes then this callback can be used to restrict the number of
>> probbed modes to only the ones that can be displayed.
>>
>> If the crtc does not implement the callback then the behaviour will
>> remain the same. Also, for a given set of crtcs that can be bound to
>> the connector, if at least one can display the mode then the mode
>> will be probbed.
> I see few possible issues/improvements here:
> 1. crtc can have different constraints depending on the encoder it is
> connected to, theoretically reverse dependency is also possible, but I
> am not aware of such hw.
> 2. there also could be similar dependency constrains between connector
> and encoder, I guess.
> 3. encoders and bridges should have also possibility to validate modes,
> they also have constrains, btw encoder_slave have such callback.
>
> Regarding 1st and 2nd point, maybe it would be good to validate modes
> according to topology described in drm_mode_get_connector::encoder_id
> and drm_mode_get_encoder::crtc_id:
> a) if connector is not connected to any encoder report to userspace
> modes filtered only by connector::mode_valid,
> b) if connector is connected to encoder, report modes filtered by
> connector, encoder and attached bridges,
> c) if full pipeline is constructed, report modes filtered by all members
> of the pipeline.

I see your point but when the full pipeline is created we already
handed the modes to userspace, right?

This patch is simple and does not take into account that: it just
iterates over all crtcs that can be bound to the set of encoders
which instead can be bound to the given connector. So, we are
filtering modes from the connector but not taking into account
the pipeline. It will work for a single pipeline, but with
multiple constraints, just like you mentioned, it will probably
make no difference.

Despite this, I think its a good initial step.

>
> Of course with this approach drm_mode_get_connector userspace should be
> aware of the fact that it should re-call drm_mode_get_connector after
> topology change to update supported modes.

I believe we should avoid this, but indeed it seems necessary.

What about a new get_modes(encoder_id, connector_id, crtc_id)
ioctl ? User could call this and know what modes a given pipeline
will support.

Best regards,
Jose Miguel Abreu

>
> Regards
> Andrzej
>