Taking tiling and rotation into account in watermark computations

Taking tiling and rotation into account in watermark computations

[Tvrtko Ursulin]

Hi all,

We need to refactor the current code a bit to allow parameters like 
plane rotation and framebuffer tiling mode be taken into account when 
calculating display watermarks.

I looked into this code a bit and am at the moment a bit confused with 
what is where and why.

For example the purpose of plane_config in intel_crtc seems a bit thin, 
or why it is created once on driver init. Then again watermark 
parameters are embedded in intel_plane, which is separate from 
plane_config. And where is the link between intel_crtc and intel_plane, 
or why intel_crtc has a plane field - is it not that there are multiple 
planes per pipe/crtc?

Part one would be trying to understand how things are. Then part two 
would be coming up with a design, if justified by the extent of work 
required, to implement this requirement.

Thanks,

Tvrtko

Re: Taking tiling and rotation into account in watermark computations

[Ville Syrjälä]

On Mon, Oct 06, 2014 at 05:11:57PM +0100, Tvrtko Ursulin wrote:
> 
> Hi all,
> 
> We need to refactor the current code a bit to allow parameters like 
> plane rotation and framebuffer tiling mode be taken into account when 
> calculating display watermarks.
> 
> I looked into this code a bit and am at the moment a bit confused with 
> what is where and why.
> 
> For example the purpose of plane_config in intel_crtc seems a bit thin, 
> or why it is created once on driver init.

That thing is only used for the BIOS fb takesover. Probably should be
renamed since it's basically just some kind of duplicated fb structure
rather than a full plane config. And I don't want to extend that
duplication into the structure we use to track the plane config
otherwise.

We now have intel_plane_state (or something like that) that's going to
form the basis of the plane config tracking.

> Then again watermark 
> parameters are embedded in intel_plane, which is separate from 
> plane_config.

The per-plane watermark stuff should get moved into the
intel_plane_state. But that requires that I find the time to get
back to the watermark code and actually finish off whatever patches
I have still pending. So the current state of the watermark code
was just meant to be a short lived thing while it continues to
evolve. Sadly the remaining stuff didn't get in when I had the
time to work on it and now I can't seem to find any time for it.
But hopefully soon I'll have some time for this again (famous
last words).

> And where is the link between intel_crtc and intel_plane, 
> or why intel_crtc has a plane field - is it not that there are multiple 
> planes per pipe/crtc?

That's the primary plane used by the drm crtc. We now expose the
primary planes as drm_planes as well, but we still need to keep
the "crtc can do scanout" idea around since it's part of the
current user space API. The atomic modeset API won't have such
silly notions any more.

> Part one would be trying to understand how things are. Then part two 
> would be coming up with a design, if justified by the extent of work 
> required, to implement this requirement.
> 
> Thanks,
> 
> Tvrtko
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: Taking tiling and rotation into account in watermark computations

[Tvrtko Ursulin]

On 10/07/2014 11:22 AM, Ville Syrjälä wrote:
> On Mon, Oct 06, 2014 at 05:11:57PM +0100, Tvrtko Ursulin wrote:
>>
>> Hi all,
>>
>> We need to refactor the current code a bit to allow parameters like
>> plane rotation and framebuffer tiling mode be taken into account when
>> calculating display watermarks.
>>
>> I looked into this code a bit and am at the moment a bit confused with
>> what is where and why.
>>
>> For example the purpose of plane_config in intel_crtc seems a bit thin,
>> or why it is created once on driver init.
>
> That thing is only used for the BIOS fb takesover. Probably should be
> renamed since it's basically just some kind of duplicated fb structure
> rather than a full plane config. And I don't want to extend that
> duplication into the structure we use to track the plane config
> otherwise.
>
> We now have intel_plane_state (or something like that) that's going to
> form the basis of the plane config tracking.
>
>> Then again watermark
>> parameters are embedded in intel_plane, which is separate from
>> plane_config.
>
> The per-plane watermark stuff should get moved into the
> intel_plane_state. But that requires that I find the time to get
> back to the watermark code and actually finish off whatever patches
> I have still pending. So the current state of the watermark code
> was just meant to be a short lived thing while it continues to
> evolve. Sadly the remaining stuff didn't get in when I had the
> time to work on it and now I can't seem to find any time for it.
> But hopefully soon I'll have some time for this again (famous
> last words).

Oh good, at least my feeling that things are a bit messy here wasn't 
wrong. :) But considering what you said, I am not sure where does that 
leave "me"?

I need to have working Y-tiled scanout as a dependency for another 
feature we are working on. Former is mostly implemented by Damien, just 
needs this per plane watermark corrections.

I can put in some hacks to make it work locally and unblock development, 
but I think sooner rather than later we will need a proper solution. If 
nothing then to move Damien's work out of the private branch.

Or in other words, is there something someone could do to help you find 
some time to finish the stuff you got pending?

Regards,

Tvrtko