Re: [PATCH] DAC palette width retrieval

[Robert Millan]

On Sat, Jul 25, 2009 at 02:24:22PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > Now I'm not completely sure where does this belong.  I think it's somewhat
> > generic and we can reasonably expect other OSes to want this information
> > too, but I'm not completely sure.
> For me putting these values to *_mask is ok but it may put some limits
> for future cards but as payloads have no way to tell them more info it
> should be a problem. Do any payloads need *_mask_offset? Even if so it
> shouldn't stop the patch from going in if it fixes problems for linux
> since only other payload where grub currently gives video info is xnu
> and Darwin doesn't boot in anything with depth different from 32
> anyway (or I couldn't figure out how to do it).

Ok.  I'll wait a few days to see if someone objects.  I'd specially like
Colin to have a look, since he's familiar with the graphics subsystem too.

> *_mask normally shouldn't be used in index color by internal grub
> functions anyway. And if payload needs a special convention for
> passing color info for indexed modes it should be handled by loader

Ok.  But if we decide that *_mask should go away, this is quite independant
from this patch.

> Check however that this patch doesn't break gfxterm

I can't right now, because of the regression I mentioned on IRC, but I will.

> > Also, tell me what you think about the "getset" hack.  It looks like a clean
> > way of providing both functions in less space, but then I'm not sure if we
> > really want both functions.  For now we only use the "set" one.  Then again,
> > once you have "set" getting "get" comes really cheap.
> >
> It's ok but once we get my mm functions in and move this part to
> vbe.mod splitting get and set will be a good idea

Actually, my disjunctive was between providing getset or only set.  I don't
see the point of two separate functions.

> +       if (status != 0x004F)
> It should be GRUB_VBE_STATUS_OK

I see.  I copied this from somewhere else, I'll see about fixing that too.

> +         /* 6 is default after mode reset.  */
> +         width = 6;
> Looks like a hack and some boards may have different value. Is there a
> better way?

It is 6 according to the VBE spec (Linux is also consistent with that).

> +       mode_info->red_mask_size = mode_info->green_mask_size
> +         = mode_info->blue_mask_size = mode_info->rsvd_mask_size
> +         = width;
> Perhaps mode_info->rsvd_mask_size could be left at 0?
> I don't think index colors have any alpha

I suppose it's fine.  Does anyone see a problem with zeroed rsvd_mask_size?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."