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

[Matt Roper <matthew.d.roper@intel.com>]

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.


Matt

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