[PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Previously rotation was ignored and wrong stride programmed
into the plane registers resulting in a corrupt image on screen.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 539c3737e823..6328788193e4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_plane *plane = intel_crtc->base.primary;
 	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
 	const enum pipe pipe = intel_crtc->pipe;
-	u32 ctl, stride;
+	u32 ctl, stride, tile_height;
 
 	ctl = I915_READ(PLANE_CTL(pipe, 0));
 	ctl &= ~PLANE_CTL_TILED_MASK;
@@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
 	 * The stride is either expressed as a multiple of 64 bytes chunks for
 	 * linear buffers or in number of tiles for tiled buffers.
 	 */
-	stride = fb->pitches[0] /
-		 intel_fb_stride_alignment(dev, fb->modifier[0],
-					   fb->pixel_format);
+	if (intel_rotation_90_or_270(plane->state->rotation)) {
+		/* stride = Surface height in tiles */
+		tile_height = intel_tile_height(dev, fb->pixel_format,
+						fb->modifier[0], 0);
+		stride = DIV_ROUND_UP(fb->height, tile_height);
+	} else {
+		stride = fb->pitches[0] /
+				intel_fb_stride_alignment(dev, fb->modifier[0],
+							  fb->pixel_format);
+	}
 
 	/*
 	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

Hi Tvrtko,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: x86_64-randconfig-x013-201540 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_display.c: In function 'skl_do_mmio_flip':
>> drivers/gpu/drm/i915/intel_display.c:11119:17: error: too many arguments to function 'intel_tile_height'
      tile_height = intel_tile_height(dev, fb->pixel_format,
                    ^
   drivers/gpu/drm/i915/intel_display.c:2231:1: note: declared here
    intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
    ^

vim +/intel_tile_height +11119 drivers/gpu/drm/i915/intel_display.c

 11113		/*
 11114		 * The stride is either expressed as a multiple of 64 bytes chunks for
 11115		 * linear buffers or in number of tiles for tiled buffers.
 11116		 */
 11117		if (intel_rotation_90_or_270(plane->state->rotation)) {
 11118			/* stride = Surface height in tiles */
 11119			tile_height = intel_tile_height(dev, fb->pixel_format,
 11120							fb->modifier[0], 0);
 11121			stride = DIV_ROUND_UP(fb->height, tile_height);
 11122		} else {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 19899 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Jindal, Sonika]

On 10/7/2015 3:31 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Previously rotation was ignored and wrong stride programmed
> into the plane registers resulting in a corrupt image on screen.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 539c3737e823..6328788193e4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>   {
>   	struct drm_device *dev = intel_crtc->base.dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_plane *plane = intel_crtc->base.primary;
>   	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
>   	const enum pipe pipe = intel_crtc->pipe;
> -	u32 ctl, stride;
> +	u32 ctl, stride, tile_height;
>
>   	ctl = I915_READ(PLANE_CTL(pipe, 0));
>   	ctl &= ~PLANE_CTL_TILED_MASK;
> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>   	 * The stride is either expressed as a multiple of 64 bytes chunks for
>   	 * linear buffers or in number of tiles for tiled buffers.
>   	 */
> -	stride = fb->pitches[0] /
> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
> -					   fb->pixel_format);
> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
> +		/* stride = Surface height in tiles */
> +		tile_height = intel_tile_height(dev, fb->pixel_format,
> +						fb->modifier[0], 0);
> +		stride = DIV_ROUND_UP(fb->height, tile_height);
Wouldn't we need a PLANE_SIZE update somewhere in case of 90/270? For 
the cases where the plane is not square and can fit after rotation as well?

> +	} else {
> +		stride = fb->pitches[0] /
> +				intel_fb_stride_alignment(dev, fb->modifier[0],
> +							  fb->pixel_format);
> +	}
>
>   	/*
>   	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Tvrtko Ursulin]

On 07/10/15 13:10, Jindal, Sonika wrote:
>
>
> On 10/7/2015 3:31 PM, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Previously rotation was ignored and wrong stride programmed
>> into the plane registers resulting in a corrupt image on screen.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 539c3737e823..6328788193e4 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct
>> intel_crtc *intel_crtc)
>>   {
>>       struct drm_device *dev = intel_crtc->base.dev;
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>> +    struct drm_plane *plane = intel_crtc->base.primary;
>>       struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
>>       const enum pipe pipe = intel_crtc->pipe;
>> -    u32 ctl, stride;
>> +    u32 ctl, stride, tile_height;
>>
>>       ctl = I915_READ(PLANE_CTL(pipe, 0));
>>       ctl &= ~PLANE_CTL_TILED_MASK;
>> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct
>> intel_crtc *intel_crtc)
>>        * The stride is either expressed as a multiple of 64 bytes
>> chunks for
>>        * linear buffers or in number of tiles for tiled buffers.
>>        */
>> -    stride = fb->pitches[0] /
>> -         intel_fb_stride_alignment(dev, fb->modifier[0],
>> -                       fb->pixel_format);
>> +    if (intel_rotation_90_or_270(plane->state->rotation)) {
>> +        /* stride = Surface height in tiles */
>> +        tile_height = intel_tile_height(dev, fb->pixel_format,
>> +                        fb->modifier[0], 0);
>> +        stride = DIV_ROUND_UP(fb->height, tile_height);
> Wouldn't we need a PLANE_SIZE update somewhere in case of 90/270? For
> the cases where the plane is not square and can fit after rotation as well?

We can just say that orientation changes are not allowed between page 
flips. Or it can be improved later if I did not understand what you 
mean? Because without this patch page flipping when rotated 90/270 does 
not work at all.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Tvrtko Ursulin]

Hi,

On 07/10/15 13:15, Tvrtko Ursulin wrote:
>
> On 07/10/15 13:10, Jindal, Sonika wrote:
>>
>>
>> On 10/7/2015 3:31 PM, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Previously rotation was ignored and wrong stride programmed
>>> into the plane registers resulting in a corrupt image on screen.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Sonika Jindal <sonika.jindal@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index 539c3737e823..6328788193e4 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct
>>> intel_crtc *intel_crtc)
>>>   {
>>>       struct drm_device *dev = intel_crtc->base.dev;
>>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>> +    struct drm_plane *plane = intel_crtc->base.primary;
>>>       struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
>>>       const enum pipe pipe = intel_crtc->pipe;
>>> -    u32 ctl, stride;
>>> +    u32 ctl, stride, tile_height;
>>>
>>>       ctl = I915_READ(PLANE_CTL(pipe, 0));
>>>       ctl &= ~PLANE_CTL_TILED_MASK;
>>> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct
>>> intel_crtc *intel_crtc)
>>>        * The stride is either expressed as a multiple of 64 bytes
>>> chunks for
>>>        * linear buffers or in number of tiles for tiled buffers.
>>>        */
>>> -    stride = fb->pitches[0] /
>>> -         intel_fb_stride_alignment(dev, fb->modifier[0],
>>> -                       fb->pixel_format);
>>> +    if (intel_rotation_90_or_270(plane->state->rotation)) {
>>> +        /* stride = Surface height in tiles */
>>> +        tile_height = intel_tile_height(dev, fb->pixel_format,
>>> +                        fb->modifier[0], 0);
>>> +        stride = DIV_ROUND_UP(fb->height, tile_height);
>> Wouldn't we need a PLANE_SIZE update somewhere in case of 90/270? For
>> the cases where the plane is not square and can fit after rotation as
>> well?
>
> We can just say that orientation changes are not allowed between page
> flips. Or it can be improved later if I did not understand what you
> mean? Because without this patch page flipping when rotated 90/270 does
> not work at all.

What do you say, is this acceptable? I would need an r-b now that the 
IGT sub-test has been merged.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Ville Syrjälä]

On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Previously rotation was ignored and wrong stride programmed
> into the plane registers resulting in a corrupt image on screen.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 539c3737e823..6328788193e4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_plane *plane = intel_crtc->base.primary;
>  	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
>  	const enum pipe pipe = intel_crtc->pipe;
> -	u32 ctl, stride;
> +	u32 ctl, stride, tile_height;
>  
>  	ctl = I915_READ(PLANE_CTL(pipe, 0));
>  	ctl &= ~PLANE_CTL_TILED_MASK;
> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>  	 * The stride is either expressed as a multiple of 64 bytes chunks for
>  	 * linear buffers or in number of tiles for tiled buffers.
>  	 */
> -	stride = fb->pitches[0] /
> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
> -					   fb->pixel_format);
> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
> +		/* stride = Surface height in tiles */
> +		tile_height = intel_tile_height(dev, fb->pixel_format,
> +						fb->modifier[0], 0);
> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> +	} else {
> +		stride = fb->pitches[0] /
> +				intel_fb_stride_alignment(dev, fb->modifier[0],
> +							  fb->pixel_format);
> +	}

I was wondering why we are allowing stride changes during page flip, but
after looking at the history it seems we are not. The reason for
updating the stride register is the fact that the units we specify it
in change between different tiling modes on SKL+. We still reject actual
stride changes during page flip, which is good because allowing it would
cause problems for my fb->offsets[] stuff since the interpretation of the
linear offset would change with the stride.

We do allow changes to the rotated stride though since we don't reject
changes to the fb height. I think I need to draw some pictures before I
can say for sure whether that can cause problems or not. But we ca
leave that for another patch if it turns out to need handling.

One thing that's dodgy here is the plane->state->rotation check. I
think currently we wait for pending flips during the atomic commit
phase after we've swapped the state. So this may end up using the
wrong rotation setting. It would be an even bigger problem if we
already allowed queueing up or replaceing pending plane updates. I
suppose the primary->fb thing doesn't suffer from this problem because
we swap that pointer only after we've waited for pending flips.

>  	/*
>  	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Daniel Vetter]

On Mon, Oct 19, 2015 at 09:20:36PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Previously rotation was ignored and wrong stride programmed
> > into the plane registers resulting in a corrupt image on screen.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 539c3737e823..6328788193e4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >  {
> >  	struct drm_device *dev = intel_crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_plane *plane = intel_crtc->base.primary;
> >  	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
> >  	const enum pipe pipe = intel_crtc->pipe;
> > -	u32 ctl, stride;
> > +	u32 ctl, stride, tile_height;
> >  
> >  	ctl = I915_READ(PLANE_CTL(pipe, 0));
> >  	ctl &= ~PLANE_CTL_TILED_MASK;
> > @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >  	 * The stride is either expressed as a multiple of 64 bytes chunks for
> >  	 * linear buffers or in number of tiles for tiled buffers.
> >  	 */
> > -	stride = fb->pitches[0] /
> > -		 intel_fb_stride_alignment(dev, fb->modifier[0],
> > -					   fb->pixel_format);
> > +	if (intel_rotation_90_or_270(plane->state->rotation)) {
> > +		/* stride = Surface height in tiles */
> > +		tile_height = intel_tile_height(dev, fb->pixel_format,
> > +						fb->modifier[0], 0);
> > +		stride = DIV_ROUND_UP(fb->height, tile_height);
> > +	} else {
> > +		stride = fb->pitches[0] /
> > +				intel_fb_stride_alignment(dev, fb->modifier[0],
> > +							  fb->pixel_format);
> > +	}
> 
> I was wondering why we are allowing stride changes during page flip, but
> after looking at the history it seems we are not. The reason for
> updating the stride register is the fact that the units we specify it
> in change between different tiling modes on SKL+. We still reject actual
> stride changes during page flip, which is good because allowing it would
> cause problems for my fb->offsets[] stuff since the interpretation of the
> linear offset would change with the stride.
> 
> We do allow changes to the rotated stride though since we don't reject
> changes to the fb height. I think I need to draw some pictures before I
> can say for sure whether that can cause problems or not. But we ca
> leave that for another patch if it turns out to need handling.
> 
> One thing that's dodgy here is the plane->state->rotation check. I
> think currently we wait for pending flips during the atomic commit
> phase after we've swapped the state. So this may end up using the
> wrong rotation setting. It would be an even bigger problem if we
> already allowed queueing up or replaceing pending plane updates. I
> suppose the primary->fb thing doesn't suffer from this problem because
> we swap that pointer only after we've waited for pending flips.

Current rule is that pageflip doesn't allow any change in any metadata.
There's some minor exception that on some platforms we can change the
tiling because someone asked for that specifically and it's possible.

atomic flips will be able to cope with this. But for legacy pageflips imo
reject everything aggressively that changes metadata (stride, tiling,
rotation).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Tvrtko Ursulin]

On 20/10/15 08:42, Daniel Vetter wrote:
> On Mon, Oct 19, 2015 at 09:20:36PM +0300, Ville Syrjälä wrote:
>> On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Previously rotation was ignored and wrong stride programmed
>>> into the plane registers resulting in a corrupt image on screen.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Sonika Jindal <sonika.jindal@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 539c3737e823..6328788193e4 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>>>   {
>>>   	struct drm_device *dev = intel_crtc->base.dev;
>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	struct drm_plane *plane = intel_crtc->base.primary;
>>>   	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
>>>   	const enum pipe pipe = intel_crtc->pipe;
>>> -	u32 ctl, stride;
>>> +	u32 ctl, stride, tile_height;
>>>
>>>   	ctl = I915_READ(PLANE_CTL(pipe, 0));
>>>   	ctl &= ~PLANE_CTL_TILED_MASK;
>>> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>>>   	 * The stride is either expressed as a multiple of 64 bytes chunks for
>>>   	 * linear buffers or in number of tiles for tiled buffers.
>>>   	 */
>>> -	stride = fb->pitches[0] /
>>> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
>>> -					   fb->pixel_format);
>>> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
>>> +		/* stride = Surface height in tiles */
>>> +		tile_height = intel_tile_height(dev, fb->pixel_format,
>>> +						fb->modifier[0], 0);
>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>> +	} else {
>>> +		stride = fb->pitches[0] /
>>> +				intel_fb_stride_alignment(dev, fb->modifier[0],
>>> +							  fb->pixel_format);
>>> +	}
>>
>> I was wondering why we are allowing stride changes during page flip, but
>> after looking at the history it seems we are not. The reason for
>> updating the stride register is the fact that the units we specify it
>> in change between different tiling modes on SKL+. We still reject actual
>> stride changes during page flip, which is good because allowing it would
>> cause problems for my fb->offsets[] stuff since the interpretation of the
>> linear offset would change with the stride.
>>
>> We do allow changes to the rotated stride though since we don't reject
>> changes to the fb height. I think I need to draw some pictures before I
>> can say for sure whether that can cause problems or not. But we ca
>> leave that for another patch if it turns out to need handling.
>>
>> One thing that's dodgy here is the plane->state->rotation check. I
>> think currently we wait for pending flips during the atomic commit
>> phase after we've swapped the state. So this may end up using the
>> wrong rotation setting. It would be an even bigger problem if we
>> already allowed queueing up or replaceing pending plane updates. I
>> suppose the primary->fb thing doesn't suffer from this problem because
>> we swap that pointer only after we've waited for pending flips.
>
> Current rule is that pageflip doesn't allow any change in any metadata.
> There's some minor exception that on some platforms we can change the
> tiling because someone asked for that specifically and it's possible.
>
> atomic flips will be able to cope with this. But for legacy pageflips imo
> reject everything aggressively that changes metadata (stride, tiling,
> rotation).

I am not sure what is the conclusion. To re-iterate, idea is not to 
allow rotation changes between page flips, just to program PLANE_STRIDE 
accordingly, when rotation is already enabled. Since otherwise page flip 
will calculate it incorrectly.

I though Ville was raising two concerns:

1. Will the plane state be swapped from under the pending page flips 
prematurely?

2. Do we need to program the stride in page flips at all?

Did I get that right? Unless 1) is true it looks safe to me to have this 
patch, and if answer to 2) is no then we could rip out more code?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Ville Syrjälä]

On Tue, Oct 20, 2015 at 10:06:58AM +0100, Tvrtko Ursulin wrote:
> 
> On 20/10/15 08:42, Daniel Vetter wrote:
> > On Mon, Oct 19, 2015 at 09:20:36PM +0300, Ville Syrjälä wrote:
> >> On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
> >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>> Previously rotation was ignored and wrong stride programmed
> >>> into the plane registers resulting in a corrupt image on screen.
> >>>
> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Cc: Sonika Jindal <sonika.jindal@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
> >>>   1 file changed, 12 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 539c3737e823..6328788193e4 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >>>   {
> >>>   	struct drm_device *dev = intel_crtc->base.dev;
> >>>   	struct drm_i915_private *dev_priv = dev->dev_private;
> >>> +	struct drm_plane *plane = intel_crtc->base.primary;
> >>>   	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
> >>>   	const enum pipe pipe = intel_crtc->pipe;
> >>> -	u32 ctl, stride;
> >>> +	u32 ctl, stride, tile_height;
> >>>
> >>>   	ctl = I915_READ(PLANE_CTL(pipe, 0));
> >>>   	ctl &= ~PLANE_CTL_TILED_MASK;
> >>> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >>>   	 * The stride is either expressed as a multiple of 64 bytes chunks for
> >>>   	 * linear buffers or in number of tiles for tiled buffers.
> >>>   	 */
> >>> -	stride = fb->pitches[0] /
> >>> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
> >>> -					   fb->pixel_format);
> >>> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
> >>> +		/* stride = Surface height in tiles */
> >>> +		tile_height = intel_tile_height(dev, fb->pixel_format,
> >>> +						fb->modifier[0], 0);
> >>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>> +	} else {
> >>> +		stride = fb->pitches[0] /
> >>> +				intel_fb_stride_alignment(dev, fb->modifier[0],
> >>> +							  fb->pixel_format);
> >>> +	}
> >>
> >> I was wondering why we are allowing stride changes during page flip, but
> >> after looking at the history it seems we are not. The reason for
> >> updating the stride register is the fact that the units we specify it
> >> in change between different tiling modes on SKL+. We still reject actual
> >> stride changes during page flip, which is good because allowing it would
> >> cause problems for my fb->offsets[] stuff since the interpretation of the
> >> linear offset would change with the stride.
> >>
> >> We do allow changes to the rotated stride though since we don't reject
> >> changes to the fb height. I think I need to draw some pictures before I
> >> can say for sure whether that can cause problems or not. But we ca
> >> leave that for another patch if it turns out to need handling.
> >>
> >> One thing that's dodgy here is the plane->state->rotation check. I
> >> think currently we wait for pending flips during the atomic commit
> >> phase after we've swapped the state. So this may end up using the
> >> wrong rotation setting. It would be an even bigger problem if we
> >> already allowed queueing up or replaceing pending plane updates. I
> >> suppose the primary->fb thing doesn't suffer from this problem because
> >> we swap that pointer only after we've waited for pending flips.
> >
> > Current rule is that pageflip doesn't allow any change in any metadata.
> > There's some minor exception that on some platforms we can change the
> > tiling because someone asked for that specifically and it's possible.
> >
> > atomic flips will be able to cope with this. But for legacy pageflips imo
> > reject everything aggressively that changes metadata (stride, tiling,
> > rotation).
> 
> I am not sure what is the conclusion. To re-iterate, idea is not to 
> allow rotation changes between page flips, just to program PLANE_STRIDE 
> accordingly, when rotation is already enabled. Since otherwise page flip 
> will calculate it incorrectly.
> 
> I though Ville was raising two concerns:
> 
> 1. Will the plane state be swapped from under the pending page flips 
> prematurely?

The answer is yes.

> 
> 2. Do we need to program the stride in page flips at all?

Yes, on SKL+. Because we allow tiling changes. We've always allowed that
because CS flips allow it as well. And IIRC we even had some specific
user for that since someone noticed that it didn't actually work on VLV.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Tvrtko Ursulin]

On 20/10/15 12:07, Ville Syrjälä wrote:
> On Tue, Oct 20, 2015 at 10:06:58AM +0100, Tvrtko Ursulin wrote:
>>
>> On 20/10/15 08:42, Daniel Vetter wrote:
>>> On Mon, Oct 19, 2015 at 09:20:36PM +0300, Ville Syrjälä wrote:
>>>> On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> Previously rotation was ignored and wrong stride programmed
>>>>> into the plane registers resulting in a corrupt image on screen.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Sonika Jindal <sonika.jindal@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>>>>>    1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 539c3737e823..6328788193e4 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>>>>>    {
>>>>>    	struct drm_device *dev = intel_crtc->base.dev;
>>>>>    	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>> +	struct drm_plane *plane = intel_crtc->base.primary;
>>>>>    	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
>>>>>    	const enum pipe pipe = intel_crtc->pipe;
>>>>> -	u32 ctl, stride;
>>>>> +	u32 ctl, stride, tile_height;
>>>>>
>>>>>    	ctl = I915_READ(PLANE_CTL(pipe, 0));
>>>>>    	ctl &= ~PLANE_CTL_TILED_MASK;
>>>>> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>>>>>    	 * The stride is either expressed as a multiple of 64 bytes chunks for
>>>>>    	 * linear buffers or in number of tiles for tiled buffers.
>>>>>    	 */
>>>>> -	stride = fb->pitches[0] /
>>>>> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
>>>>> -					   fb->pixel_format);
>>>>> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
>>>>> +		/* stride = Surface height in tiles */
>>>>> +		tile_height = intel_tile_height(dev, fb->pixel_format,
>>>>> +						fb->modifier[0], 0);
>>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>>>> +	} else {
>>>>> +		stride = fb->pitches[0] /
>>>>> +				intel_fb_stride_alignment(dev, fb->modifier[0],
>>>>> +							  fb->pixel_format);
>>>>> +	}
>>>>
>>>> I was wondering why we are allowing stride changes during page flip, but
>>>> after looking at the history it seems we are not. The reason for
>>>> updating the stride register is the fact that the units we specify it
>>>> in change between different tiling modes on SKL+. We still reject actual
>>>> stride changes during page flip, which is good because allowing it would
>>>> cause problems for my fb->offsets[] stuff since the interpretation of the
>>>> linear offset would change with the stride.
>>>>
>>>> We do allow changes to the rotated stride though since we don't reject
>>>> changes to the fb height. I think I need to draw some pictures before I
>>>> can say for sure whether that can cause problems or not. But we ca
>>>> leave that for another patch if it turns out to need handling.
>>>>
>>>> One thing that's dodgy here is the plane->state->rotation check. I
>>>> think currently we wait for pending flips during the atomic commit
>>>> phase after we've swapped the state. So this may end up using the
>>>> wrong rotation setting. It would be an even bigger problem if we
>>>> already allowed queueing up or replaceing pending plane updates. I
>>>> suppose the primary->fb thing doesn't suffer from this problem because
>>>> we swap that pointer only after we've waited for pending flips.
>>>
>>> Current rule is that pageflip doesn't allow any change in any metadata.
>>> There's some minor exception that on some platforms we can change the
>>> tiling because someone asked for that specifically and it's possible.
>>>
>>> atomic flips will be able to cope with this. But for legacy pageflips imo
>>> reject everything aggressively that changes metadata (stride, tiling,
>>> rotation).
>>
>> I am not sure what is the conclusion. To re-iterate, idea is not to
>> allow rotation changes between page flips, just to program PLANE_STRIDE
>> accordingly, when rotation is already enabled. Since otherwise page flip
>> will calculate it incorrectly.
>>
>> I though Ville was raising two concerns:
>>
>> 1. Will the plane state be swapped from under the pending page flips
>> prematurely?
>
> The answer is yes.

And is that OK? Would it not make more sense to wait for pending flips 
and then swap state?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Ville Syrjälä]

On Tue, Oct 20, 2015 at 12:44:05PM +0100, Tvrtko Ursulin wrote:
> 
> On 20/10/15 12:07, Ville Syrjälä wrote:
> > On Tue, Oct 20, 2015 at 10:06:58AM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 20/10/15 08:42, Daniel Vetter wrote:
> >>> On Mon, Oct 19, 2015 at 09:20:36PM +0300, Ville Syrjälä wrote:
> >>>> On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
> >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>
> >>>>> Previously rotation was ignored and wrong stride programmed
> >>>>> into the plane registers resulting in a corrupt image on screen.
> >>>>>
> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>> Cc: Sonika Jindal <sonika.jindal@intel.com>
> >>>>> ---
> >>>>>    drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
> >>>>>    1 file changed, 12 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index 539c3737e823..6328788193e4 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >>>>>    {
> >>>>>    	struct drm_device *dev = intel_crtc->base.dev;
> >>>>>    	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>> +	struct drm_plane *plane = intel_crtc->base.primary;
> >>>>>    	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
> >>>>>    	const enum pipe pipe = intel_crtc->pipe;
> >>>>> -	u32 ctl, stride;
> >>>>> +	u32 ctl, stride, tile_height;
> >>>>>
> >>>>>    	ctl = I915_READ(PLANE_CTL(pipe, 0));
> >>>>>    	ctl &= ~PLANE_CTL_TILED_MASK;
> >>>>> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >>>>>    	 * The stride is either expressed as a multiple of 64 bytes chunks for
> >>>>>    	 * linear buffers or in number of tiles for tiled buffers.
> >>>>>    	 */
> >>>>> -	stride = fb->pitches[0] /
> >>>>> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
> >>>>> -					   fb->pixel_format);
> >>>>> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
> >>>>> +		/* stride = Surface height in tiles */
> >>>>> +		tile_height = intel_tile_height(dev, fb->pixel_format,
> >>>>> +						fb->modifier[0], 0);
> >>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>>>> +	} else {
> >>>>> +		stride = fb->pitches[0] /
> >>>>> +				intel_fb_stride_alignment(dev, fb->modifier[0],
> >>>>> +							  fb->pixel_format);
> >>>>> +	}
> >>>>
> >>>> I was wondering why we are allowing stride changes during page flip, but
> >>>> after looking at the history it seems we are not. The reason for
> >>>> updating the stride register is the fact that the units we specify it
> >>>> in change between different tiling modes on SKL+. We still reject actual
> >>>> stride changes during page flip, which is good because allowing it would
> >>>> cause problems for my fb->offsets[] stuff since the interpretation of the
> >>>> linear offset would change with the stride.
> >>>>
> >>>> We do allow changes to the rotated stride though since we don't reject
> >>>> changes to the fb height. I think I need to draw some pictures before I
> >>>> can say for sure whether that can cause problems or not. But we ca
> >>>> leave that for another patch if it turns out to need handling.
> >>>>
> >>>> One thing that's dodgy here is the plane->state->rotation check. I
> >>>> think currently we wait for pending flips during the atomic commit
> >>>> phase after we've swapped the state. So this may end up using the
> >>>> wrong rotation setting. It would be an even bigger problem if we
> >>>> already allowed queueing up or replaceing pending plane updates. I
> >>>> suppose the primary->fb thing doesn't suffer from this problem because
> >>>> we swap that pointer only after we've waited for pending flips.
> >>>
> >>> Current rule is that pageflip doesn't allow any change in any metadata.
> >>> There's some minor exception that on some platforms we can change the
> >>> tiling because someone asked for that specifically and it's possible.
> >>>
> >>> atomic flips will be able to cope with this. But for legacy pageflips imo
> >>> reject everything aggressively that changes metadata (stride, tiling,
> >>> rotation).
> >>
> >> I am not sure what is the conclusion. To re-iterate, idea is not to
> >> allow rotation changes between page flips, just to program PLANE_STRIDE
> >> accordingly, when rotation is already enabled. Since otherwise page flip
> >> will calculate it incorrectly.
> >>
> >> I though Ville was raising two concerns:
> >>
> >> 1. Will the plane state be swapped from under the pending page flips
> >> prematurely?
> >
> > The answer is yes.
> 
> And is that OK?

No, we could misprogram the stride and corrupt the display.

> Would it not make more sense to wait for pending flips 
> and then swap state?

I think eventually we want to queue up multiple flips and/or allow
later flips to override previous ones, so consulting the current
state from the mmio flip is a not a good idea in any case. I would
just add work->rotation and populate it with a snapshot at the time
when the flip is queued.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Tvrtko Ursulin]

