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@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
