[Intel-gfx] [PATCH 0/7] drm/i915: Add pipe scaler for Gen9 in atomic path

Thu Nov 3 09:23:47 UTC 2016

Hi,

Please review the patches and comments.

On 10/28/2016 6:05 PM, Nabendu Maiti wrote:
> Attaching old discussion thread for easy reference.
>
> On Tue, Jan 05, 2016 at 05:18:40PM +0200, Ville Syrjälä wrote:
>
>> On Tue, Jan 05, 2016 at 03:50:38PM +0100, Daniel Vetter wrote:
>
>> > On Mon, Jan 04, 2016 at 07:06:15PM +0200, Ville Syrjälä wrote:
>
>> > > On Mon, Jan 04, 2016 at 11:16:39AM +0100, Maarten Lankhorst wrote:
>
>> > > > Hey,
>
>> > > >
>
>> > > > Op 23-12-15 om 12:05 schreef Nabendu Maiti:
>
>> > > > > This patch is adding pipesource size as property as intel
>
>> > > > > property.User application is allowed to change the pipe source
> size in runtime on BXT/SKL.
>
>> > > > > Added the property as inteli crtc property.
>
>> > > > >
>
>> > > > > Comments and suggestions are requested for whether we can keep
>
>> > > > > the property as intel crtc property or move to drm level.
>
>> > > > >
>
>> > > > This property would get lost on a modeset. But why do you need a
> pipe_src property?
>
>> > >
>
>> > > It's needed if we want to use the panel fitter with
>
>> > > non-eDP/LVDS/DSI displays.
>
>> > >
>
>> > > Last time this came up I decided that we want to expose this via a
>
>> > > new "fixed_mode" property. Ie. userspace can choose the actual
>
>> > > display timings by setting the "fixed_mode" property with a
>
>> > > specific mode, and then the normal mode property will basically
>
>> > > just control just the pipe src size just like it already does for
>
>> > > eDP/LVDS/DSI where we already have the fixed_mode internally (just
>
>> > > not exposed to usersapce). If the fixed_mode is not specified,
>
>> > > things will work just as they do right now. Obviously for
>
>> > > eDP/LVDS/DSI we will reject any request to change/unset the fixed
> mode.
>
>> > >
>
>> > > The benefit of that approach is that things will work exactly the
>
>> > > same way as before unless the user explicitly sets the fixed_mode,
>
>> > > and once it's set thigns will work exactly the same way as they
>
>> > > have worked so far for eDP/LVDS/DSI. Also it keeps the policy of
>
>> > > choosing the fixed mode strictly is userspace for external displays.
>
>> > >
>
>> > > And as a bonus we will also expose the eDP/LVDS/DSI fixed_mode to
>
>> > > userspace giving userspace more information on what the actual
>
>> > > panel timings are. Currently userspace has to more or less guess
>
>> > > that one of the modes the connector claims to support has the
>
>> > > actual panel timings.
>
>> > >
>
>> > > So far I've not heard any opposing opinions to this plan.
>
>> >
>
>> > Hm yeah, pipe_src would run into all kinds of fun in conjunction
>
>> > with the existing fixed mode stuff we have. Just exposing the fixed
>
>> > as a prop might be simpler. But then we need to figure out what to
>
>> > do wrt the clock in the mode passed in through userspace - currently
>
>> > the fixed mode always wins, but for manual DRRS it would be nice if
>
>> > userspace could control it, and doesn't have to know that there's a
> fixed_mode.
>
>>
>
>> We could have the user mode vrefresh indicate the desired refresh rate
>
>> of the fixed mode as well. In fact I've been wanting to add a check to
>
>> make sure the user mode refresh rate isn't too far off from the
>
>> fixed/downlclock mode vrefresh, but didn't get around to it yet.
>
>
>
> Yeah agreed, userspace vrefresh should control (or at least be checked
>
> against) the fixed mode vrefresh.
>
>
>
>> > So semantically I think only exposing the pipe src to expose panel
>
>> > fitters would be cleaner. Then we'd need to deal with some internal
>
>> > troubles to make sure we combine everything correctly, but that
>
>> > shouldn't be too hard really.
>
>>
>
>> I think it's quite a bit more work since we have to redo all the fb
>
>> size checks and whatnot to use the property when specified. I once
>
>> outlined a more detailed plan for his approach too (too lazy to dig up
>
>> the mail), but I think the fixed mode prop is a simpler approach and
>
>> won't actually require much in the way of userspace changes either.
>
>> It'll be enough to set the property once and then even the legacy
>
>> modeset ioctls will work exactly like they do now for eDP/LVDS/DSI.
>
>
>
> One downside of the fixed mode against an explicit pipe src rectangle is
> that the explict rectangle allows us to letter/pillar/stretch/move too.
>
> With just a fixed_mode that's much harder to pull off.
>
>
>
> I think for i915 it should be fairly ok-ish to implement this. We just
> need to move pipe src rectangle to drm_crtc_state, and adjust all the
> pfit config logic to work on the pipe level. Panels would then only
> replace the output mode with the fixed panel mode, and leave
> scaling/pfit selection to the core crtc code.
>
> -Daniel
>
> --
>
> Daniel Vetter
>
> Software Engineer, Intel Corporation
>
> http://blog.ffwll.ch
>
> _______________________________________________
>
> Intel-gfx mailing list
>
> Intel-gfx at lists.freedesktop.org <mailto:Intel-gfx at lists.freedesktop.org>
>
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> ..>
> On 10/03/2016 04:57 PM, Ville Syrjälä wrote:
>> On Wed, Sep 21, 2016 at 07:47:37PM +0530, Maiti, Nabendu Bikash wrote:
>>> Hi,
>>>
>>>
>>> On 9/20/2016 1:55 PM, Ville Syrjälä wrote:
>>>> On Tue, Aug 30, 2016 at 10:30:54AM +0530, Nabendu Maiti wrote:
>>>>> Following patch series add pipe scaler functionality in atomic path.The pipe
>>>>> scaler can be changed dynamically without modeset.Apart from default panel
>>>>> fitter supported scaling modes -CENTER/ASPECT/FULLSCREEN custom scaling mode
>>>>> mode is added as there is no such restriction on GEn9 pipe scaler.
>>>> Some quick observations:
>>>> - missing any interaction with drm core, so all generic fb size checks
>>>>   and whatnot will not work out, I think
>>> Pipe scaler is not dependent on fp I think. We have fb size checks are
>>> done in plane check.
>> You need to explain how this all interacts with the legacy pipe/plane
>> size == mode hdisplay/vdisplay stuff.
> There is no direct interaction with the plane. This properties use
> current modeset timings - horizontal and
> vertical timing as stored in pipe_src_w and pipe_src_h variables. They
> are filled just after pll and clock are set
> and adjusted modeset structures are filled up. This design will not
> interfere with current existing code of pipe/plane.
>
> Clipping and clamping on plane level is already done on Blended data
> output of  pipe using existing code. pipe
> level clamping and clipping will happen when pipe scaler using out of
> boundary value. Current code use clamping only.
>
>>>> - the way it's done goes against what I've suggested in the past. My
>>>>   idea has been to add a "fixed mode" property to connectors instead,
>>>>   which I think would minimize the impact on the core, and it would
>>>>   follow closely the way eDP/LVDS/DSI already works today.
>>> yes using fixed mode we can do also but I wanted to be part of crtc
>>> property instead of connector property. As fixed mode is basically
>>> intended for fixed mode panels.But we may use pipe scaler for fixed mode
>>> and dynamic mode panels.
>> That doesn't say much. The fixed mode apporach, I think, might be easier
>> to incorporate in a way that keeps the legacy apporach working. Adding a
>> totally different way to configure the pipe src size will mean more weird
>> interactions between the properties.
>>
>> Also it culd be supported with non-atomic userspace reasonably easily.
>> We'll need some sort of userspace for this anyway, otherwise it's just
>> untested/unused code.
> There will be no change in legacy code path. They will work as it is.
> Only downside will be mixup of legacy and non-atomic use.
> But individually they should work as fine as before. The configuration
> of pipe_src is done as before. They will be
> changed if user explicitly want to change them. Internal checks are
> there if some odd unaccepted values are provided.
>
> Yes I can write the userspace igt if this design is fine. This design is
> tested with inhouse val tool (igt based) on android
> plaatform.
>>>> - There's no need to restrict the feature to gen9+ only. It should work
>>>>   out just fine for at least all pch platforms. gmch platforms would be
>>>>   more challenging
>>> This code I designed to use gen9+, and properties like crtc destination
>>> size and offsets also exposed.There is no restrictions on modes (eg.
>>> pillerbox/letterbox) and down scaling ratios as previous platforms.
>>> Currently scaling mode is part of connector property and implemented as
>>> legacy property. I created new scaling mode as atomic property. I think
>>> gen9+ onward platforms may have proper atomic pipe scaling properties
>>> and user space may use it fully dynamically without modeset.
>> None of that tells me why it's gen9+ only. IIRC the panel fitter
>> configuration been very flexible ever since ILK, so the only real
>> difference should be which registers to write.
> There is no hardware pipe scaler before gen7 (VLV/BXT). VLV +CHT
> platform also can be supported on this design
> with little modification. Also Letterbox/pillerbox modes can be
> programmatically selected and according to proper
> register write. But as far as I know old platforms has limited
> functionality of panel fitting either by changing clock/ doing
> nearby supported modeset. I understand fixed mode is the timing required
> by panel and adjusted mode is the nearest permissive
> timing available from gen platform. But using hardware scaler we don't
> change timing. We keep the timing intact, but modify the
> pixel data came from blender pipe. We modify the data size by
> enlarging/encorching in between pipe and encoder.
> ..
>>>> - the pfiter_recalculate thing looks pretty wrong atomic wise
>>> Sorry, I couldn't get it. Are you referring pipe scaler registers are
>>> not written together with other registers?  pfiter_calculate only
>>> calculate and stores the data for later commit. Please provide more
>>> details on it.
>> It's going through encoder->crtc links and whanot. That's not going
>> to fly.
> There is no use of encoder->crtc in the design, everything maintained in
> drm crtc level. (Note Initial few patches using the legacy encoder->crtc
> iteration to find current scaling mode). Final code will work
> independently without using legacy fitting mode.
> Pfit_calculate find out the pipe out size and position in panel if not
> provided by user depending on the source size.
> One use case for this kind of design may be following. One document
> visibile on panel using several planes. Now clicking on it
> will enlarge some part of the document and fit the display without
> blanking (no modeset happens).And go back to original
> on another double click. This is the the dynamic way we change pipe
> scaler config pillerbox/letterbox etc.
>
> Regards,
> Nabendu
>
>>>>> Nabendu Maiti (7):
>>>>>   drm/i915: Add pipe scaler pipe source drm property
>>>>>   drm/i915: Add pipe_src size property calculations in atomic path.
>>>>>   drm/i915: Panel fitting calculation for GEN9
>>>>>   drm/i915: Adding atomic fitting mode property for GEN9
>>>>>   drm/i915: Add pipe scaler co-ordinate and size property for Gen9
>>>>>   drm/i915: Update pipe-scaler according to destination size
>>>>>   drm/i915: Pipescaler destination size limit check on Gen9
>>>>>
>>>>>  drivers/gpu/drm/drm_atomic.c         |  35 ++++++++++
>>>>>  drivers/gpu/drm/drm_crtc.c           |  56 +++++++++++++++
>>>>>  drivers/gpu/drm/i915/intel_display.c | 128 +++++++++++++++++++++++++++++++++--
>>>>>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>>>>>  drivers/gpu/drm/i915/intel_panel.c   |  34 +++++++++-
>>>>>  include/drm/drm_crtc.h               |  14 ++++
>>>>>  include/uapi/drm/drm_mode.h          |   1 +
>>>>>  7 files changed, 263 insertions(+), 8 deletions(-)
>>>>>
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>> --
>>> Regards,
>>> Nabendu
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

-- 
Regards,
Nabendu