Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
From: Neil Armstrong
Date: Thu Jul 06 2023 - 04:03:25 EST
On 06/07/2023 09:59, Maxime Ripard wrote:
On Thu, Jul 06, 2023 at 09:33:15AM +0200, Neil Armstrong wrote:
On 06/07/2023 09:24, Maxime Ripard wrote:
On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote:
On 05/07/2023 19:53, Maxime Ripard wrote:
On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote:
On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
Either way, I'm not really sure it's a good idea to multiply the
capabilities flags of the DSI host, and we should just stick to the
spec. If the spec says that we have to support DSC while video is
output, then that's what the panels should expect.
Except some panels supports DSC & non-DSC, Video and Command mode, and
all that is runtime configurable. How do you handle that ?
In this case, most of the constraints are going to be on the encoder
still so it should be the one driving it. The panel will only care about
which mode has been selected, but it shouldn't be the one driving it,
and thus we still don't really need to expose the host capabilities.
This is an interesting perspective. This means that we can and actually have
to extend the drm_display_mode with the DSI data and compression
information.
I wouldn't extend drm_display_mode, but extending one of the state
structures definitely.
We already have some extra variables in drm_connector_state for HDMI,
I don't think it would be a big deal to add a few for MIPI-DSI.
We also floated the idea for a while to create bus-specific states, with
helpers to match. Maybe it would be a good occasion to start doing it?
For example, the panel that supports all four types for the 1080p should
export several modes:
1920x1080-command
1920x1080-command-DSC
1920x1080-video
1920x1080-video-DSC
where video/command and DSC are some kinds of flags and/or information in
the drm_display_mode? Ideally DSC also has several sub-flags, which denote
what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
etc).
So we have two things to do, right? We need to expose what the panel can
take (ie, EDID for HDMI), and then we need to tell it what we picked
(infoframes).
We already express the former in mipi_dsi_device, so we could extend the
flags stored there.
And then, we need to tie what the DSI host chose to a given atomic state
so the panel knows what was picked and how it should set everything up.
This is definitely something we need. Marijn has been stuck with the
panels that support different models ([1]).
Would you prefer a separate API for this kind of information or
abusing atomic_enable() is fine from your point of view?
My vote would be for going with existing operations, with the slight
fear of ending up with another DSI-specific hack (like
pre_enable_prev_first).
I don't think we can get away without getting access to the atomic_state
from the panel at least.
Choosing one setup over another is likely going to depend on the mode,
and that's only available in the state.
We don't have to go the whole way though and create the sub-classes of
drm_connector_state, but I think we should at least provide it to the
panel.
What do you think of creating a new set of atomic_* callbacks for
panels, call that new set of functions from msm and start from there?
We are (somewhat) bound by the panel_bridge, but yeah, it seems possible.
Bridges have access to the atomic state already, so it's another place
to plumb this through but I guess it would still be doable?
It's definitely doable, but I fear we won't be able to test most of the
panel drivers, should we introduce a new atomic set of panel callbacks ?
That was my original intent yeah :)
Creating an atomic_enable/disable/ etc. and then switch
drm_panel_enable() to take the state (and fixing up all the callers), or
create a drm_panel_enable_atomic() function.
The latter is probably simpler, something like:
int drm_panel_enable_atomic(struct drm_panel *panel,
struct drm_atomic_state *state)
{
struct drm_panel_funcs *funcs = panel->funcs;
if (funcs->atomic_enable)
return funcs->atomic_enable(panel, state);
return funcs->enable(panel);
}
And we should probably mention that it supersedes/deprecates
drm_panel_enable.
We've switched most of the other atomic hooks to take the full
drm_atomic_state so I'd prefer to use it. However, for it to be somewhat
useful we'd need to have access to the connector assigned to that panel.
drm_panel doesn't store the drm_connector it's connected to at all, and
of_drm_find_panel() doesn't take it as an argument so we can't fill it
when we retrieve it either.
So I guess we can go for:
- Create a new set of atomic hooks
- Create a new set of functions to call those hooks, that we would
document as deprecating the former functions. Those functions would
take a pointer to the drm_connector_state of the drm_connector it's
connected to.
- We add a TODO item to add a pointer to the connector in drm_panel
- We add a TODO item that depend on the first one to switch the new
functions and hooks to drm_atomic_state
- We add a TODO item to convert callers of drm_panel_enable et al. to
our new functions.
It should work in all setups, paves a nice way forward and documents the
trade-offs we had to take and eventually address. And without creating a
dependency on 30+ patches series.
Does it sound like a plan?
Yep that looks a fine plan to start of
Or shall be simply move the "new" panel driver supporting atomic to bridge
and only use panel_bridge for basic panels ?
I don't think we can expect panel_bridge to be used all the time any
time soon, so I'd rather avoid to rely on it.
Ack
Maxime