Re: [Intel-gfx] [PATCH] drm/i915: remove duplicated memsets in skl_allocate_pipe_ddb()

[Jani Nikula]

On Wed, 09 Nov 2016, Matt Roper  wrote:
> On Wed, Nov 09, 2016 at 11:27:37AM +0200, Jani Nikula wrote:
>> On Wed, 09 Nov 2016, Matt Roper  wrote:
>> > On Tue, Nov 08, 2016 at 03:21:22PM -0200, Paulo Zanoni wrote:
>> >> One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB
>> >> partitioning for multi-screen cases"), and the other was added by
>> >> 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen
>> >> cases"). I'm confused and I'll let the maintainers find out what went
>> >> wrong here.
>> >> 
>> >> Cc: Jani Nikula 
>> >> Signed-off-by: Paulo Zanoni 
>> >
>> > I think this is just an artifact of having the patch in both -next and
>> > -fixes.  The context probably changed later in -next causing a conflict
>> > when the two were merged, and then the merge resolution accidentally
>> > duplicated this hunk.  The merge commit ac4139ed7 is where we wound up
>> > with the two copies:
>> 
>> I don't have that merge commit. Is that a nightly rebuild?
>
> Yeah, it's part of a nightly rebuild.  Looks like
> 82282577545 is the equivalent now.
>
>> I don't think we even have this issue in anywhere other than
>> nightly. There isn't a branch we could apply this fix to.
>
> Yeah, the patch can't apply anywhere directly, but I thought we could
> make rerere forget its resolution of intel_pm.c for that merge and then
> fix it by hand so that future merges behave correctly?

Only if it was a manual fixup that went wrong. For silent conflicts, we
need to add a fixup patch for the merge.

BR,
Jani.


>
>
> Matt
>
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > diff --cc drivers/gpu/drm/i915/intel_pm.c
>> > index db24f89,cc9e0c0..88e28c9
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@@ -3361,11 -3404,13 +3404,17 @@@ skl_allocate_pipe_ddb(struct 
>> > intel_crtc
>> > unsigned int total_data_rate;
>> > int num_active;
>> > int id, i;
>> > +   unsigned plane_data_rate[I915_MAX_PLANES] = {};
>> > +   unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
>> > + 
>> > +   /* Clear the partitioning for disabled planes. */
>> > +   memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>> > +   memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>> >   
>> >  +  /* Clear the partitioning for disabled planes. */
>> >  +  memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>> >  +  memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>> >  +
>> > if (WARN_ON(!state))
>> > return 0;
>> >   
>> > Dropping one looks good to me.
>> >
>> > Reviewed-by: Matt Roper 
>> >
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_pm.c | 4 
>> >>  1 file changed, 4 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
>> >> b/drivers/gpu/drm/i915/intel_pm.c
>> >> index 88e28c9..cc9e0c0 100644
>> >> --- a/drivers/gpu/drm/i915/intel_pm.c
>> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> >> @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state 
>> >> *cstate,
>> >>   memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>> >>   memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>> >>  
>> >> - /* Clear the partitioning for disabled planes. */
>> >> - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>> >> - memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>> >> -
>> >>   if (WARN_ON(!state))
>> >>   return 0;
>> >>  
>> >> -- 
>> >> 2.7.4
>> >> 
>> >> ___
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: remove duplicated memsets in skl_allocate_pipe_ddb()

[Matt Roper]

On Wed, Nov 09, 2016 at 11:27:37AM +0200, Jani Nikula wrote:
> On Wed, 09 Nov 2016, Matt Roper  wrote:
> > On Tue, Nov 08, 2016 at 03:21:22PM -0200, Paulo Zanoni wrote:
> >> One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB
> >> partitioning for multi-screen cases"), and the other was added by
> >> 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen
> >> cases"). I'm confused and I'll let the maintainers find out what went
> >> wrong here.
> >> 
> >> Cc: Jani Nikula 
> >> Signed-off-by: Paulo Zanoni 
> >
> > I think this is just an artifact of having the patch in both -next and
> > -fixes.  The context probably changed later in -next causing a conflict
> > when the two were merged, and then the merge resolution accidentally
> > duplicated this hunk.  The merge commit ac4139ed7 is where we wound up
> > with the two copies:
> 
> I don't have that merge commit. Is that a nightly rebuild?

Yeah, it's part of a nightly rebuild.  Looks like
82282577545 is the equivalent now.

> I don't think we even have this issue in anywhere other than
> nightly. There isn't a branch we could apply this fix to.

Yeah, the patch can't apply anywhere directly, but I thought we could
make rerere forget its resolution of intel_pm.c for that merge and then
fix it by hand so that future merges behave correctly?


Matt


> 
> BR,
> Jani.
> 
> 
> >
> > diff --cc drivers/gpu/drm/i915/intel_pm.c
> > index db24f89,cc9e0c0..88e28c9
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@@ -3361,11 -3404,13 +3404,17 @@@ skl_allocate_pipe_ddb(struct 
> > intel_crtc
> > unsigned int total_data_rate;
> > int num_active;
> > int id, i;
> > +   unsigned plane_data_rate[I915_MAX_PLANES] = {};
> > +   unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
> > + 
> > +   /* Clear the partitioning for disabled planes. */
> > +   memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > +   memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> >   
> >  +  /* Clear the partitioning for disabled planes. */
> >  +  memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> >  +  memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> >  +
> > if (WARN_ON(!state))
> > return 0;
> >   
> > Dropping one looks good to me.
> >
> > Reviewed-by: Matt Roper 
> >
> >> ---
> >>  drivers/gpu/drm/i915/intel_pm.c | 4 
> >>  1 file changed, 4 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
> >> b/drivers/gpu/drm/i915/intel_pm.c
> >> index 88e28c9..cc9e0c0 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state 
> >> *cstate,
> >>memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> >>memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> >>  
> >> -  /* Clear the partitioning for disabled planes. */
> >> -  memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> >> -  memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> >> -
> >>if (WARN_ON(!state))
> >>return 0;
> >>  
> >> -- 
> >> 2.7.4
> >> 
> >> ___
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: remove duplicated memsets in skl_allocate_pipe_ddb()

[Jani Nikula]

On Wed, 09 Nov 2016, Matt Roper  wrote:
> On Tue, Nov 08, 2016 at 03:21:22PM -0200, Paulo Zanoni wrote:
>> One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB
>> partitioning for multi-screen cases"), and the other was added by
>> 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen
>> cases"). I'm confused and I'll let the maintainers find out what went
>> wrong here.
>> 
>> Cc: Jani Nikula 
>> Signed-off-by: Paulo Zanoni 
>
> I think this is just an artifact of having the patch in both -next and
> -fixes.  The context probably changed later in -next causing a conflict
> when the two were merged, and then the merge resolution accidentally
> duplicated this hunk.  The merge commit ac4139ed7 is where we wound up
> with the two copies:

I don't have that merge commit. Is that a nightly rebuild?

I don't think we even have this issue in anywhere other than
nightly. There isn't a branch we could apply this fix to.

BR,
Jani.


>
> diff --cc drivers/gpu/drm/i915/intel_pm.c
> index db24f89,cc9e0c0..88e28c9
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@@ -3361,11 -3404,13 +3404,17 @@@ skl_allocate_pipe_ddb(struct 
> intel_crtc
> unsigned int total_data_rate;
> int num_active;
> int id, i;
> +   unsigned plane_data_rate[I915_MAX_PLANES] = {};
> +   unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
> + 
> +   /* Clear the partitioning for disabled planes. */
> +   memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> +   memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>   
>  +  /* Clear the partitioning for disabled planes. */
>  +  memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>  +  memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>  +
> if (WARN_ON(!state))
> return 0;
>   
> Dropping one looks good to me.
>
> Reviewed-by: Matt Roper 
>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 4 
>>  1 file changed, 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 88e28c9..cc9e0c0 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>>  memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>>  memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>>  
>> -/* Clear the partitioning for disabled planes. */
>> -memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>> -memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>> -
>>  if (WARN_ON(!state))
>>  return 0;
>>  
>> -- 
>> 2.7.4
>> 
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: remove duplicated memsets in skl_allocate_pipe_ddb()

[Matt Roper]

On Tue, Nov 08, 2016 at 03:21:22PM -0200, Paulo Zanoni wrote:
> One of the memsets was added by 5a920b85f2c6 ("drm/i915/gen9: fix DDB
> partitioning for multi-screen cases"), and the other was added by
> 01c72d6c17 ("drm/i915/gen9: fix DDB partitioning for multi-screen
> cases"). I'm confused and I'll let the maintainers find out what went
> wrong here.
> 
> Cc: Jani Nikula 
> Signed-off-by: Paulo Zanoni 

I think this is just an artifact of having the patch in both -next and
-fixes.  The context probably changed later in -next causing a conflict
when the two were merged, and then the merge resolution accidentally
duplicated this hunk.  The merge commit ac4139ed7 is where we wound up
with the two copies:

diff --cc drivers/gpu/drm/i915/intel_pm.c
index db24f89,cc9e0c0..88e28c9
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@@ -3361,11 -3404,13 +3404,17 @@@ skl_allocate_pipe_ddb(struct 
intel_crtc
unsigned int total_data_rate;
int num_active;
int id, i;
+   unsigned plane_data_rate[I915_MAX_PLANES] = {};
+   unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
+ 
+   /* Clear the partitioning for disabled planes. */
+   memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
+   memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
  
 +  /* Clear the partitioning for disabled planes. */
 +  memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
 +  memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
 +
if (WARN_ON(!state))
return 0;
  
Dropping one looks good to me.

Reviewed-by: Matt Roper 

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 88e28c9..cc9e0c0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3411,10 +3411,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>   memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>   memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>  
> - /* Clear the partitioning for disabled planes. */
> - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> - memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> -
>   if (WARN_ON(!state))
>   return 0;
>  
> -- 
> 2.7.4
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx