Re: [PATCH v2 04/20] drm/i915: Do not update pfit state when toggling crtc enabled.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 07-07-15 om 11:26 schreef Daniel Vetter:
> On Tue, Jul 07, 2015 at 09:08:15AM +0200, Maarten Lankhorst wrote:
>> This must be done in advance, and during crtc_disable all scalers
> "in advance" ... before what exactly? Yes I'm harping a bit about commit
> messages today ;-)
>
>> can be force disabled.
> Why does it matter that all scalers can be force disabled?
>
>> This means intel_atomic_setup_scalers is only called in 1 place now,
>> during crtc_check.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> tbh I don't really understand what's the problem and what's the solution
> implemented here. Judging from comments the trouble is that we don't
> correctly recover the pfit state for skl scalers at hw readout time?
>
> I guess we should fix that for at lest the crtc level scaler (including
> cross-checking of all scaler state) and in sanitize_plane force-disabling
> any plane that is using a scaler. But really not much clue here.
intel_atomic_setup_scalers should only be called for the new state,
but it's called on the old state which doesn't need to be modified.

The crtc_disable function can just disable all scalers since there's no point in
doing a full recheck.

Alternatively it could only disable the crtc scaler id if it's >= 0, but I think
disabling all might be better for paranoia reasons.

>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  | 14 ++------
>>  drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++-------------
>>  drivers/gpu/drm/i915/intel_dp.c      |  2 +-
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>  4 files changed, 48 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 0aeced82201e..429677a111d5 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -272,17 +272,12 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
> This function is way too big. Yeah not your doing, but imo would be good
> to split it up a bit. Especially since it already has a comment explaining
> the high-level flow which would be a good pattern to structure the
> subfunction extraction after.
I think adding a pipe_to_scaler_id or something would remove the whole plane
special casing block and make it a lot more readable.
Not checking intel_plane->pipe would be a good thing too, not sure how that could even happen.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx