[PATCH] drm/pl111: Use max memory bandwidth for resolution

[Linus Walleij]

On Fri, Jan 26, 2018 at 2:27 PM, Linus Walleij <linus.walleij at linaro.org> wrote:
> On Thu, Jan 25, 2018 at 4:46 AM, Eric Anholt <eric at anholt.net> wrote:
>
>>> +     pl111_choose_max_resolution(dev, priv->memory_bw,
>>> +                                 &mode_config->max_width,
>>> +                                 &mode_config->max_height, &bpp);
>>> +     dev_info(dev->dev, "cap resolution at %u x %u, %u BPP\n",
>>> +              mode_config->max_width, mode_config->max_height, bpp);
>>
>> I think this is the wrong place in the pipeline to be doing this, but I
>> don't have a complete solution so I'm not necessarily saying no.
>
> So currently the driver does this:
>
> mode_config->max_width = 1024;
> mode_config->max_height = 768;
>
> And that is because it cannot really handle anything. I guess ideally
> the DRM driver should set these to -1 or something so that any widths
> and heights negotiated will work.
>
>>  Things I think we should do for bandwidth limits:
>>
>> A new pl111_mode_valid() rejects modes with width*height*2 > bandwidth
>> (if we can't scan it out with our smallest format, don't advertise it).
>>
>> pl111_display_check() rejects modes with width*height*bpp > bandwidth
>> (if we can't scan out this particular configuration, let them know we
>> can't set the mode).
>>
>> Ideally given those two things, fbdev and X11 would notice that the
>> preferred mode fails at 24bpp and fall back to 16bpp.  I don't think
>> either of those does so today, though.
>>
>> Interested in tackling any of these?
>
> I tried the pl111_display_check() version. It just made the driver
> fail to initialize anything, at least when using the dumb VGA
> bridge.

I guess this is because it gets called from
drm_simple_display_pipe_funcs
at which point the driver framework has already decided to go with
this format. And that is backed by crtc.

We would need to extend this with a new function such as
.crtc_valid() that can check both mode (for resolution)
and format (for BPP).

But then I start to wonde how "simple" drm_simple_display_pipe
is becoming :/

I can't figure out if the crtc is even the right place to address this...

> There are .mode_valid() callbacks on the bridges we use
> (panel and dumb VGA) but neither uses it at the moment, hm.
> I could just assign my own .mode_valid() callback to the bridge,
> but it seems a bit fragile. But it's worth a hack, I'll try it.

It turns out that this passes only an struct drm_display_mode
which does not concern itself with display engine details
like BPP.

So the bridges just put limitations on modes, not on BPP,
which makes a lot of sense, it corresponds to what the
hardware does.

It's evident when I think about it...

The check needs to be done in the
drm_simple_display_pipe_funcs  or setting that up as
per above. I just don't really see exactly where?

Yours,
Linus Walleij