Re: [PATCH 05/23] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Mon, 2015-03-09 at 16:19 -0700, Matt Roper wrote:
> On Wed, Mar 04, 2015 at 04:33:17PM +0100, Daniel Vetter wrote:
> > On Tue, Mar 03, 2015 at 03:21:59PM +0200, Ander Conselvan de Oliveira wrote:
> > > For the atomic conversion, the mode set paths need to be changed to rely
> > > on an atomic state instead of using the staged config. By using an
> > > atomic state for the legacy code, we will be able to convert the code
> > > base in small chunks.
> > > 
> > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > 
> > Two small comments below.
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++--------
> > >  1 file changed, 91 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 798de7b..97d4df5 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -37,6 +37,7 @@
> > >  #include <drm/i915_drm.h>
> > >  #include "i915_drv.h"
> > >  #include "i915_trace.h"
> > > +#include <drm/drm_atomic.h>
> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_dp_helper.h>
> > >  #include <drm/drm_crtc_helper.h>
> > > @@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev)
> > >  	return true;
> > >  }
> > >  
> > > -static struct intel_crtc_state *
> > > +static void
> > > +clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct drm_crtc_state tmp_state;
> > > +
> > > +	/* Clear only the intel specific part of the crtc state */
> > > +	tmp_state = crtc_state->base;
> > > +	memset(crtc_state, 0, sizeof *crtc_state);
> > > +	crtc_state->base = tmp_state;
> > > +}
> > 
> > I guess this is to clear out state which we want to recompute, and our
> > compute_config code assumes that it's always kzalloc'ed a new config?
> > 
> > I think this should be part of the crtc duplicate_state callback to make
> > sure we're doing this consistently.
> 
> If we zero out the config in duplicate_state(), then we're not really
> "duplicating" anymore are we?  Ultimately we want to be able to
> duplicate the existing state, tweak a couple things, and then pass that
> state through the atomic pipeline, so it feels like doing a clear in the
> duplicate function is the wrong move, even if it would work for the
> frankenstein-style codebase we have at the moment.

Yeah, I guess I can't run away from inspecting the code and fixing
whatever expects the crtc_state to be zeroed.

Ander

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