On 20/10/15 12:53, Ville Syrjälä wrote:
> On Tue, Oct 20, 2015 at 12:44:05PM +0100, Tvrtko Ursulin wrote:
>>
>> On 20/10/15 12:07, Ville Syrjälä wrote:
>>> On Tue, Oct 20, 2015 at 10:06:58AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 20/10/15 08:42, Daniel Vetter wrote:
>>>>> On Mon, Oct 19, 2015 at 09:20:36PM +0300, Ville Syrjälä wrote:
>>>>>> On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
>>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>
>>>>>>> Previously rotation was ignored and wrong stride programmed
>>>>>>> into the plane registers resulting in a corrupt image on screen.
>>>>>>>
>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>> Cc: Sonika Jindal <sonika.jindal@intel.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>>>>>>>     1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>>> index 539c3737e823..6328788193e4 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>>>>>>>     {
>>>>>>>     	struct drm_device *dev = intel_crtc->base.dev;
>>>>>>>     	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>> +	struct drm_plane *plane = intel_crtc->base.primary;
>>>>>>>     	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
>>>>>>>     	const enum pipe pipe = intel_crtc->pipe;
>>>>>>> -	u32 ctl, stride;
>>>>>>> +	u32 ctl, stride, tile_height;
>>>>>>>
>>>>>>>     	ctl = I915_READ(PLANE_CTL(pipe, 0));
>>>>>>>     	ctl &= ~PLANE_CTL_TILED_MASK;
>>>>>>> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>>>>>>>     	 * The stride is either expressed as a multiple of 64 bytes chunks for
>>>>>>>     	 * linear buffers or in number of tiles for tiled buffers.
>>>>>>>     	 */
>>>>>>> -	stride = fb->pitches[0] /
>>>>>>> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
>>>>>>> -					   fb->pixel_format);
>>>>>>> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
>>>>>>> +		/* stride = Surface height in tiles */
>>>>>>> +		tile_height = intel_tile_height(dev, fb->pixel_format,
>>>>>>> +						fb->modifier[0], 0);
>>>>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>>>>>> +	} else {
>>>>>>> +		stride = fb->pitches[0] /
>>>>>>> +				intel_fb_stride_alignment(dev, fb->modifier[0],
>>>>>>> +							  fb->pixel_format);
>>>>>>> +	}
>>>>>>
>>>>>> I was wondering why we are allowing stride changes during page flip, but
>>>>>> after looking at the history it seems we are not. The reason for
>>>>>> updating the stride register is the fact that the units we specify it
>>>>>> in change between different tiling modes on SKL+. We still reject actual
>>>>>> stride changes during page flip, which is good because allowing it would
>>>>>> cause problems for my fb->offsets[] stuff since the interpretation of the
>>>>>> linear offset would change with the stride.
>>>>>>
>>>>>> We do allow changes to the rotated stride though since we don't reject
>>>>>> changes to the fb height. I think I need to draw some pictures before I
>>>>>> can say for sure whether that can cause problems or not. But we ca
>>>>>> leave that for another patch if it turns out to need handling.
>>>>>>
>>>>>> One thing that's dodgy here is the plane->state->rotation check. I
>>>>>> think currently we wait for pending flips during the atomic commit
>>>>>> phase after we've swapped the state. So this may end up using the
>>>>>> wrong rotation setting. It would be an even bigger problem if we
>>>>>> already allowed queueing up or replaceing pending plane updates. I
>>>>>> suppose the primary->fb thing doesn't suffer from this problem because
>>>>>> we swap that pointer only after we've waited for pending flips.
>>>>>
>>>>> Current rule is that pageflip doesn't allow any change in any metadata.
>>>>> There's some minor exception that on some platforms we can change the
>>>>> tiling because someone asked for that specifically and it's possible.
>>>>>
>>>>> atomic flips will be able to cope with this. But for legacy pageflips imo
>>>>> reject everything aggressively that changes metadata (stride, tiling,
>>>>> rotation).
>>>>
>>>> I am not sure what is the conclusion. To re-iterate, idea is not to
>>>> allow rotation changes between page flips, just to program PLANE_STRIDE
>>>> accordingly, when rotation is already enabled. Since otherwise page flip
>>>> will calculate it incorrectly.
>>>>
>>>> I though Ville was raising two concerns:
>>>>
>>>> 1. Will the plane state be swapped from under the pending page flips
>>>> prematurely?
>>>
>>> The answer is yes.
>>
>> And is that OK?
>
> No, we could misprogram the stride and corrupt the display.

Oh I did not mean that but if it is OK to replace the state under queued 
flips. Sounded like a bug to me but what do I know.

>> Would it not make more sense to wait for pending flips
>> and then swap state?
>
> I think eventually we want to queue up multiple flips and/or allow
> later flips to override previous ones, so consulting the current
> state from the mmio flip is a not a good idea in any case. I would
> just add work->rotation and populate it with a snapshot at the time
> when the flip is queued.

Yes I can certainly do that if deemed acceptable.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Ville Syrjälä]

On Tue, Oct 20, 2015 at 01:05:22PM +0100, Tvrtko Ursulin wrote:
> 
> On 20/10/15 12:53, Ville Syrjälä wrote:
> > On Tue, Oct 20, 2015 at 12:44:05PM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 20/10/15 12:07, Ville Syrjälä wrote:
> >>> On Tue, Oct 20, 2015 at 10:06:58AM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>> On 20/10/15 08:42, Daniel Vetter wrote:
> >>>>> On Mon, Oct 19, 2015 at 09:20:36PM +0300, Ville Syrjälä wrote:
> >>>>>> On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
> >>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>>
> >>>>>>> Previously rotation was ignored and wrong stride programmed
> >>>>>>> into the plane registers resulting in a corrupt image on screen.
> >>>>>>>
> >>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>> Cc: Sonika Jindal <sonika.jindal@intel.com>
> >>>>>>> ---
> >>>>>>>     drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
> >>>>>>>     1 file changed, 12 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>>>> index 539c3737e823..6328788193e4 100644
> >>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>>>> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >>>>>>>     {
> >>>>>>>     	struct drm_device *dev = intel_crtc->base.dev;
> >>>>>>>     	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>>>> +	struct drm_plane *plane = intel_crtc->base.primary;
> >>>>>>>     	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
> >>>>>>>     	const enum pipe pipe = intel_crtc->pipe;
> >>>>>>> -	u32 ctl, stride;
> >>>>>>> +	u32 ctl, stride, tile_height;
> >>>>>>>
> >>>>>>>     	ctl = I915_READ(PLANE_CTL(pipe, 0));
> >>>>>>>     	ctl &= ~PLANE_CTL_TILED_MASK;
> >>>>>>> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> >>>>>>>     	 * The stride is either expressed as a multiple of 64 bytes chunks for
> >>>>>>>     	 * linear buffers or in number of tiles for tiled buffers.
> >>>>>>>     	 */
> >>>>>>> -	stride = fb->pitches[0] /
> >>>>>>> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
> >>>>>>> -					   fb->pixel_format);
> >>>>>>> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
> >>>>>>> +		/* stride = Surface height in tiles */
> >>>>>>> +		tile_height = intel_tile_height(dev, fb->pixel_format,
> >>>>>>> +						fb->modifier[0], 0);
> >>>>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>>>>>> +	} else {
> >>>>>>> +		stride = fb->pitches[0] /
> >>>>>>> +				intel_fb_stride_alignment(dev, fb->modifier[0],
> >>>>>>> +							  fb->pixel_format);
> >>>>>>> +	}
> >>>>>>
> >>>>>> I was wondering why we are allowing stride changes during page flip, but
> >>>>>> after looking at the history it seems we are not. The reason for
> >>>>>> updating the stride register is the fact that the units we specify it
> >>>>>> in change between different tiling modes on SKL+. We still reject actual
> >>>>>> stride changes during page flip, which is good because allowing it would
> >>>>>> cause problems for my fb->offsets[] stuff since the interpretation of the
> >>>>>> linear offset would change with the stride.
> >>>>>>
> >>>>>> We do allow changes to the rotated stride though since we don't reject
> >>>>>> changes to the fb height. I think I need to draw some pictures before I
> >>>>>> can say for sure whether that can cause problems or not. But we ca
> >>>>>> leave that for another patch if it turns out to need handling.
> >>>>>>
> >>>>>> One thing that's dodgy here is the plane->state->rotation check. I
> >>>>>> think currently we wait for pending flips during the atomic commit
> >>>>>> phase after we've swapped the state. So this may end up using the
> >>>>>> wrong rotation setting. It would be an even bigger problem if we
> >>>>>> already allowed queueing up or replaceing pending plane updates. I
> >>>>>> suppose the primary->fb thing doesn't suffer from this problem because
> >>>>>> we swap that pointer only after we've waited for pending flips.
> >>>>>
> >>>>> Current rule is that pageflip doesn't allow any change in any metadata.
> >>>>> There's some minor exception that on some platforms we can change the
> >>>>> tiling because someone asked for that specifically and it's possible.
> >>>>>
> >>>>> atomic flips will be able to cope with this. But for legacy pageflips imo
> >>>>> reject everything aggressively that changes metadata (stride, tiling,
> >>>>> rotation).
> >>>>
> >>>> I am not sure what is the conclusion. To re-iterate, idea is not to
> >>>> allow rotation changes between page flips, just to program PLANE_STRIDE
> >>>> accordingly, when rotation is already enabled. Since otherwise page flip
> >>>> will calculate it incorrectly.
> >>>>
> >>>> I though Ville was raising two concerns:
> >>>>
> >>>> 1. Will the plane state be swapped from under the pending page flips
> >>>> prematurely?
> >>>
> >>> The answer is yes.
> >>
> >> And is that OK?
> >
> > No, we could misprogram the stride and corrupt the display.
> 
> Oh I did not mean that but if it is OK to replace the state under queued 
> flips. Sounded like a bug to me but what do I know.

Would be perfectly normal if we allowed queuing up flips etc. I would
try to move the code towards that in any case.

Note that with CS flips all the state has been snaphost already and
emitted into the ring, so there it just works (or should at least).
With mmio flips it's a bit different naturally since we need to store
the snapshot somewhere else. I don't really like the plane->fb
situation either, so that too should get fixed to take an explicit
snapshot for the flip.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx