On 11/26/24 4:56 PM, Maxime Ripard wrote:
On Tue, Nov 26, 2024 at 12:48:20AM +0100, Marek Vasut wrote:
On 11/22/24 2:32 PM, Dmitry Baryshkov wrote:
On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote:
The Pixel PLL is not very capable and may come up with wildly inaccurate
clock. Since DPI panels are often tolerant to slightly higher pixel clock
without being operated outside of specification, calculate two Pixel PLL
from either mode clock or display_timing .pixelclock.max , whichever is
higher. Since the Pixel PLL output clock frequency calculation always
returns lower frequency than the requested clock frequency, passing in
the higher clock frequency should result in output clock frequency which
is closer to the expected pixel clock.
For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency
without this patch is 65 MHz which is out of the panel specification of
68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within
the specification and far more accurate.
Granted that most of the panel drivers do not implement get_timings()
and granted that there are no current users of that interface, I think
we should move away from it (and maybe even drop it completely from
drm_panel).
It does fit DPI and LVDS panels and their descriptions in datasheets the
best.
What about achieving the same via slightly different approach: register
a non-preferred mode with the clock equal to the max clock allowed. The
bridge driver can then use the default and the "max" mode to select PLL
clock.
I understand that this suggestion doesn't follow the DPI panel
specifics, which are closer to the continuous timings rather than fixed
set of modes, however I really don't think that it's worth keeping the
interface for the sake of a single driver. Original commit 2938931f3732
("drm/panel: Add display timing support") from 2014 mentions using those
timings for .mode_fixup(), but I couldn't find a trace of the
corresponding implementation.
Another possible option might be to impletent adjusting modes in
.atomic_check() / .mode_fixup().
Something like this ?
static const struct display_timing chefree_ch101olhlwh_002_timing = {
.pixelclock = { 68900000, 71100000, 73400000 },
...
};
static const struct panel_desc chefree_ch101olhlwh_002 = {
.timings = &chefree_ch101olhlwh_002_timing,
.num_timings = 1,
...
};
... would turn into ...
static const struct drm_display_mode chefree_ch101olhlwh_002_mode[3] = {
{
.clock = 68900000,
...
}, {
.clock = 71100000,
...
}, {
.clock = 73400000,
...
}
};
static const struct panel_desc chefree_ch101olhlwh_002 = {
.modes = &chefree_ch101olhlwh_002_mode,
.num_timings = 3,
...
};
?
Except that doesn't work if you want to keep your driver at the expected
framerate. To reduce the pixel clock, you also need to reduce the
blanking period within the acceptable boundaries if you want to keep the
same framerate.
Each mode entry would have to have its own full set of timings of
course, not just .clock . Those can be converted from display_timing
just like clock.
